From 9cd19f5396f5fe2f03d3cb9e5b4fada0715d7c7a Mon Sep 17 00:00:00 2001 From: SimFG Date: Fri, 2 Sep 2022 21:12:59 +0800 Subject: [PATCH] Return nil error when handling grpc request (#18955) Signed-off-by: SimFG Signed-off-by: SimFG --- internal/proxy/impl.go | 60 +++++----- internal/proxy/privilege_interceptor.go | 11 +- internal/proxy/proxy_test.go | 22 ++-- internal/rootcoord/root_coord.go | 148 ++++++++++++++---------- internal/util/funcutil/policy.go | 4 +- 5 files changed, 131 insertions(+), 114 deletions(-) diff --git a/internal/proxy/impl.go b/internal/proxy/impl.go index e016d9adf6..4ba2f8bfdb 100644 --- a/internal/proxy/impl.go +++ b/internal/proxy/impl.go @@ -3813,7 +3813,7 @@ func (node *Proxy) InvalidateCredentialCache(ctx context.Context, request *proxy zap.String("role", typeutil.ProxyRole), zap.String("username", request.Username)) if !node.checkHealthy() { - return unhealthyStatus(), errorutil.UnhealthyError() + return unhealthyStatus(), nil } username := request.Username @@ -3837,7 +3837,7 @@ func (node *Proxy) UpdateCredentialCache(ctx context.Context, request *proxypb.U zap.String("role", typeutil.ProxyRole), zap.String("username", request.Username)) if !node.checkHealthy() { - return unhealthyStatus(), errorutil.UnhealthyError() + return unhealthyStatus(), nil } credInfo := &internalpb.CredentialInfo{ @@ -3860,7 +3860,7 @@ func (node *Proxy) UpdateCredentialCache(ctx context.Context, request *proxypb.U func (node *Proxy) CreateCredential(ctx context.Context, req *milvuspb.CreateCredentialRequest) (*commonpb.Status, error) { log.Debug("CreateCredential", zap.String("role", typeutil.ProxyRole), zap.String("username", req.Username)) if !node.checkHealthy() { - return unhealthyStatus(), errorutil.UnhealthyError() + return unhealthyStatus(), nil } // validate params username := req.Username @@ -3913,7 +3913,7 @@ func (node *Proxy) CreateCredential(ctx context.Context, req *milvuspb.CreateCre func (node *Proxy) UpdateCredential(ctx context.Context, req *milvuspb.UpdateCredentialRequest) (*commonpb.Status, error) { log.Debug("UpdateCredential", zap.String("role", typeutil.ProxyRole), zap.String("username", req.Username)) if !node.checkHealthy() { - return unhealthyStatus(), errorutil.UnhealthyError() + return unhealthyStatus(), nil } rawOldPassword, err := crypto.Base64Decode(req.OldPassword) if err != nil { @@ -3974,7 +3974,7 @@ func (node *Proxy) UpdateCredential(ctx context.Context, req *milvuspb.UpdateCre func (node *Proxy) DeleteCredential(ctx context.Context, req *milvuspb.DeleteCredentialRequest) (*commonpb.Status, error) { log.Debug("DeleteCredential", zap.String("role", typeutil.ProxyRole), zap.String("username", req.Username)) if !node.checkHealthy() { - return unhealthyStatus(), errorutil.UnhealthyError() + return unhealthyStatus(), nil } if req.Username == util.UserRoot { @@ -3997,7 +3997,7 @@ func (node *Proxy) DeleteCredential(ctx context.Context, req *milvuspb.DeleteCre func (node *Proxy) ListCredUsers(ctx context.Context, req *milvuspb.ListCredUsersRequest) (*milvuspb.ListCredUsersResponse, error) { log.Debug("ListCredUsers", zap.String("role", typeutil.ProxyRole)) if !node.checkHealthy() { - return &milvuspb.ListCredUsersResponse{Status: unhealthyStatus()}, errorutil.UnhealthyError() + return &milvuspb.ListCredUsersResponse{Status: unhealthyStatus()}, nil } rootCoordReq := &milvuspb.ListCredUsersRequest{ Base: &commonpb.MsgBase{ @@ -4040,7 +4040,7 @@ func (node *Proxy) SendRetrieveResult(ctx context.Context, req *internalpb.Retri func (node *Proxy) CreateRole(ctx context.Context, req *milvuspb.CreateRoleRequest) (*commonpb.Status, error) { logger.Debug("CreateRole", zap.Any("req", req)) if code, ok := node.checkHealthyAndReturnCode(); !ok { - return errorutil.UnhealthyStatus(code), errorutil.UnhealthyError() + return errorutil.UnhealthyStatus(code), nil } var roleName string @@ -4051,7 +4051,7 @@ func (node *Proxy) CreateRole(ctx context.Context, req *milvuspb.CreateRoleReque return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_IllegalArgument, Reason: err.Error(), - }, err + }, nil } result, err := node.rootCoord.CreateRole(ctx, req) @@ -4060,7 +4060,7 @@ func (node *Proxy) CreateRole(ctx context.Context, req *milvuspb.CreateRoleReque return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_UnexpectedError, Reason: err.Error(), - }, err + }, nil } return result, nil } @@ -4068,20 +4068,20 @@ func (node *Proxy) CreateRole(ctx context.Context, req *milvuspb.CreateRoleReque func (node *Proxy) DropRole(ctx context.Context, req *milvuspb.DropRoleRequest) (*commonpb.Status, error) { logger.Debug("DropRole", zap.Any("req", req)) if code, ok := node.checkHealthyAndReturnCode(); !ok { - return errorutil.UnhealthyStatus(code), errorutil.UnhealthyError() + return errorutil.UnhealthyStatus(code), nil } if err := ValidateRoleName(req.RoleName); err != nil { return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_IllegalArgument, Reason: err.Error(), - }, err + }, nil } if IsDefaultRole(req.RoleName) { errMsg := fmt.Sprintf("the role[%s] is a default role, which can't be droped", req.RoleName) return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_IllegalArgument, Reason: errMsg, - }, errors.New(errMsg) + }, nil } result, err := node.rootCoord.DropRole(ctx, req) if err != nil { @@ -4089,7 +4089,7 @@ func (node *Proxy) DropRole(ctx context.Context, req *milvuspb.DropRoleRequest) return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_UnexpectedError, Reason: err.Error(), - }, err + }, nil } return result, nil } @@ -4097,19 +4097,19 @@ func (node *Proxy) DropRole(ctx context.Context, req *milvuspb.DropRoleRequest) func (node *Proxy) OperateUserRole(ctx context.Context, req *milvuspb.OperateUserRoleRequest) (*commonpb.Status, error) { logger.Debug("OperateUserRole", zap.Any("req", req)) if code, ok := node.checkHealthyAndReturnCode(); !ok { - return errorutil.UnhealthyStatus(code), errorutil.UnhealthyError() + return errorutil.UnhealthyStatus(code), nil } if err := ValidateUsername(req.Username); err != nil { return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_IllegalArgument, Reason: err.Error(), - }, err + }, nil } if err := ValidateRoleName(req.RoleName); err != nil { return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_IllegalArgument, Reason: err.Error(), - }, err + }, nil } result, err := node.rootCoord.OperateUserRole(ctx, req) @@ -4118,7 +4118,7 @@ func (node *Proxy) OperateUserRole(ctx context.Context, req *milvuspb.OperateUse return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_UnexpectedError, Reason: err.Error(), - }, err + }, nil } return result, nil } @@ -4126,7 +4126,7 @@ func (node *Proxy) OperateUserRole(ctx context.Context, req *milvuspb.OperateUse func (node *Proxy) SelectRole(ctx context.Context, req *milvuspb.SelectRoleRequest) (*milvuspb.SelectRoleResponse, error) { logger.Debug("SelectRole", zap.Any("req", req)) if code, ok := node.checkHealthyAndReturnCode(); !ok { - return &milvuspb.SelectRoleResponse{Status: errorutil.UnhealthyStatus(code)}, errorutil.UnhealthyError() + return &milvuspb.SelectRoleResponse{Status: errorutil.UnhealthyStatus(code)}, nil } if req.Role != nil { @@ -4136,7 +4136,7 @@ func (node *Proxy) SelectRole(ctx context.Context, req *milvuspb.SelectRoleReque ErrorCode: commonpb.ErrorCode_IllegalArgument, Reason: err.Error(), }, - }, err + }, nil } } @@ -4148,7 +4148,7 @@ func (node *Proxy) SelectRole(ctx context.Context, req *milvuspb.SelectRoleReque ErrorCode: commonpb.ErrorCode_UnexpectedError, Reason: err.Error(), }, - }, err + }, nil } return result, nil } @@ -4156,7 +4156,7 @@ func (node *Proxy) SelectRole(ctx context.Context, req *milvuspb.SelectRoleReque func (node *Proxy) SelectUser(ctx context.Context, req *milvuspb.SelectUserRequest) (*milvuspb.SelectUserResponse, error) { logger.Debug("SelectUser", zap.Any("req", req)) if code, ok := node.checkHealthyAndReturnCode(); !ok { - return &milvuspb.SelectUserResponse{Status: errorutil.UnhealthyStatus(code)}, errorutil.UnhealthyError() + return &milvuspb.SelectUserResponse{Status: errorutil.UnhealthyStatus(code)}, nil } if req.User != nil { @@ -4166,7 +4166,7 @@ func (node *Proxy) SelectUser(ctx context.Context, req *milvuspb.SelectUserReque ErrorCode: commonpb.ErrorCode_IllegalArgument, Reason: err.Error(), }, - }, err + }, nil } } @@ -4178,7 +4178,7 @@ func (node *Proxy) SelectUser(ctx context.Context, req *milvuspb.SelectUserReque ErrorCode: commonpb.ErrorCode_UnexpectedError, Reason: err.Error(), }, - }, err + }, nil } return result, nil } @@ -4218,20 +4218,20 @@ func (node *Proxy) validPrivilegeParams(req *milvuspb.OperatePrivilegeRequest) e func (node *Proxy) OperatePrivilege(ctx context.Context, req *milvuspb.OperatePrivilegeRequest) (*commonpb.Status, error) { logger.Debug("OperatePrivilege", zap.Any("req", req)) if code, ok := node.checkHealthyAndReturnCode(); !ok { - return errorutil.UnhealthyStatus(code), errorutil.UnhealthyError() + return errorutil.UnhealthyStatus(code), nil } if err := node.validPrivilegeParams(req); err != nil { return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_IllegalArgument, Reason: err.Error(), - }, err + }, nil } curUser, err := GetCurUserFromContext(ctx) if err != nil { return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_UnexpectedError, Reason: err.Error(), - }, err + }, nil } req.Entity.Grantor.User = &milvuspb.UserEntity{Name: curUser} result, err := node.rootCoord.OperatePrivilege(ctx, req) @@ -4240,7 +4240,7 @@ func (node *Proxy) OperatePrivilege(ctx context.Context, req *milvuspb.OperatePr return &commonpb.Status{ ErrorCode: commonpb.ErrorCode_UnexpectedError, Reason: err.Error(), - }, err + }, nil } return result, nil } @@ -4274,7 +4274,7 @@ func (node *Proxy) validGrantParams(req *milvuspb.SelectGrantRequest) error { func (node *Proxy) SelectGrant(ctx context.Context, req *milvuspb.SelectGrantRequest) (*milvuspb.SelectGrantResponse, error) { logger.Debug("SelectGrant", zap.Any("req", req)) if code, ok := node.checkHealthyAndReturnCode(); !ok { - return &milvuspb.SelectGrantResponse{Status: errorutil.UnhealthyStatus(code)}, errorutil.UnhealthyError() + return &milvuspb.SelectGrantResponse{Status: errorutil.UnhealthyStatus(code)}, nil } if err := node.validGrantParams(req); err != nil { @@ -4283,7 +4283,7 @@ func (node *Proxy) SelectGrant(ctx context.Context, req *milvuspb.SelectGrantReq ErrorCode: commonpb.ErrorCode_IllegalArgument, Reason: err.Error(), }, - }, err + }, nil } result, err := node.rootCoord.SelectGrant(ctx, req) @@ -4294,7 +4294,7 @@ func (node *Proxy) SelectGrant(ctx context.Context, req *milvuspb.SelectGrantReq ErrorCode: commonpb.ErrorCode_UnexpectedError, Reason: err.Error(), }, - }, err + }, nil } return result, nil } diff --git a/internal/proxy/privilege_interceptor.go b/internal/proxy/privilege_interceptor.go index e1f818e5c2..9842435f16 100644 --- a/internal/proxy/privilege_interceptor.go +++ b/internal/proxy/privilege_interceptor.go @@ -6,6 +6,9 @@ import ( "reflect" "strings" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "github.com/milvus-io/milvus/internal/proto/commonpb" "github.com/milvus-io/milvus/internal/util" @@ -161,13 +164,7 @@ func PrivilegeInterceptor(ctx context.Context, req interface{}) (context.Context } log.Debug("permission deny", zap.String("policy", policy), zap.Strings("roles", roleNames)) - return ctx, &ErrPermissionDenied{} -} - -type ErrPermissionDenied struct{} - -func (e *ErrPermissionDenied) Error() string { - return "permission deny" + return ctx, status.Error(codes.PermissionDenied, "permission deny") } // isCurUserObject Determine whether it is an Object of type User that operates on its own user information, diff --git a/internal/proxy/proxy_test.go b/internal/proxy/proxy_test.go index 747ddba65a..0f3fcf166b 100644 --- a/internal/proxy/proxy_test.go +++ b/internal/proxy/proxy_test.go @@ -2604,7 +2604,7 @@ func TestProxy(t *testing.T) { t.Run("InvalidateCredCache fail, unhealthy", func(t *testing.T) { defer wg.Done() resp, err := proxy.InvalidateCredentialCache(ctx, &proxypb.InvalidateCredCacheRequest{Username: "xxx"}) - assert.Error(t, err) + assert.NoError(t, err) assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode) }) @@ -2612,7 +2612,7 @@ func TestProxy(t *testing.T) { t.Run("UpdateCredentialCache fail, unhealthy", func(t *testing.T) { defer wg.Done() resp, err := proxy.UpdateCredentialCache(ctx, &proxypb.UpdateCredCacheRequest{Username: "xxx", Password: "xxx"}) - assert.Error(t, err) + assert.NoError(t, err) assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode) }) @@ -2620,7 +2620,7 @@ func TestProxy(t *testing.T) { t.Run("CreateCredential fail, unhealthy", func(t *testing.T) { defer wg.Done() resp, err := proxy.CreateCredential(ctx, &milvuspb.CreateCredentialRequest{Username: "xxx"}) - assert.Error(t, err) + assert.NoError(t, err) assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode) }) @@ -2628,7 +2628,7 @@ func TestProxy(t *testing.T) { t.Run("UpdateCredential fail, unhealthy", func(t *testing.T) { defer wg.Done() resp, err := proxy.UpdateCredential(ctx, &milvuspb.UpdateCredentialRequest{Username: "xxx"}) - assert.Error(t, err) + assert.NoError(t, err) assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode) }) @@ -2636,7 +2636,7 @@ func TestProxy(t *testing.T) { t.Run("DeleteCredential fail, unhealthy", func(t *testing.T) { defer wg.Done() resp, err := proxy.DeleteCredential(ctx, &milvuspb.DeleteCredentialRequest{Username: "xxx"}) - assert.Error(t, err) + assert.NoError(t, err) assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode) }) @@ -2644,7 +2644,7 @@ func TestProxy(t *testing.T) { t.Run("ListCredUsers fail, unhealthy", func(t *testing.T) { defer wg.Done() resp, err := proxy.ListCredUsers(ctx, &milvuspb.ListCredUsersRequest{}) - assert.Error(t, err) + assert.NoError(t, err) assert.NotEqual(t, commonpb.ErrorCode_Success, resp.Status.ErrorCode) }) @@ -3274,10 +3274,10 @@ func testProxyRole(ctx context.Context, t *testing.T, proxy *Proxy) { t.Run("Select Role", func(t *testing.T) { defer wg.Done() - _, err := proxy.SelectRole(ctx, &milvuspb.SelectRoleRequest{Role: &milvuspb.RoleEntity{Name: " "}}) - assert.Error(t, err) + resp, _ := proxy.SelectRole(ctx, &milvuspb.SelectRoleRequest{Role: &milvuspb.RoleEntity{Name: " "}}) + assert.NotEqual(t, commonpb.ErrorCode_Success, resp.Status.ErrorCode) - resp, _ := proxy.SelectRole(ctx, &milvuspb.SelectRoleRequest{}) + resp, _ = proxy.SelectRole(ctx, &milvuspb.SelectRoleRequest{}) assert.Equal(t, commonpb.ErrorCode_Success, resp.Status.ErrorCode) roleNum := len(resp.Results) @@ -3436,8 +3436,8 @@ func testProxyPrivilege(ctx context.Context, t *testing.T, proxy *Proxy) { assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode) req.Entity.Role = &milvuspb.RoleEntity{Name: "admin"} - _, err := proxy.OperatePrivilege(context.Background(), req) - assert.Error(t, err) + resp, _ = proxy.OperatePrivilege(context.Background(), req) + assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode) resp, _ = proxy.OperatePrivilege(ctx, req) assert.Equal(t, commonpb.ErrorCode_Success, resp.ErrorCode) diff --git a/internal/rootcoord/root_coord.go b/internal/rootcoord/root_coord.go index ea498267e5..2b2b9bb08d 100644 --- a/internal/rootcoord/root_coord.go +++ b/internal/rootcoord/root_coord.go @@ -2480,8 +2480,9 @@ func (c *Core) CreateRole(ctx context.Context, in *milvuspb.CreateRoleRequest) ( err := c.MetaTable.CreateRole(util.DefaultTenant, &milvuspb.RoleEntity{Name: entity.Name}) if err != nil { - logger.Error("fail to create role", zap.String("role_name", entity.Name), zap.Error(err)) - return failStatus(commonpb.ErrorCode_CreateRoleFailure, "CreateCollection role failed: "+err.Error()), err + errMsg := "fail to create role" + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_CreateRoleFailure, errMsg), nil } logger.Debug(method+" success", zap.String("role_name", entity.Name)) @@ -2509,8 +2510,9 @@ func (c *Core) DropRole(ctx context.Context, in *milvuspb.DropRoleRequest) (*com return errorutil.UnhealthyStatus(code), errorutil.UnhealthyError() } if _, err := c.MetaTable.SelectRole(util.DefaultTenant, &milvuspb.RoleEntity{Name: in.RoleName}, false); err != nil { - logger.Error("the role isn't existed", zap.String("role_name", in.RoleName), zap.Error(err)) - return failStatus(commonpb.ErrorCode_DropRoleFailure, fmt.Sprintf("the role isn't existed, role name: %s", in.RoleName)), err + errMsg := "the role isn't existed" + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_DropRoleFailure, errMsg), nil } grantEntities, err := c.MetaTable.SelectGrant(util.DefaultTenant, &milvuspb.GrantEntity{ @@ -2518,37 +2520,39 @@ func (c *Core) DropRole(ctx context.Context, in *milvuspb.DropRoleRequest) (*com }) if len(grantEntities) != 0 { errMsg := "fail to drop the role that it has privileges. Use REVOKE API to revoke privileges" - logger.Error(errMsg, zap.String("role_name", in.RoleName), zap.Error(err)) - return failStatus(commonpb.ErrorCode_DropRoleFailure, errMsg), errors.New(errMsg) + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_DropRoleFailure, errMsg), nil } roleResults, err := c.MetaTable.SelectRole(util.DefaultTenant, &milvuspb.RoleEntity{Name: in.RoleName}, true) if err != nil { errMsg := "fail to select a role by role name" - logger.Error("fail to select a role by role name", zap.String("role_name", in.RoleName), zap.Error(err)) - return failStatus(commonpb.ErrorCode_DropRoleFailure, errMsg), err + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_DropRoleFailure, errMsg), nil } logger.Debug("role to user info", zap.Int("counter", len(roleResults))) for _, roleResult := range roleResults { for index, userEntity := range roleResult.Users { - if err = c.MetaTable.OperateUserRole(util.DefaultTenant, &milvuspb.UserEntity{Name: userEntity.Name}, &milvuspb.RoleEntity{Name: roleResult.Role.Name}, milvuspb.OperateUserRoleType_RemoveUserFromRole); err != nil { + if err = c.MetaTable.OperateUserRole(util.DefaultTenant, + &milvuspb.UserEntity{Name: userEntity.Name}, + &milvuspb.RoleEntity{Name: roleResult.Role.Name}, milvuspb.OperateUserRoleType_RemoveUserFromRole); err != nil { if common.IsIgnorableError(err) { continue } errMsg := "fail to remove user from role" - logger.Error(errMsg, zap.String("role_name", roleResult.Role.Name), zap.String("username", userEntity.Name), zap.Int("current_index", index), zap.Error(err)) - return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, errMsg), err + log.Error(errMsg, zap.Any("in", in), zap.String("role_name", roleResult.Role.Name), zap.String("username", userEntity.Name), zap.Int("current_index", index), zap.Error(err)) + return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, errMsg), nil } } } if err = c.MetaTable.DropGrant(util.DefaultTenant, &milvuspb.RoleEntity{Name: in.RoleName}); err != nil { errMsg := "fail to drop the grant" - logger.Error(errMsg, zap.String("role_name", in.RoleName), zap.Error(err)) - return failStatus(commonpb.ErrorCode_DropRoleFailure, errMsg), err + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_DropRoleFailure, errMsg), nil } if err = c.MetaTable.DropRole(util.DefaultTenant, in.RoleName); err != nil { errMsg := "fail to drop the role" - logger.Error(errMsg, zap.String("role_name", in.RoleName), zap.Error(err)) - return failStatus(commonpb.ErrorCode_DropRoleFailure, errMsg), err + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_DropRoleFailure, errMsg), nil } logger.Debug(method+" success", zap.String("role_name", in.RoleName)) @@ -2576,20 +2580,20 @@ func (c *Core) OperateUserRole(ctx context.Context, in *milvuspb.OperateUserRole if _, err := c.MetaTable.SelectRole(util.DefaultTenant, &milvuspb.RoleEntity{Name: in.RoleName}, false); err != nil { errMsg := "fail to check the role name" - logger.Error(errMsg, zap.String("role_name", in.RoleName), zap.Error(err)) - return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, errMsg), err + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, errMsg), nil } if _, err := c.MetaTable.SelectUser(util.DefaultTenant, &milvuspb.UserEntity{Name: in.Username}, false); err != nil { errMsg := "fail to check the username" - logger.Error(errMsg, zap.String("username", in.Username), zap.Error(err)) - return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, errMsg), err + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, errMsg), nil } updateCache := true if err := c.MetaTable.OperateUserRole(util.DefaultTenant, &milvuspb.UserEntity{Name: in.Username}, &milvuspb.RoleEntity{Name: in.RoleName}, in.Type); err != nil { if !common.IsIgnorableError(err) { errMsg := "fail to operate user to role" - logger.Error(errMsg, zap.String("role_name", in.RoleName), zap.String("username", in.Username), zap.Error(err)) - return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, errMsg), err + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, errMsg), nil } updateCache = false } @@ -2603,14 +2607,16 @@ func (c *Core) OperateUserRole(ctx context.Context, in *milvuspb.OperateUserRole opType = int32(typeutil.CacheRemoveUserFromRole) default: errMsg := "invalid operate type for the OperateUserRole api" - logger.Error(errMsg, zap.Any("op_type", in.Type)) - return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, errMsg), errors.New(errMsg) + log.Error(errMsg, zap.Any("in", in)) + return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, errMsg), nil } if err := c.proxyClientManager.RefreshPolicyInfoCache(ctx, &proxypb.RefreshPolicyInfoCacheRequest{ OpType: opType, OpKey: funcutil.EncodeUserRoleCache(in.Username, in.RoleName), }); err != nil { - return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, err.Error()), err + errMsg := "fail to refresh policy info cache" + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_OperateUserRoleFailure, errMsg), nil } } @@ -2636,25 +2642,25 @@ func (c *Core) SelectRole(ctx context.Context, in *milvuspb.SelectRoleRequest) ( if in.Role != nil { if _, err := c.MetaTable.SelectRole(util.DefaultTenant, &milvuspb.RoleEntity{Name: in.Role.Name}, false); err != nil { - errMsg := "fail to select the role to check the role name" - logger.Error(errMsg, zap.String("role_name", in.Role.Name), zap.Error(err)) if common.IsKeyNotExistError(err) { return &milvuspb.SelectRoleResponse{ Status: succStatus(), }, nil } + errMsg := "fail to select the role to check the role name" + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) return &milvuspb.SelectRoleResponse{ Status: failStatus(commonpb.ErrorCode_SelectRoleFailure, errMsg), - }, err + }, nil } } roleResults, err := c.MetaTable.SelectRole(util.DefaultTenant, in.Role, in.IncludeUserInfo) if err != nil { errMsg := "fail to select the role" - logger.Error(errMsg, zap.Error(err)) + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) return &milvuspb.SelectRoleResponse{ Status: failStatus(commonpb.ErrorCode_SelectRoleFailure, errMsg), - }, err + }, nil } logger.Debug(method + " success") @@ -2682,25 +2688,25 @@ func (c *Core) SelectUser(ctx context.Context, in *milvuspb.SelectUserRequest) ( if in.User != nil { if _, err := c.MetaTable.SelectUser(util.DefaultTenant, &milvuspb.UserEntity{Name: in.User.Name}, false); err != nil { - errMsg := "fail to select the user to check the username" - logger.Error(errMsg, zap.String("username", in.User.Name), zap.Error(err)) if common.IsKeyNotExistError(err) { return &milvuspb.SelectUserResponse{ Status: succStatus(), }, nil } + errMsg := "fail to select the user to check the username" + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) return &milvuspb.SelectUserResponse{ Status: failStatus(commonpb.ErrorCode_SelectUserFailure, errMsg), - }, err + }, nil } } userResults, err := c.MetaTable.SelectUser(util.DefaultTenant, in.User, in.IncludeRoleInfo) if err != nil { errMsg := "fail to select the user" - log.Error(errMsg, zap.Error(err)) + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) return &milvuspb.SelectUserResponse{ Status: failStatus(commonpb.ErrorCode_SelectUserFailure, errMsg), - }, err + }, nil } logger.Debug(method + " success") @@ -2714,10 +2720,10 @@ func (c *Core) SelectUser(ctx context.Context, in *milvuspb.SelectUserRequest) ( func (c *Core) isValidRole(entity *milvuspb.RoleEntity) error { if entity == nil { - return fmt.Errorf("the role entity is nil") + return errors.New("the role entity is nil") } if entity.Name == "" { - return fmt.Errorf("the name in the role entity is empty") + return errors.New("the name in the role entity is empty") } if _, err := c.MetaTable.SelectRole(util.DefaultTenant, &milvuspb.RoleEntity{Name: entity.Name}, false); err != nil { return err @@ -2727,46 +2733,46 @@ func (c *Core) isValidRole(entity *milvuspb.RoleEntity) error { func (c *Core) isValidObject(entity *milvuspb.ObjectEntity) error { if entity == nil { - return fmt.Errorf("the object entity is nil") + return errors.New("the object entity is nil") } if _, ok := commonpb.ObjectType_value[entity.Name]; !ok { - return fmt.Errorf("the object type in the object entity is invalid, current value: %s", entity.Name) + return fmt.Errorf("the object type in the object entity[name: %s] is invalid", entity.Name) } return nil } func (c *Core) isValidGrantor(entity *milvuspb.GrantorEntity, object string) error { if entity == nil { - return fmt.Errorf("the grantor entity is nil") + return errors.New("the grantor entity is nil") } if entity.User == nil { - return fmt.Errorf("the user entity in the grantor entity is nil") + return errors.New("the user entity in the grantor entity is nil") } if entity.User.Name == "" { - return fmt.Errorf("the name in the user entity of the grantor entity is empty") + return errors.New("the name in the user entity of the grantor entity is empty") } - if _, err := c.MetaTable.SelectUser(util.DefaultTenant, &milvuspb.UserEntity{Name: entity.GetUser().Name}, false); err != nil { + if _, err := c.MetaTable.SelectUser(util.DefaultTenant, &milvuspb.UserEntity{Name: entity.User.Name}, false); err != nil { return err } if entity.Privilege == nil { - return fmt.Errorf("the privilege entity in the grantor entity is nil") + return errors.New("the privilege entity in the grantor entity is nil") } if util.IsAnyWord(entity.Privilege.Name) { return nil } if privilegeName := util.PrivilegeNameForMetastore(entity.Privilege.Name); privilegeName == "" { - return fmt.Errorf("the privilege name in the privilege entity is invalid, current value: %s", entity.Privilege.Name) + return fmt.Errorf("the privilege name[%s] in the privilege entity is invalid", entity.Privilege.Name) } privileges, ok := util.ObjectPrivileges[object] if !ok { - return fmt.Errorf("the object type is invalid, current value: %s", object) + return fmt.Errorf("the object type[%s] is invalid", object) } for _, privilege := range privileges { if privilege == entity.Privilege.Name { return nil } } - return fmt.Errorf("the privilege name is invalid, current value: %s", entity.Privilege.Name) + return fmt.Errorf("the privilege name[%s] is invalid", entity.Privilege.Name) } // OperatePrivilege operate the privilege, including grant and revoke @@ -2787,20 +2793,25 @@ func (c *Core) OperatePrivilege(ctx context.Context, in *milvuspb.OperatePrivile } if in.Type != milvuspb.OperatePrivilegeType_Grant && in.Type != milvuspb.OperatePrivilegeType_Revoke { errMsg := fmt.Sprintf("invalid operate privilege type, current type: %s, valid value: [%s, %s]", in.Type, milvuspb.OperatePrivilegeType_Grant, milvuspb.OperatePrivilegeType_Revoke) - return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, errMsg), errors.New(errMsg) + log.Error(errMsg, zap.Any("in", in)) + return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, errMsg), nil } if in.Entity == nil { errMsg := "the grant entity in the request is nil" - return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, errMsg), errors.New(errMsg) + log.Error(errMsg, zap.Any("in", in)) + return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, errMsg), nil } if err := c.isValidObject(in.Entity.Object); err != nil { - return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, err.Error()), err + log.Error("", zap.Error(err)) + return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, err.Error()), nil } if err := c.isValidRole(in.Entity.Role); err != nil { - return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, err.Error()), err + log.Error("", zap.Error(err)) + return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, err.Error()), nil } if err := c.isValidGrantor(in.Entity.Grantor, in.Entity.Object.Name); err != nil { - return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, err.Error()), err + log.Error("", zap.Error(err)) + return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, err.Error()), nil } logger.Debug("before PrivilegeNameForMetastore", zap.String("privilege", in.Entity.Grantor.Privilege.Name)) @@ -2815,8 +2826,8 @@ func (c *Core) OperatePrivilege(ctx context.Context, in *milvuspb.OperatePrivile if err := c.MetaTable.OperatePrivilege(util.DefaultTenant, in.Entity, in.Type); err != nil { if !common.IsIgnorableError(err) { errMsg := "fail to operate the privilege" - logger.Error(errMsg, zap.Error(err)) - return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, errMsg), err + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, errMsg), nil } updateCache = false } @@ -2830,14 +2841,16 @@ func (c *Core) OperatePrivilege(ctx context.Context, in *milvuspb.OperatePrivile opType = int32(typeutil.CacheRevokePrivilege) default: errMsg := "invalid operate type for the OperatePrivilege api" - logger.Error(errMsg, zap.Any("op_type", in.Type)) - return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, errMsg), errors.New(errMsg) + log.Error(errMsg, zap.Any("in", in)) + return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, errMsg), nil } if err := c.proxyClientManager.RefreshPolicyInfoCache(ctx, &proxypb.RefreshPolicyInfoCacheRequest{ OpType: opType, OpKey: funcutil.PolicyForPrivilege(in.Entity.Role.Name, in.Entity.Object.Name, in.Entity.ObjectName, in.Entity.Grantor.Privilege.Name), }); err != nil { - return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, err.Error()), err + errMsg := "fail to refresh policy info cache" + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) + return failStatus(commonpb.ErrorCode_OperatePrivilegeFailure, errMsg), nil } } @@ -2865,20 +2878,23 @@ func (c *Core) SelectGrant(ctx context.Context, in *milvuspb.SelectGrantRequest) } if in.Entity == nil { errMsg := "the grant entity in the request is nil" + log.Error(errMsg, zap.Any("in", in)) return &milvuspb.SelectGrantResponse{ Status: failStatus(commonpb.ErrorCode_SelectGrantFailure, errMsg), - }, errors.New(errMsg) + }, nil } if err := c.isValidRole(in.Entity.Role); err != nil { + log.Error("", zap.Any("in", in), zap.Error(err)) return &milvuspb.SelectGrantResponse{ Status: failStatus(commonpb.ErrorCode_SelectGrantFailure, err.Error()), - }, err + }, nil } if in.Entity.Object != nil { if err := c.isValidObject(in.Entity.Object); err != nil { + log.Error("", zap.Any("in", in), zap.Error(err)) return &milvuspb.SelectGrantResponse{ Status: failStatus(commonpb.ErrorCode_SelectGrantFailure, err.Error()), - }, err + }, nil } } @@ -2890,10 +2906,10 @@ func (c *Core) SelectGrant(ctx context.Context, in *milvuspb.SelectGrantRequest) } if err != nil { errMsg := "fail to select the grant" - logger.Error(errMsg, zap.Error(err)) + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) return &milvuspb.SelectGrantResponse{ Status: failStatus(commonpb.ErrorCode_SelectGrantFailure, errMsg), - }, err + }, nil } logger.Debug(method + " success") @@ -2919,15 +2935,19 @@ func (c *Core) ListPolicy(ctx context.Context, in *internalpb.ListPolicyRequest) policies, err := c.MetaTable.ListPolicy(util.DefaultTenant) if err != nil { + errMsg := "fail to list policy" + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) return &internalpb.ListPolicyResponse{ - Status: failStatus(commonpb.ErrorCode_ListPolicyFailure, "fail to list policy"), - }, err + Status: failStatus(commonpb.ErrorCode_ListPolicyFailure, errMsg), + }, nil } userRoles, err := c.MetaTable.ListUserRole(util.DefaultTenant) if err != nil { + errMsg := "fail to list user-role" + log.Error(errMsg, zap.Any("in", in), zap.Error(err)) return &internalpb.ListPolicyResponse{ Status: failStatus(commonpb.ErrorCode_ListPolicyFailure, "fail to list user-role"), - }, err + }, nil } logger.Debug(method + " success") diff --git a/internal/util/funcutil/policy.go b/internal/util/funcutil/policy.go index 7669bad32f..a4ee9df70b 100644 --- a/internal/util/funcutil/policy.go +++ b/internal/util/funcutil/policy.go @@ -33,13 +33,13 @@ func GetVersion(m proto.GeneratedMessage) (string, error) { func GetPrivilegeExtObj(m proto.GeneratedMessage) (commonpb.PrivilegeExt, error) { _, md := descriptor.MessageDescriptorProto(m) if md == nil { - log.Error("MessageDescriptorProto result is nil") + log.Warn("MessageDescriptorProto result is nil") return commonpb.PrivilegeExt{}, fmt.Errorf("MessageDescriptorProto result is nil") } extObj, err := proto.GetExtension(md.Options, commonpb.E_PrivilegeExtObj) if err != nil { - log.Error("GetExtension fail", zap.Error(err)) + log.Warn("GetExtension fail", zap.Error(err)) return commonpb.PrivilegeExt{}, err } privilegeExt := extObj.(*commonpb.PrivilegeExt)