From ee6a56b78c4ae0617336c0c5acc31bc49bad5587 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Thu, 26 Nov 2020 17:21:54 +0530 Subject: [PATCH 1/4] Add cache to store UID to UserID mapping in EOS --- changelog/unreleased/eos-user-cache.md | 7 +++++++ pkg/storage/utils/eosfs/eosfs.go | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+) create mode 100644 changelog/unreleased/eos-user-cache.md diff --git a/changelog/unreleased/eos-user-cache.md b/changelog/unreleased/eos-user-cache.md new file mode 100644 index 0000000000..38bcb65a54 --- /dev/null +++ b/changelog/unreleased/eos-user-cache.md @@ -0,0 +1,7 @@ +Enhancement: Add cache to store UID to UserID mapping in EOS + +Previously, we used to send an RPC to the user provider service for every lookup +of user IDs from the UID stored in EOS. This PR adds an in-memory lock-protected +cache to store this mapping. + +https://github.com/cs3org/reva/pull/1340 diff --git a/pkg/storage/utils/eosfs/eosfs.go b/pkg/storage/utils/eosfs/eosfs.go index 087f1e696f..945d6b614c 100644 --- a/pkg/storage/utils/eosfs/eosfs.go +++ b/pkg/storage/utils/eosfs/eosfs.go @@ -29,6 +29,7 @@ import ( "regexp" "strconv" "strings" + "sync" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" @@ -179,6 +180,12 @@ type eosfs struct { chunkHandler *chunking.ChunkHandler singleUserUID string singleUserGID string + userIDCache *userCache +} + +type userCache struct { + *sync.RWMutex + cache map[string]*userpb.UserId } // NewEOSFS returns a storage.FS interface implementation that connects to an @@ -213,6 +220,10 @@ func NewEOSFS(c *Config) (storage.FS, error) { c: eosClient, conf: c, chunkHandler: chunking.NewChunkHandler(c.CacheDirectory), + userIDCache: &userCache{ + &sync.RWMutex{}, + make(map[string]*userpb.UserId), + }, } return eosfs, nil @@ -1455,6 +1466,12 @@ func (fs *eosfs) getUIDGateway(ctx context.Context, u *userpb.UserId) (string, s } func (fs *eosfs) getUserIDGateway(ctx context.Context, uid string) (*userpb.UserId, error) { + fs.userIDCache.RLock() + if userID, ok := fs.userIDCache.cache[uid]; ok { + fs.userIDCache.RUnlock() + return userID, nil + } + fs.userIDCache.RUnlock() client, err := pool.GetGatewayServiceClient(fs.conf.GatewaySvc) if err != nil { return nil, errors.Wrap(err, "eos: error getting gateway grpc client") @@ -1469,6 +1486,9 @@ func (fs *eosfs) getUserIDGateway(ctx context.Context, uid string) (*userpb.User if getUserResp.Status.Code != rpc.Code_CODE_OK { return nil, errors.Wrap(err, "eos: grpc get user failed") } + fs.userIDCache.Lock() + fs.userIDCache.cache[uid] = getUserResp.User.Id + fs.userIDCache.Unlock() return getUserResp.User.Id, nil } From 80d491bee0f35b78838fcd79799dcb97888e3a37 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Thu, 26 Nov 2020 17:33:43 +0530 Subject: [PATCH 2/4] Return false in split method in gateway SP for invalid paths instead of panic --- internal/grpc/services/gateway/storageprovider.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go index 399ab087a4..4b9bcd09df 100644 --- a/internal/grpc/services/gateway/storageprovider.go +++ b/internal/grpc/services/gateway/storageprovider.go @@ -2016,7 +2016,7 @@ func (s *svc) split(ctx context.Context, p string, i int) bool { // validate that we have always at least two elements if len(parts) < 2 { - panic(fmt.Sprintf("split: len(parts) < 2: path:%s parts:%+v", p, parts)) + return false } // validate the share folder is always the second element, first element is always the hardcoded value of "home" From 321284ec5aa42191a6c35cb275e8baed34b62327 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Thu, 26 Nov 2020 18:05:43 +0530 Subject: [PATCH 3/4] Fix minor bugs in publicshare pkg --- pkg/publicshare/manager/json/json.go | 13 +++++++++---- pkg/publicshare/manager/memory/memory.go | 2 +- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/pkg/publicshare/manager/json/json.go b/pkg/publicshare/manager/json/json.go index 9e4c1a1702..4d61b29751 100644 --- a/pkg/publicshare/manager/json/json.go +++ b/pkg/publicshare/manager/json/json.go @@ -363,16 +363,19 @@ func (m *manager) ListPublicShares(ctx context.Context, u *user.User, filters [] // RevokePublicShare undocumented. func (m *manager) RevokePublicShare(ctx context.Context, u *user.User, ref *link.PublicShareReference) error { m.mutex.Lock() - defer m.mutex.Unlock() - db, err := m.readDb() if err != nil { return err } + m.mutex.Unlock() switch { - case ref.GetId().OpaqueId != "": - delete(db, ref.GetId().OpaqueId) + case ref.GetId() != nil && ref.GetId().OpaqueId != "": + if _, ok := db[ref.GetId().OpaqueId]; ok { + delete(db, ref.GetId().OpaqueId) + } else { + return errors.New("reference does not exist") + } case ref.GetToken() != "": share, err := m.getByToken(ctx, ref.GetToken()) if err != nil { @@ -383,6 +386,8 @@ func (m *manager) RevokePublicShare(ctx context.Context, u *user.User, ref *link return errors.New("reference does not exist") } + m.mutex.Lock() + defer m.mutex.Unlock() return m.writeDb(db) } diff --git a/pkg/publicshare/manager/memory/memory.go b/pkg/publicshare/manager/memory/memory.go index e8c3372785..6f3e8f1421 100644 --- a/pkg/publicshare/manager/memory/memory.go +++ b/pkg/publicshare/manager/memory/memory.go @@ -194,7 +194,7 @@ func (m *manager) ListPublicShares(ctx context.Context, u *user.User, filters [] func (m *manager) RevokePublicShare(ctx context.Context, u *user.User, ref *link.PublicShareReference) error { // check whether the reference exists switch { - case ref.GetId().OpaqueId != "": + case ref.GetId() != nil && ref.GetId().OpaqueId != "": s, err := m.getPublicShareByTokenID(ctx, *ref.GetId()) if err != nil { return errors.New("reference does not exist") From a766f293e46f7ef048497e4d7db02464721d8844 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Fri, 27 Nov 2020 21:45:49 +0530 Subject: [PATCH 4/4] Use sync map instead of a locked one --- pkg/storage/utils/eosfs/eosfs.go | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/pkg/storage/utils/eosfs/eosfs.go b/pkg/storage/utils/eosfs/eosfs.go index 945d6b614c..6f47e3a93b 100644 --- a/pkg/storage/utils/eosfs/eosfs.go +++ b/pkg/storage/utils/eosfs/eosfs.go @@ -180,12 +180,7 @@ type eosfs struct { chunkHandler *chunking.ChunkHandler singleUserUID string singleUserGID string - userIDCache *userCache -} - -type userCache struct { - *sync.RWMutex - cache map[string]*userpb.UserId + userIDCache sync.Map } // NewEOSFS returns a storage.FS interface implementation that connects to an @@ -220,10 +215,7 @@ func NewEOSFS(c *Config) (storage.FS, error) { c: eosClient, conf: c, chunkHandler: chunking.NewChunkHandler(c.CacheDirectory), - userIDCache: &userCache{ - &sync.RWMutex{}, - make(map[string]*userpb.UserId), - }, + userIDCache: sync.Map{}, } return eosfs, nil @@ -1466,12 +1458,10 @@ func (fs *eosfs) getUIDGateway(ctx context.Context, u *userpb.UserId) (string, s } func (fs *eosfs) getUserIDGateway(ctx context.Context, uid string) (*userpb.UserId, error) { - fs.userIDCache.RLock() - if userID, ok := fs.userIDCache.cache[uid]; ok { - fs.userIDCache.RUnlock() - return userID, nil + if userIDInterface, ok := fs.userIDCache.Load(uid); ok { + return userIDInterface.(*userpb.UserId), nil } - fs.userIDCache.RUnlock() + client, err := pool.GetGatewayServiceClient(fs.conf.GatewaySvc) if err != nil { return nil, errors.Wrap(err, "eos: error getting gateway grpc client") @@ -1486,9 +1476,8 @@ func (fs *eosfs) getUserIDGateway(ctx context.Context, uid string) (*userpb.User if getUserResp.Status.Code != rpc.Code_CODE_OK { return nil, errors.Wrap(err, "eos: grpc get user failed") } - fs.userIDCache.Lock() - fs.userIDCache.cache[uid] = getUserResp.User.Id - fs.userIDCache.Unlock() + + fs.userIDCache.Store(uid, getUserResp.User.Id) return getUserResp.User.Id, nil }