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.
| // 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. | ||
| fpaMIDataplaneClient, err := c.fpaMIdataplaneClientBuilder.ManagedIdentitiesDataplane(existingCluster.ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL) |
There was a problem hiding this comment.
require this in needswork.
| 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
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: miguelsorianod 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 |
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
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/cluster/identity/fetch_msi_identities_info.go:140
- needsWork only gates on MSIIdentitiesEarliestRecheckTime. This can (1) panic when existingCluster.Identity is nil and (2) incorrectly skip resolution when Identity.UserAssignedIdentities changes or contains entries with missing ClientID/PrincipalID, leaving identity metadata empty until the recheck timer expires.
earliestRecheckTime := existingCluster.ServiceProviderProperties.MSIIdentitiesEarliestRecheckTime
if earliestRecheckTime != nil && c.clock.Now().Before(earliestRecheckTime.Time) {
return false
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:215
- replacementIdentity can be nil when Cosmos contains key-only UserAssignedIdentities entries (nil map values). Assigning ClientID/PrincipalID on a nil pointer will panic; initialize the map value before writing fields.
_, 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/cluster/identity/fetch_msi_identities_info.go:88
- The doc comment says ClientID/PrincipalID are only set when the dataplane returns non-empty values, but the implementation assigns the returned values even when they are nil/empty (e.g. when an identity doesn't exist). Update the comment so it matches the actual behavior.
// 4. Matches each returned credential by ResourceID (case-insensitive.
// ARM IDs are case-insensitive and response order is not assumed)
// and sets ClientID and PrincipalID when the dataplane returns
// non-empty values.
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:148
- This controller introduces new reconciliation logic (needsWork gating, case-insensitive identity matching, and Cosmos writes for identity metadata) but has no unit tests. Add tests covering: Identity/UserAssignedIdentities nil handling, nil map values, needsWork behavior when a new identity is added while MSIIdentitiesEarliestRecheckTime is in the future, and correct updates when the dataplane returns nil IDs.
func (c *fetchMSIIdentitiesInfoSyncer) SyncOnce(ctx context.Context, key controllerutils.HCPClusterKey) error {
backend/pkg/controllers/cluster/identity/fetch_msi_identities_info.go:74
- Doc comment grammar: "independently on" and "associated to" are incorrect; also the phrasing is a bit unclear about what the fake client returns.
This issue also appears in the following locations of the same file:
- line 85
- line 138
// 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 requested. The returned information
// in those environments is the information associated to the "MI Mock" identity.
internal/api/coreapi/types_cluster.go:197
- This PR adds a new Cosmos-persisted field (MSIIdentitiesEarliestRecheckTime) written by a backend controller. docs/cosmos-data-flow.md should be updated to document the new read/write behavior so it stays in sync with the codebase documentation conventions.
// MSIIdentitiesEarliestRecheckTime is the earliest time at which
// FetchMSIIdentitiesInfo controller should re-query the Managed Identities Data Plane
// for ClientID/PrincipalID of Identity.UserAssignedIdentities. Nil means
// recheck immediately. The same recheck time applies across all entries in
// that map.
// This allows the controller to avoid repeatedly hitting the Managed
// Identities Data Plane to recheck that the desired state is true.
// Controllers should set this field with substantial jitter: without another
// concern, jitter of 50% is considered normal so that any storms are quickly
// dissipated. Additionally, long recheck times are recommended for resources
// outside of their active phases. Order of at least six hours is, with
// durations up to 24 hours considered normal.
// Written by: FetchMSIIdentitiesInfoController
MSIIdentitiesEarliestRecheckTime *metav1.Time `json:"msiIdentitiesEarliestRecheckTime,omitempty"`
| // We get all the Managed Identities information in a single Managed Identities Data Plane Credentials request to minimize | ||
| // calls to the Managed Identities Data Plane Service. | ||
| fpaMIDataplaneCredentialsRequest := dataplane.UserAssignedIdentitiesRequest{IdentityIDs: identitiesToSyncResourceIDStrs} | ||
| fpaMIDataplaneCredentials, err := fpaMIDataplaneClient.GetUserAssignedIdentitiesCredentials(ctx, fpaMIDataplaneCredentialsRequest) |
There was a problem hiding this comment.
When we resync here; what happens to the credentials already stored in KV will they continue to work?
There was a problem hiding this comment.
The credentials in KV should continue to work.
There was a problem hiding this comment.
to complete:
- New credentials are generated each time
- Previous creds are not invalidated, they continue to be valid until they expire
- Each credentials are generated with an expiration of 90d since cert credential was generated.
| // Match case-insensitively: ARM resource IDs are case-insensitive and the | ||
| // MI dataplane may return a different casing than Cosmos, as well as different order than | ||
| // how it's been requested. | ||
| // We do not store the Resource ID lowercased because the resource id ends up exposed to the end-user in the Cluster payload API response | ||
| // in the `identity` section and we want to preserve the casing as received from the original request. |
There was a problem hiding this comment.
we want to preserve the casing as received from the original request
Is this required. The code will be a lot cleaner if you don't do this.
There was a problem hiding this comment.
This information is provided in the identity section, and the resource ids are provided by the end-user. My understanding is that we need to preserve the casing because of ARM requirements. cc Ben Vesel (@bennerv) can you confirm this or point to where it's documented, if anywhere?
|
|
||
| replacement := existingCluster.DeepCopy() | ||
|
|
||
| for idx, fpaMIDataplaneCredential := range fpaMIDataplaneCredentials.ExplicitIdentities { |
There was a problem hiding this comment.
this loop doesn't guarantee that missing elements are cleared in the existing map. Write it so it does.
There was a problem hiding this comment.
The elements that we request to the MI DP are the elements that are originally in the identity section. That section and the resource ids within it(the keys of the map) are written by the user (or derived by the frontend from customerProperties.platform.operatorsAuthentication.userAssignedIdentities.{ControlPlaneOperators,ServiceManagedIdentity} which is controlled by the user. If those were to change it would be because of an user-initiated change in the frontend. This includes removals.
This means that we always request what's currently in the identity section to the MI DP service.
As a side note: we currently do not support changing the identities at the API level. This is related to the other question in https://github.com/Azure/ARO-HCP/pull/6301/changes#diff-a8acfaa5778ff377026d3035895fe20c0a629560b3f830a51c5e6ec6aebf88ffR146. Support for that still needs to be implemented. The change needed is not only relaxing the restriction at API level, but also doing the necessary changes that come as a consequence of that (issuing of new credentials, propagating the changes to different components (CS, Hypershift, OCP, ...), those components implementing the support themselves and so on)
|
Miguel Soriano (@miguelsorianod): The following test 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. |
| return true | ||
| } | ||
|
|
||
| // TODO do we actually want to implement continuous syncing of the identities as of now? Changing this over time |
There was a problem hiding this comment.
This is very relevant to the current implementation in this PR.
This means for example that if someone were to recreate an identity with the same resource id, this controller would reconcile it and other controllers that depend on this would reconcile the changes, which would be send downstream where changing the identities is not supported (CS at least doesn't currently support it), with whatever effects would have attempting to change the data there (denied api responses, persisting the new values but not really passing it down the line ending up with inaccurate data they indicate is in use and so on)
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.