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

Populate ClientID and ObjectID of cluster and platform workload identities #3860

Merged

Conversation

tsatam
Copy link
Collaborator

@tsatam tsatam commented Sep 23, 2024

Which issue this PR addresses:

Fixes ARO-8609

What this PR does / why we need it:

Updates the cluster install process for workload identity clusters:

  • Populates the ClientID and PrincipalID (ObjectID) fields on the clusterdoc for the cluster MSI, within Identity.UserAssignedIdentities[${CLUSTER_MSI_RESOURCE_ID}]
  • Populates the ClientID and ObjectID (PrincipalID) fields on the clusterdoc for each platform workload identity, within Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities[*]

Test plan for issue:

  • Unit tests were added for each new operation outlined above

Is there any documentation that needs to be updated for this PR?

No

How do you know this will function as expected in production?

E2E tests will be added as part of the wider MIWI effort, as well as extensive manual functional testing, before this feature is available to users in production.

@tsatam tsatam added chainsaw Pull requests or issues owned by Team Chainsaw loki hold Hold labels Sep 23, 2024
@tsatam tsatam force-pushed the tsatam/ARO-8609-populate-clientid-objectid-of-cluster-identities branch from 00eb7a3 to 5036d2e Compare September 23, 2024 22:11
@tsatam tsatam changed the base branch from master to kimorris27/ARO-4360-cluster-msi-changes September 23, 2024 22:11
@tsatam tsatam force-pushed the tsatam/ARO-8609-populate-clientid-objectid-of-cluster-identities branch 4 times, most recently from 67dad10 to 950f87a Compare September 24, 2024 13:26
@github-actions github-actions bot added the needs-rebase branch needs a rebase label Sep 24, 2024
@tsatam tsatam changed the base branch from kimorris27/ARO-4360-cluster-msi-changes to master September 24, 2024 13:26
Copy link

Please rebase pull request.

@tsatam tsatam force-pushed the tsatam/ARO-8609-populate-clientid-objectid-of-cluster-identities branch from 950f87a to 06701b2 Compare September 24, 2024 13:28
@github-actions github-actions bot removed the needs-rebase branch needs a rebase label Sep 24, 2024
@tsatam tsatam removed the hold Hold label Sep 24, 2024
@tsatam
Copy link
Collaborator Author

tsatam commented Sep 24, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tsatam
Copy link
Collaborator Author

tsatam commented Sep 27, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tsatam
Copy link
Collaborator Author

tsatam commented Sep 30, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tsatam tsatam force-pushed the tsatam/ARO-8609-populate-clientid-objectid-of-cluster-identities branch 2 times, most recently from 77281c7 to a612338 Compare October 1, 2024 13:37
@tsatam tsatam requested a review from fahlmant as a code owner October 1, 2024 13:37
@tsatam
Copy link
Collaborator Author

tsatam commented Oct 1, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tsatam tsatam force-pushed the tsatam/ARO-8609-populate-clientid-objectid-of-cluster-identities branch from a612338 to 7744d31 Compare October 1, 2024 14:35
@tsatam
Copy link
Collaborator Author

tsatam commented Oct 1, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@lranjbar lranjbar left a comment

Choose a reason for hiding this comment

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

Small nit, I think it will be good to add a little bit more here. It's not blocking because the code is already covered by the existing test.

pkg/cluster/clustermsi.go Show resolved Hide resolved
cadenmarchese
cadenmarchese previously approved these changes Oct 2, 2024
@tsatam
Copy link
Collaborator Author

tsatam commented Oct 2, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tsatam tsatam force-pushed the tsatam/ARO-8609-populate-clientid-objectid-of-cluster-identities branch 2 times, most recently from 2565389 to 0001da4 Compare October 2, 2024 20:15
Adds a comment and unit tests indicating its usage
@tsatam tsatam force-pushed the tsatam/ARO-8609-populate-clientid-objectid-of-cluster-identities branch from 0001da4 to 1ece0d9 Compare October 2, 2024 20:19
@tsatam
Copy link
Collaborator Author

tsatam commented Oct 2, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tsatam tsatam merged commit f83191f into master Oct 3, 2024
20 checks passed
@tsatam tsatam deleted the tsatam/ARO-8609-populate-clientid-objectid-of-cluster-identities branch October 3, 2024 21:28
slawande2 pushed a commit that referenced this pull request Oct 7, 2024
…ities (#3860)

* Add new clusterIdentityIDs manager function

* Add clusterIdentityIDs step to install for WI clusters

* Add new client wrapper for armmsi UserAssignedIdentitiesClient

* Add userAssignedIdentities client to cluster manager

* Add new platformWorkloadIdentityIDs manager function

* Add platformWorkloadIdentityIDs step to install for WI clusters

* Do not allow clusterIdentityIDs to be called for a CSP cluster

* Perform all clientID/objectID enrichment before dynamic validation

* Return UserAssignedIdentitiesClient implementation instead of interface in constructor

* Use cluster MSI credentials for userAssignedIdentities client

This requires moving client instantiation from the cluster manager constructor to the
initializeClusterMsiClients install step.

* Extract ExplicitIdentity access/handling in clustermsi to common function

* Preserve passed-in casing on cluster identity resource IDs

* Actually use extracted identity from getSingleExpectedIdentity

* Clarify purpose of getSingleExplicitIdentity function

Adds a comment and unit tests indicating its usage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chainsaw Pull requests or issues owned by Team Chainsaw loki
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants