server/auth: protect rangePermCache with a RW lock

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

server/auth/range_perm_cache.go

@@ -105,34 +105,48 @@ func checkKeyPoint(lg *zap.Logger, cachedPerms *unifiedRangePermissions, key []b
 	return false
 }
 
-func (as *authStore) isRangeOpPermitted(tx backend.ReadTx, userName string, key, rangeEnd []byte, permtyp authpb.Permission_Type) bool {
+func (as *authStore) isRangeOpPermitted(userName string, key, rangeEnd []byte, permtyp authpb.Permission_Type) bool {
-	// assumption: tx is Lock()ed
+	as.rangePermCacheMu.RLock()
-	_, ok := as.rangePermCache[userName]
+	defer as.rangePermCacheMu.RUnlock()
+
+	rangePerm, ok := as.rangePermCache[userName]
 	if !ok {
+		as.lg.Error(
+			"user doesn't exist",
+			zap.String("user-name", userName),
+		)
+		return false
+	}
+
+	if len(rangeEnd) == 0 {
+		return checkKeyPoint(as.lg, rangePerm, key, permtyp)
+	}
+
+	return checkKeyInterval(as.lg, rangePerm, key, rangeEnd, permtyp)
+}
+
+func (as *authStore) refreshRangePermCache(tx backend.ReadTx) {
+	// 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)
+
+	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),
 			)
-			return false
+			continue
 		}
 		as.rangePermCache[userName] = perms
 	}
-
-	if len(rangeEnd) == 0 {
-		return checkKeyPoint(as.lg, as.rangePermCache[userName], key, permtyp)
-	}
-
-	return checkKeyInterval(as.lg, as.rangePermCache[userName], key, rangeEnd, permtyp)
-}
-
-func (as *authStore) clearCachedPerm() {
-	as.rangePermCache = make(map[string]*unifiedRangePermissions)
-}
-
-func (as *authStore) invalidateCachedPerm(userName string) {
-	delete(as.rangePermCache, userName)
 }
 
 type unifiedRangePermissions struct {

server/auth/store.go

@@ -208,7 +208,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
 	bcryptCost    int // the algorithm cost / strength for hashing auth passwords
@@ -243,7 +250,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))
 
@@ -422,6 +429,7 @@ func (as *authStore) UserAdd(r *pb.AuthUserAddRequest) (*pb.AuthUserAddResponse,
 	putUser(as.lg, tx, newUser)
 
 	as.commitRevision(tx)
+	as.refreshRangePermCache(tx)
 
 	as.lg.Info("added a user", zap.String("user-name", r.Name))
 	return &pb.AuthUserAddResponse{}, nil
@@ -445,8 +453,8 @@ func (as *authStore) UserDelete(r *pb.AuthUserDeleteRequest) (*pb.AuthUserDelete
 	delUser(tx, r.Name)
 
 	as.commitRevision(tx)
+	as.refreshRangePermCache(tx)
 
-	as.invalidateCachedPerm(r.Name)
 	as.tokenProvider.invalidateUser(r.Name)
 
 	as.lg.Info(
@@ -487,8 +495,8 @@ func (as *authStore) UserChangePassword(r *pb.AuthUserChangePasswordRequest) (*p
 	putUser(as.lg, tx, updatedUser)
 
 	as.commitRevision(tx)
+	as.refreshRangePermCache(tx)
 
-	as.invalidateCachedPerm(r.Name)
 	as.tokenProvider.invalidateUser(r.Name)
 
 	as.lg.Info(
@@ -532,9 +540,8 @@ func (as *authStore) UserGrantRole(r *pb.AuthUserGrantRoleRequest) (*pb.AuthUser
 
 	putUser(as.lg, tx, user)
 
-	as.invalidateCachedPerm(r.User)
-
 	as.commitRevision(tx)
+	as.refreshRangePermCache(tx)
 
 	as.lg.Info(
 		"granted a role to a user",
@@ -610,9 +617,8 @@ func (as *authStore) UserRevokeRole(r *pb.AuthUserRevokeRoleRequest) (*pb.AuthUs
 
 	putUser(as.lg, tx, updatedUser)
 
-	as.invalidateCachedPerm(r.Name)
-
 	as.commitRevision(tx)
+	as.refreshRangePermCache(tx)
 
 	as.lg.Info(
 		"revoked a role from a user",
@@ -678,11 +684,8 @@ 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.refreshRangePermCache(tx)
 
 	as.lg.Info(
 		"revoked a permission on range",
@@ -730,10 +733,10 @@ func (as *authStore) RoleDelete(r *pb.AuthRoleDeleteRequest) (*pb.AuthRoleDelete
 
 		putUser(as.lg, tx, updatedUser)
 
-		as.invalidateCachedPerm(string(user.Name))
 	}
 
 	as.commitRevision(tx)
+	as.refreshRangePermCache(tx)
 
 	as.lg.Info("deleted a role", zap.String("role-name", r.Role))
 	return &pb.AuthRoleDeleteResponse{}, nil
@@ -818,11 +821,8 @@ 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.refreshRangePermCache(tx)
 
 	as.lg.Info(
 		"granted/updated a permission to a user",
@@ -867,7 +867,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
 	}
 
@@ -929,7 +929,15 @@ func getUser(lg *zap.Logger, tx backend.ReadTx, username string) *authpb.User {
 }
 
 func getAllUsers(lg *zap.Logger, tx backend.ReadTx) []*authpb.User {
-	_, vs := tx.UnsafeRange(buckets.AuthUsers, []byte{0}, []byte{0xff}, -1)
+	var vs [][]byte
+	err := tx.UnsafeForEach(buckets.AuthUsers, 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
 	}

server/auth/store_test.go

@@ -27,6 +27,7 @@ import (
 	"go.etcd.io/etcd/api/v3/authpb"
 	pb "go.etcd.io/etcd/api/v3/etcdserverpb"
 	"go.etcd.io/etcd/api/v3/v3rpc/rpctypes"
+	"go.etcd.io/etcd/pkg/v3/adt"
 	"go.etcd.io/etcd/server/v3/mvcc/backend"
 	betesting "go.etcd.io/etcd/server/v3/mvcc/backend/testing"
 
@@ -153,7 +154,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)
@@ -167,6 +169,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) {
@@ -216,7 +223,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)
@@ -230,6 +238,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, HashedPassword: encodePassword("pwd1"), 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) {
@@ -524,17 +573,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)
 	}
@@ -544,12 +620,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)
 	}