Add UserRefs in Group spec to support User membership

[chenwng]

This PR adds UserRefs field in the Group spec to allow user to manage Users in a Group in OpenStack.

Notes to reviewers:

MaxItems of UserRefs is set to 1024 to allow a large group. Please suggest if you consider this value is inappropriate.

Close #905

[mandre]

Just a first pass for now, I'll look again after you've moved the update logic into its own reconciler.

[mandre]

Looking good, thanks for the quick iteration. Let me know if you want to add the UserIDs in status in a follow-up change or here.

[mandre]

Just in order to be consistent:

Suggested change

	func (c groupErrorClient) AddUser(ctx context.Context, groupID, userID string) error {
	return c.error
	}
	func (c groupErrorClient) RemoveUser(ctx context.Context, groupID, userID string) error {
	return c.error
	}
	func (c groupErrorClient) ListUsers(ctx context.Context, groupID string) ([]users.User, error) {
	return nil, c.error
	}
	func (e groupErrorClient) AddUser(_ context.Context, _, _ string) error {
	return e.error
	}
	func (e groupErrorClient) RemoveUser(_ context.Context, _, _ string) error {
	return e.error
	}
	func (e groupErrorClient) ListUsers(_ context.Context, _ string) ([]users.User, error) {
	return nil, e.error
	}

[mandre]

Now that begs the question whether we should add a UserIDs field to the GroupResourceStatus. I think we should, possibly as a follow-up (up to you if you want to tackle it in this PR or later).

This would make for a nice cleanup in all the tests (if you need to shell out to openstack this typically indicates that something is missing in ORC).

[chenwng]

I agree it would be better to have UserIDs in the status. Not just for testing but also for user visibility. I just haven't found a good way to achive this without breaking the existing status update pattern.

One idea I have is reuse the Group.Extra field to carry the user name/ids from reconcileUserRefs to ApplyResourceStatus within osResourceT. I know it is a hack and might not be the best way. What do you think? If there is a better way, I would be happy to hear.

Also I'd like to have the user info in the status like below to be more verbose for user:

status:
  resource:
    users:
     - name: user1
       id: 12345
     - name: user2
       id: 67890

[winiciusallan]

I think you can create a custom osResource under osclients/group.go, and add to it the User struct, like we do for ports. For example:

type GroupExt struct {
    groups.Group
    users.Users
}

Then we could populate it as desired and write on status. That's another alternative that I've thought.

[mandre]

The problem with a more complex struct that just user IDs is that we're only duplicating data that is already in the User object own status (which should be the source or truth).

[chenwng]

Seems creating a wrapper struct for Group and UserIDs is a way to go. I will follow this.

Containing UserName/UserID in the status is surely kind of duplcated, but from the point of view of an admin/user, it would save efforts to match between UserName and UserID when he looks at the output of a Group. If conciseness is prefered, I have no issue with it.

[winiciusallan]

I have a concern about drifting the states. If someone does a change in the User user-a, and this user is part of a group, will a new event be triggered on the Group to reconcile the user list?

Answering myself: I think so, since we register a WatchEventHandler. And yeah, I agree that's a downside having duplicated data for users =/.

[mandre]

Inconsistent heading

Suggested change

	# Step 04
	## Step 04

[chenwng]

It seems I need to push AI harder to find things like this :D