feat: add controller that calculates Cluster Data Plane Identities extra information - #6592
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new backend cluster-watching controller that resolves Azure User Assigned Managed Identity metadata (ClientID/PrincipalID) for data plane operator identities configured on an HCP cluster and persists the resolved information to ServiceProviderCluster.Status.
Changes:
- Introduces
FetchDataPlaneOperatorsManagedIdentitiesInfocontroller to query AzureUserAssignedIdentitiesusing the cluster’s Service Managed Identity and store results underServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities. - Extends
ServiceProviderClusterStatuswith new status structures for data plane operator managed identity resolution + recheck gating. - Regenerates deepcopy code and wires the controller into backend startup.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/api/coreapi/zz_generated.deepcopy.go | Regenerated deepcopy methods for the new ServiceProviderCluster status types. |
| internal/api/coreapi/types_serviceprovider_cluster.go | Adds DataPlaneOperatorsManagedIdentities status fields and associated types + writer annotations. |
| backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info.go | New controller implementing Azure lookups, deduping, persistence, and recheck gating. |
| backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info_test.go | Unit tests for helper logic (matching/deduping/needsWork). |
| backend/pkg/app/backend.go | Registers/runs the new controller under leader election. |
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
034dd76 to
32dd9d5
Compare
|
Pushed
Added two unit tests covering both paths. Local validation: AI-generated. Review for accuracy. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 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 (2)
internal/api/coreapi/types_serviceprovider_cluster.go:338
- The new status fields use JSON tags
clientID/principalID, but existing API/Cosmos types consistently useclientId/principalId(e.g. internal/api/coreapi/identity.go:29-31, internal/api/coreapi/types_operation.go:45-48). Since this is a newly introduced persisted schema, aligning the tag casing avoids inconsistent field names across documents and potential consumer confusion.
ClientID *string `json:"clientID,omitempty"`
// PrincipalID is the Principal ID of the Azure User Assigned Managed Identity represented by ResourceID.
// This field is an output: it is fetched from Azure and written here by the controller.
// Written by: FetchDataPlaneOperatorsManagedIdentitiesInfoController
PrincipalID *string `json:"principalID,omitempty"`
internal/api/coreapi/types_serviceprovider_cluster.go:244
- This PR adds a new controller that reads/writes Cosmos fields on ServiceProviderCluster.Status, and adds a new persisted status field. Per CLAUDE.md:260-267, docs/cosmos-data-flow.md should be regenerated whenever changes touch backend/pkg/controllers/ or internal/api/types_*.go; the current docs/cosmos-data-flow.md has no mention of FetchDataPlaneOperatorsManagedIdentitiesInfo or dataPlaneOperatorsManagedIdentities.
// cluster that runs in the cluster's data plane.
// For example, the Cluster's CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators map
// contains the set of required data plane operators associated to a Cluster.
// Written by: FetchDataPlaneOperatorsManagedIdentitiesInfoController
DataPlaneOperatorsManagedIdentities ServiceProviderClusterDataPlaneOperatorsManagedIdentities `json:"dataPlaneOperatorsManagedIdentities,omitempty"`
|
/retest e2e-parallel The AI-generated. Review for accuracy. |
| return nil | ||
| } | ||
|
|
||
| existingServiceProviderCluster, err := corecosmosstorage.GetOrCreateServiceProviderCluster(ctx, c.resourcesDBClient, key.GetResourceID()) |
There was a problem hiding this comment.
no, use a lister
There was a problem hiding this comment.
Done in 83790720d. Switched the ServiceProviderCluster read to a lister too: added serviceProviderClusterLister corelisters.ServiceProviderClusterLister (from backendInformers.ServiceProviderClusters()), replacing GetOrCreateServiceProviderCluster with c.serviceProviderClusterLister.Get(...) plus if cosmosstorageutils.IsNotFoundError(err) { return nil }. On a missing SPC we now no-op and let the dedicated create-controller create it (then we pick it up on requeue) — matching the sibling serviceprovidercluster_properties_sync controller. resourcesDBClient is retained only for the Replace.
AI-generated. Review for accuracy.
| } | ||
|
|
||
| func (c *fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer) 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.
no, use a lister
There was a problem hiding this comment.
Done in 83790720d. Switched the HCPCluster read to the informer-backed lister: added a clusterLister corelisters.ClusterLister field, populated from the backendInformers the constructor already receives (_, clusterLister := backendInformers.Clusters()), and replaced the direct resourcesDBClient.HCPClusters(...).Get with c.clusterLister.Get(ctx, ...). No backend.go wiring change needed.
AI-generated. Review for accuracy.
| func (c *fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer) needsWork(spc *coreapi.ServiceProviderCluster, desiredDataPlaneOperatorsResourceIDStrs map[string]struct{}) bool { | ||
| // Only honor EarliestRecheckTime when the desired identity set still matches | ||
| // SPC. Any mismatch should fall through to return true and query Azure. | ||
| if c.desiredDataPlaneOperatorResourceIDsMatchSPC(desiredDataPlaneOperatorsResourceIDStrs, spc) { |
There was a problem hiding this comment.
don't abbreviate serviceProviderCluster as spc anywhere.
There was a problem hiding this comment.
Done in 83790720d. Spelled out serviceProviderCluster everywhere it was abbreviated spc in the controller and its test — including the function desiredDataPlaneOperatorResourceIDsMatchSPC → desiredDataPlaneOperatorResourceIDsMatchServiceProviderCluster, all params/locals (spc, spcIdentities, matchingSPCIdentities, updatedSPC), the test function name, and the SPC references in doc comments.
AI-generated. Review for accuracy.
| // Accumulate Get failures and keep going so successfully resolved identities | ||
| // can still be persisted. Preserve any previously resolved ClientID/PrincipalID | ||
| // so a transient failure does not wipe known values. | ||
| if existingIdentity := existingServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.Identities[identityResourceIDStr]; existingIdentity != nil { |
There was a problem hiding this comment.
No, don't do this. If we got an error, then clear the data. It doesn't matter what kind of error it was or if we have previous data. That way we never have stale data. You can create a struct member for "RetrievalError" that contains the first 1024 characters of the failure.
There was a problem hiding this comment.
Done in 83790720d. Added RetrievalError *string (json retrievalError,omitempty, with a // Written by: annotation) to ServiceProviderClusterDataPlaneOperatorManagedIdentity, and regenerated deepcopy. On a Get error we now clear ClientID/PrincipalID and set RetrievalError to the first 1024 chars (rune-safe) of the failure — no stale data is ever retained. Non-NotFound errors are still accumulated (fatal → EarliestRecheckTime cleared so the workqueue retries); a successful Get clears RetrievalError back to nil.
One judgment call I'd like your read on: I also set RetrievalError on the Azure ResourceNotFound branch (kept non-fatal, as today) so that whenever ClientID/PrincipalID are nil there's always an explanation for why. If you'd prefer ResourceNotFound to stay a clean "known-absent" signal with no RetrievalError, I'll restrict it to the transient-error branch only — just say the word.
AI-generated. Review for accuracy.
32dd9d5 to
8379072
Compare
|
/test e2e-parallel AI-generated. Review for accuracy. |
|
Pushed
Validation: AI-generated. Review for accuracy. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 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 (2)
backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info.go:227
ServiceManagedIdentityis guarded unconditionally before building the Azure client, butDataPlaneOperatorscan be empty/nil in the core model (anduniqueDataPlaneOperatorResourceIDsreturns an empty set in that case). That means this controller can repeatedly return an error for clusters that have no data-plane operator identities to resolve (because SMI is nil), even though there is no work to do. Consider only requiring/building the SMI-backedUserAssignedIdentitiesClientwhenlen(identitiesToSync) > 0so empty desired sets can no-op/clear status without needing a Service Managed Identity.
smiResourceID := existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity
if smiResourceID == nil {
// ServiceManagedIdentity is optional in the cluster model (*azcorearm.ResourceID with
// omitempty). The SMI client builder dereferences smiResourceID.String() internally, so a
// nil value would panic and crash the backend process. Return a tracked error instead so the
internal/api/coreapi/types_serviceprovider_cluster.go:245
- This PR adds a new Cosmos-persisted status subtree (
status.dataPlaneOperatorsManagedIdentities) written by a new backend controller. PerCLAUDE.md, changes that add Cosmos reads/writes or new Cosmos-stored fields must be reflected indocs/cosmos-data-flow.md, but that doc currently has no entry for this field/controller.
// For example, the Cluster's CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators map
// contains the set of required data plane operators associated to a Cluster.
// Written by: FetchDataPlaneOperatorsManagedIdentitiesInfoController
DataPlaneOperatorsManagedIdentities ServiceProviderClusterDataPlaneOperatorsManagedIdentities `json:"dataPlaneOperatorsManagedIdentities,omitempty"`
}
8379072 to
918949f
Compare
|
Pushed
AI-generated. Review for accuracy. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 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 (1)
internal/api/coreapi/types_serviceprovider_cluster.go:322
- This comment describes a data plane operator as a "customer operator", while the earlier field comment describes it as a Kubernetes operator. Please align the terminology, and consider wrapping the long sentence for readability.
// A cluster's data plane operator is a customer operator associated to the cluster that runs in the cluster's data plane.
| // A cluster's data plane operator is a kubernetes operator associated to the | ||
| // cluster that runs in the cluster's data plane. |
| if cosmosstorageutils.IsPreconditionFailedError(err) { | ||
| // Status (including any new DataPlaneOperatorsManagedIdentitiesEarliestRecheckTime) was not written. | ||
| // needsWork will still see the previously persisted value. | ||
| return errors.Join(errs...) | ||
| } | ||
| if err != nil { | ||
| // Same as precondition failure: DataPlaneOperatorsManagedIdentitiesEarliestRecheckTime was not | ||
| // persisted, so needsWork will still see the previously persisted value. |
|
/approve |
|
/lgtm |
|
/test lint (The AI-generated. Review for accuracy. |
|
digging into latency debugging. New page shows rule of small numbers. in the meantime. /retest |
…tra information We add a controller that retrieves the Client ID and Principal ID associated to the Data Plane operators identities associated to the ARO-HCP Cluster. We leverage the Service Managed Identity associated to the ARO-HCP Cluster to retrieve the Data Plane operators identities information. We use Azure Go SDK's UserAssignedIdentities API to retrieve it. This is a different method than what's done for MSI based identities where the Managed Identities Data Plane service is used instead. This is because for the MSI based identities, on the environments where the managed identities data plane service is not available, we use the mi mock identity instead, which includes its clientid+principalid instead of the ones associated to the identities passed in the cluster payload. By using the mock managed identities data plane client we retrieve that transparently. We do that also because that identity/information is the one that needs to be used by the control plane operators themselves on the control plane side. Address still-applicable review comments (rebased onto latest main): - uniqueDataPlaneOperatorResourceIDs now returns nil when any desired ResourceID is nil, as its doc states and as SyncOnce (identitiesToSync == nil) and the unit tests rely on; previously it called ResourceID.String() unconditionally and panicked on a nil entry. - Guard against a nil cluster ServiceManagedIdentity in SyncOnce before building the Service Managed Identity client. The SMI client builder dereferences smiResourceID.String() internally, so a nil ServiceManagedIdentity (optional in the cluster model) would panic and crash the backend; return a tracked error so the workqueue retries once it is populated. - Clear EarliestRecheckTime to nil on accumulated Azure Get failures (initialize it to nil and only set the jittered recheck time in the success branch). Previously a future EarliestRecheckTime could be persisted alongside a partial update after the desired identity set changed, and needsWork would then suppress workqueue retries until that future time even though SyncOnce returned an error. - internal/api/coreapi: add explicit "Written by:" annotations to the ResourceID/ClientID/PrincipalID leaf fields and clarify input vs output, per the CLAUDE.md cosmos-data-flow convention. - Fix gci import grouping in the controller unit test so `make lint` passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
918949f to
de749e6
Compare
| // RetrievalError, when non-nil, is the error (truncated to the first 1024 characters) from the | ||
| // most recent attempt to retrieve this identity's metadata from Azure. When set, ClientID and | ||
| // PrincipalID are nil because the last retrieval attempt failed - either the identity was not | ||
| // found in Azure or the Get call returned an error - and any previously resolved values are no | ||
| // longer trustworthy. It is nil when the last retrieval succeeded. |
| | Read | `HCPOpenShiftCluster` | <ul><li>`ServiceProviderProperties.DeletionTimestamp` (SyncOnce: must be nil)</li><li>`CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators` (desired identity ResourceIDs, deduplicated + lowercased)</li><li>`CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity` (SyncOnce: must not be nil)</li><li>`ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL` (used to build the SMI client)</li><li>`ID` (subscription / resource group / name)</li></ul> | | ||
| | Read | `ServiceProviderCluster` | <ul><li>`Status.DataPlaneOperatorsManagedIdentities.Identities` (needsWork: compared to the desired ResourceID set)</li><li>`Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime` (needsWork: honored only when identities match)</li></ul> | | ||
| | Read | Azure (UserAssignedIdentitiesClient) | <ul><li>`Get` once per unique ResourceID -> `Properties.ClientID`, `Properties.PrincipalID`</li></ul> | | ||
| | **Write** | **`ServiceProviderCluster`** | <ul><li>**`Status.DataPlaneOperatorsManagedIdentities.Identities[<lowercased resourceID>]`** = `{ResourceID, ClientID, PrincipalID, RetrievalError}` — ClientID/PrincipalID from Azure on success (RetrievalError nil); on any Get failure (including ResourceNotFound) ClientID/PrincipalID are cleared (nil) and RetrievalError is set to the first 1024 chars of the error. Identities no longer present on the cluster are pruned.</li><li>**`Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime`** = now + jittered 12h interval when all Gets succeed; left nil (cleared) when any Get error is accumulated, so the next needsWork re-queries Azure</li></ul> | |
| // For ClientID and PrincipalID of the identity, we set the value returned from the Azure API as is. This includes the cases where the | ||
| // value is nil or empty. RetrievalError is left nil because the retrieval succeeded. | ||
| replacementIdentity.ClientID = currentMI.Properties.ClientID | ||
| replacementIdentity.PrincipalID = currentMI.Properties.PrincipalID | ||
| } | ||
|
|
||
| if len(errs) == 0 { | ||
| // Set an earliest recheck time for the controller so we do not hit the Azure API 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). | ||
| // On Get failures we skip this branch entirely: EarliestRecheckTime stays nil (see the | ||
| // replacement initialization above), so needsWork keeps returning true and the workqueue | ||
| // retry re-queries Azure instead of waiting out a stale recheck interval. | ||
| recheckAt := metav1.NewTime(c.clock.Now().Add(wait.Jitter( | ||
| dataPlaneOperatorsManagedIdentitiesRecheckInterval, | ||
| dataPlaneOperatorsManagedIdentitiesRecheckJitter, | ||
| ))) | ||
| replacement.Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime = &recheckAt | ||
| } |
| const ( | ||
| fetchDataPlaneOperatorsManagedIdentitiesInfoControllerName = "FetchDataPlaneOperatorsManagedIdentitiesInfo" | ||
|
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, machi1990, redhat-chai-bot The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Rebase of #6300 (single commit, original author preserved) onto current
main, with the still-applicable review comments addressed.This adds a backend controller that enriches ARO-HCP clusters with Azure data plane operator managed-identity metadata (ClientID / PrincipalID) by querying Azure
UserAssignedIdentitiesusing the cluster's Service Managed Identity, and persists the results ontoServiceProviderCluster.Status.Rebase
main— no conflicts.Review comments addressed
uniqueDataPlaneOperatorResourceIDsnow returnsnilwhen anyResourceIDis nil, as its doc states and callers/tests rely on (previously it calledresourceID.String()unconditionally and could panic / never returned nil). This was the root cause of the originalci/prow/test-unitfailure.EarliestRecheckTime— explicit nil guard beforeDeepCopy()on first sync (defensive/intent-signaling per reviewer request).types_serviceprovider_cluster.go— resolved the openTODOs in the new status field comment block, added// Written by:writer annotations and input/output labels on each new leaf field (per the cosmos-data-flow convention inCLAUDE.md), and clarified what a data plane operator is.Comments intentionally not actioned
DataPlaneOperatorsshort-circuit;ServiceManagedIdentity/ResourceIDnil guards — "mandatory, always present from validated API").desiredDataPlaneOperatorResourceIDsMatchSPCOperatorNamevalidation / nil deref — already resolved by the author's prior redesign (the function compares lowercased resource-ID string keys and never dereferences aresourceID); changing it would break the passing tests and the current design.Validation
make lint— 0 issues repo-wide.make test— the branch-touched packages (backend/pkg/controllers/cluster/identity,internal/api/coreapi) pass;backend/pkg/appcompiles and lints clean. (The only local failures were pre-existingtooling/packages that requirepromtool/bicep/azon$PATH— they fail identically on a cleanmaincheckout and are untouched by this change.)make verify-deepcopy— passes (no drift;zz_generated.deepcopy.goregenerated viamake deepcopy, not hand-edited).Automated rebase + review-fix assist; please review before merge.
AI-generated. Review for accuracy.
David Eads (@deads2k) requested in Slack thread