feat: add controller that retrieves information about MSI based identities - #6301
Conversation
33ef906 to
6778e34
Compare
| "github.com/Azure/ARO-HCP/internal/utils" | ||
| ) | ||
|
|
||
| const fetchMSIIdentitiesInfoControllerName = "FetchMSIIdentitiesInfo" |
There was a problem hiding this comment.
Alternative name: FetchMIDataplaneBasedIdentitiesInfo
| @@ -0,0 +1,234 @@ | |||
| // Copyright 2026 Microsoft Corporation | |||
There was a problem hiding this comment.
TODO decide where to place this file
There was a problem hiding this comment.
What do you think of https://github.com/miguelsorianod/ARO-HCP/tree/6778e3492763e31429168696ae9995407cfa98a8/backend/pkg/controllers/clusterpropertiescontroller package for placement of this controller and the one in #6300 ?
There was a problem hiding this comment.
Edit: with the re-arrangement of the controllers, the new pkg name is https://github.com/miguelsorianod/ARO-HCP/blob/5227da6dd31b94b095ea39560598ea3d0f5e14dc/backend/pkg/controllers/cluster/properties/
There was a problem hiding this comment.
This has been moved to pkg/controllers/cluster/identity
There was a problem hiding this comment.
Pull request overview
Adds a new backend cluster-watching controller that resolves MSI user-assigned identity metadata (ClientID/PrincipalID) via Microsoft’s Managed Identities Data Plane service and persists it into the cluster’s Identity.UserAssignedIdentities stored in Cosmos DB.
Changes:
- Introduces
FetchMSIIdentitiesInfocontroller syncer to fetch and persist ClientID/PrincipalID for cluster-managed identities. - Wires the new controller into the backend leader-election controller runner.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| backend/pkg/controllers/fetch_msi_identities_info.go | Adds the new controller that calls the MI dataplane and updates HCPOpenShiftCluster.Identity.UserAssignedIdentities in Cosmos. |
| backend/pkg/app/backend.go | Starts the new controller under leader election alongside existing backend controllers. |
Comments suppressed due to low confidence (2)
backend/pkg/controllers/fetch_msi_identities_info.go:128
- SyncOnce iterates over existingCluster.Identity.UserAssignedIdentities without guarding against Identity == nil or an empty/nil map. HCPOpenShiftCluster.Identity is optional (omitempty) and other controllers (e.g. IdentityMigration) explicitly handle Identity == nil, so this can panic.
// updating the managed identities. Maybe we could have a case where the resourceid is the same but the clientid/principalid has changed? Is this
// what we want?
var identitiesToSyncResourceIDStrs []string
backend/pkg/controllers/fetch_msi_identities_info.go:92
- Use the shared controller name constant instead of a string literal so all controller identity surfaces (metrics/logging/degraded controller docs) stay consistent.
}
controller := controllerutils.NewClusterWatchingController(
| fetchMSIIdentitiesInfoController := controllers.NewFetchMSIIdentitiesInfoController( | ||
| b.options.CosmosDBClient, | ||
| backendInformers, | ||
| b.options.FPAMIDataplaneClientBuilder, | ||
| } |
| _, replacementIdentity, ok := c.findUserAssignedIdentityByResourceID(replacement.Identity.UserAssignedIdentities, credentialResourceID) | ||
| if !ok { | ||
| syncErrors = append(syncErrors, utils.TrackError(fmt.Errorf("unexpected Managed Identities Data Plane Credential %s Resource ID is not found in the cluster's identities", credentialResourceID))) | ||
| continue | ||
| } | ||
|
|
||
| // TODO should we check if existingCluster/replacementIdentity.Identity is nil and initialize it? or are we guaranteed that after Frontend stores to cosmos | ||
| // that section is not nil? | ||
| // TODO as of now if the returned information from the MIDataplane has nil/empty ClientID/PrincipalID we don't set it in the replacement. Do | ||
| // we want to follow that approach or 1:1 set what's returned from the MIDataplane? That means that if for some reason it's set and the MIDataplane | ||
| // stops setting it we would be unsetting it too. | ||
| if fpaMIDataplaneCredential.ClientID != nil && len(*fpaMIDataplaneCredential.ClientID) > 0 { | ||
| replacementIdentity.ClientID = fpaMIDataplaneCredential.ClientID | ||
| } else { | ||
| syncErrors = append(syncErrors, utils.TrackError(fmt.Errorf("unexpected Managed Identities Data Plane Credential %s Client ID is nil or empty", credentialResourceID))) | ||
| } |
| // future managed-identity updates can refresh the values. | ||
| func NewFetchMSIIdentitiesInfoController( | ||
| resourcesDBClient database.ResourcesDBClient, | ||
| activeOperationLister listers.ActiveOperationLister, |
| // fetchMSIIdentitiesInfoSyncer fetches ClientID and PrincipalID for the | ||
| // cluster's MSI-based user-assigned managed identities and writes them onto | ||
| // HCPOpenShiftCluster.Identity.UserAssignedIdentities in Cosmos. |
| return existingCluster.ServiceProviderProperties.DeletionTimestamp == nil | ||
| } | ||
|
|
||
| func (c *fetchMSIIdentitiesInfoSyncer) SyncOnce(ctx context.Context, key controllerutils.HCPClusterKey) error { |
| return utils.TrackError(fmt.Errorf("failed to get Managed Identities Data Plane Credentials: %w", err)) | ||
| } | ||
| if len(fpaMIDataplaneCredentials.ExplicitIdentities) == 0 { | ||
| return utils.TrackError(fmt.Errorf("returned number of Managed Identities Data Plane Credentials is 0")) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
backend/pkg/app/backend.go:851
- The controller initialization block does not compile: it references a non-existent option field (CosmosDBClient), omits the activeOperationLister parameter required by NewFetchMSIIdentitiesInfoController, and has mismatched parentheses/braces.
fetchMSIIdentitiesInfoController := controllers.NewFetchMSIIdentitiesInfoController(
b.options.CosmosDBClient,
backendInformers,
b.options.FPAMIDataplaneClientBuilder,
}
backend/pkg/controllers/fetch_msi_identities_info.go:184
- findUserAssignedIdentityByResourceID can return a nil *UserAssignedIdentity (older Cosmos records can contain present-but-nil map values). The current code will panic when assigning ClientID/PrincipalID. Capture the Cosmos key and initialize the map value when it is nil.
_, replacementIdentity, ok := c.findUserAssignedIdentityByResourceID(replacement.Identity.UserAssignedIdentities, credentialResourceID)
if !ok {
syncErrors = append(syncErrors, utils.TrackError(fmt.Errorf("unexpected Managed Identities Data Plane Credential %s Resource ID is not found in the cluster's identities", credentialResourceID)))
continue
}
backend/pkg/controllers/fetch_msi_identities_info.go:115
- This controller introduces non-trivial behavior (dataplane calls, case-insensitive matching, partial update + error aggregation) but has no unit tests. Adding tests for the nil/empty identity cases and for nil map values would help prevent regressions.
func (c *fetchMSIIdentitiesInfoSyncer) SyncOnce(ctx context.Context, key controllerutils.HCPClusterKey) error {
existingCluster, err := c.resourcesDBClient.HCPClusters(key.SubscriptionID, key.ResourceGroupName).Get(ctx, key.HCPClusterName)
if database.IsNotFoundError(err) {
return nil // cluster doesn't exist, no work to do
}
if err != nil {
return utils.TrackError(fmt.Errorf("failed to get Cluster: %w", err))
}
backend/pkg/controllers/fetch_msi_identities_info.go:136
- Spelling/grammar in this comment: "independently on what identity is request" should be "independently of which identity is requested".
// 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.
| // TODO do we need to check if existingCluster.Identity is nil or are we guaranteed that after Frontend stores to cosmos | ||
| // that section is not nil? | ||
| // TODO do we need to check if existingCluster.Identity.UserAssignedIdentities is nil or are we guaranteed that after Frontend stores to cosmos | ||
| // that section is not nil? | ||
| // TODO we do not check if the ClientID/PrincipalID is set to stop early. This is because in the future we might allow | ||
| // updating the managed identities. Maybe we could have a case where the resourceid is the same but the clientid/principalid has changed? Is this | ||
| // what we want? | ||
| var identitiesToSyncResourceIDStrs []string | ||
| for identityResourceIDStr := range existingCluster.Identity.UserAssignedIdentities { | ||
| identitiesToSyncResourceIDStrs = append(identitiesToSyncResourceIDStrs, identityResourceIDStr) | ||
| } |
| // non-empty values. | ||
| // 4. Replaces the HCPCluster document only when the identity map changed. | ||
| // | ||
| // It does not stop early when ClientID/PrincipalID are already set, so |
| // TODO should we check if existingCluster/replacementIdentity.Identity is nil and initialize it? or are we guaranteed that after Frontend stores to cosmos | ||
| // that section is not nil? |
There was a problem hiding this comment.
|
|
||
| // fetchMSIIdentitiesInfoSyncer fetches ClientID and PrincipalID for the | ||
| // cluster's MSI-based user-assigned managed identities and writes them onto | ||
| // HCPOpenShiftCluster.Identity.UserAssignedIdentities in Cosmos. |
There was a problem hiding this comment.
#6300 for DP identities stores the field in ServiceProviderCluster: https://github.com/miguelsorianod/ARO-HCP/blob/61e2bf3a399569767391971651346b9440559eb3/backend/pkg/controllers/fetch_data_plane_operators_managed_identities_info.go#L38
Meaning that consumer of these info e.g #6269 has to read these info from two objects, this adds congnitive load; can we consider store both CP, DP, SMI extra info in one cosmo object i.e ServiceProviderCluster in this case?
And when presenting the info to the API for the .Identity.UserAssignedIdentities we read from the extra info from the ServiceProviderCluster
Thoughts Miguel Soriano (@miguelsorianod) David Eads (@deads2k) ?
There was a problem hiding this comment.
The decision to store on Cluster object is based on "is the information needed to reply to the user when reading from ARM". If the answer is yes, then the field goes on the Cluster object. If the answer is no, then the field goes on the ServiceProviderCluster object.
There was a problem hiding this comment.
What about ETag concerns? For the data plane operators identities (the other PR), based on that criteria we would put it the extra information associated to the resource ids in ServiceProviderCluster. However, at that point because the original resource ids come from the Cluster resource, if we have its associated extra information in the ServiceProviderCluster, we would have no guarantee that the information is consistent between the Cosmos resources. If we were to place them in the Cluster's ServiceProviderProperties in that case we would ensure that the data is consistent because the replace would fail if the resource ids have changed (which are set in .customerProperties.platform.operatorsAuthentication.userAssignedIdentities of the Cluster type).
Isn't in that case better to put it in the Cluster object, even when it's not exposed to the user?
|
|
||
| // fetchMSIIdentitiesInfoSyncer fetches ClientID and PrincipalID for the | ||
| // cluster's MSI-based user-assigned managed identities and writes them onto | ||
| // HCPOpenShiftCluster.Identity.UserAssignedIdentities in Cosmos. |
There was a problem hiding this comment.
The decision to store on Cluster object is based on "is the information needed to reply to the user when reading from ARM". If the answer is yes, then the field goes on the Cluster object. If the answer is no, then the field goes on the ServiceProviderCluster object.
| } | ||
|
|
||
| func (c *fetchMSIIdentitiesInfoSyncer) needsWork(existingCluster *api.HCPOpenShiftCluster) bool { | ||
| return existingCluster.ServiceProviderProperties.DeletionTimestamp == nil |
There was a problem hiding this comment.
needs EarliestRecheckTime. We have enough of these, I'm willing to consider a ControllerToEarliestRecheckTime map[string]*metav1.Time on ServiceProviderCluster.
| if err != nil { | ||
| return utils.TrackError(fmt.Errorf("failed to get Managed Identities Data Plane Credentials: %w", err)) | ||
| } | ||
| if len(fpaMIDataplaneCredentials.ExplicitIdentities) == 0 { |
There was a problem hiding this comment.
seems like the check below covers this and is more clear.
| // control plane operators managed identities and for the service managed identity and store it in the Managed | ||
| // Identities Key Vault (a Management Cluster scoped resource). Do we want to do it here at the same time because | ||
| // we are already calling the Managed Identities Data Plane Service and getting credentials here? As relevant context, | ||
| // these set of initial credentials should be stored in the Managed Identities Key Vault before creating the HostedCluster | ||
| // and those credentials have a limited lifespan (unknown which without investigating further). |
There was a problem hiding this comment.
these look like separate concerns to me
- list of identities and information
- credentials for those identities
and we'd want separate consistency check frequency, error handling, and retries.
| // and the MI dataplane may return a different casing than Cosmos. | ||
| // It returns the Cosmos map key (preserving stored casing), the matching | ||
| // identity value (or nil if not found), and whether a match was found. | ||
| func (c *fetchMSIIdentitiesInfoSyncer) findUserAssignedIdentityByResourceID(identities map[string]*arm.UserAssignedIdentity, resourceIDStr string) (string, *arm.UserAssignedIdentity, bool) { |
There was a problem hiding this comment.
super ugly. Can we normalize into lowercase in a future step?
| // TODO as of now if the returned information from the MIDataplane has nil/empty ClientID/PrincipalID we don't set it in the replacement. Do | ||
| // we want to follow that approach or 1:1 set what's returned from the MIDataplane? That means that if for some reason it's set and the MIDataplane | ||
| // stops setting it we would be unsetting it too. | ||
| if fpaMIDataplaneCredential.ClientID != nil && len(*fpaMIDataplaneCredential.ClientID) > 0 { |
There was a problem hiding this comment.
code would be a lot easier to read if you checked this and the principalID at the top of the for loop and errored and continued. Is it really valuable to only write half of it? Seems like it would just fail in a different spot.
When there is no known value, why leave old data versus clearing the data? You chose the opposite path in your other identity controller. I'm inclined to be consistent.
6778e34 to
4d93278
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 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 (6)
backend/pkg/controllers/fetch_msi_identities_info.go:218
- replacement.Identity.UserAssignedIdentities can contain present-but-nil values (older Cosmos records). findUserAssignedIdentityByResourceID returns the map value as-is, so replacementIdentity can be nil and the subsequent field assignments will panic. Ensure the map entry is initialized before writing ClientID/PrincipalID.
_, replacementIdentity, ok := c.findUserAssignedIdentityByResourceID(replacement.Identity.UserAssignedIdentities, credentialResourceID)
if !ok {
// The MIDataplane service should return a Resource ID that matches one of the identities in the cluster's identities. That is even if the identity actually does not exist anymore in Azure.
// If it does not, we return an error instead of accumulating it.
return utils.TrackError(fmt.Errorf("unexpected Managed Identities Data Plane Credential %s Resource ID is not found in the cluster's identities", credentialResourceID))
backend/pkg/controllers/fetch_msi_identities_info.go:251
- This controller reads/writes new Cosmos fields (Identity.UserAssignedIdentities ClientID/PrincipalID and ServiceProviderProperties.MSIIdentitiesEarliestRecheckTime). docs/cosmos-data-flow.md should be updated to reflect these additional reads/writes so it stays in sync with the implementation.
_, err = c.resourcesDBClient.HCPClusters(existingCluster.ID.SubscriptionID, existingCluster.ID.ResourceGroupName).Replace(ctx, replacement, nil)
if cosmosstorageutils.IsPreconditionFailedError(err) {
// Status (including any new MSIIdentitiesEarliestRecheckTime) was not written.
// needsWork will still see the previously persisted value.
return nil
backend/pkg/controllers/fetch_msi_identities_info.go:154
- The new controller has non-trivial reconciliation logic (throttling via MSIIdentitiesEarliestRecheckTime, case-insensitive resource ID matching, and handling nil identity map values). Please add unit tests covering these behaviors to prevent regressions, consistent with other controllers in backend/pkg/controllers.
// 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 {
existingCluster, err := c.resourcesDBClient.HCPClusters(key.SubscriptionID, key.ResourceGroupName).Get(ctx, key.HCPClusterName)
if cosmosstorageutils.IsNotFoundError(err) {
return nil // cluster doesn't exist, no work to do
}
if err != nil {
return utils.TrackError(fmt.Errorf("failed to get Cluster: %w", err))
backend/pkg/controllers/fetch_msi_identities_info.go:168
- SyncOnce assumes existingCluster.Identity and Identity.UserAssignedIdentities are non-nil; clusters with a nil identity (or no user-assigned identities) will panic when ranging the map. Add a guard to safely no-op when there is nothing to sync.
This issue also appears on line 214 of the same file.
var identitiesToSyncResourceIDStrs []string
for identityResourceIDStr := range existingCluster.Identity.UserAssignedIdentities {
if len(identityResourceIDStr) == 0 {
// This should not happen, so if it does, we return an error instead of accumulating it.
return utils.TrackError(fmt.Errorf("unexpected empty identity Resource ID string"))
}
identitiesToSyncResourceIDStrs = append(identitiesToSyncResourceIDStrs, identityResourceIDStr)
}
backend/pkg/controllers/fetch_msi_identities_info.go:239
- MSIIdentitiesEarliestRecheckTime is described as a throttle once the desired state is true, but the controller sets a long recheck interval unconditionally—even when one or more identities still have empty ClientID/PrincipalID. That can delay convergence if identities become available later. Consider only setting a long recheck time when all identities are resolved; otherwise keep it nil so the controller retries promptly.
// Set an earliest recheck time for the controller so we do not hit the Managed Identities Data Plane service too often.
// The value below is only honored once Replace persists it. A Replace failure leaves Cosmos unchanged, so needsWork will still see the
// previously persisted value (if any).
// TODO this is more or less reasonable for now because we currently do not support identities replacement, but at the moment we need to support
// that we will need to change this because we should detect when the identities provided by the end-user are changed. A possibility could be
// to store the information in a separate field so we can then compare the previous and latest evaluated values and use that as one of the conditions
backend/pkg/controllers/fetch_msi_identities_info.go:173
- Minor grammar in the comment: "identity is request" should be "identity is requested", and "independently on" should be "independently of".
// 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.
4d93278 to
74c8e64
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (5)
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:242
- This controller writes new Cosmos fields (MSIIdentitiesEarliestRecheckTime and ClientID/PrincipalID under Identity.UserAssignedIdentities). Please update docs/cosmos-data-flow.md so the documented read/write flows stay accurate.
replacement.ServiceProviderProperties.MSIIdentitiesEarliestRecheckTime = &earliestRecheckAt
identitiesUnchanged := equality.Semantic.DeepEqual(replacement.Identity.UserAssignedIdentities, existingCluster.Identity.UserAssignedIdentities)
recheckUnchanged := equality.Semantic.DeepEqual(replacement.ServiceProviderProperties.MSIIdentitiesEarliestRecheckTime, existingCluster.ServiceProviderProperties.MSIIdentitiesEarliestRecheckTime)
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:106
- There’s extensive unit test coverage for other cluster controllers under backend/pkg/controllers/cluster/**. This new controller introduces non-trivial behavior (dataplane client calls, case-insensitive matching, recheck-time gating, and Cosmos Replace semantics) but has no tests in this PR.
func NewFetchMSIIdentitiesInfoController(
clock utilsclock.PassiveClock,
resourcesDBClient corecosmosstorage.ResourcesDBClient,
backendInformers coreinformers.BackendInformers,
fpaMIdataplaneClientBuilder azureclient.FPAMIDataplaneClientBuilder,
) controllerutils.Controller {
if clock == nil {
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:239
- MSIIdentitiesEarliestRecheckTime is set even when some identities are still unresolved (nil/empty ClientID/PrincipalID). That makes the controller wait ~12h before trying again, which conflicts with the field comment (“avoid recheck when desired state is true”). Consider only setting the long recheck interval once all identities are fully resolved; otherwise keep it nil (or use a short retry).
// Set an earliest recheck time for the controller so we do not hit the Managed Identities Data Plane service too often.
// The value below is only honored once Replace persists it. A Replace failure leaves Cosmos unchanged, so needsWork will still see the
// previously persisted value (if any).
// TODO this is more or less reasonable for now because we currently do not support identities replacement, but at the moment we need to support
// that we will need to change this because we should detect when the identities provided by the end-user are changed. A possibility could be
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:173
- Spelling/grammar: “independently on what identity is request” should be “regardless of which identity is requested” (or similar).
// 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.
internal/api/coreapi/types_cluster.go:196
- Written-by annotations in core API types appear to use the controller name (e.g. "ClusterPropertiesSync"), without a "Controller" suffix. This new field’s annotation is the only one using "...Controller", which makes grepping/auditing writers inconsistent.
// Written by: FetchMSIIdentitiesInfoController
| var identitiesToSyncResourceIDStrs []string | ||
| for identityResourceIDStr := range existingCluster.Identity.UserAssignedIdentities { | ||
| if len(identityResourceIDStr) == 0 { | ||
| // This should not happen, so if it does, we return an error instead of accumulating it. | ||
| return utils.TrackError(fmt.Errorf("unexpected empty identity Resource ID string")) | ||
| } | ||
| identitiesToSyncResourceIDStrs = append(identitiesToSyncResourceIDStrs, identityResourceIDStr) | ||
| } |
| _, replacementIdentity, ok := c.findUserAssignedIdentityByResourceID(replacement.Identity.UserAssignedIdentities, credentialResourceID) | ||
| if !ok { | ||
| // The MIDataplane service should return a Resource ID that matches one of the identities in the cluster's identities. That is even if the identity actually does not exist anymore in Azure. | ||
| // If it does not, we return an error instead of accumulating it. | ||
| return utils.TrackError(fmt.Errorf("unexpected Managed Identities Data Plane Credential %s Resource ID is not found in the cluster's identities", credentialResourceID)) | ||
| } | ||
|
|
||
| // For ClientID and PrincipalID of the identity, we set the value returned from the MIDataplane service. This includes the cases where the | ||
| // value is nil or empty. At the moment of writing this (2026-08-11), when the actual identity does not exist in Azure, the MIDataplane service | ||
| // returns null for ClientID and PrincipalID. | ||
| replacementIdentity.ClientID = fpaMIDataplaneCredential.ClientID | ||
| replacementIdentity.PrincipalID = fpaMIDataplaneCredential.ObjectID |
74c8e64 to
89fa24f
Compare
|
[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 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (6)
internal/api/coreapi/types_serviceprovider_cluster.go:301
- Typo in comment: "may be be" -> "may be".
// It may be be nil or empty.
internal/api/coreapi/types_serviceprovider_cluster.go:310
- The
ResourceIDfield comment repeats the same sentence twice, which is likely accidental and adds noise to generated docs.
// 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"`
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:222
desiredMSIResourceIDsMatchSPConly checks for key presence inControlPlaneOperatorsIdentities. If the map contains a key with a nil value (or a value with nilResourceID), the function can incorrectly treat SPC as "matching" and honorEarliestRecheckTime, delaying a necessary refetch.
resourceIDStr := strings.ToLower(identity.resourceID.String())
_, ok := spcMSIManagedIdentities.ControlPlaneOperatorsIdentities[resourceIDStr]
if !ok {
return false
}
internal/api/coreapi/types_serviceprovider_cluster.go:297
- Typo in comment: "may be be" -> "may be".
This issue also appears on line 301 of the same file.
// It may be be nil or empty.
backend/pkg/controllers/cluster/identity/cluster_identity_sync.go:190
- The service-managed-identity sync branch (the
else if spcServiceManagedIdentity != nil ...path) is new behavior but has no unit test coverage incluster_identity_sync_test.go. Adding a focused test would help prevent regressions around case-insensitive matching and nil/empty ClientID/PrincipalID propagation.
// 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
principalID = spcServiceManagedIdentity.PrincipalID
internal/api/coreapi/types_serviceprovider_cluster.go:293
- This comment says the identity is "keyed by OperatorName", but the map key for
ControlPlaneOperatorsIdentitiesis the fully-lowercased Resource ID. That mismatch makes the API docs confusing for readers.
This issue also appears on line 308 of the same file.
// ResourceID is the Azure Resource ID of the Azure User Assigned Managed Identity that is associated to the corresponding
// control plane operator (keyed by OperatorName). Its value comes from the Cluster's CustomerProperties.
// The ControlPlaneOperatorsIdentities map key is the fully lowercased form
// of this ID used for lookups.
// Its value comes from the Cluster's CustomerProperties.
| name: "regex anchored to suffix", | ||
| defaultDuration: 30 * time.Second, | ||
| overrides: []InertiaController{ | ||
| {ControllerNameMatcher: regexp.MustCompile(`Migration$`), Duration: 90 * time.Second}, | ||
| }, | ||
| controllerName: "IdentityMigration", | ||
| controllerName: "FooControllerName", | ||
| expectedDuration: 90 * time.Second, |
| // ControlPlaneOperatorsIdentities is a map containing extra information about | ||
| // the Managed Service Identity (MSI) based Azure User-Assigned Managed Identities | ||
| // for the cluster's control plane operators. |
There was a problem hiding this comment.
where will these pods run?
| // outside of their active phases. Order of at least six hours is, with | ||
| // durations up to 24 hours considered normal. | ||
| // Written by: FetchMSIIdentitiesInfo | ||
| EarliestRecheckTime *metav1.Time `json:"earliestRecheckTime,omitempty"` |
There was a problem hiding this comment.
I still predict we'll want a shared map for this, but I'll allow it to start.
| // ServiceManagedIdentity holds resolved ClientID/PrincipalID for the cluster's | ||
| // service managed identity. |
There was a problem hiding this comment.
useless description. What uses this later, where does it run, why is it special, what does it do?
| // The ControlPlaneOperatorsIdentities map key is the fully lowercased form | ||
| // of this ID used for lookups. | ||
| // Its value comes from the Cluster's CustomerProperties. | ||
| ResourceID *azcorearm.ResourceID `json:"resourceId,omitempty"` |
There was a problem hiding this comment.
| ResourceID *azcorearm.ResourceID `json:"resourceId,omitempty"` | |
| UserManagedIdentityResourceID *azcorearm.ResourceID `json:"resourceId,omitempty"` |
| 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"` |
There was a problem hiding this comment.
| ResourceID *azcorearm.ResourceID `json:"resourceId,omitempty"` | |
| UserManagedIdentityResourceID *azcorearm.ResourceID `json:"resourceId,omitempty"` |
| // Only honor EarliestRecheckTime when the desired identity set still matches | ||
| // SPC. Any mismatch (or future "must work now" conditions added alongside | ||
| // this check) should fall through to return true and query the dataplane. | ||
| if c.desiredMSIResourceIDsMatchSPC(desiredIdentitiesToFetch, existingSPC) { |
There was a problem hiding this comment.
don't abbreviate spc
| } | ||
|
|
||
| func (c *fetchMSIIdentitiesInfoSyncer) SyncOnce(ctx context.Context, key controllerutils.HCPClusterKey) error { | ||
| existingCluster, err := c.resourcesDBClient.HCPClusters(key.SubscriptionID, key.ResourceGroupName).Get(ctx, key.HCPClusterName) |
There was a problem hiding this comment.
use lister. eventual consistency is fine.
| spcCRUD := c.resourcesDBClient.ServiceProviderClusters(key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName) | ||
| existingSPC, err := spcCRUD.Get(ctx, coreapi.ServiceProviderClusterResourceName) |
There was a problem hiding this comment.
same, no reason for live get.
| 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 { |
There was a problem hiding this comment.
why not just do the easy deepequal on everything?
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
acceptable controller behavior. Better be right about the claims of the azure API. Guess we'll find out.
| // OperatorName is the name of the control plane operator associated to the Azure User Assigned Managed Identity. | ||
| // The set of recognized operator names by the service is located in internal/azure/cluster_scoped_identities_config.go. | ||
| // Its value comes from the Cluster's CustomerProperties. | ||
| OperatorName string `json:"operatorName,omitempty"` |
There was a problem hiding this comment.
We cannot introduce the current design as is regarding OperatorName:
Although right now we enforce that each identity is used in one operator only, there are plans to relax that restriction. This would make this fall short.
The options at that point that I can think of are:
- Change this to OperatorNames []string. However, then further questions arise around what controller would be the responsible of updating them. The fetch msi identities info would have more responsibilities which would be also detecting changes on operatornames and have unecessary checks against Azure when only the operatornames change. Alternatives would be writing their own dedicated controllers but then we would have two controllers that potentially write the same parent entries
- Do not introduce OperatorName at all. In that case we will lose the ability to correlate the identity resource id to the operator name from the information in the SPC itself. It should be possible to iterater over the Cluster's properties and see what operator names match the resourceids, but that's interaction with the other type
- Key by operator name instead of resourceid at the SPC level. However, that has other downsides where
identitydoesn't contain the operatorName, as well as we would end up with duplicated resourceids in the spc across operators entries
There was a problem hiding this comment.
Even in the case of the current PR where OperatorName is a single string, that's not good enough either because at some point the resourceid can be associated to a different operator
There was a problem hiding this comment.
What we ended up deciding is to remove OperatorName. Which operators are associated to each resource id is tracked on the Cluster document: Cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators.
and it will need to be reconstructed by combining both documents information
|
/hold until #6301 (comment) is resolved, changes are reviewed and unit tests implemented (unit tests are incomplete as well as need to be reviewed for correctness as the design has iterated several times) |
fc66c4d to
0ca61c1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 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 (6)
backend/pkg/utils/statusutils/inertia_test.go:77
- This test case expects the
Migration$override to apply, butcontrollerNameno longer matches that regex, so the assertion will fail.
controllerName: "FooControllerName",
internal/api/coreapi/types_serviceprovider_cluster.go:323
- The
ServiceManagedIdentity.ResourceIDcomment duplicates the same sentence twice ("Its value comes from...").
// 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"`
internal/api/coreapi/types_serviceprovider_cluster.go:315
- Typo/grammar in the
PrincipalIDcomment: it’s missing "is" after the field name and "may be be" has a duplicated word.
// 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"`
internal/api/coreapi/types_serviceprovider_cluster.go:307
- The
ResourceIDfield comment says the identity is "keyed by OperatorName", but this struct is stored in a map keyed by lowercased resource ID (per the parent field comment). This is confusing/inconsistent documentation.
// ResourceID is the Azure Resource ID of the Azure User Assigned Managed Identity that is associated to the corresponding
// control plane operator (keyed by OperatorName). Its value comes from the Cluster's CustomerProperties.
// The ControlPlaneOperatorsIdentities map key is the fully lowercased form
// of this ID used for lookups.
// Its value comes from the Cluster's CustomerProperties.
internal/api/coreapi/types_serviceprovider_cluster.go:311
- Typo/grammar in the
ClientIDcomment: "Client ID" is missing an article and "may be be" has a duplicated word.
This issue also appears in the following locations of the same file:
- line 312
- line 321
// 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"`
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:164
- With a 1-minute cooldown, this controller will re-run very frequently per cluster. Since
SyncOncealways does Cosmos reads for both the Cluster and ServiceProviderCluster (and only then checksEarliestRecheckTime), this can drive significant RU/latency even whenEarliestRecheckTimeis far in the future. Consider either (a) using informer/lister cache reads to decideneedsWorkbefore hitting Cosmos, and/or (b) increasing the cooldown and relying on informer events + error requeues for responsiveness.
1*time.Minute,
syncer,
0ca61c1 to
4cdd182
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (4)
backend/pkg/utils/statusutils/inertia_test.go:79
- The test case "regex anchored to suffix" uses a controllerName that does not match the
Migration$regex, so the expected 90s override will never apply and the test will fail.
name: "regex anchored to suffix",
defaultDuration: 30 * time.Second,
overrides: []InertiaController{
{ControllerNameMatcher: regexp.MustCompile(`Migration$`), Duration: 90 * time.Second},
},
controllerName: "FooControllerName",
expectedDuration: 90 * time.Second,
},
internal/api/coreapi/types_serviceprovider_cluster.go:314
- The PrincipalID field comment is missing "is" and repeats "be be"; this reads as a typo and can be confusing for generated docs.
// 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"`
internal/api/coreapi/types_serviceprovider_cluster.go:322
- The ResourceID field comment repeats the same sentence twice ("Its value comes from...") which looks accidental.
// 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"`
internal/api/coreapi/types_serviceprovider_cluster.go:310
- Typo in the comment: "It may be be nil or empty."
This issue also appears in the following locations of the same file:
- line 311
- line 320
// 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"`
| identities := &msiBasedIdentitiesToFetch{} | ||
|
|
||
| for operatorName, operatorIdentityResourceID := range cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators { | ||
| if len(operatorName) == 0 { | ||
| return nil, utils.TrackError(fmt.Errorf("unexpected empty operator name for control plane operator")) | ||
| } | ||
| if operatorIdentityResourceID == nil { | ||
| return nil, utils.TrackError(fmt.Errorf("unexpected nil identity Resource ID string for control plane operator %q", operatorName)) | ||
| } | ||
|
|
||
| identities.controlPlaneOperators = append(identities.controlPlaneOperators, &controlPlaneOperatorIdentityToFetch{ | ||
| resourceID: coreapi.DeepCopyResourceID(operatorIdentityResourceID), | ||
| }) | ||
| } |
|
/hold until changes are reviewed and unit tests implemented (unit tests are incomplete as well as need to be reviewed for correctness as the design has iterated several times) |
…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.
4cdd182 to
956c5fd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (5)
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:278
- This strict count check will fail if the request contains duplicate identity IDs (currently possible due to per-operator collection) and can also be fragile if the MI dataplane returns a subset/unique list. Once identities are de-duplicated, this becomes safer; additionally consider validating that every requested (lowercased) resource ID has a returned entry (and optionally error on unexpected extras) rather than relying on length equality.
if len(fpaMIDataplaneCredentials.ExplicitIdentities) != len(identitiesToSyncResourceIDStrs) {
return utils.TrackError(fmt.Errorf("unexpected number of Managed Identities Data Plane Credentials. Expected: %d, Received: %d", len(identitiesToSyncResourceIDStrs), len(fpaMIDataplaneCredentials.ExplicitIdentities)))
}
internal/api/coreapi/types_serviceprovider_cluster.go:310
- Correct the typo 'be be' to 'be'.
// It may be be nil or empty.
ClientID *string `json:"clientId,omitempty"`
internal/api/coreapi/types_serviceprovider_cluster.go:321
- The
ResourceIDcomment duplicates the same sentence twice, which makes the field documentation harder to read and maintain. Remove the repeated sentence so the comment is a single clear statement.
// 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"`
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:261
- Correct grammar in the comment: 'identity is request' should be 'identity is requested'.
// 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_test.go:42
- Given the API/docs explicitly note that multiple operators may share a single identity, the matching logic should be covered by a test where
ControlPlaneOperatorscontains two operator names pointing at the same resource ID. Add a test case verifyingdesiredMSIResourceIDsMatchSPCbehaves correctly with shared identities (and that collection de-duplicates appropriately once fixed).
func TestDesiredMSIResourceIDsMatchSPC(t *testing.T) {
t.Parallel()
| for operatorName, operatorIdentityResourceID := range cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators { | ||
| if len(operatorName) == 0 { | ||
| return nil, utils.TrackError(fmt.Errorf("unexpected empty operator name for control plane operator")) | ||
| } | ||
| if operatorIdentityResourceID == nil { | ||
| return nil, utils.TrackError(fmt.Errorf("unexpected nil identity Resource ID string for control plane operator %q", operatorName)) | ||
| } | ||
|
|
||
| identities.controlPlaneOperators = append(identities.controlPlaneOperators, &controlPlaneOperatorIdentityToFetch{ | ||
| resourceID: coreapi.DeepCopyResourceID(operatorIdentityResourceID), | ||
| }) | ||
| } |
| // If the number of control plane operators is different, the identities do not match. | ||
| if len(desiredIdentitiesToFetch.controlPlaneOperators) != len(spcMSIManagedIdentities.ControlPlaneOperatorsIdentities) { | ||
| return false | ||
| } | ||
|
|
||
| for _, identity := range desiredIdentitiesToFetch.controlPlaneOperators { | ||
| // SPC map keys are lowercased strings. ResourceID.String() may re-canonicalize casing so we lowercase. | ||
| resourceIDStr := strings.ToLower(identity.resourceID.String()) |
| {ControllerNameMatcher: regexp.MustCompile(`Migration$`), Duration: 90 * time.Second}, | ||
| }, | ||
| controllerName: "IdentityMigration", | ||
| controllerName: "FooControllerName", |
|
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. |
|
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. |
We add a controller that retrieves the Client ID and Principal ID associated to the following identities associated to an ARO-HCP Cluster:
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 mock msi 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 side.