From 3cd74037db73cc78e4ec239db1022711259fb431 Mon Sep 17 00:00:00 2001 From: sthuang <167743503+shaoting-huang@users.noreply.github.com> Date: Mon, 13 Jan 2025 10:02:57 +0800 Subject: [PATCH] fix: restore rbac with empty meta panic (#39141) related: https://github.com/milvus-io/milvus/issues/38985 Signed-off-by: shaoting-huang --- internal/metastore/kv/rootcoord/kv_catalog.go | 34 +++++++++---------- internal/proxy/impl.go | 3 ++ tests/integration/rbac/rbac_backup_test.go | 6 +++- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/internal/metastore/kv/rootcoord/kv_catalog.go b/internal/metastore/kv/rootcoord/kv_catalog.go index b37a301ab8..6063cdf9d2 100644 --- a/internal/metastore/kv/rootcoord/kv_catalog.go +++ b/internal/metastore/kv/rootcoord/kv_catalog.go @@ -1489,7 +1489,7 @@ func (kc *Catalog) RestoreRBAC(ctx context.Context, tenant string, meta *milvusp log.Ctx(ctx).Warn("failed to restore rbac, try to rollback", zap.Error(err)) // roll back role for _, role := range needRollbackRole { - err = kc.DropRole(ctx, tenant, role.Name) + err = kc.DropRole(ctx, tenant, role.GetName()) if err != nil { log.Ctx(ctx).Warn("failed to rollback roles after restore failed", zap.Error(err)) } @@ -1505,7 +1505,7 @@ func (kc *Catalog) RestoreRBAC(ctx context.Context, tenant string, meta *milvusp for _, user := range needRollbackUser { // roll back user - err = kc.DropCredential(ctx, user.User) + err = kc.DropCredential(ctx, user.GetUser()) if err != nil { log.Ctx(ctx).Warn("failed to rollback users after restore failed", zap.Error(err)) } @@ -1513,7 +1513,7 @@ func (kc *Catalog) RestoreRBAC(ctx context.Context, tenant string, meta *milvusp // roll back privilege group for _, group := range needRollbackPrivilegeGroups { - err = kc.DropPrivilegeGroup(ctx, group.GroupName) + err = kc.DropPrivilegeGroup(ctx, group.GetGroupName()) if err != nil { log.Ctx(ctx).Warn("failed to rollback privilege groups after restore failed", zap.Error(err)) } @@ -1527,7 +1527,7 @@ func (kc *Catalog) RestoreRBAC(ctx context.Context, tenant string, meta *milvusp return err } existRoleMap := lo.SliceToMap(existRoles, func(entity *milvuspb.RoleResult) (string, struct{}) { return entity.GetRole().GetName(), struct{}{} }) - for _, role := range meta.Roles { + for _, role := range meta.GetRoles() { if _, ok := existRoleMap[role.GetName()]; ok { log.Ctx(ctx).Warn("failed to restore, role already exists", zap.String("role", role.GetName())) err = errors.Newf("role [%s] already exists", role.GetName()) @@ -1545,11 +1545,11 @@ func (kc *Catalog) RestoreRBAC(ctx context.Context, tenant string, meta *milvusp if err != nil { return err } - existPrivGroupMap := lo.SliceToMap(existPrivGroups, func(entity *milvuspb.PrivilegeGroupInfo) (string, struct{}) { return entity.GroupName, struct{}{} }) - for _, group := range meta.PrivilegeGroups { - if _, ok := existPrivGroupMap[group.GroupName]; ok { - log.Ctx(ctx).Warn("failed to restore, privilege group already exists", zap.String("group", group.GroupName)) - err = errors.Newf("privilege group [%s] already exists", group.GroupName) + existPrivGroupMap := lo.SliceToMap(existPrivGroups, func(entity *milvuspb.PrivilegeGroupInfo) (string, struct{}) { return entity.GetGroupName(), struct{}{} }) + for _, group := range meta.GetPrivilegeGroups() { + if _, ok := existPrivGroupMap[group.GetGroupName()]; ok { + log.Ctx(ctx).Warn("failed to restore, privilege group already exists", zap.String("group", group.GetGroupName())) + err = errors.Newf("privilege group [%s] already exists", group.GetGroupName()) return err } err = kc.SavePrivilegeGroup(ctx, group) @@ -1564,9 +1564,9 @@ func (kc *Catalog) RestoreRBAC(ctx context.Context, tenant string, meta *milvusp if err != nil { return err } - existPrivGroupMap = lo.SliceToMap(existPrivGroups, func(entity *milvuspb.PrivilegeGroupInfo) (string, struct{}) { return entity.GroupName, struct{}{} }) - for _, grant := range meta.Grants { - privName := grant.Grantor.Privilege.Name + existPrivGroupMap = lo.SliceToMap(existPrivGroups, func(entity *milvuspb.PrivilegeGroupInfo) (string, struct{}) { return entity.GetGroupName(), struct{}{} }) + for _, grant := range meta.GetGrants() { + privName := grant.GetGrantor().GetPrivilege().GetName() if util.IsPrivilegeNameDefined(privName) { grant.Grantor.Privilege.Name = util.PrivilegeNameForMetastore(privName) } else if _, ok := existPrivGroupMap[privName]; ok { @@ -1589,7 +1589,7 @@ func (kc *Catalog) RestoreRBAC(ctx context.Context, tenant string, meta *milvusp return err } existUserMap := lo.SliceToMap(existUser, func(entity *milvuspb.UserResult) (string, struct{}) { return entity.GetUser().GetName(), struct{}{} }) - for _, user := range meta.Users { + for _, user := range meta.GetUsers() { if _, ok := existUserMap[user.GetUser()]; ok { log.Ctx(ctx).Info("failed to restore, user already exists", zap.String("user", user.GetUser())) err = errors.Newf("user [%s] already exists", user.GetUser()) @@ -1597,8 +1597,8 @@ func (kc *Catalog) RestoreRBAC(ctx context.Context, tenant string, meta *milvusp } // restore user err = kc.CreateCredential(ctx, &model.Credential{ - Username: user.User, - EncryptedPassword: user.Password, + Username: user.GetUser(), + EncryptedPassword: user.GetPassword(), }) if err != nil { return err @@ -1607,9 +1607,9 @@ func (kc *Catalog) RestoreRBAC(ctx context.Context, tenant string, meta *milvusp // restore user role mapping entity := &milvuspb.UserEntity{ - Name: user.User, + Name: user.GetUser(), } - for _, role := range user.Roles { + for _, role := range user.GetRoles() { err = kc.AlterUserRole(ctx, tenant, entity, role, milvuspb.OperateUserRoleType_AddUserToRole) if err != nil { return err diff --git a/internal/proxy/impl.go b/internal/proxy/impl.go index e4734c2cf3..017fa52614 100644 --- a/internal/proxy/impl.go +++ b/internal/proxy/impl.go @@ -5656,6 +5656,9 @@ func (node *Proxy) RestoreRBAC(ctx context.Context, req *milvuspb.RestoreRBACMet if err := merr.CheckHealthy(node.GetStateCode()); err != nil { return merr.Status(err), nil } + if req.RBACMeta == nil { + return merr.Success(), nil + } result, err := node.rootCoord.RestoreRBAC(ctx, req) if err != nil { diff --git a/tests/integration/rbac/rbac_backup_test.go b/tests/integration/rbac/rbac_backup_test.go index 49e61ee4dc..ce526321da 100644 --- a/tests/integration/rbac/rbac_backup_test.go +++ b/tests/integration/rbac/rbac_backup_test.go @@ -154,8 +154,12 @@ func (s *RBACBackupTestSuite) TestBackup() { s.Equal(groupName, backupRBACResp.GetRBACMeta().PrivilegeGroups[0].GroupName) s.Equal(2, len(backupRBACResp.GetRBACMeta().PrivilegeGroups[0].Privileges)) + restoreRBACResp, err := s.Cluster.Proxy.RestoreRBAC(ctx, &milvuspb.RestoreRBACMetaRequest{}) + s.NoError(err) + s.True(merr.Ok(restoreRBACResp)) + // test restore, expect to failed due to role/user already exist - restoreRBACResp, err := s.Cluster.Proxy.RestoreRBAC(ctx, &milvuspb.RestoreRBACMetaRequest{ + restoreRBACResp, err = s.Cluster.Proxy.RestoreRBAC(ctx, &milvuspb.RestoreRBACMetaRequest{ RBACMeta: backupRBACResp.GetRBACMeta(), }) s.NoError(err)