PoC msi based identities info fetching with two controller stages - #6525
Conversation
…tatus The FetchMSIIdentitiesInfoi controller now writes ClientID/PrincipalID into ServiceProviderCluster.Status.MSIManagedIdentities (lowercased resource IDs) instead of HCPOpenShiftCluster.Identity. The IdentityMigration controller has been changed to fill the existing Cluster's Identity.UserAssignedIdentities entries from that SPC status via case-insensitive lookup, preserving Identity map key casing and leaving the keys in Identity.UserAssignedIdentities unchanged when there's no corresponding match in SPC. Entries that exist in SPC but don't exist in Identity.UserAssignedIdentities are also not added to Identity.UserAssignedIdentities. It no longer reads from Cluster Service.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: miguelsorianod The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Miguel Soriano (@miguelsorianod): The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold as it's poc and it's also non refined on details |
There was a problem hiding this comment.
Pull request overview
This PR introduces a two-stage PoC flow for MSI identity metadata in Cosmos: one controller fetches ClientID/PrincipalID for MSI-based user-assigned identities and writes them to ServiceProviderCluster.Status.MSIManagedIdentities, and IdentityMigration then fills missing identity fields on existing clusters from that SPC status (case-insensitive lookup, preserving original identity key casing).
Changes:
- Added
FetchMSIIdentitiesInfocontroller to query the Managed Identities Data Plane and persist resolved identity metadata ontoServiceProviderCluster.Status.MSIManagedIdentities. - Updated
IdentityMigrationto stop reading Cluster Service and instead backfillIdentity.UserAssignedIdentities[*].ClientID/PrincipalIDfrom SPC status using lowercased resource ID matching. - Extended core API types, deepcopy generation, and
docs/cosmos-data-flow.mdto reflect the new SPC status field and read/write paths.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/api/coreapi/zz_generated.deepcopy.go | Adds deepcopy support for newly introduced SPC MSI identity status types. |
| internal/api/coreapi/types_serviceprovider_cluster.go | Introduces ServiceProviderCluster.Status.MSIManagedIdentities and related structs to store resolved MSI identity metadata. |
| docs/cosmos-data-flow.md | Updates Cosmos read/write documentation for IdentityMigration and documents FetchMSIIdentitiesInfo behavior and field writers/readers. |
| backend/pkg/controllers/cluster/properties/identity_migration.go | Switches IdentityMigration to fill identity fields from SPC status instead of Cluster Service. |
| backend/pkg/controllers/cluster/properties/identity_migration_test.go | Updates unit tests to validate the new SPC-backed migration behavior (but currently contains compilation issues). |
| backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go | Adds new controller that queries MI Data Plane and writes resolved identity metadata into SPC status. |
| backend/pkg/app/backend.go | Wires in the new FetchMSIIdentitiesInfo controller and removes the Cluster Service client dependency from IdentityMigration. |
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (10)
backend/pkg/controllers/cluster/properties/identity_migration_test.go:75
- The map value type is *coreapi.UserAssignedIdentity, but the test uses a non-pointer composite literal. This won’t compile; use &coreapi.UserAssignedIdentity{...}.
testIdentityResourceID: {
ClientID: stringPtr(testClientID),
PrincipalID: stringPtr(testPrincipalID),
},
},
backend/pkg/controllers/cluster/properties/identity_migration_test.go:90
- The map value type is *coreapi.UserAssignedIdentity, but the test uses a non-pointer composite literal ({}). This won’t compile; use &coreapi.UserAssignedIdentity{} instead.
testIdentityResourceID: {},
backend/pkg/controllers/cluster/properties/identity_migration_test.go:102
- The map value type is *coreapi.UserAssignedIdentity, but the test uses a non-pointer composite literal. This won’t compile; use &coreapi.UserAssignedIdentity{...}.
testIdentityResourceID: {
ClientID: stringPtr(testClientID),
PrincipalID: stringPtr(testPrincipalID),
},
},
backend/pkg/controllers/cluster/properties/identity_migration_test.go:121
- The map value type is *coreapi.UserAssignedIdentity, but the test uses a non-pointer composite literal. This won’t compile; use &coreapi.UserAssignedIdentity{...}.
testIdentityResourceID: {
ClientID: stringPtr(testClientID),
PrincipalID: stringPtr(testPrincipalID),
},
},
backend/pkg/controllers/cluster/properties/identity_migration_test.go:144
- The map value type is *coreapi.UserAssignedIdentity, but the test uses a non-pointer composite literal ({}). This won’t compile; use &coreapi.UserAssignedIdentity{} instead.
testIdentityResourceID: {},
backend/pkg/controllers/cluster/properties/identity_migration_test.go:178
- The map value type is *coreapi.UserAssignedIdentity, but the test uses a non-pointer composite literal ({}). This won’t compile; use &coreapi.UserAssignedIdentity{} instead.
mixedCaseIdentityResourceID: {},
backend/pkg/controllers/cluster/properties/identity_migration_test.go:195
- The map value type is *coreapi.UserAssignedIdentity, but the test uses a non-pointer composite literal ({}). This won’t compile; use &coreapi.UserAssignedIdentity{} instead.
testIdentityResourceID: {},
backend/pkg/controllers/cluster/properties/identity_migration_test.go:212
- The map value type is *coreapi.UserAssignedIdentity, but the test uses a non-pointer composite literal ({}). This won’t compile; use &coreapi.UserAssignedIdentity{} instead.
testIdentityResourceID: {},
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:195
- Grammar in this comment is a bit unclear (e.g., “independently on what identity is request”). Consider tightening the wording for readability.
// 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 on what identity is request. The returned information is
// the information associated to the "MI Mock" identity.
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:217
- Extra period at the end of this comment (“should not happen..”).
// If this occurs, we return an error instead of accumulating it as this is unexpected and should not happen..
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| testIdentityResourceID: { | ||
| ClientID: stringPtr(testClientID), | ||
| PrincipalID: stringPtr(testPrincipalID), | ||
| }, | ||
| }, |
| lowerResourceIDStr: { | ||
| OperatorName: operatorName, | ||
| ResourceID: metadataapi.Must(azcorearm.ParseResourceID(lowerResourceIDStr)), | ||
| ClientID: stringPtr(clientID), | ||
| PrincipalID: stringPtr(principalID), | ||
| }, |
| // TODO do we actually want to implement continuous syncing of the identities as of now? Changing this over time | ||
| // would have downstream effects and we do not have the support for those other pieces yet. | ||
| func (c *fetchMSIIdentitiesInfoSyncer) SyncOnce(ctx context.Context, key controllerutils.HCPClusterKey) error { |
| // It periodically checks each cluster and populates Identity.UserAssignedIdentities | ||
| // from ServiceProviderCluster.Status.MSIManagedIdentities when the identity map | ||
| // is missing keys, has empty ClientID/PrincipalID, or has unexpected entries. | ||
| // Map keys in Identity keep the casing from CustomerProperties; SPC lookups use | ||
| // lowercased resource IDs. Keys remain even when SPC does not yet have a matching | ||
| // identity entry. |
PoC that builds on top of #6301.
The FetchMSIIdentitiesInfoi controller now writes ClientID/PrincipalID into
ServiceProviderCluster.Status.MSIManagedIdentities (lowercased resource IDs)
instead of HCPOpenShiftCluster.Identity.
The IdentityMigration controller has been changed to fill the existing
Cluster's Identity.UserAssignedIdentities entries from
that SPC status via case-insensitive lookup, preserving Identity map key casing
and leaving the keys in Identity.UserAssignedIdentities unchanged when there's no
corresponding match in SPC. Entries that exist in SPC but don't exist in Identity.UserAssignedIdentities
are also not added to Identity.UserAssignedIdentities. It no longer reads from Cluster Service.