fix grant/revoke v2 meta and unclear error messages [milvus-io#38110]

Signed-off-by: shaoting-huang <[email protected]>

configs/milvus.yaml

@@ -19,7 +19,7 @@ etcd:
   # Endpoints used to access etcd service. You can change this parameter as the endpoints of your own etcd cluster.
   # Environment variable: ETCD_ENDPOINTS
   # etcd preferentially acquires valid address from environment variable ETCD_ENDPOINTS when Milvus is started.
-  endpoints: localhost:2379
+  endpoints: etcd:2379
   # Root prefix of the key to where Milvus stores data in etcd.
   # It is recommended to change this parameter before starting Milvus for the first time.
   # To share an etcd instance among multiple Milvus instances, consider changing this to a different value for each Milvus instance before you start them.
@@ -98,7 +98,7 @@ minio:
   # minio.address and minio.port together generate the valid access to MinIO or S3 service.
   # MinIO preferentially acquires the valid IP address from the environment variable MINIO_ADDRESS when Milvus is started.
   # Default value applies when MinIO or S3 is running on the same network with Milvus.
-  address: localhost
+  address: minio:9000
   port: 9000 # Port of MinIO or S3 service.
   # Access key ID that MinIO or S3 issues to user for authorized access.
   # Environment variable: MINIO_ACCESS_KEY_ID or minio.accessKeyID
@@ -178,7 +178,7 @@ pulsar:
   # pulsar.address and pulsar.port together generate the valid access to Pulsar.
   # Pulsar preferentially acquires the valid IP address from the environment variable PULSAR_ADDRESS when Milvus is started.
   # Default value applies when Pulsar is running on the same network with Milvus.
-  address: localhost
+  address: pulsar://pulsar:6650
   port: 6650 # Port of Pulsar service.
   webport: 80 # Web port of of Pulsar service. If you connect direcly without proxy, should use 8080.
   # The maximum size of each message in Pulsar. Unit: Byte.
@@ -806,7 +806,7 @@ common:
         readwrite:
           privileges: ListDatabases,SelectOwnership,SelectUser,DescribeResourceGroup,ListResourceGroups,FlushAll,TransferNode,TransferReplica,UpdateResourceGroups # Cluster level readwrite privileges
         admin:
-          privileges: ListDatabases,SelectOwnership,SelectUser,DescribeResourceGroup,ListResourceGroups,FlushAll,TransferNode,TransferReplica,UpdateResourceGroups,BackupRBAC,RestoreRBAC,CreateDatabase,DropDatabase,CreateOwnership,DropOwnership,ManageOwnership,CreateResourceGroup,DropResourceGroup,UpdateUser # Cluster level admin privileges
+          privileges: ListDatabases,SelectOwnership,SelectUser,DescribeResourceGroup,ListResourceGroups,FlushAll,TransferNode,TransferReplica,UpdateResourceGroups,BackupRBAC,RestoreRBAC,CreateDatabase,DropDatabase,CreateOwnership,DropOwnership,ManageOwnership,CreateResourceGroup,DropResourceGroup,UpdateUser,RenameCollection # Cluster level admin privileges
       database:
         readonly:
           privileges: ShowCollections,DescribeDatabase # Database level readonly privileges
@@ -818,9 +818,9 @@ common:
         readonly:
           privileges: Query,Search,IndexDetail,GetFlushState,GetLoadState,GetLoadingProgress,HasPartition,ShowPartitions,DescribeCollection,DescribeAlias,GetStatistics,ListAliases # Collection level readonly privileges
         readwrite:
-          privileges: Query,Search,IndexDetail,GetFlushState,GetLoadState,GetLoadingProgress,HasPartition,ShowPartitions,DescribeCollection,DescribeAlias,GetStatistics,ListAliases,Load,Release,Insert,Delete,Upsert,Import,Flush,Compaction,LoadBalance,RenameCollection,CreateIndex,DropIndex,CreatePartition,DropPartition # Collection level readwrite privileges
+          privileges: Query,Search,IndexDetail,GetFlushState,GetLoadState,GetLoadingProgress,HasPartition,ShowPartitions,DescribeCollection,DescribeAlias,GetStatistics,ListAliases,Load,Release,Insert,Delete,Upsert,Import,Flush,Compaction,LoadBalance,CreateIndex,DropIndex,CreatePartition,DropPartition # Collection level readwrite privileges
         admin:
-          privileges: Query,Search,IndexDetail,GetFlushState,GetLoadState,GetLoadingProgress,HasPartition,ShowPartitions,DescribeCollection,DescribeAlias,GetStatistics,ListAliases,Load,Release,Insert,Delete,Upsert,Import,Flush,Compaction,LoadBalance,RenameCollection,CreateIndex,DropIndex,CreatePartition,DropPartition,CreateAlias,DropAlias # Collection level admin privileges
+          privileges: Query,Search,IndexDetail,GetFlushState,GetLoadState,GetLoadingProgress,HasPartition,ShowPartitions,DescribeCollection,DescribeAlias,GetStatistics,ListAliases,Load,Release,Insert,Delete,Upsert,Import,Flush,Compaction,LoadBalance,CreateIndex,DropIndex,CreatePartition,DropPartition,CreateAlias,DropAlias # Collection level admin privileges
     tlsMode: 0
   session:
     ttl: 30 # ttl value when session granting a lease to register service

internal/proxy/impl.go

@@ -5254,7 +5254,7 @@ func (node *Proxy) validPrivilegeParams(req *milvuspb.OperatePrivilegeRequest) e
 	return nil
 }
 
