You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@servicecomb.apache.org by li...@apache.org on 2021/08/16 07:01:52 UTC
[servicecomb-service-center] branch master updated: sliding window
based account lock deletion (#1136)
This is an automated email from the ASF dual-hosted git repository.
littlecui pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/servicecomb-service-center.git
The following commit(s) were added to refs/heads/master by this push:
new 92a4cd6 sliding window based account lock deletion (#1136)
92a4cd6 is described below
commit 92a4cd6d3b9ed014e82c60ff7fcc43eca6ba7daa
Author: little-cui <su...@qq.com>
AuthorDate: Mon Aug 16 15:01:46 2021 +0800
sliding window based account lock deletion (#1136)
* Bug: should not DELETE the locks recent 20min produced
* Optimize: refactor account lock mgr
---
datasource/account.go | 1 -
datasource/account_test.go | 51 --------------------------------
datasource/etcd/account_lock.go | 14 ++-------
datasource/etcd/etcd.go | 2 +-
datasource/mongo/account_lock.go | 14 ++-------
datasource/mongo/bootstrap/bootstrap.go | 1 +
datasource/mongo/mongo.go | 2 +-
datasource/options.go | 5 +---
etc/conf/app.yaml | 1 +
server/job/account/account.go | 9 +++---
server/server.go | 21 +++----------
server/service/account/account.go | 21 +++++++++++--
server/service/account/account_test.go | 52 +++++++++++++++++++++++++++++++++
server/service/rbac/blocker.go | 16 +---------
test/test.go | 3 --
15 files changed, 89 insertions(+), 124 deletions(-)
diff --git a/datasource/account.go b/datasource/account.go
index fe098e4..d0235a0 100644
--- a/datasource/account.go
+++ b/datasource/account.go
@@ -59,7 +59,6 @@ type AccountLockManager interface {
ListLock(ctx context.Context) ([]*AccountLock, int64, error)
DeleteLock(ctx context.Context, key string) error
DeleteLockList(ctx context.Context, keys []string) error
- Ban(ctx context.Context, key string) error
}
type AccountLock struct {
Key string `json:"key,omitempty"`
diff --git a/datasource/account_test.go b/datasource/account_test.go
index 5ba4bdf..a6e839b 100644
--- a/datasource/account_test.go
+++ b/datasource/account_test.go
@@ -119,54 +119,3 @@ func TestAccount(t *testing.T) {
assert.NoError(t, err)
})
}
-
-func TestAccountLock(t *testing.T) {
- var banTime int64
-
- t.Run("ban account TestAccountLock, should return no error", func(t *testing.T) {
- err := datasource.GetAccountLockManager().Ban(context.Background(), "TestAccountLock")
- assert.NoError(t, err)
-
- lock, err := datasource.GetAccountLockManager().GetLock(context.Background(), "TestAccountLock")
- assert.NoError(t, err)
- assert.Equal(t, datasource.StatusBanned, lock.Status)
- assert.Less(t, time.Now().Unix(), lock.ReleaseAt)
-
- banTime = lock.ReleaseAt
- })
-
- t.Run("ban account TestAccountLock again, should return a new release time", func(t *testing.T) {
- time.Sleep(time.Second)
-
- err := datasource.GetAccountLockManager().Ban(context.Background(), "TestAccountLock")
- assert.NoError(t, err)
-
- lock, err := datasource.GetAccountLockManager().GetLock(context.Background(), "TestAccountLock")
- assert.NoError(t, err)
- assert.Equal(t, datasource.StatusBanned, lock.Status)
- assert.Less(t, banTime, lock.ReleaseAt)
- })
-
- t.Run("ban account TestAccountLock again, should refresh releaseAt", func(t *testing.T) {
- lock1, err := datasource.GetAccountLockManager().GetLock(context.Background(), "TestAccountLock")
- assert.NoError(t, err)
- assert.Equal(t, datasource.StatusBanned, lock1.Status)
-
- time.Sleep(time.Second)
- err = datasource.GetAccountLockManager().Ban(context.Background(), "TestAccountLock")
- assert.NoError(t, err)
-
- lock2, err := datasource.GetAccountLockManager().GetLock(context.Background(), "TestAccountLock")
- assert.NoError(t, err)
- assert.Less(t, lock1.ReleaseAt, lock2.ReleaseAt)
- })
-
- t.Run("delete account lock, should return no error", func(t *testing.T) {
- err := datasource.GetAccountLockManager().DeleteLock(context.Background(), "TestAccountLock")
- assert.NoError(t, err)
-
- lock, err := datasource.GetAccountLockManager().GetLock(context.Background(), "TestAccountLock")
- assert.Equal(t, datasource.ErrAccountLockNotExist, err)
- assert.Nil(t, lock)
- })
-}
diff --git a/datasource/etcd/account_lock.go b/datasource/etcd/account_lock.go
index 69d3f0d..d1c9ee6 100644
--- a/datasource/etcd/account_lock.go
+++ b/datasource/etcd/account_lock.go
@@ -19,7 +19,6 @@ import (
"context"
"encoding/json"
"fmt"
- "time"
"github.com/apache/servicecomb-service-center/datasource"
"github.com/apache/servicecomb-service-center/datasource/etcd/path"
@@ -28,7 +27,6 @@ import (
)
type AccountLockManager struct {
- releaseAfter time.Duration
}
func (al AccountLockManager) UpsertLock(ctx context.Context, lock *datasource.AccountLock) error {
@@ -110,14 +108,6 @@ func (al AccountLockManager) DeleteLockList(ctx context.Context, keys []string)
return nil
}
-func NewAccountLockManager(ReleaseAfter time.Duration) datasource.AccountLockManager {
- return &AccountLockManager{releaseAfter: ReleaseAfter}
-}
-
-func (al AccountLockManager) Ban(ctx context.Context, key string) error {
- l := &datasource.AccountLock{}
- l.Key = key
- l.Status = datasource.StatusBanned
- l.ReleaseAt = time.Now().Add(al.releaseAfter).Unix()
- return al.UpsertLock(ctx, l)
+func NewAccountLockManager() datasource.AccountLockManager {
+ return &AccountLockManager{}
}
diff --git a/datasource/etcd/etcd.go b/datasource/etcd/etcd.go
index 1054968..c1a8797 100644
--- a/datasource/etcd/etcd.go
+++ b/datasource/etcd/etcd.go
@@ -110,7 +110,7 @@ func NewDataSource(opts datasource.Options) (datasource.DataSource, error) {
return nil, err
}
inst.accountManager = &AccountManager{}
- inst.accountLockManager = NewAccountLockManager(opts.ReleaseAccountAfter)
+ inst.accountLockManager = NewAccountLockManager()
inst.roleManager = &RoleManager{}
inst.metadataManager = newMetadataManager(opts.SchemaNotEditable, opts.InstanceTTL)
inst.sysManager = newSysManager()
diff --git a/datasource/mongo/account_lock.go b/datasource/mongo/account_lock.go
index 36e8ed7..e3c0022 100644
--- a/datasource/mongo/account_lock.go
+++ b/datasource/mongo/account_lock.go
@@ -18,7 +18,6 @@ package mongo
import (
"context"
"fmt"
- "time"
"github.com/apache/servicecomb-service-center/datasource"
"github.com/apache/servicecomb-service-center/datasource/mongo/client"
@@ -30,7 +29,6 @@ import (
)
type AccountLockManager struct {
- releaseAfter time.Duration
}
func (al *AccountLockManager) UpsertLock(ctx context.Context, lock *datasource.AccountLock) error {
@@ -127,14 +125,6 @@ func (al *AccountLockManager) DeleteLockList(ctx context.Context, keys []string)
return nil
}
-func (al *AccountLockManager) Ban(ctx context.Context, key string) error {
- return al.UpsertLock(ctx, &datasource.AccountLock{
- Key: key,
- Status: datasource.StatusBanned,
- ReleaseAt: time.Now().Add(al.releaseAfter).Unix(),
- })
-}
-
-func NewAccountLockManager(ReleaseAfter time.Duration) datasource.AccountLockManager {
- return &AccountLockManager{releaseAfter: ReleaseAfter}
+func NewAccountLockManager() datasource.AccountLockManager {
+ return &AccountLockManager{}
}
diff --git a/datasource/mongo/bootstrap/bootstrap.go b/datasource/mongo/bootstrap/bootstrap.go
index 47b4dcf..56af2b8 100644
--- a/datasource/mongo/bootstrap/bootstrap.go
+++ b/datasource/mongo/bootstrap/bootstrap.go
@@ -18,6 +18,7 @@
package bootstrap
import (
+ // registry
_ "github.com/apache/servicecomb-service-center/datasource/mongo"
// heartbeat
diff --git a/datasource/mongo/mongo.go b/datasource/mongo/mongo.go
index d91a253..1460102 100644
--- a/datasource/mongo/mongo.go
+++ b/datasource/mongo/mongo.go
@@ -92,7 +92,7 @@ func NewDataSource(opts datasource.Options) (datasource.DataSource, error) {
inst.roleManager = &RoleManager{}
inst.metadataManager = &MetadataManager{SchemaNotEditable: opts.SchemaNotEditable, InstanceTTL: opts.InstanceTTL}
inst.accountManager = &AccountManager{}
- inst.accountLockManager = NewAccountLockManager(opts.ReleaseAccountAfter)
+ inst.accountLockManager = NewAccountLockManager()
inst.metricsManager = &MetricsManager{}
return inst, nil
}
diff --git a/datasource/options.go b/datasource/options.go
index 3f310b4..7a136db 100644
--- a/datasource/options.go
+++ b/datasource/options.go
@@ -18,8 +18,6 @@
package datasource
import (
- "time"
-
"github.com/little-cui/etcdadpt"
)
@@ -30,6 +28,5 @@ type Options struct {
EnableCache bool
SchemaNotEditable bool
// InstanceTTL: the default ttl of instance lease
- InstanceTTL int64
- ReleaseAccountAfter time.Duration
+ InstanceTTL int64
}
diff --git a/etc/conf/app.yaml b/etc/conf/app.yaml
index cae4628..03ba0ac 100644
--- a/etc/conf/app.yaml
+++ b/etc/conf/app.yaml
@@ -164,6 +164,7 @@ rbac:
privateKeyFile: ./private.key
publicKeyFile: ./public.key
releaseLockAfter: 15m # failure login attempt causes account blocking, that is block duration
+ retainLockHistoryFor: 20m # the ttl of lock history
scope: '*' # specify auth resource scope, can be account,role,service,service/schema,...
metrics:
diff --git a/server/job/account/account.go b/server/job/account/account.go
index 8467c39..f2a3097 100644
--- a/server/job/account/account.go
+++ b/server/job/account/account.go
@@ -27,7 +27,7 @@ import (
"github.com/go-chassis/foundation/gopool"
)
-const releasedLockHistoryCleanupInterval = 20 * time.Minute
+const CleanupInterval = 1 * time.Minute
func init() {
startReleasedLockHistoryCleanupJob()
@@ -36,9 +36,9 @@ func init() {
// startReleasedLockHistoryCleanupJob cause of accountsvc.IsBanned may be never called
// after locked account, then run a job to cleanup the released lock
func startReleasedLockHistoryCleanupJob() {
- log.Info(fmt.Sprintf("start released lock history cleanup job(every %s)", releasedLockHistoryCleanupInterval))
+ log.Info(fmt.Sprintf("start released lock history cleanup job(every %s)", CleanupInterval))
gopool.Go(func(ctx context.Context) {
- tick := time.NewTicker(releasedLockHistoryCleanupInterval)
+ tick := time.NewTicker(CleanupInterval)
defer tick.Stop()
for {
select {
@@ -59,9 +59,10 @@ func CleanupReleasedLockHistory(ctx context.Context) error {
if err != nil {
return err
}
+ now := time.Now().Unix()
var keys []string
for _, lock := range locks {
- if lock.ReleaseAt > time.Now().Unix() {
+ if lock.ReleaseAt > now {
continue
}
keys = append(keys, lock.Key)
diff --git a/server/server.go b/server/server.go
index 07a7280..d9955dc 100644
--- a/server/server.go
+++ b/server/server.go
@@ -47,8 +47,7 @@ import (
)
const (
- defaultCollectPeriod = 30 * time.Second
- defaultReleaseLockAfter = 15 * time.Minute
+ defaultCollectPeriod = 30 * time.Second
)
var server ServiceCenterServer
@@ -113,7 +112,6 @@ func (s *ServiceCenterServer) initEndpoints() {
func (s *ServiceCenterServer) initDatasource() {
// init datasource
kind := config.GetString("registry.kind", "", config.WithStandby("registry_plugin"))
- releaseLockAfter := getReleaseLockAfter()
tlsConfig, err := getDatasourceTLSConfig()
if err != nil {
log.Fatal("get datasource tlsConfig failed", err)
@@ -139,25 +137,14 @@ func (s *ServiceCenterServer) initDatasource() {
}
},
},
- EnableCache: config.GetRegistry().EnableCache,
- InstanceTTL: config.GetRegistry().InstanceTTL,
- SchemaNotEditable: config.GetRegistry().SchemaNotEditable,
- ReleaseAccountAfter: releaseLockAfter,
+ EnableCache: config.GetRegistry().EnableCache,
+ InstanceTTL: config.GetRegistry().InstanceTTL,
+ SchemaNotEditable: config.GetRegistry().SchemaNotEditable,
}); err != nil {
log.Fatal("init datasource failed", err)
}
}
-func getReleaseLockAfter() time.Duration {
- releaseLockAfter := config.GetString("rbac.releaseLockAfter", "15m")
- d, err := time.ParseDuration(releaseLockAfter)
- if err != nil {
- log.Warn("releaseAfter is invalid, use default config")
- d = defaultReleaseLockAfter
- }
- return d
-}
-
func getDatasourceTLSConfig() (*tls.Config, error) {
if config.GetSSL().SslEnabled {
return tlsconf.ClientConfig()
diff --git a/server/service/account/account.go b/server/service/account/account.go
index 79eaad8..36a6336 100644
--- a/server/service/account/account.go
+++ b/server/service/account/account.go
@@ -5,9 +5,14 @@ import (
"fmt"
"time"
+ "github.com/apache/servicecomb-service-center/datasource"
"github.com/apache/servicecomb-service-center/pkg/log"
+ "github.com/apache/servicecomb-service-center/server/config"
+)
- "github.com/apache/servicecomb-service-center/datasource"
+const (
+ defaultReleaseLockAfter = 15 * time.Minute
+ defaultRetainLockHistoryFor = 20 * time.Minute
)
func IsBanned(ctx context.Context, key string) (bool, error) {
@@ -32,11 +37,21 @@ func IsBanned(ctx context.Context, key string) (bool, error) {
}
return false, nil
}
+
func Ban(ctx context.Context, key string) error {
- return datasource.GetAccountLockManager().Ban(ctx, key)
+ return Lock(ctx, key, datasource.StatusBanned)
}
-func UpsertLock(ctx context.Context, lock *datasource.AccountLock) error {
+func Lock(ctx context.Context, key, status string) error {
+ duration := config.GetDuration("rbac.retainLockHistoryFor", defaultRetainLockHistoryFor)
+ if status == datasource.StatusBanned {
+ duration = config.GetDuration("rbac.releaseLockAfter", defaultReleaseLockAfter)
+ }
+ lock := &datasource.AccountLock{
+ Key: key,
+ Status: status,
+ ReleaseAt: time.Now().Add(duration).Unix(),
+ }
return datasource.GetAccountLockManager().UpsertLock(ctx, lock)
}
diff --git a/server/service/account/account_test.go b/server/service/account/account_test.go
index 54c446e..2c18e9d 100644
--- a/server/service/account/account_test.go
+++ b/server/service/account/account_test.go
@@ -7,6 +7,7 @@ import (
_ "github.com/apache/servicecomb-service-center/test"
+ "github.com/apache/servicecomb-service-center/datasource"
"github.com/apache/servicecomb-service-center/server/service/account"
"github.com/stretchr/testify/assert"
)
@@ -48,3 +49,54 @@ func TestListLock(t *testing.T) {
assert.Fail(t, "test key not found")
})
}
+
+func TestBan(t *testing.T) {
+ var banTime int64
+
+ t.Run("ban account TestAccountLock, should return no error", func(t *testing.T) {
+ err := account.Ban(context.Background(), "TestAccountLock")
+ assert.NoError(t, err)
+
+ lock, err := datasource.GetAccountLockManager().GetLock(context.Background(), "TestAccountLock")
+ assert.NoError(t, err)
+ assert.Equal(t, datasource.StatusBanned, lock.Status)
+ assert.Less(t, time.Now().Unix(), lock.ReleaseAt)
+
+ banTime = lock.ReleaseAt
+ })
+
+ t.Run("ban account TestAccountLock again, should return a new release time", func(t *testing.T) {
+ time.Sleep(time.Second)
+
+ err := account.Ban(context.Background(), "TestAccountLock")
+ assert.NoError(t, err)
+
+ lock, err := datasource.GetAccountLockManager().GetLock(context.Background(), "TestAccountLock")
+ assert.NoError(t, err)
+ assert.Equal(t, datasource.StatusBanned, lock.Status)
+ assert.Less(t, banTime, lock.ReleaseAt)
+ })
+
+ t.Run("ban account TestAccountLock again, should refresh releaseAt", func(t *testing.T) {
+ lock1, err := datasource.GetAccountLockManager().GetLock(context.Background(), "TestAccountLock")
+ assert.NoError(t, err)
+ assert.Equal(t, datasource.StatusBanned, lock1.Status)
+
+ time.Sleep(time.Second)
+ err = account.Ban(context.Background(), "TestAccountLock")
+ assert.NoError(t, err)
+
+ lock2, err := datasource.GetAccountLockManager().GetLock(context.Background(), "TestAccountLock")
+ assert.NoError(t, err)
+ assert.Less(t, lock1.ReleaseAt, lock2.ReleaseAt)
+ })
+
+ t.Run("delete account lock, should return no error", func(t *testing.T) {
+ err := datasource.GetAccountLockManager().DeleteLock(context.Background(), "TestAccountLock")
+ assert.NoError(t, err)
+
+ lock, err := datasource.GetAccountLockManager().GetLock(context.Background(), "TestAccountLock")
+ assert.Equal(t, datasource.ErrAccountLockNotExist, err)
+ assert.Nil(t, lock)
+ })
+}
diff --git a/server/service/rbac/blocker.go b/server/service/rbac/blocker.go
index f57620b..6f6d943 100644
--- a/server/service/rbac/blocker.go
+++ b/server/service/rbac/blocker.go
@@ -63,21 +63,7 @@ func TryLockAccount(key string) {
if !allow {
status = datasource.StatusBanned
}
- saveLock(key, status)
-}
-
-func saveLock(key, status string) {
- var err error
- ctx := context.Background()
- if status == datasource.StatusBanned {
- err = accountsvc.Ban(ctx, key)
- } else {
- err = accountsvc.UpsertLock(ctx, &datasource.AccountLock{
- Key: key,
- Status: status,
- ReleaseAt: time.Now().Unix(),
- })
- }
+ err := accountsvc.Lock(context.Background(), key, status)
if err != nil {
log.Error(fmt.Sprintf("can not ban account %s", key), err)
}
diff --git a/test/test.go b/test/test.go
index 7f14d47..3a42d3c 100644
--- a/test/test.go
+++ b/test/test.go
@@ -19,8 +19,6 @@
package test
import (
- "time"
-
_ "github.com/apache/servicecomb-service-center/server/init"
_ "github.com/apache/servicecomb-service-center/server/bootstrap"
@@ -47,7 +45,6 @@ func init() {
Config: etcdadpt.Config{
Kind: kind,
},
- ReleaseAccountAfter: 3 * time.Second,
})
core.ServiceAPI = disco.AssembleResources()
}