diff --git a/auth/range_perm_cache.go b/auth/range_perm_cache.go index 7b6c182409c..613017ee82f 100644 --- a/auth/range_perm_cache.go +++ b/auth/range_perm_cache.go @@ -113,38 +113,48 @@ func checkKeyPoint(lg *zap.Logger, cachedPerms *unifiedRangePermissions, key []b return false } -func (as *authStore) isRangeOpPermitted(tx backend.BatchTx, userName string, key, rangeEnd []byte, permtyp authpb.Permission_Type) bool { - // assumption: tx is Lock()ed - _, ok := as.rangePermCache[userName] +func (as *authStore) isRangeOpPermitted(userName string, key, rangeEnd []byte, permtyp authpb.Permission_Type) bool { + as.rangePermCacheMu.RLock() + defer as.rangePermCacheMu.RUnlock() + + rangePerm, ok := as.rangePermCache[userName] if !ok { - perms := getMergedPerms(as.lg, tx, userName) - if perms == nil { - if as.lg != nil { - as.lg.Warn( - "failed to create a merged permission", - zap.String("user-name", userName), - ) - } else { - plog.Errorf("failed to create a unified permission of user %s", userName) - } - return false - } - as.rangePermCache[userName] = perms + as.lg.Error( + "user doesn't exist", + zap.String("user-name", userName), + ) + return false } if len(rangeEnd) == 0 { - return checkKeyPoint(as.lg, as.rangePermCache[userName], key, permtyp) + return checkKeyPoint(as.lg, rangePerm, key, permtyp) } - return checkKeyInterval(as.lg, as.rangePermCache[userName], key, rangeEnd, permtyp) + return checkKeyInterval(as.lg, rangePerm, key, rangeEnd, permtyp) } -func (as *authStore) clearCachedPerm() { +func (as *authStore) refreshRangePermCache(tx backend.BatchTx) { + // Note that every authentication configuration update calls this method and it invalidates the entire + // rangePermCache and reconstruct it based on information of users and roles stored in the backend. + // This can be a costly operation. + as.rangePermCacheMu.Lock() + defer as.rangePermCacheMu.Unlock() + as.rangePermCache = make(map[string]*unifiedRangePermissions) -} -func (as *authStore) invalidateCachedPerm(userName string) { - delete(as.rangePermCache, userName) + users := getAllUsers(as.lg, tx) + for _, user := range users { + userName := string(user.Name) + perms := getMergedPerms(as.lg, tx, userName) + if perms == nil { + as.lg.Error( + "failed to create a merged permission", + zap.String("user-name", userName), + ) + continue + } + as.rangePermCache[userName] = perms + } } type unifiedRangePermissions struct { diff --git a/auth/store.go b/auth/store.go index a1d2e077cfe..4d5928f251f 100644 --- a/auth/store.go +++ b/auth/store.go @@ -215,7 +215,14 @@ type authStore struct { enabled bool enabledMu sync.RWMutex - rangePermCache map[string]*unifiedRangePermissions // username -> unifiedRangePermissions + // rangePermCache needs to be protected by rangePermCacheMu + // rangePermCacheMu needs to be write locked only in initialization phase or configuration changes + // Hot paths like Range(), needs to acquire read lock for improving performance + // + // Note that BatchTx and ReadTx cannot be a mutex for rangePermCache because they are independent resources + // see also: https://github.com/etcd-io/etcd/pull/13920#discussion_r849114855 + rangePermCache map[string]*unifiedRangePermissions // username -> unifiedRangePermissions + rangePermCacheMu sync.RWMutex tokenProvider TokenProvider syncConsistentIndex saveConsistentIndexFunc @@ -258,7 +265,7 @@ func (as *authStore) AuthEnable() error { as.enabled = true as.tokenProvider.enable() - as.rangePermCache = make(map[string]*unifiedRangePermissions) + as.refreshRangePermCache(tx) as.setRevision(getRevision(tx)) @@ -457,6 +464,7 @@ func (as *authStore) UserAdd(r *pb.AuthUserAddRequest) (*pb.AuthUserAddResponse, as.commitRevision(tx) as.saveConsistentIndex(tx) + as.refreshRangePermCache(tx) if as.lg != nil { as.lg.Info("added a user", zap.String("user-name", r.Name)) @@ -489,8 +497,8 @@ func (as *authStore) UserDelete(r *pb.AuthUserDeleteRequest) (*pb.AuthUserDelete as.commitRevision(tx) as.saveConsistentIndex(tx) + as.refreshRangePermCache(tx) - as.invalidateCachedPerm(r.Name) as.tokenProvider.invalidateUser(r.Name) if as.lg != nil { @@ -542,8 +550,8 @@ func (as *authStore) UserChangePassword(r *pb.AuthUserChangePasswordRequest) (*p as.commitRevision(tx) as.saveConsistentIndex(tx) + as.refreshRangePermCache(tx) - as.invalidateCachedPerm(r.Name) as.tokenProvider.invalidateUser(r.Name) if as.lg != nil { @@ -595,10 +603,9 @@ func (as *authStore) UserGrantRole(r *pb.AuthUserGrantRoleRequest) (*pb.AuthUser putUser(as.lg, tx, user) - as.invalidateCachedPerm(r.User) - as.commitRevision(tx) as.saveConsistentIndex(tx) + as.refreshRangePermCache(tx) if as.lg != nil { as.lg.Info( @@ -682,10 +689,9 @@ func (as *authStore) UserRevokeRole(r *pb.AuthUserRevokeRoleRequest) (*pb.AuthUs putUser(as.lg, tx, updatedUser) - as.invalidateCachedPerm(r.Name) - as.commitRevision(tx) as.saveConsistentIndex(tx) + as.refreshRangePermCache(tx) if as.lg != nil { as.lg.Info( @@ -755,12 +761,9 @@ func (as *authStore) RoleRevokePermission(r *pb.AuthRoleRevokePermissionRequest) putRole(as.lg, tx, updatedRole) - // TODO(mitake): currently single role update invalidates every cache - // It should be optimized. - as.clearCachedPerm() - as.commitRevision(tx) as.saveConsistentIndex(tx) + as.refreshRangePermCache(tx) if as.lg != nil { as.lg.Info( @@ -816,11 +819,11 @@ func (as *authStore) RoleDelete(r *pb.AuthRoleDeleteRequest) (*pb.AuthRoleDelete putUser(as.lg, tx, updatedUser) - as.invalidateCachedPerm(string(user.Name)) } as.commitRevision(tx) as.saveConsistentIndex(tx) + as.refreshRangePermCache(tx) if as.lg != nil { as.lg.Info("deleted a role", zap.String("role-name", r.Role)) @@ -910,12 +913,9 @@ func (as *authStore) RoleGrantPermission(r *pb.AuthRoleGrantPermissionRequest) ( putRole(as.lg, tx, role) - // TODO(mitake): currently single role update invalidates every cache - // It should be optimized. - as.clearCachedPerm() - as.commitRevision(tx) as.saveConsistentIndex(tx) + as.refreshRangePermCache(tx) if as.lg != nil { as.lg.Info( @@ -976,7 +976,7 @@ func (as *authStore) isOpPermitted(userName string, revision uint64, key, rangeE return nil } - if as.isRangeOpPermitted(tx, userName, key, rangeEnd, permTyp) { + if as.isRangeOpPermitted(userName, key, rangeEnd, permTyp) { return nil } @@ -1042,7 +1042,15 @@ func getUser(lg *zap.Logger, tx backend.BatchTx, username string) *authpb.User { } func getAllUsers(lg *zap.Logger, tx backend.BatchTx) []*authpb.User { - _, vs := tx.UnsafeRange(authUsersBucketName, []byte{0}, []byte{0xff}, -1) + var vs [][]byte + err := tx.UnsafeForEach(authUsersBucketName, func(k []byte, v []byte) error { + vs = append(vs, v) + return nil + }) + if err != nil { + lg.Panic("failed to get users", + zap.Error(err)) + } if len(vs) == 0 { return nil } diff --git a/auth/store_test.go b/auth/store_test.go index e1259516cb4..5024b21cac9 100644 --- a/auth/store_test.go +++ b/auth/store_test.go @@ -28,6 +28,7 @@ import ( "go.etcd.io/etcd/etcdserver/api/v3rpc/rpctypes" pb "go.etcd.io/etcd/etcdserver/etcdserverpb" "go.etcd.io/etcd/mvcc/backend" + "go.etcd.io/etcd/pkg/adt" "go.uber.org/zap" "golang.org/x/crypto/bcrypt" @@ -151,7 +152,8 @@ func TestUserAdd(t *testing.T) { as, tearDown := setupAuthStore(t) defer tearDown(t) - ua := &pb.AuthUserAddRequest{Name: "foo", Options: &authpb.UserAddOptions{NoPassword: false}} + const userName = "foo" + ua := &pb.AuthUserAddRequest{Name: userName, Options: &authpb.UserAddOptions{NoPassword: false}} _, err := as.UserAdd(ua) // add an existing user if err == nil { t.Fatalf("expected %v, got %v", ErrUserAlreadyExist, err) @@ -165,6 +167,11 @@ func TestUserAdd(t *testing.T) { if err != ErrUserEmpty { t.Fatal(err) } + + if _, ok := as.rangePermCache[userName]; !ok { + t.Fatalf("user %s should be added but it doesn't exist in rangePermCache", userName) + + } } func TestRecover(t *testing.T) { @@ -213,7 +220,8 @@ func TestUserDelete(t *testing.T) { defer tearDown(t) // delete an existing user - ud := &pb.AuthUserDeleteRequest{Name: "foo"} + const userName = "foo" + ud := &pb.AuthUserDeleteRequest{Name: userName} _, err := as.UserDelete(ud) if err != nil { t.Fatal(err) @@ -227,6 +235,47 @@ func TestUserDelete(t *testing.T) { if err != ErrUserNotFound { t.Fatalf("expected %v, got %v", ErrUserNotFound, err) } + + if _, ok := as.rangePermCache[userName]; ok { + t.Fatalf("user %s should be deleted but it exists in rangePermCache", userName) + + } +} + +func TestUserDeleteAndPermCache(t *testing.T) { + as, tearDown := setupAuthStore(t) + defer tearDown(t) + + // delete an existing user + const deletedUserName = "foo" + ud := &pb.AuthUserDeleteRequest{Name: deletedUserName} + _, err := as.UserDelete(ud) + if err != nil { + t.Fatal(err) + } + + // delete a non-existing user + _, err = as.UserDelete(ud) + if err != ErrUserNotFound { + t.Fatalf("expected %v, got %v", ErrUserNotFound, err) + } + + if _, ok := as.rangePermCache[deletedUserName]; ok { + t.Fatalf("user %s should be deleted but it exists in rangePermCache", deletedUserName) + } + + // add a new user + const newUser = "bar" + ua := &pb.AuthUserAddRequest{Name: newUser, Options: &authpb.UserAddOptions{NoPassword: false}} + _, err = as.UserAdd(ua) + if err != nil { + t.Fatal(err) + } + + if _, ok := as.rangePermCache[newUser]; !ok { + t.Fatalf("user %s should exist but it doesn't exist in rangePermCache", deletedUserName) + + } } func TestUserChangePassword(t *testing.T) { @@ -503,17 +552,44 @@ func TestUserRevokePermission(t *testing.T) { t.Fatal(err) } - _, err = as.UserGrantRole(&pb.AuthUserGrantRoleRequest{User: "foo", Role: "role-test"}) + const userName = "foo" + _, err = as.UserGrantRole(&pb.AuthUserGrantRoleRequest{User: userName, Role: "role-test"}) if err != nil { t.Fatal(err) } - _, err = as.UserGrantRole(&pb.AuthUserGrantRoleRequest{User: "foo", Role: "role-test-1"}) + _, err = as.UserGrantRole(&pb.AuthUserGrantRoleRequest{User: userName, Role: "role-test-1"}) if err != nil { t.Fatal(err) } - u, err := as.UserGet(&pb.AuthUserGetRequest{Name: "foo"}) + perm := &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte("WriteKeyBegin"), + RangeEnd: []byte("WriteKeyEnd"), + } + _, err = as.RoleGrantPermission(&pb.AuthRoleGrantPermissionRequest{ + Name: "role-test-1", + Perm: perm, + }) + if err != nil { + t.Fatal(err) + } + + if _, ok := as.rangePermCache[userName]; !ok { + t.Fatalf("User %s should have its entry in rangePermCache", userName) + } + unifiedPerm := as.rangePermCache[userName] + pt1 := adt.NewBytesAffinePoint([]byte("WriteKeyBegin")) + if !unifiedPerm.writePerms.Contains(pt1) { + t.Fatal("rangePermCache should contain WriteKeyBegin") + } + pt2 := adt.NewBytesAffinePoint([]byte("OutOfRange")) + if unifiedPerm.writePerms.Contains(pt2) { + t.Fatal("rangePermCache should not contain OutOfRange") + } + + u, err := as.UserGet(&pb.AuthUserGetRequest{Name: userName}) if err != nil { t.Fatal(err) } @@ -523,12 +599,12 @@ func TestUserRevokePermission(t *testing.T) { t.Fatalf("expected %v, got %v", expected, u.Roles) } - _, err = as.UserRevokeRole(&pb.AuthUserRevokeRoleRequest{Name: "foo", Role: "role-test-1"}) + _, err = as.UserRevokeRole(&pb.AuthUserRevokeRoleRequest{Name: userName, Role: "role-test-1"}) if err != nil { t.Fatal(err) } - u, err = as.UserGet(&pb.AuthUserGetRequest{Name: "foo"}) + u, err = as.UserGet(&pb.AuthUserGetRequest{Name: userName}) if err != nil { t.Fatal(err) }