-func (node *Proxy) validOperatePrivilegeV2Params(req *milvuspb.OperatePrivilegeV2Request) error {
+func (node *Proxy) validateOperatePrivilegeV2Params(req *milvuspb.OperatePrivilegeV2Request) error {
 	if req.Role == nil {
 		return fmt.Errorf("the role in the request is nil")
 	}
@@ -5267,8 +5267,10 @@ func (node *Proxy) validOperatePrivilegeV2Params(req *milvuspb.OperatePrivilegeV
 	if req.Type != milvuspb.OperatePrivilegeType_Grant && req.Type != milvuspb.OperatePrivilegeType_Revoke {
 		return fmt.Errorf("the type in the request not grant or revoke")
 	}
-	if err := ValidateObjectName(req.DbName); err != nil {
-		return err
+	if req.DbName != "" && !util.IsAnyWord(req.DbName) {
+		if err := ValidateDatabaseName(req.DbName); err != nil {
+			return err
+		}
 	}
 	if err := ValidateObjectName(req.CollectionName); err != nil {
 		return err
@@ -5287,7 +5289,7 @@ func (node *Proxy) OperatePrivilegeV2(ctx context.Context, req *milvuspb.Operate
 	if err := merr.CheckHealthy(node.GetStateCode()); err != nil {
 		return merr.Status(err), nil
 	}
-	if err := node.validOperatePrivilegeV2Params(req); err != nil {
+	if err := node.validateOperatePrivilegeV2Params(req); err != nil {
 		return merr.Status(err), nil
 	}
 	curUser, err := GetCurUserFromContext(ctx)

internal/proxy/util.go

@@ -157,25 +157,25 @@ func validateCollectionNameOrAlias(entity, entityType string) error {
 
 func ValidatePrivilegeGroupName(groupName string) error {
 	if groupName == "" {
-		return merr.WrapErrDatabaseNameInvalid(groupName, "privilege group name couldn't be empty")
+		return merr.WrapErrPrivilegeGroupNameInvalid("privilege group name should not be empty")
 	}
 
 	if len(groupName) > Params.ProxyCfg.MaxNameLength.GetAsInt() {
-		return merr.WrapErrDatabaseNameInvalid(groupName,
-			fmt.Sprintf("the length of a privilege group name must be less than %d characters", Params.ProxyCfg.MaxNameLength.GetAsInt()))
+		return merr.WrapErrPrivilegeGroupNameInvalid(
+			"the length of a privilege group name %s must be less than %s characters", groupName, Params.ProxyCfg.MaxNameLength.GetValue())
 	}
 
 	firstChar := groupName[0]
 	if firstChar != '_' && !isAlpha(firstChar) {
-		return merr.WrapErrDatabaseNameInvalid(groupName,
-			"the first character of a privilege group name must be an underscore or letter")
+		return merr.WrapErrPrivilegeGroupNameInvalid(
+			"the first character of a privilege group name %s must be an underscore or letter", groupName)
 	}
 
 	for i := 1; i < len(groupName); i++ {
 		c := groupName[i]
 		if c != '_' && !isAlpha(c) && !isNumber(c) {
-			return merr.WrapErrDatabaseNameInvalid(groupName,
-				"privilege group name can only contain numbers, letters and underscores")
+			return merr.WrapErrParameterInvalidMsg(
+				"privilege group name %s can only contain numbers, letters and underscores", groupName)
 		}
 	}
 	return nil
@@ -925,7 +925,7 @@ func ValidateObjectName(entity string) error {
 	if util.IsAnyWord(entity) {
 		return nil
 	}
-	return validateName(entity, "role name")
+	return validateName(entity, "object name")
 }
 
 func ValidateObjectType(entity string) error {

internal/rootcoord/meta_table.go

@@ -1524,6 +1524,10 @@ func (mt *MetaTable) OperatePrivilegeGroup(groupName string, privileges []*milvu
 	mt.permissionLock.Lock()
 	defer mt.permissionLock.Unlock()
 
+	if util.IsBuiltinPrivilegeGroup(groupName) {
+		return merr.WrapErrParameterInvalidMsg("the privilege group name [%s] is defined by built in privilege groups in system", groupName)
+	}
+
 	// validate input params
 	definedByUsers, err := mt.IsCustomPrivilegeGroup(groupName)
 	if err != nil {

internal/rootcoord/meta_table_test.go

@@ -2105,6 +2105,8 @@ func TestMetaTable_PrivilegeGroup(t *testing.T) {
 	assert.NoError(t, err)
 	err = mt.OperatePrivilegeGroup("", []*milvuspb.PrivilegeEntity{}, milvuspb.OperatePrivilegeGroupType_AddPrivilegesToGroup)
 	assert.Error(t, err)
+	err = mt.OperatePrivilegeGroup("ClusterReadOnly", []*milvuspb.PrivilegeEntity{}, milvuspb.OperatePrivilegeGroupType_AddPrivilegesToGroup)
+	assert.Error(t, err)
 	err = mt.OperatePrivilegeGroup("pg3", []*milvuspb.PrivilegeEntity{}, milvuspb.OperatePrivilegeGroupType_AddPrivilegesToGroup)
 	assert.Error(t, err)
 	_, err = mt.GetPrivilegeGroupRoles("")

internal/rootcoord/root_coord.go

@@ -2629,26 +2629,27 @@ func (c *Core) OperatePrivilege(ctx context.Context, in *milvuspb.OperatePrivile
 		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_OperatePrivilegeFailure), nil
 	}
 
+	privName := in.Entity.Grantor.Privilege.Name
 	switch in.Version {
 	case "v2":
-		if err := c.isValidPrivilegeV2(ctx, in.Entity.Grantor.Privilege.Name,
-			in.Entity.DbName, in.Entity.ObjectName); err != nil {
+		if err := c.isValidPrivilegeV2(ctx, privName, in.Entity.DbName, in.Entity.ObjectName); err != nil {
 			ctxLog.Error("", zap.Error(err))
 			return merr.StatusWithErrorCode(err, commonpb.ErrorCode_OperatePrivilegeFailure), nil
 		}
+		// set up object type for metastore, to be compatible with v1 version
+		in.Entity.Object.Name = util.GetObjectType(privName)
 	default:
-		if err := c.isValidPrivilege(ctx, in.Entity.Grantor.Privilege.Name, in.Entity.Object.Name); err != nil {
+		if err := c.isValidPrivilege(ctx, privName, in.Entity.Object.Name); err != nil {
 			ctxLog.Error("", zap.Error(err))
 			return merr.StatusWithErrorCode(err, commonpb.ErrorCode_OperatePrivilegeFailure), nil
 		}
 		// set up object name if it is global object type and not built in privilege group
-		if in.Entity.Object.Name == commonpb.ObjectType_Global.String() && !lo.Contains(lo.Keys(util.BuiltinPrivilegeGroups), in.Entity.Grantor.Privilege.Name) {
+		if in.Entity.Object.Name == commonpb.ObjectType_Global.String() && !util.IsBuiltinPrivilegeGroup(in.Entity.Grantor.Privilege.Name) {
 			in.Entity.ObjectName = util.AnyWord
 		}
 	}
 
-	// set up privilege name for metastore
-	privName := in.Entity.Grantor.Privilege.Name
+	privName = in.Entity.Grantor.Privilege.Name
 
 	redoTask := newBaseRedoTask(c.stepExecutor)
 	redoTask.AddSyncStep(NewSimpleStep("operate privilege meta data", func(ctx context.Context) ([]nestedStep, error) {
@@ -3087,12 +3088,12 @@ func (c *Core) CreatePrivilegeGroup(ctx context.Context, in *milvuspb.CreatePriv
 	ctxLog.Debug(method)
 
 	if err := merr.CheckHealthy(c.GetStateCode()); err != nil {
-		return merr.Status(err), nil
+		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_CreatePrivilegeGroupFailure), nil
 	}
 
 	if err := c.meta.CreatePrivilegeGroup(in.GroupName); err != nil {
 		ctxLog.Warn("fail to create privilege group", zap.Error(err))
-		return merr.Status(err), nil
+		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_CreatePrivilegeGroupFailure), nil
 	}
 
 	ctxLog.Debug(method + " success")
@@ -3110,12 +3111,12 @@ func (c *Core) DropPrivilegeGroup(ctx context.Context, in *milvuspb.DropPrivileg
 	ctxLog.Debug(method)
 
 	if err := merr.CheckHealthy(c.GetStateCode()); err != nil {
-		return merr.Status(err), nil
+		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_DropPrivilegeGroupFailure), nil
 	}
 
 	if err := c.meta.DropPrivilegeGroup(in.GroupName); err != nil {
 		ctxLog.Warn("fail to drop privilege group", zap.Error(err))
-		return merr.Status(err), nil
+		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_DropPrivilegeGroupFailure), nil
 	}
 
 	ctxLog.Debug(method + " success")
@@ -3302,8 +3303,7 @@ func (c *Core) OperatePrivilegeGroup(ctx context.Context, in *milvuspb.OperatePr
 	if err != nil {
 		errMsg := "fail to execute task when operate privilege group"
 		ctxLog.Warn(errMsg, zap.Error(err))
-		status := merr.StatusWithErrorCode(errors.New(errMsg), commonpb.ErrorCode_OperatePrivilegeGroupFailure)
-		return status, nil
+		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_OperatePrivilegeGroupFailure), nil
 	}
 
 	ctxLog.Debug(method + " success")
@@ -3319,13 +3319,17 @@ func (c *Core) expandPrivilegeGroups(grants []*milvuspb.GrantEntity, groups map[
 		if err != nil {
 			return nil, err
 		}
-		if objectType := util.GetObjectType(privilegeName); objectType != "" {
-			grant.Object.Name = objectType
+		objectType := &milvuspb.ObjectEntity{
+			Name: util.GetObjectType(privilegeName),
+		}
+		objectName := grant.ObjectName
+		if objectType.Name == commonpb.ObjectType_Global.String() {
+			objectName = util.AnyWord
 		}
 		return &milvuspb.GrantEntity{
 			Role:       grant.Role,
-			Object:     grant.Object,
-			ObjectName: grant.ObjectName,
+			Object:     objectType,
+			ObjectName: objectName,
 			Grantor: &milvuspb.GrantorEntity{
 				User: grant.Grantor.User,
 				Privilege: &milvuspb.PrivilegeEntity{
@@ -3338,20 +3342,16 @@ func (c *Core) expandPrivilegeGroups(grants []*milvuspb.GrantEntity, groups map[
 
 	for _, grant := range grants {
 		privName := grant.Grantor.Privilege.Name
-		if privGroup, exists := groups[privName]; !exists {
-			newGrant, err := createGrantEntity(grant, privName)
+		privGroup, exists := groups[privName]
+		if !exists {
+			privGroup = []*milvuspb.PrivilegeEntity{{Name: privName}}
+		}
+		for _, priv := range privGroup {
+			newGrant, err := createGrantEntity(grant, priv.Name)
 			if err != nil {
 				return nil, err
 			}
 			newGrants = append(newGrants, newGrant)
-		} else {
-			for _, priv := range privGroup {
-				newGrant, err := createGrantEntity(grant, priv.Name)
-				if err != nil {
-					return nil, err
-				}
-				newGrants = append(newGrants, newGrant)
-			}
 		}
 	}
 	// uniq by role + object + object name + grantor user + privilege name + db name

pkg/util/constant.go

@@ -332,7 +332,6 @@ var (
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeFlush.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeCompaction.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeLoadBalance.String()),
-		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeRenameCollection.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeCreateIndex.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeDropIndex.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeCreatePartition.String()),
@@ -384,6 +383,7 @@ var (
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeCreateResourceGroup.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeDropResourceGroup.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeUpdateUser.String()),
+		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeRenameCollection.String()),
 	)
 )
 
@@ -475,5 +475,5 @@ func GetObjectType(privName string) string {
 			return objectType
 		}
 	}
-	return ""
+	return commonpb.ObjectType_Global.String()
 }

pkg/util/merr/errors.go

@@ -147,7 +147,8 @@ var (
 	// this operation is denied because the user not authorized, user need to login in first
 	ErrPrivilegeNotAuthenticated = newMilvusError("not authenticated", 1400, false)
 	// this operation is denied because the user has no permission to do this, user need higher privilege
-	ErrPrivilegeNotPermitted = newMilvusError("privilege not permitted", 1401, false)
+	ErrPrivilegeNotPermitted     = newMilvusError("privilege not permitted", 1401, false)
+	ErrPrivilegeGroupInvalidName = newMilvusError("invalid privilege group name", 1402, false)
 
 	// Alias related
 	ErrAliasNotFound               = newMilvusError("alias not found", 1600, false)

pkg/util/merr/utils.go

@@ -455,6 +455,14 @@ func WrapErrDatabaseNameInvalid(database any, msg ...string) error {
 	return err
 }
 
+func WrapErrPrivilegeGroupNameInvalid(privilegeGroup any, msg ...string) error {
+	err := wrapFields(ErrPrivilegeGroupInvalidName, value("privielgeGroup", privilegeGroup))
+	if len(msg) > 0 {
+		err = errors.Wrap(err, strings.Join(msg, "->"))
+	}
+	return err
+}
+
 // Collection related
 func WrapErrCollectionNotFound(collection any, msg ...string) error {
 	err := wrapFields(ErrCollectionNotFound, value("collection", collection))