Add User Management Resources #983

wsquan171 · 2023-09-21T21:59:14Z

This PR adds the following 3 resources related to user management:

Local users (/api/v1/node/users)
Roles (policy/api/v1/aaa/roles)
Role bindings (policy/api/v1/aaa/role-bindings)

Users

As password is not returned on GET, and it's possible for terraform created users to change the password out of terraform, password updates from config is not checked. Password resets could be done by deactivating then reactivate the same account.
SSH keys (api/v1/node/users/{userid}/ssh-keys) and auth policy (/api/v1/node/aaa/auth-policy) not included

Roles

Role validation API doesn't return error on invalid set of feature permissions. Instead, a list of "recommended" permissions will be returned. An error is raised reporting such missing permissions if the list is not empty.
Multi-tenancy API endpoints supports GET only, thus not included.

Role Bindings

For local users, role bindings is owned by NSX and created upon user creation. Role Binding Create and Delete is not allowed. Such role bindings must be imported into terraform for modifications.
To remove an entire path of a set of roles from a binding, the path needs to be marked for delete in the PUT call, with non-empty role in the list. A random role is picked from old state for the call to make NSX API validation pass.
Full CRUD for remote user bindings are tested with LDAP.
Acc test requires env NSXT_TEST_LDAP_USER to be set to a valid LDAP user, and NSX is enabled with LDAP identity source.

Partially resolve #943

nsxt/resource_nsxt_node_user.go

annakhm · 2023-09-25T23:08:42Z

nsxt/resource_nsxt_node_user.go

