Repository: milvus-io/milvus-sdk-go
Pull request #697 (2024-04-05 20:27:39)

enhance: Add Describe User methods
resolves: #698

I've added an entity:

```
type UserDescription struct {
	Name  string
	Roles []string
}
```

And two methods and tests for them:

- `DescribeUser(ctx context.Context, username string) (entity.UserDescription, error)` - which returns UserDescription for a specific user.
- `DescribeUsers(ctx context.Context) ([]entity.UserDescription, error)` - which returns UserDescription for all users presented. 

The thing that I want to raise is that I am not sure if it is feasible to return empty `UserDescription` entity in case the user from `DescribeUser` method doesn't exist, I would highly appreciate suggestions here.

Also, I am not much familiar with the `milvus-sdk-go` codebase and this is my first PR here, so any comments/feedback/suggestions would be highly appreciated, thanks!

sre-ci-robot: Welcome @punkerpunker! It looks like this is your first PR to milvus-io/milvus-sdk-go 🎉
punkerpunker: Failing tests doesn't seem to be related to this specific change btw
congqixia: @punkerpunker thanks for the contribution. I've fixed failing cases. could you please rebase and fix the lint issue reported in `lint` action?
punkerpunker: @congqixia seems like fixed
congqixia: /kind enhancement
punkerpunker: Thanks @congqixia, once again the tests falling doesn't seem to be related to the change :)
punkerpunker: Hey @congqixia, are we concerned about something in particular so we don't merge this one yet? Want to start working on tf-provider, and that piece is missing :) 
congqixia: @punkerpunker sorry for the late reply. Master branch e2e cases was unstable due to rapid change of milvus master behavior. We have just fixed the cases. You can rebase your PR now

