From 311f860676133e94c5fc8367bc447a46b09b948c Mon Sep 17 00:00:00 2001 From: SimFG Date: Fri, 30 Aug 2024 15:17:00 +0800 Subject: [PATCH] enhance: support to drop the role which is related the privilege list (#35727) - issue: #35545 Signed-off-by: SimFG --- go.mod | 2 +- go.sum | 4 +-- internal/metastore/kv/rootcoord/kv_catalog.go | 20 +++++++++++-- .../metastore/kv/rootcoord/kv_catalog_test.go | 15 +++++++--- internal/proxy/meta_cache.go | 6 ++++ internal/proxy/meta_cache_test.go | 10 +++++-- internal/rootcoord/root_coord.go | 28 +++++++++++++------ pkg/go.mod | 2 +- pkg/go.sum | 4 +-- pkg/util/funcutil/policy.go | 4 +++ pkg/util/funcutil/policy_test.go | 7 +++++ 11 files changed, 78 insertions(+), 24 deletions(-) diff --git a/go.mod b/go.mod index 5ea2a4f3c0c6a..d7e212bb4221e 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 github.com/klauspost/compress v1.17.7 github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d - github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240820032106-b34be93a2271 + github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240822040249-4bbc8f623cbb github.com/minio/minio-go/v7 v7.0.61 github.com/pingcap/log v1.1.1-0.20221015072633-39906604fb81 github.com/prometheus/client_golang v1.14.0 diff --git a/go.sum b/go.sum index 9aba4146accb7..28b297dd79e48 100644 --- a/go.sum +++ b/go.sum @@ -598,8 +598,8 @@ github.com/milvus-io/cgosymbolizer v0.0.0-20240722103217-b7dee0e50119 h1:9VXijWu github.com/milvus-io/cgosymbolizer v0.0.0-20240722103217-b7dee0e50119/go.mod h1:DvXTE/K/RtHehxU8/GtDs4vFtfw64jJ3PaCnFri8CRg= github.com/milvus-io/gorocksdb v0.0.0-20220624081344-8c5f4212846b h1:TfeY0NxYxZzUfIfYe5qYDBzt4ZYRqzUjTR6CvUzjat8= github.com/milvus-io/gorocksdb v0.0.0-20220624081344-8c5f4212846b/go.mod h1:iwW+9cWfIzzDseEBCCeDSN5SD16Tidvy8cwQ7ZY8Qj4= -github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240820032106-b34be93a2271 h1:YUWBgtRHmvkxMPTfOrY3FIq0K5XHw02Z18z7cyaMH04= -github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240820032106-b34be93a2271/go.mod h1:/6UT4zZl6awVeXLeE7UGDWZvXj3IWkRsh3mqsn0DiAs= +github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240822040249-4bbc8f623cbb h1:S3QIkNv9N1Vd1UKtdaQ4yVDPFAwFiPSAjN07axzbR70= +github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240822040249-4bbc8f623cbb/go.mod h1:/6UT4zZl6awVeXLeE7UGDWZvXj3IWkRsh3mqsn0DiAs= github.com/milvus-io/pulsar-client-go v0.6.10 h1:eqpJjU+/QX0iIhEo3nhOqMNXL+TyInAs1IAHZCrCM/A= github.com/milvus-io/pulsar-client-go v0.6.10/go.mod h1:lQqCkgwDF8YFYjKA+zOheTk1tev2B+bKj5j7+nm8M1w= github.com/minio/asm2plan9s v0.0.0-20200509001527-cdd76441f9d8 h1:AMFGa4R4MiIpspGNG7Z948v4n35fFGB3RR3G/ry4FWs= diff --git a/internal/metastore/kv/rootcoord/kv_catalog.go b/internal/metastore/kv/rootcoord/kv_catalog.go index 55ca2390268e5..8e8e762b1783d 100644 --- a/internal/metastore/kv/rootcoord/kv_catalog.go +++ b/internal/metastore/kv/rootcoord/kv_catalog.go @@ -1160,11 +1160,25 @@ func (kc *Catalog) ListGrant(ctx context.Context, tenant string, entity *milvusp func (kc *Catalog) DeleteGrant(ctx context.Context, tenant string, role *milvuspb.RoleEntity) error { var ( - k = funcutil.HandleTenantForEtcdKey(GranteePrefix, tenant, role.Name+"/") - err error + k = funcutil.HandleTenantForEtcdKey(GranteePrefix, tenant, role.Name+"/") + err error + removeKeys []string ) - if err = kc.Txn.RemoveWithPrefix(k); err != nil { + removeKeys = append(removeKeys, k) + + // the values are the grantee id list + _, values, err := kc.Txn.LoadWithPrefix(k) + if err != nil { + log.Warn("fail to load grant privilege entities", zap.String("key", k), zap.Error(err)) + return err + } + for _, v := range values { + granteeIDKey := funcutil.HandleTenantForEtcdKey(GranteeIDPrefix, tenant, v+"/") + removeKeys = append(removeKeys, granteeIDKey) + } + + if err = kc.Txn.MultiSaveAndRemoveWithPrefix(nil, removeKeys); err != nil { log.Error("fail to remove with the prefix", zap.String("key", k), zap.Error(err)) } return err diff --git a/internal/metastore/kv/rootcoord/kv_catalog_test.go b/internal/metastore/kv/rootcoord/kv_catalog_test.go index 69737f16931b2..b58010516aa14 100644 --- a/internal/metastore/kv/rootcoord/kv_catalog_test.go +++ b/internal/metastore/kv/rootcoord/kv_catalog_test.go @@ -2322,12 +2322,18 @@ func TestRBAC_Grant(t *testing.T) { kvmock = mocks.NewTxnKV(t) c = &Catalog{Txn: kvmock} - errorRole = "error-role" - errorRolePrefix = funcutil.HandleTenantForEtcdKey(GranteePrefix, tenant, errorRole+"/") + errorRole = "error-role" + errorRolePrefix = funcutil.HandleTenantForEtcdKey(GranteePrefix, tenant, errorRole+"/") + loadErrorRole = "load-error-role" + loadErrorRolePrefix = funcutil.HandleTenantForEtcdKey(GranteePrefix, tenant, loadErrorRole+"/") + granteeID = "123456" + granteePrefix = funcutil.HandleTenantForEtcdKey(GranteeIDPrefix, tenant, granteeID+"/") ) - kvmock.EXPECT().RemoveWithPrefix(errorRolePrefix).Call.Return(errors.New("mock removeWithPrefix error")) - kvmock.EXPECT().RemoveWithPrefix(mock.Anything).Call.Return(nil) + kvmock.EXPECT().LoadWithPrefix(loadErrorRolePrefix).Call.Return(nil, nil, errors.New("mock loadWithPrefix error")) + kvmock.EXPECT().LoadWithPrefix(mock.Anything).Call.Return(nil, []string{granteeID}, nil) + kvmock.EXPECT().MultiSaveAndRemoveWithPrefix(mock.Anything, []string{errorRolePrefix, granteePrefix}, mock.Anything).Call.Return(errors.New("mock removeWithPrefix error")) + kvmock.EXPECT().MultiSaveAndRemoveWithPrefix(mock.Anything, mock.Anything, mock.Anything).Call.Return(nil) tests := []struct { isValid bool @@ -2337,6 +2343,7 @@ func TestRBAC_Grant(t *testing.T) { }{ {true, "role1", "valid role1"}, {false, errorRole, "invalid errorRole"}, + {false, loadErrorRole, "invalid load errorRole"}, } for _, test := range tests { diff --git a/internal/proxy/meta_cache.go b/internal/proxy/meta_cache.go index 49bc8a4920dbb..593e19bbca238 100644 --- a/internal/proxy/meta_cache.go +++ b/internal/proxy/meta_cache.go @@ -1221,6 +1221,12 @@ func (m *MetaCache) RefreshPolicyInfo(op typeutil.CacheOp) (err error) { for user := range m.userToRoles { delete(m.userToRoles[user], op.OpKey) } + + for policy := range m.privilegeInfos { + if funcutil.PolicyCheckerWithRole(policy, op.OpKey) { + delete(m.privilegeInfos, policy) + } + } case typeutil.CacheRefresh: resp, err := m.rootCoord.ListPolicy(context.Background(), &internalpb.ListPolicyRequest{}) if err != nil { diff --git a/internal/proxy/meta_cache_test.go b/internal/proxy/meta_cache_test.go index ec8fe63c37d00..9ea940e92068d 100644 --- a/internal/proxy/meta_cache_test.go +++ b/internal/proxy/meta_cache_test.go @@ -691,9 +691,13 @@ func TestMetaCache_PolicyInfo(t *testing.T) { t.Run("Delete user or drop role", func(t *testing.T) { client.listPolicy = func(ctx context.Context, in *internalpb.ListPolicyRequest) (*internalpb.ListPolicyResponse, error) { return &internalpb.ListPolicyResponse{ - Status: merr.Success(), - PolicyInfos: []string{"policy1", "policy2", "policy3"}, - UserRoles: []string{funcutil.EncodeUserRoleCache("foo", "role1"), funcutil.EncodeUserRoleCache("foo", "role2"), funcutil.EncodeUserRoleCache("foo2", "role2"), funcutil.EncodeUserRoleCache("foo2", "role3")}, + Status: merr.Success(), + PolicyInfos: []string{ + funcutil.PolicyForPrivilege("role2", "Collection", "collection1", "read", "default"), + "policy2", + "policy3", + }, + UserRoles: []string{funcutil.EncodeUserRoleCache("foo", "role1"), funcutil.EncodeUserRoleCache("foo", "role2"), funcutil.EncodeUserRoleCache("foo2", "role2"), funcutil.EncodeUserRoleCache("foo2", "role3")}, }, nil } err := InitMetaCache(context.Background(), client, qc, mgr) diff --git a/internal/rootcoord/root_coord.go b/internal/rootcoord/root_coord.go index 4661dc8179ed3..d75ba5e23a97a 100644 --- a/internal/rootcoord/root_coord.go +++ b/internal/rootcoord/root_coord.go @@ -2249,13 +2249,15 @@ func (c *Core) DropRole(ctx context.Context, in *milvuspb.DropRoleRequest) (*com return merr.StatusWithErrorCode(errors.New(errMsg), commonpb.ErrorCode_DropRoleFailure), nil } - grantEntities, err := c.meta.SelectGrant(util.DefaultTenant, &milvuspb.GrantEntity{ - Role: &milvuspb.RoleEntity{Name: in.RoleName}, - }) - if len(grantEntities) != 0 { - errMsg := "fail to drop the role that it has privileges. Use REVOKE API to revoke privileges" - ctxLog.Warn(errMsg, zap.Any("grants", grantEntities), zap.Error(err)) - return merr.StatusWithErrorCode(errors.New(errMsg), commonpb.ErrorCode_DropRoleFailure), nil + if !in.ForceDrop { + grantEntities, err := c.meta.SelectGrant(util.DefaultTenant, &milvuspb.GrantEntity{ + Role: &milvuspb.RoleEntity{Name: in.RoleName}, + }) + if len(grantEntities) != 0 { + errMsg := "fail to drop the role that it has privileges. Use REVOKE API to revoke privileges" + ctxLog.Warn(errMsg, zap.Any("grants", grantEntities), zap.Error(err)) + return merr.StatusWithErrorCode(errors.New(errMsg), commonpb.ErrorCode_DropRoleFailure), nil + } } redoTask := newBaseRedoTask(c.stepExecutor) redoTask.AddSyncStep(NewSimpleStep("drop role meta data", func(ctx context.Context) ([]nestedStep, error) { @@ -2265,6 +2267,16 @@ func (c *Core) DropRole(ctx context.Context, in *milvuspb.DropRoleRequest) (*com } return nil, err })) + redoTask.AddAsyncStep(NewSimpleStep("drop the privilege list of this role", func(ctx context.Context) ([]nestedStep, error) { + if !in.ForceDrop { + return nil, nil + } + err := c.meta.DropGrant(util.DefaultTenant, &milvuspb.RoleEntity{Name: in.RoleName}) + if err != nil { + ctxLog.Warn("drop the privilege list failed for the role", zap.Error(err)) + } + return nil, err + })) redoTask.AddAsyncStep(NewSimpleStep("drop role cache", func(ctx context.Context) ([]nestedStep, error) { err := c.proxyClientManager.RefreshPolicyInfoCache(ctx, &proxypb.RefreshPolicyInfoCacheRequest{ OpType: int32(typeutil.CacheDropRole), @@ -2275,7 +2287,7 @@ func (c *Core) DropRole(ctx context.Context, in *milvuspb.DropRoleRequest) (*com } return nil, err })) - err = redoTask.Execute(ctx) + err := redoTask.Execute(ctx) if err != nil { errMsg := "fail to execute task when dropping the role" ctxLog.Warn(errMsg, zap.Error(err)) diff --git a/pkg/go.mod b/pkg/go.mod index 1171b67820d3e..0b9765782063e 100644 --- a/pkg/go.mod +++ b/pkg/go.mod @@ -13,7 +13,7 @@ require ( github.com/expr-lang/expr v1.15.7 github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 github.com/klauspost/compress v1.17.7 - github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240820032106-b34be93a2271 + github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240822040249-4bbc8f623cbb github.com/nats-io/nats-server/v2 v2.10.12 github.com/nats-io/nats.go v1.34.1 github.com/panjf2000/ants/v2 v2.7.2 diff --git a/pkg/go.sum b/pkg/go.sum index 45c207a36e8bf..242f315cdd565 100644 --- a/pkg/go.sum +++ b/pkg/go.sum @@ -494,8 +494,8 @@ github.com/milvus-io/cgosymbolizer v0.0.0-20240722103217-b7dee0e50119 h1:9VXijWu github.com/milvus-io/cgosymbolizer v0.0.0-20240722103217-b7dee0e50119/go.mod h1:DvXTE/K/RtHehxU8/GtDs4vFtfw64jJ3PaCnFri8CRg= github.com/milvus-io/gorocksdb v0.0.0-20220624081344-8c5f4212846b h1:TfeY0NxYxZzUfIfYe5qYDBzt4ZYRqzUjTR6CvUzjat8= github.com/milvus-io/gorocksdb v0.0.0-20220624081344-8c5f4212846b/go.mod h1:iwW+9cWfIzzDseEBCCeDSN5SD16Tidvy8cwQ7ZY8Qj4= -github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240820032106-b34be93a2271 h1:YUWBgtRHmvkxMPTfOrY3FIq0K5XHw02Z18z7cyaMH04= -github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240820032106-b34be93a2271/go.mod h1:/6UT4zZl6awVeXLeE7UGDWZvXj3IWkRsh3mqsn0DiAs= +github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240822040249-4bbc8f623cbb h1:S3QIkNv9N1Vd1UKtdaQ4yVDPFAwFiPSAjN07axzbR70= +github.com/milvus-io/milvus-proto/go-api/v2 v2.3.4-0.20240822040249-4bbc8f623cbb/go.mod h1:/6UT4zZl6awVeXLeE7UGDWZvXj3IWkRsh3mqsn0DiAs= github.com/milvus-io/pulsar-client-go v0.6.10 h1:eqpJjU+/QX0iIhEo3nhOqMNXL+TyInAs1IAHZCrCM/A= github.com/milvus-io/pulsar-client-go v0.6.10/go.mod h1:lQqCkgwDF8YFYjKA+zOheTk1tev2B+bKj5j7+nm8M1w= github.com/minio/highwayhash v1.0.2 h1:Aak5U0nElisjDCfPSG79Tgzkn2gl66NxOMspRrKnA/g= diff --git a/pkg/util/funcutil/policy.go b/pkg/util/funcutil/policy.go index fc0482b35c579..384d3a436f1b8 100644 --- a/pkg/util/funcutil/policy.go +++ b/pkg/util/funcutil/policy.go @@ -132,3 +132,7 @@ func SplitObjectName(objectName string) (string, string) { names := strings.Split(objectName, ".") return names[0], names[1] } + +func PolicyCheckerWithRole(policy, roleName string) bool { + return strings.Contains(policy, fmt.Sprintf(`"V0":"%s"`, roleName)) +} diff --git a/pkg/util/funcutil/policy_test.go b/pkg/util/funcutil/policy_test.go index 8659b82205561..006e465442b94 100644 --- a/pkg/util/funcutil/policy_test.go +++ b/pkg/util/funcutil/policy_test.go @@ -72,3 +72,10 @@ func Test_PolicyForResource(t *testing.T) { `COLLECTION-db.col1`, PolicyForResource("db", "COLLECTION", "col1")) } + +func Test_PolicyCheckerWithRole(t *testing.T) { + a := PolicyForPrivilege("admin", "COLLECTION", "col1", "ALL", "default") + b := PolicyForPrivilege("foo", "COLLECTION", "col1", "ALL", "default") + assert.True(t, PolicyCheckerWithRole(a, "admin")) + assert.False(t, PolicyCheckerWithRole(b, "admin")) +}