feat: add controllers that retrieve and sync information about MSI based identities - #6589
Merged
openshift-merge-bot[bot] merged 1 commit intoAug 20, 2026
Conversation
openshift-ci
Bot
requested review from
Gerd Oberlechner (geoberle) and
Jan-Hendrik Boll (janboll)
August 18, 2026 18:32
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves MSI identity metadata resolution/syncing fully into Cosmos by introducing two new backend controllers that (1) fetch ClientID/PrincipalID for MSI-based identities via the Managed Identities Data Plane and store them on ServiceProviderCluster.Status.MSIManagedIdentities, and (2) sync those resolved IDs into HCPOpenShiftCluster.Identity.UserAssignedIdentities. It also removes the legacy Cluster Service–backed identity migration controller and updates docs/wiring accordingly.
Changes:
- Added
FetchMSIIdentitiesInfoandClusterIdentitySynccontrollers underbackend/pkg/controllers/cluster/identity/. - Added new
ServiceProviderClusterstatus types + deepcopy generation for persisted MSI identity metadata. - Removed the legacy
IdentityMigrationcontroller/tests and updated backend wiring + docs (Cosmos data flow, resource-creation map).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/api/coreapi/zz_generated.deepcopy.go | Adds deepcopy support for new ServiceProviderCluster MSI identity status types. |
| internal/api/coreapi/types_serviceprovider_cluster.go | Introduces Status.MSIManagedIdentities schema and related identity detail structs. |
| internal/api/coreapi/types_cluster.go | Updates writer annotation for HCPOpenShiftCluster.Identity to ClusterIdentitySync. |
| backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go | New controller to query MI Data Plane and persist resolved identity metadata onto SPC. |
| backend/pkg/controllers/cluster/identity/fetch_msi_identities_info_test.go | Unit tests for identity-set matching and recheck gating. |
| backend/pkg/controllers/cluster/identity/fetch_msi_identities_info_synconce_test.go | SyncOnce-focused unit tests covering dataplane responses, errors, and gating behavior. |
| backend/pkg/controllers/cluster/identity/cluster_identity_sync.go | New controller to sync resolved ClientID/PrincipalID from SPC into Cluster Identity UAIs. |
| backend/pkg/controllers/cluster/identity/cluster_identity_sync_test.go | Unit tests validating syncing behavior, casing, nil handling, and “no-op” scenarios. |
| backend/pkg/app/backend.go | Wires in the new controllers and removes the old identity migration controller startup. |
| backend/pkg/utils/statusutils/inertia_test.go | Updates regex test data to align with renamed controller naming in the suite. |
| docs/cosmos-data-flow.md | Updates Cosmos read/write documentation for the new controllers and fields. |
| docs/resource-creation.mm | Updates controller taxonomy to reflect the new identity-sync controller. |
| backend/pkg/controllers/cluster/properties/identity_migration.go | Removes legacy CS-backed identity migration controller implementation. |
| backend/pkg/controllers/cluster/properties/identity_migration_test.go | Removes legacy controller unit tests. |
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+257
to
+266
| identitiesToSyncResourceIDStrs := msiBasedIdentitiesToFetch.resourceIDStrings() | ||
|
|
||
| // On environments where the real Managed Identities Data Plane service is not available, a | ||
| // fake implementation of the Managed Identities Data Plane client is used, which always returns the same information and | ||
| // same set of credentials for all requests, independently of which identity is requested. The returned information is | ||
| // the information associated to the "MI Mock" identity. | ||
| fpaMIDataplaneClient, err := c.fpaMIdataplaneClientBuilder.ManagedIdentitiesDataplane(existingCluster.ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL) | ||
| if err != nil { | ||
| return utils.TrackError(fmt.Errorf("failed to get Managed Identities Data Plane Client: %w", err)) | ||
| } |
Comment on lines
+307
to
+330
| // ClientID is Client ID of the Azure User Assigned Managed Identity represented by ResourceID. | ||
| // Fetched from Azure and written here by the FetchMSIIdentitiesInfo. | ||
| // It may be be nil or empty. | ||
| ClientID *string `json:"clientId,omitempty"` | ||
| // PrincipalID Principal ID of the Azure User Assigned Managed Identity represented by ResourceID. | ||
| // Fetched from Azure and written here by the FetchMSIIdentitiesInfo. | ||
| // It may be be nil or empty. | ||
| PrincipalID *string `json:"principalId,omitempty"` | ||
| } | ||
|
|
||
| // ServiceProviderClusterServiceManagedIdentity is the resolved metadata for the | ||
| // cluster's service managed identity. | ||
| type ServiceProviderClusterServiceManagedIdentity struct { | ||
| // ResourceID is the Azure Resource ID of the Azure User Assigned Managed Identity that is associated to the cluster's Service Managed Identity. | ||
| // Its value comes from the Cluster's CustomerProperties. Its value comes from the Cluster's CustomerProperties. | ||
| ResourceID *azcorearm.ResourceID `json:"resourceId,omitempty"` | ||
| // ClientID is Client ID of the Azure User Assigned Managed Identity represented by ResourceID. | ||
| // Fetched from Azure and written here by the FetchMSIIdentitiesInfo. | ||
| // It may be be nil or empty. | ||
| ClientID *string `json:"clientId,omitempty"` | ||
| // PrincipalID Principal ID of the Azure User Assigned Managed Identity represented by ResourceID. | ||
| // Fetched from Azure and written here by the FetchMSIIdentitiesInfo. | ||
| // It may be be nil or empty. | ||
| PrincipalID *string `json:"principalId,omitempty"` |
David Eads (deads2k)
requested changes
Aug 18, 2026
| } | ||
|
|
||
| // Get the cluster from Cosmos | ||
| clusterCRUD := c.resourcesDBClient.HCPClusters(key.SubscriptionID, key.ResourceGroupName) |
Collaborator
There was a problem hiding this comment.
dont' do this. Just used the cached value.
| // PrincipalID are updated in place. Keys missing from SPC are left as-is. | ||
| func (c *clusterIdentitySyncer) updateIdentityUserAssignedIdentitiesFromSPC( | ||
| identityUserAssignedIdentities map[string]*coreapi.UserAssignedIdentity, | ||
| spcControlPlaneOperatorsIdentities map[string]*coreapi.ServiceProviderClusterControlPlaneOperatorIdentity, |
Collaborator
There was a problem hiding this comment.
dont' abbreviate serviceProviderCluster as spc anywhere.
| clientID = spcServiceManagedIdentity.ClientID | ||
| principalID = spcServiceManagedIdentity.PrincipalID | ||
| } else { // otherwise, we leave the identity as-is | ||
| continue |
Collaborator
There was a problem hiding this comment.
this is incorrect. When we don't have data, we need to set the value to &coreapi.UserAssignedIdentity{}
| var clientID, principalID *string | ||
|
|
||
| // If we found the identity in the SPC control plane operators identities, we use the ClientID and PrincipalID from the SPC. | ||
| if spcIdentity, ok := spcControlPlaneOperatorsIdentities[lowerResourceIDStr]; ok && spcIdentity != nil { |
Collaborator
There was a problem hiding this comment.
factor as a switch/case
Comment on lines
+184
to
+185
| clientID = spcIdentity.ClientID | ||
| principalID = spcIdentity.PrincipalID |
Collaborator
There was a problem hiding this comment.
just inline assigned to the identityUserAssignedIdentities map
| // If we found the identity in the SPC service managed identity, we use the ClientID and PrincipalID from the SPC. | ||
| } else if spcServiceManagedIdentity != nil && spcServiceManagedIdentity.ResourceID != nil && | ||
| strings.ToLower(spcServiceManagedIdentity.ResourceID.String()) == lowerResourceIDStr { | ||
| clientID = spcServiceManagedIdentity.ClientID |
Collaborator
There was a problem hiding this comment.
just inline assigned to the identityUserAssignedIdentities map
| return nil | ||
| } | ||
|
|
||
| spcCRUD := c.resourcesDBClient.ServiceProviderClusters(key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName) |
| controlPlaneOperatorsUnchanged := equality.Semantic.DeepEqual(replacement.Status.MSIManagedIdentities.ControlPlaneOperatorsIdentities, existingSPC.Status.MSIManagedIdentities.ControlPlaneOperatorsIdentities) | ||
| serviceManagedIdentityUnchanged := equality.Semantic.DeepEqual(replacement.Status.MSIManagedIdentities.ServiceManagedIdentity, existingSPC.Status.MSIManagedIdentities.ServiceManagedIdentity) | ||
| recheckUnchanged := equality.Semantic.DeepEqual(replacement.Status.MSIManagedIdentities.EarliestRecheckTime, existingSPC.Status.MSIManagedIdentities.EarliestRecheckTime) | ||
| if controlPlaneOperatorsUnchanged && serviceManagedIdentityUnchanged && recheckUnchanged { |
Collaborator
There was a problem hiding this comment.
how about just doing equality.Semantic.DeepEqual(replacement, existingSPC) right here instead of partial checks.
…ased identities refactor: move MSI identity resolution onto SPC and sync into cluster Identity We add a controller that retrieves the Client ID and Principal ID associated to the following identities associated to an ARO-HCP Cluster: - The Control Plane operators identities - The Service Managed Identity We leverage Microsoft's Managed Identities Data Plane service to retrieve the information. When the service is not available (outside of AME tenants) the fake managed identities data plane client is leveraged which returns the information associated to the MI Mock Identity for all requests/responses to it. We do not directly use Azure Go SDK's UserAssignedIdentities client because otherwise we would return the information of clientid+principalid of the passed identities in the payload instead of the actual clientid+principalid that ends up being used in the management cluster. Additionally, we replace IdentityMigration (CS-backed) with ClusterIdentitySync, which keeps HCPOpenShiftCluster.Identity.UserAssignedIdentities ClientID/PrincipalID in sync with ServiceProviderCluster.Status.MSIManagedIdentities. Keys are never deleted from the HCPOpenShiftCluster.Identity.UserAssignedIdentities map. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Chai-bot (redhat-chai-bot)
force-pushed
the
msoriano-fetchinfo-controlplaneoperators-and-smi-mis
branch
from
August 18, 2026 19:17
4ccd0f8 to
4b6af93
Compare
Collaborator
Author
|
Addressed all review feedback in the latest force-push ( David Eads (@deads2k)'s review:
Copilot suggestions: All changes amend the single commit. AI-generated. Review for accuracy. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
docs/cosmos-data-flow.md:948
- Documentation mismatch: ClusterIdentitySync clears ClientID/PrincipalID for identity keys that don't have a matching entry in ServiceProviderCluster.Status.MSIManagedIdentities (sets an empty UserAssignedIdentity), but the data-flow table says those keys are left unchanged. This should reflect the actual behavior so readers don't assume stale values are preserved.
| **Write** | **`HCPOpenShiftCluster`** | <ul><li>**`Identity.UserAssignedIdentities[key].ClientID` / `PrincipalID`** = from SPC when a lowercased match exists; keys absent from SPC are left unchanged</li></ul> |
Collaborator
|
/approve |
Collaborator
Author
|
/retest e2e-parallel AI-generated. Review for accuracy. |
Collaborator
Author
|
/test e2e-parallel AI-generated. Review for accuracy. |
Collaborator
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, machi1990, redhat-chai-bot The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Collaborator
Author
|
/test e2e-parallel AI-generated. Review for accuracy. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds two new controllers in
backend/pkg/controllers/cluster/identity/that retrieve and synchronize MSI-based identity information for ARO-HCP clusters:ClusterIdentitySync— KeepsHCPOpenShiftCluster.Identity.UserAssignedIdentitiesClientID/PrincipalID in sync withServiceProviderCluster(replaces the old CS-backedIdentityMigrationcontroller fromcluster/properties/)FetchMSIIdentitiesInfo— Fetches ClientID/PrincipalID for control-plane operator identities and the service managed identity via Microsoft's Managed Identities Data Plane service, storing results onServiceProviderCluster.Status.MSIManagedIdentitiesChanges
cluster_identity_sync.go,fetch_msi_identities_info.goIdentityDetails,MSIManagedIdentitiesintypes_serviceprovider_cluster.go+ generated deepcopyidentity_migration.go/identity_migration_test.gofromcluster/properties/backend.go), docs (cosmos-data-flow.md,resource-creation.mm)inertia_test.goregex to match renamed controllerBased on
Rebased from PR #6301 onto current
main. Addresses review comments from that PR:SyncOncetest cases forFetchMSIIdentitiesInfocovering happy path, case-insensitive matching, identity-not-found, deleting-cluster skip, recheck gating, error handling, and nil-guard edge cases)inertia_test.goregex mismatch and lint issuesmake lintandgo test ./pkg/controllers/cluster/identity/...both pass cleanly.AI-generated. Review for accuracy.
David Eads (@deads2k) requested in Slack thread