+
+	user, err := client.Createuser(userProp)
+	if err != nil {
+		return fmt.Errorf("failed to create Nsxt Node user: %s", err.Error())


Please use handleCreateError here for consistency, same below

nsxt/resource_nsxt_node_user.go

annakhm · 2023-09-25T23:16:53Z

nsxt/resource_nsxt_node_user.go

+	}
+	user, err := client.Get(id)
+	if err != nil {
+		if isNotFoundError(err) {


This behavior is covered with handleReadError

annakhm · 2023-09-25T23:18:19Z

We have policy API for roles, but not for users?

vmwclabot · 2023-09-28T10:14:30Z

@wsquan171, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit 371af4d must be signed by [email protected]

wsquan171 · 2023-09-29T04:19:47Z

We have policy API for roles, but not for users?

Unfortunately, yes. Local (node) users are only supported with management API.

wsquan171 · 2023-09-29T04:21:37Z

/test-all

wsquan171 · 2023-09-29T05:27:20Z

/test-all

wsquan171 · 2023-09-29T19:08:32Z

/test-all

annakhm · 2023-10-03T20:54:23Z

nsxt/resource_nsxt_node_user.go

+
+		r := regexp.MustCompile("^[a-zA-Z][a-zA-Z0-9@-_.\\-]*$")
+		if ok := r.MatchString(v); !ok {
+			es = append(es, fmt.Errorf("must be a valid username matching: ^[a-zA-Z][a-zA-Z0-9@-_.\\-]*$"))


I don't think we should expect users to understand regex..

removed regex from the error message.

annakhm · 2023-10-03T20:56:10Z

nsxt/resource_nsxt_node_user.go

+		return handleReadError(d, "User", id, err)
+	}
+
+	// Password not return on GET


Would there be a non-empty diff after apply since password is not set in state?

Per terraform dev guide:

If the Update callback returns with or without an error, the full state is saved. If the ID becomes blank, the resource is destroyed (even within an update, though this shouldn't happen except in error scenarios).

Verified that by changing only the password, terraform plan returns empty action plan after the initial apply, and reports:

No changes. Your infrastructure matches the configuration.

Thanks for verifying. Since password is not nested attribute, no problem indeed.

Signed-off-by: Shawn Wang <[email protected]>

wsquan171 · 2023-10-05T16:32:14Z

/test-all

wsquan171 · 2023-10-05T17:34:24Z

/test-all

annakhm · 2023-10-05T19:28:37Z

nsxt/resource_nsxt_node_user.go

+	// A second deletion attempt should be deemed as NotFound, which is then treated as successful.
+	var err error
+	for i := 0; i < 2; i++ {
+		err = client.Delete(id)


does the first delete always return error?

Essentially, the first DELETE call actually went through, with HTTP status 200. It's just the returned HTTP header content-type: text/html; charset=UTF-8 would always trigger sdk to report it back as internal server error, and such error type casting made checking status code very difficult on our end.

The second call will make NSX return 404, which is ignored on delete calls.

This issue still exists on NSX 4.1.2 rtqa.

annakhm · 2023-10-05T19:33:52Z

nsxt/resource_nsxt_policy_role_binding.go

+		},
+
+		Schema: map[string]*schema.Schema{
+			"display_name": getDataSourceDisplayNameSchema(),


is there a reason we don't allow controlling nsx_id here and in the other policy resource?

For roles, role (role identifier) is used as the uuid in NSX, thus no other nsx_id is needed / supported.
For bindings, NSX does not allow providing an ID on create. It's a POST call under .../role-bindings URL, and the id can only be determined by NSX.

nsxt/resource_nsxt_policy_role.go

annakhm · 2023-10-05T19:42:12Z

nsxt/resource_nsxt_policy_role.go

+	}
+}
+
+func policyUserManagementRolePatch(id string, d *schema.ResourceData, m interface{}) error {


Since the actual API call is Update and not Patch, I would suggest to change function name to improve readability

annakhm · 2023-10-05T19:46:43Z

nsxt/resource_nsxt_policy_role_binding.go

+		Schema: map[string]*schema.Schema{
+			"display_name": getDataSourceDisplayNameSchema(),
+			"description":  getDataSourceDescriptionSchema(),
+			"revision":     getRevisionSchema(),


I'm guessing path is not exposed in this API as well?

Yes. Neither role or role bindings exports policy path.

Signed-off-by: Shawn Wang <[email protected]>

wsquan171 · 2023-10-05T21:02:51Z

/test-all

annakhm · 2023-10-11T01:09:25Z

Thanks Shawn!

annakhm · 2023-10-11T01:09:45Z

/test-all

wsquan171 force-pushed the user-mgmt branch 2 times, most recently from 07f5f85 to 19f5dca Compare September 21, 2023 22:13

wsquan171 requested review from ksamoray and annakhm September 21, 2023 22:21

annakhm reviewed Sep 25, 2023

View reviewed changes

nsxt/resource_nsxt_node_user.go Outdated Show resolved Hide resolved

annakhm reviewed Sep 25, 2023

View reviewed changes

nsxt/resource_nsxt_node_user.go Show resolved Hide resolved

annakhm reviewed Sep 25, 2023

View reviewed changes

nsxt/resource_nsxt_node_user.go Outdated Show resolved Hide resolved

annakhm reviewed Sep 25, 2023

View reviewed changes

wsquan171 force-pushed the user-mgmt branch 3 times, most recently from f34d2f9 to a7507d2 Compare September 28, 2023 09:46

vmwclabot added the dco-required DCO Required label Sep 28, 2023

wsquan171 force-pushed the user-mgmt branch from 371af4d to 2701eff Compare September 28, 2023 18:28

vmwclabot removed the dco-required DCO Required label Sep 28, 2023

wsquan171 force-pushed the user-mgmt branch 2 times, most recently from 5dd38fe to a8e5802 Compare September 29, 2023 04:17

wsquan171 force-pushed the user-mgmt branch from a8e5802 to 072b2e0 Compare September 29, 2023 04:20

wsquan171 force-pushed the user-mgmt branch from 072b2e0 to b404774 Compare September 29, 2023 19:08

annakhm reviewed Oct 3, 2023

View reviewed changes

wsquan171 added 3 commits October 5, 2023 09:03

Add node user resource

b6d0de7

Signed-off-by: Shawn Wang <[email protected]>

Add user management role resource

a37ee2b

Signed-off-by: Shawn Wang <[email protected]>

Add user management role binding resource

739e42a

Signed-off-by: Shawn Wang <[email protected]>

wsquan171 added 4 commits October 5, 2023 09:03

Add Doc for node user

7915292

Signed-off-by: Shawn Wang <[email protected]>

Add Doc for user management role

404a07e

Signed-off-by: Shawn Wang <[email protected]>

Add acc test for role bindings using LDAP user

6a9f88d

Signed-off-by: Shawn Wang <[email protected]>

Add doc for role bindings

3c2d9a5

Signed-off-by: Shawn Wang <[email protected]>

wsquan171 force-pushed the user-mgmt branch from b404774 to 2efe1c6 Compare October 5, 2023 16:29

annakhm reviewed Oct 5, 2023

View reviewed changes

nsxt/resource_nsxt_policy_role.go Outdated Show resolved Hide resolved

annakhm reviewed Oct 5, 2023

View reviewed changes

nsxt/resource_nsxt_policy_role.go Show resolved Hide resolved

annakhm reviewed Oct 5, 2023

View reviewed changes

Address comments

3afda02

Signed-off-by: Shawn Wang <[email protected]>

wsquan171 force-pushed the user-mgmt branch from 2efe1c6 to 3afda02 Compare October 5, 2023 21:01

annakhm approved these changes Oct 11, 2023

View reviewed changes

wsquan171 merged commit bd0d7ce into vmware:master Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add User Management Resources #983

Add User Management Resources #983

wsquan171 commented Sep 21, 2023 •

edited

Loading

annakhm Sep 25, 2023

wsquan171 Sep 29, 2023

annakhm Sep 25, 2023

wsquan171 Sep 29, 2023

annakhm commented Sep 25, 2023

vmwclabot commented Sep 28, 2023

wsquan171 commented Sep 29, 2023

wsquan171 commented Sep 29, 2023

wsquan171 commented Sep 29, 2023

wsquan171 commented Sep 29, 2023

annakhm Oct 3, 2023

wsquan171 Oct 5, 2023

annakhm Oct 3, 2023

wsquan171 Oct 5, 2023

annakhm Oct 5, 2023

wsquan171 commented Oct 5, 2023

wsquan171 commented Oct 5, 2023

annakhm Oct 5, 2023

wsquan171 Oct 5, 2023

annakhm Oct 5, 2023

wsquan171 Oct 5, 2023

annakhm Oct 5, 2023

wsquan171 Oct 5, 2023

annakhm Oct 5, 2023

wsquan171 Oct 5, 2023

wsquan171 commented Oct 5, 2023

annakhm commented Oct 11, 2023

annakhm commented Oct 11, 2023

Add User Management Resources #983

Add User Management Resources #983

Conversation

wsquan171 commented Sep 21, 2023 • edited Loading

Users

Roles

Role Bindings

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annakhm commented Sep 25, 2023

vmwclabot commented Sep 28, 2023

wsquan171 commented Sep 29, 2023

wsquan171 commented Sep 29, 2023

wsquan171 commented Sep 29, 2023

wsquan171 commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsquan171 commented Oct 5, 2023

wsquan171 commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsquan171 commented Oct 5, 2023

annakhm commented Oct 11, 2023

annakhm commented Oct 11, 2023

wsquan171 commented Sep 21, 2023 •

edited

Loading