Patch:
diff --git a/client/client.go b/client/client.go
index 39a76fc..9ca981b 100644
--- a/client/client.go
+++ b/client/client.go
@@ -188,7 +188,11 @@ type Client interface {
 	ListRoles(ctx context.Context) ([]entity.Role, error)
 	// ListUsers lists the user objects in system.
 	ListUsers(ctx context.Context) ([]entity.User, error)
-	// Grant adds object privileged for role.
+	// DescribeUser describes specific user attributes in the system
+	DescribeUser(ctx context.Context, username string) (entity.UserDescription, error)
+	// DescribeUsers describe all users attributes in the system
+	DescribeUsers(ctx context.Context) ([]entity.UserDescription, error)
+	// Grant adds privilege for role.
 	Grant(ctx context.Context, role string, objectType entity.PriviledgeObjectType, object string) error
 	// Revoke removes privilege from role.
 	Revoke(ctx context.Context, role string, objectType entity.PriviledgeObjectType, object string) error
diff --git a/client/rbac.go b/client/rbac.go
index dba0c1a..d758edb 100644
--- a/client/rbac.go
+++ b/client/rbac.go
@@ -149,6 +149,80 @@ func (c *GrpcClient) ListUsers(ctx context.Context) ([]entity.User, error) {
 	return users, nil
 }
 
+// DescribeUser lists the user descriptions in the system (name, roles)
+func (c *GrpcClient) DescribeUser(ctx context.Context, username string) (entity.UserDescription, error) {
+	if c.Service == nil {
+		return entity.UserDescription{}, ErrClientNotReady
+	}
+
+	req := &milvuspb.SelectUserRequest{
+		User: &milvuspb.UserEntity{
+			Name: username,
+		},
+		IncludeRoleInfo: true,
+	}
+
+	resp, err := c.Service.SelectUser(ctx, req)
+
+	if err != nil {
+		return entity.UserDescription{}, err
+	}
+	if err = handleRespStatus(resp.GetStatus()); err != nil {
+		return entity.UserDescription{}, err
+	}
+	results := resp.GetResults()
+
+	if len(results) == 0 {
+		return entity.UserDescription{}, nil
+	}
+
+	userDescription := entity.UserDescription{
+		Name:  results[0].GetUser().GetName(),
+		Roles: make([]string, 0, len(results[0].GetRoles())),
+	}
+
+	for _, role := range results[0].GetRoles() {
+		userDescription.Roles = append(userDescription.Roles, role.GetName())
+	}
+	return userDescription, nil
+}
+
+// DescribeUsers lists all users with descriptions (names, roles)
+func (c *GrpcClient) DescribeUsers(ctx context.Context) ([]entity.UserDescription, error) {
+	if c.Service == nil {
+		return nil, ErrClientNotReady
+	}
+
+	req := &milvuspb.SelectUserRequest{
+		IncludeRoleInfo: true,
+	}
+
+	resp, err := c.Service.SelectUser(ctx, req)
+
+	if err != nil {
+		return nil, err
+	}
+	if err = handleRespStatus(resp.GetStatus()); err != nil {
+		return nil, err
+	}
+	results := resp.GetResults()
+
+	userDescriptions := make([]entity.UserDescription, 0, len(results))
+
+	for _, result := range results {
+		userDescription := entity.UserDescription{
+			Name:  result.GetUser().GetName(),
+			Roles: make([]string, 0, len(result.GetRoles())),
+		}
+		for _, role := range result.GetRoles() {
+			userDescription.Roles = append(userDescription.Roles, role.GetName())
+		}
+		userDescriptions = append(userDescriptions, userDescription)
+	}
+
+	return userDescriptions, nil
+}
+
 // Grant adds object privileged for role.
 func (c *GrpcClient) Grant(ctx context.Context, role string, objectType entity.PriviledgeObjectType, object string) error {
 	if c.Service == nil {
diff --git a/entity/rbac.go b/entity/rbac.go
index 32819d9..cd0f5b9 100644
--- a/entity/rbac.go
+++ b/entity/rbac.go
@@ -7,6 +7,12 @@ type User struct {
 	Name string
 }
 
+// UserDescription is the model for RBAC user description object.
+type UserDescription struct {
+	Name  string
+	Roles []string
+}
+
 // Role is the model for RBAC role object.
 type Role struct {
 	Name string


Test patch:
diff --git a/client/rbac_test.go b/client/rbac_test.go
index 355b306..038b589 100644
--- a/client/rbac_test.go
+++ b/client/rbac_test.go
@@ -347,6 +347,164 @@ func (s *RBACSuite) TestListUser() {
 	})
 }
 
+func (s *RBACSuite) TestDescribeUser() {
+	ctx := context.Background()
+	userName := "testUser"
+
+	s.Run("normal run", func() {
+		ctx, cancel := context.WithCancel(ctx)
+		defer cancel()
+		defer s.resetMock()
+		s.mock.EXPECT().SelectUser(mock.Anything, mock.Anything).Run(func(ctx context.Context, req *milvuspb.SelectUserRequest) {
+			s.True(req.GetIncludeRoleInfo())
+			s.Equal(req.GetUser().GetName(), userName)
+		}).Return(&milvuspb.SelectUserResponse{
+			Status: &commonpb.Status{ErrorCode: commonpb.ErrorCode_Success},
+			Results: []*milvuspb.UserResult{
+				{
+					User: &milvuspb.UserEntity{
+						Name: userName,
+					},
+					Roles: []*milvuspb.RoleEntity{
+						{Name: "role1"},
+						{Name: "role2"},
+					},
+				},
+			},
+		}, nil)
+
+		userDesc, err := s.client.DescribeUser(ctx, userName)
+
+		s.NoError(err)
+		s.Equal(userDesc.Name, userName)
+		s.ElementsMatch(userDesc.Roles, []string{"role1", "role2"})
+	})
+
+	s.Run("rpc error", func() {
+		ctx, cancel := context.WithCancel(ctx)
+		defer cancel()
+		defer s.resetMock()
+		s.mock.EXPECT().SelectUser(mock.Anything, mock.Anything).Return(nil, errors.New("mock error"))
+
+		_, err := s.client.DescribeUser(ctx, userName)
+		s.Error(err)
+	})
+
+	s.Run("status error", func() {
+		ctx, cancel := context.WithCancel(ctx)
+		defer cancel()
+		defer s.resetMock()
+		s.mock.EXPECT().SelectUser(mock.Anything, mock.Anything).Return(&milvuspb.SelectUserResponse{
+			Status: &commonpb.Status{ErrorCode: commonpb.ErrorCode_UnexpectedError},
+		}, nil)
+
+		_, err := s.client.DescribeUser(ctx, userName)
+		s.Error(err)
+	})
+
+	s.Run("service not ready", func() {
+		ctx, cancel := context.WithCancel(ctx)
+		defer cancel()
+
+		c := &GrpcClient{}
+		_, err := c.DescribeUser(ctx, userName)
+		s.Error(err)
+		s.ErrorIs(err, ErrClientNotReady)
+	})
+}
+
+func (s *RBACSuite) TestDescribeUsers() {
+	ctx := context.Background()
+
+	s.Run("normal run", func() {
+		ctx, cancel := context.WithCancel(ctx)
+		defer cancel()
+		defer s.resetMock()
+
+		mockResults := []*milvuspb.UserResult{
+			{
+				User: &milvuspb.UserEntity{
+					Name: "user1",
+				},
+				Roles: []*milvuspb.RoleEntity{
+					{Name: "role1"},
+					{Name: "role2"},
+				},
+			},
+			{
+				User: &milvuspb.UserEntity{
+					Name: "user2",
+				},
+				Roles: []*milvuspb.RoleEntity{
+					{Name: "role3"},
+				},
+			},
+		}
+
+		s.mock.EXPECT().SelectUser(mock.Anything, mock.Anything).Run(func(ctx context.Context, req *milvuspb.SelectUserRequest) {
+			s.True(req.GetIncludeRoleInfo())
+		}).Return(&milvuspb.SelectUserResponse{
+			Status:  &commonpb.Status{ErrorCode: commonpb.ErrorCode_Success},
+			Results: mockResults,
+		}, nil)
+
+		userDescs, err := s.client.DescribeUsers(ctx)
+
+		s.NoError(err)
+		s.Len(userDescs, 2)
+
+		expectedDescs := []entity.UserDescription{
+			{
+				Name:  "user1",
+				Roles: []string{"role1", "role2"},
+			},
+			{
+				Name:  "user2",
+				Roles: []string{"role3"},
+			},
+		}
+
+		for i, desc := range userDescs {
+			s.Equal(expectedDescs[i].Name, desc.Name)
+			s.ElementsMatch(expectedDescs[i].Roles, desc.Roles)
+		}
+	})
+
+	s.Run("rpc error", func() {
+		ctx, cancel := context.WithCancel(ctx)
+		defer cancel()
+		defer s.resetMock()
+
+		s.mock.EXPECT().SelectUser(mock.Anything, mock.Anything).Return(nil, errors.New("mock error"))
+
+		_, err := s.client.DescribeUsers(ctx)
+		s.Error(err)
+	})
+
+	s.Run("status error", func() {
+		ctx, cancel := context.WithCancel(ctx)
+		defer cancel()
+		defer s.resetMock()
+
+		s.mock.EXPECT().SelectUser(mock.Anything, mock.Anything).Return(&milvuspb.SelectUserResponse{
+			Status: &commonpb.Status{ErrorCode: commonpb.ErrorCode_UnexpectedError},
+		}, nil)
+
+		_, err := s.client.DescribeUsers(ctx)
+		s.Error(err)
+	})
+
+	s.Run("service not ready", func() {
+		ctx, cancel := context.WithCancel(ctx)
+		defer cancel()
+
+		c := &GrpcClient{}
+		_, err := c.DescribeUsers(ctx)
+		s.Error(err)
+		s.ErrorIs(err, ErrClientNotReady)
+	})
+}
+
 func (s *RBACSuite) TestGrant() {
 	ctx := context.Background()
 
diff --git a/test/testcases/resource_group_test.go b/test/testcases/resource_group_test.go
index 2f390b7..d8b4162 100644
--- a/test/testcases/resource_group_test.go
+++ b/test/testcases/resource_group_test.go
@@ -117,7 +117,7 @@ func TestRgDefault(t *testing.T) {
 
 	// try to drop default rg
 	errDropDefault := mc.DropResourceGroup(ctx, common.DefaultRgName)
-	common.CheckErr(t, errDropDefault, false, "delete default rg is not permitted")
+	common.CheckErr(t, errDropDefault, false, "default resource group is not deletable")
 }
 
 // test create rg with invalid name
@@ -185,7 +185,7 @@ func TestDropRgNonEmpty(t *testing.T) {
 
 	// drop rg and rg available node is not 0
 	errDrop := mc.DropResourceGroup(ctx, rgName)
-	common.CheckErr(t, errDrop, false, "delete non-empty rg is not permitted")
+	common.CheckErr(t, errDrop, false, "resource group's limits node num is not 0")
 }
 
 // drop empty default rg
@@ -216,7 +216,7 @@ func TestDropEmptyRg(t *testing.T) {
 
 	// drop empty default rg
 	errDrop := mc.DropResourceGroup(ctx, common.DefaultRgName)
-	common.CheckErr(t, errDrop, false, "delete default rg is not permitted")
+	common.CheckErr(t, errDrop, false, "default resource group is not deletable")
 }
 
 // test list rgs
@@ -262,7 +262,7 @@ func TestTransferInvalidNodes(t *testing.T) {
 	invalidNodes := []invalidNodesStruct{
 		{nodesNum: 0, errMsg: "invalid parameter[expected=NumNode > 0][actual=invalid NumNode 0]"},
 		{nodesNum: -1, errMsg: "invalid parameter[expected=NumNode > 0][actual=invalid NumNode -1]"},
-		{nodesNum: 99, errMsg: "nodes not enough"},
+		{nodesNum: 99, errMsg: "resource group node not enough"},
 	}
 	// transfer node
 	for _, invalidNode := range invalidNodes {
@@ -280,15 +280,15 @@ func TestTransferRgNotExisted(t *testing.T) {
 
 	// source not exist
 	errSource := mc.TransferNode(ctx, common.GenRandomString(6), common.DefaultRgName, newRgNode)
-	common.CheckErr(t, errSource, false, "source resource group not found")
+	common.CheckErr(t, errSource, false, "resource group not found")
 
 	// target not exist
 	errTarget := mc.TransferNode(ctx, common.DefaultRgName, common.GenRandomString(6), newRgNode)
-	common.CheckErr(t, errTarget, false, "target resource group not found")
+	common.CheckErr(t, errTarget, false, "resource group not found")
 
 	// transfer to self
 	errSelf := mc.TransferNode(ctx, common.DefaultRgName, common.DefaultRgName, newRgNode)
-	common.CheckErr(t, errSelf, true)
+	common.CheckErr(t, errSelf, false, "source resource group and target resource group should not be the same")
 
 	defaultRg, _ := mc.DescribeResourceGroup(ctx, common.DefaultRgName)
 	require.Equal(t, configQnNodes, defaultRg.AvailableNodesNumber)
@@ -451,7 +451,7 @@ func TestTransferReplicaRgNotExisted(t *testing.T) {
 
 	// transfer to self -> error
 	errSelf := mc.TransferReplica(ctx, rgName, rgName, collName, 1)
-	common.CheckErr(t, errSelf, false, "replicas of same collection in target resource group")
+	common.CheckErr(t, errSelf, false, "source resource group and target resource group should not be the same")
 
 	// transfer to default rg
 	errTransfer := mc.TransferReplica(ctx, rgName, common.DefaultRgName, collName, 1)