Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Principle Identity resource #1021

Merged
merged 6 commits into from
Nov 15, 2023
Merged

Add Principle Identity resource #1021

merged 6 commits into from
Nov 15, 2023

Conversation

wsquan171
Copy link
Contributor

@wsquan171 wsquan171 commented Nov 6, 2023

This PR adds support of Principle Identity resource. A principle identity is identified as a pair of PI name and cert. If an API is called with SSL client cert matching with a registered PI, the caller is recognized as the PI, granted with its roles and associated privileges.

Currently, the only non-deprecated API on NSX are:

  • createWithPem: PI's cert pem is in-lined as part of PI creation call. The inlined pem has to be unique / new to NSX. NSX then creates a certificate in the background, and populates the MP cert id in certificate_id in response. This cert, however, will not be removed by NSX upon PI deletion.
  • updateCert: If a PI's cert has expired, it can be updated by using this API call to replace the current certificate_id with another cert that is already in the system. The issue is:
    • NSX UI no longer shows MP API for cert management. This means if user imports their own cert on UI they have no way to find the underlying ID. Even if figured out, the cert belongs to internal PI nsx_policy and will cause trouble to be used for PI
    • our tf provider currently only supports cert as a data source
    • Other metadatas (tags, display_name, description) can also be updated with this API if and only if the certificate_id is changed and valid
  • delete

As a result, PI is in general immutable. This is also reflected on NSX UI. In this implementation, I decided to:

  • Make all fields ForceNew
  • Leave out display_name, description and revision. As they're either not used or won't be reflected even on UI.
  • Use createWithPem for PI creation and leave out updateCert to avoid res ownership issues and confusion. Cert updates are handled as PI recreation
  • Deletes the NSX-imported pem when the PI is deleted.

Also made some changes to role binding resource to reuse some common arguments. Added TLS cert generation for test.

Principle Identity is also removed from role binding resource, as bindings created for PI is immutable from that API as well.

@wsquan171
Copy link
Contributor Author

/test-all

@wsquan171 wsquan171 requested review from ksamoray and annakhm November 6, 2023 22:46
}

// Clean up underlying cert imported by NSX if exists
if len(certID) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like the cert is created automatically by the create API, but not deleted nu delete API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

@martinrohrbach
Copy link
Contributor

@wsquan171 @annakhm I'm gonna be a little bold here but since I noticed this PR I wanted to ask if you could maybe also expose the "is_protected" boolean in this resource? We do use PIs quite a lot but at the same time set is_protected to false so we can still manipulate objects from the admin GUI if need be.

I believe this boolean cannot be updated after the PI user has been created. Our workaround is to recreate the PI user with the new setting, all objects that "belonged" to the old user are still working as intended afawcs.

@wsquan171
Copy link
Contributor Author

@wsquan171 @annakhm I'm gonna be a little bold here but since I noticed this PR I wanted to ask if you could maybe also expose the "is_protected" boolean in this resource?

Sure. Wasn't aware of this use case and misread is_protected as _protection where the latter was a common read-only attr. Will add this.

@vmwclabot
Copy link
Member

@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.

@wsquan171
Copy link
Contributor Author

/test-all

Required: true,
},
"role": {
Type: schema.TypeList,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if list of sub-clauses here is justified. Does role_display_name convey useful information?
If not, seems more user friendly to just offer a list of string roles? Unless we expect this object to grow in API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@vmwclabot
Copy link
Member

@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.

@vmwclabot vmwclabot added the dco-required DCO Required label Nov 14, 2023
@wsquan171
Copy link
Contributor Author

/test-all

@wsquan171 wsquan171 merged commit dcb04f8 into vmware:master Nov 15, 2023
2 checks passed
@wsquan171 wsquan171 deleted the pi branch November 15, 2023 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants