From de749e6528496930ff478fed4fddf543dad042eb Mon Sep 17 00:00:00 2001 From: Miguel Soriano Date: Tue, 28 Jul 2026 12:48:01 +0200 Subject: [PATCH] feat: add controller that calculates Cluster Data Plane Identities extra 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 --- backend/pkg/app/backend.go | 8 + ...plane_operators_managed_identities_info.go | 353 +++++++++++ ..._operators_managed_identities_info_test.go | 599 ++++++++++++++++++ docs/cosmos-data-flow.md | 24 + .../coreapi/types_serviceprovider_cluster.go | 66 ++ internal/api/coreapi/zz_generated.deepcopy.go | 71 +++ 6 files changed, 1121 insertions(+) create mode 100644 backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info.go create mode 100644 backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info_test.go diff --git a/backend/pkg/app/backend.go b/backend/pkg/app/backend.go index 69b8962b0a1..2320bf21e88 100644 --- a/backend/pkg/app/backend.go +++ b/backend/pkg/app/backend.go @@ -1012,6 +1012,13 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context, b.options.FPAMIDataplaneClientBuilder, ) + fetchDataPlaneOperatorsManagedIdentitiesInfoController := clusteridentity.NewFetchDataPlaneOperatorsManagedIdentitiesInfoController( + b.clock, + b.options.ResourcesDBClient, + backendInformers, + b.options.SMIClientBuilder, + ) + leaderElectionConfig := leaderelection.LeaderElectionConfig{ Lock: b.options.LeaderElectionLock, LeaseDuration: sharedleaderelection.RecommendedLeaseDuration, @@ -1124,6 +1131,7 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context, go virtualMachineResourceSKUsCachedReaderController.Run(ctx, 20) go backupScheduleController.Run(ctx, 20) go fetchMSIIdentitiesInfoController.Run(ctx, 20) + go fetchDataPlaneOperatorsManagedIdentitiesInfoController.Run(ctx, 20) }, OnStoppedLeading: func() { // This needs to be defined even though it does nothing. diff --git a/backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info.go b/backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info.go new file mode 100644 index 00000000000..99d2bdc0445 --- /dev/null +++ b/backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info.go @@ -0,0 +1,353 @@ +// Copyright 2026 Microsoft Corporation +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package identity + +import ( + "context" + "errors" + "fmt" + "strings" + "time" + + "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" + utilsclock "k8s.io/utils/clock" + + azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + + azureclient "github.com/Azure/ARO-HCP/backend/pkg/azure/client" + "github.com/Azure/ARO-HCP/backend/pkg/utils/controllerutils" + "github.com/Azure/ARO-HCP/internal/api/coreapi" + "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/corecosmosstorage" + "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/cosmosstorageutils" + "github.com/Azure/ARO-HCP/internal/database/informers/coreinformers" + "github.com/Azure/ARO-HCP/internal/database/listers/corelisters" + "github.com/Azure/ARO-HCP/internal/utils" +) + +const ( + fetchDataPlaneOperatorsManagedIdentitiesInfoControllerName = "FetchDataPlaneOperatorsManagedIdentitiesInfo" + + // dataPlaneOperatorsManagedIdentitiesRecheckInterval is the base interval + // before re-querying Azure for ClientID/PrincipalID when the desired set of + // identities is already fully resolved. Combined with + // dataPlaneOperatorsManagedIdentitiesRecheckJitter via wait.Jitter. + dataPlaneOperatorsManagedIdentitiesRecheckInterval = 12 * time.Hour + dataPlaneOperatorsManagedIdentitiesRecheckJitter = 0.5 + + // maxRetrievalErrorLength bounds the number of runes persisted in a + // ServiceProviderClusterDataPlaneOperatorManagedIdentity.RetrievalError so a + // verbose Azure error cannot bloat the ServiceProviderCluster document. + maxRetrievalErrorLength = 1024 +) + +// fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer reconciles +// ServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities from the +// cluster's configured data plane operator managed identities. +type fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer struct { + clock utilsclock.PassiveClock + clusterLister corelisters.ClusterLister + serviceProviderClusterLister corelisters.ServiceProviderClusterLister + resourcesDBClient corecosmosstorage.ResourcesDBClient + + smiClientBuilder azureclient.ServiceManagedIdentityClientBuilder +} + +var _ controllerutils.ClusterSyncer = (*fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer)(nil) + +// NewFetchDataPlaneOperatorsManagedIdentitiesInfoController creates a cluster-watching +// controller that keeps ServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities +// in sync with the cluster's CustomerProperties data plane operator managed identities. +// +// On each sync it: +// 1. Reads every operator -> ResourceID entry from +// Cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators +// and deduplicates by lowercased ResourceID (multiple operators may share one +// identity). +// 2. Via needsWork, skips Azure calls when EarliestRecheckTime is still in the +// future AND the unique ResourceIDs stored on the ServiceProviderCluster still +// match that desired set. If the desired ResourceIDs have changed, +// EarliestRecheckTime is ignored so Azure is queried immediately. +// EarliestRecheckTime is shared across every entry in the Identities map. +// 3. Otherwise uses the cluster's Service Managed Identity to call Azure +// UserAssignedIdentitiesClient Get once per unique ResourceID and resolve +// ClientID and PrincipalID. +// 4. Rebuilds Status.DataPlaneOperatorsManagedIdentities.Identities as a full +// desired map keyed by lowercased ResourceID (ResourceID, ClientID, +// PrincipalID, RetrievalError). Entries that are no longer present on the +// cluster are pruned. Every desired ResourceID is written into the map: +// - ParseResourceID of a set key failing returns immediately without writing. +// That cannot happen for keys produced from ResourceID.String(). +// - ResourceNotFound keeps the entry, clears ClientID and PrincipalID (nil) +// and records the error in RetrievalError, so the ServiceProviderCluster still +// lists the customer-configured identity while signaling that Azure does not +// currently have it. This is not treated as a sync failure. +// - Any other Get failure clears ClientID and PrincipalID (nil), records the +// error in RetrievalError, is accumulated, and processing continues. A +// successful Get with nil Properties fails the whole sync immediately without +// writing. +// - Otherwise ClientID and PrincipalID are written as returned by Azure, +// including nil or empty values, and RetrievalError is left nil. +// 5. After every identity is processed without a failing Get, sets +// EarliestRecheckTime on the in-memory replacement to now plus a jittered +// interval (including when some identities were ResourceNotFound). When any +// Get failures were accumulated, EarliestRecheckTime is left nil (cleared) +// and the accumulated error is returned so needsWork keeps returning true and +// the workqueue retry re-queries Azure. +// 6. Writes the ServiceProviderCluster when the desired status differs, then +// returns any accumulated Get errors. needsWork observes EarliestRecheckTime +// and the desired-vs-stored ResourceID match from the informer cache, so a wait is +// introduced only after a successful Replace persists a matching set with a +// future EarliestRecheckTime. If Replace fails (or hits a precondition +// failure), the new EarliestRecheckTime is not stored; the workqueue requeues +// and the next needsWork still sees the previously persisted value (typically +// nil or already past, or a mismatched identity set), so the controller does +// not wait out the recheck interval after write failures either. +func NewFetchDataPlaneOperatorsManagedIdentitiesInfoController( + clock utilsclock.PassiveClock, + resourcesDBClient corecosmosstorage.ResourcesDBClient, + backendInformers coreinformers.BackendInformers, + smiClientBuilder azureclient.ServiceManagedIdentityClientBuilder, +) controllerutils.Controller { + if clock == nil { + clock = utilsclock.RealClock{} + } + + _, clusterLister := backendInformers.Clusters() + _, serviceProviderClusterLister := backendInformers.ServiceProviderClusters() + + syncer := &fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer{ + clock: clock, + clusterLister: clusterLister, + serviceProviderClusterLister: serviceProviderClusterLister, + resourcesDBClient: resourcesDBClient, + smiClientBuilder: smiClientBuilder, + } + + controller := controllerutils.NewClusterWatchingController( + fetchDataPlaneOperatorsManagedIdentitiesInfoControllerName, + resourcesDBClient, + backendInformers, + nil, + 1*time.Minute, + syncer, + ) + + return controller +} + +// needsWork reports whether Azure should be queried for data plane operator +// managed identity metadata. desiredDataPlaneOperatorIdentities must already be +// the unique lowercased ResourceID set from CustomerProperties. EarliestRecheckTime +// is honored only when those ResourceIDs still match the ServiceProviderCluster; on +// mismatch it returns true immediately. When identities match, it returns false while +// EarliestRecheckTime is in the future, and true when EarliestRecheckTime is +// nil or already past. Callers must skip needsWork entirely when the cluster +// is deleting. +func (c *fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer) needsWork(serviceProviderCluster *coreapi.ServiceProviderCluster, desiredDataPlaneOperatorsResourceIDStrs map[string]struct{}) bool { + // Only honor EarliestRecheckTime when the desired identity set still matches the + // ServiceProviderCluster. Any mismatch should fall through to return true and query Azure. + if c.desiredDataPlaneOperatorResourceIDsMatchServiceProviderCluster(desiredDataPlaneOperatorsResourceIDStrs, serviceProviderCluster) { + earliestRecheckTime := serviceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime + if earliestRecheckTime != nil && c.clock.Now().Before(earliestRecheckTime.Time) { + return false + } + } + + return true +} + +func (c *fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer) SyncOnce(ctx context.Context, key controllerutils.HCPClusterKey) error { + existingCluster, err := c.clusterLister.Get(ctx, key.SubscriptionID, key.ResourceGroupName, 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)) + } + + if existingCluster.ServiceProviderProperties.DeletionTimestamp != nil { + return nil + } + + existingServiceProviderCluster, err := c.serviceProviderClusterLister.Get(ctx, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName) + if cosmosstorageutils.IsNotFoundError(err) { + // The ServiceProviderCluster has not been created yet. The dedicated + // CreateServiceProviderCluster controller creates it; we pick it up on a + // later requeue once it exists. + return nil + } + if err != nil { + return utils.TrackError(fmt.Errorf("failed to get ServiceProviderCluster: %w", err)) + } + + desiredDataPlaneOperators := existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators + identitiesToSync := c.uniqueDataPlaneOperatorResourceIDs(desiredDataPlaneOperators) + if identitiesToSync == nil { + return utils.TrackError(fmt.Errorf("data plane operator managed identity ResourceID is nil")) + } + if !c.needsWork(existingServiceProviderCluster, identitiesToSync) { + return nil + } + + replacement := existingServiceProviderCluster.DeepCopy() + // EarliestRecheckTime is intentionally initialized to nil and only set to a future + // value in the len(errs) == 0 success branch below. On any accumulated Get error it + // stays nil so needsWork keeps returning true and the workqueue retry re-queries Azure + // instead of being gated by a stale (possibly future) recheck time persisted alongside + // a partial update. + replacement.Status.DataPlaneOperatorsManagedIdentities = coreapi.ServiceProviderClusterDataPlaneOperatorsManagedIdentities{ + Identities: make(map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity, len(identitiesToSync)), + EarliestRecheckTime: nil, + } + + 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 + // workqueue retries once the cluster's Service Managed Identity is populated. + return utils.TrackError(fmt.Errorf("cluster ServiceManagedIdentity is nil; cannot resolve data plane operator managed identities")) + } + userAssignedIdentitiesClient, err := c.smiClientBuilder.UserAssignedIdentitiesClient(ctx, existingCluster.ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL, smiResourceID, existingCluster.ID.SubscriptionID) + if err != nil { + return utils.TrackError(fmt.Errorf("failed to get User Assigned Identities Client: %w", err)) + } + + errs := []error{} + for identityResourceIDStr := range identitiesToSync { + resourceID, err := azcorearm.ParseResourceID(identityResourceIDStr) + if err != nil { + // We should never get a nil ResourceID from uniqueDataPlaneOperatorResourceIDs because it's built from + // the Cluster's customer properties which should have been validated beforehand. Because of this, we return an error instead of accumulating. + return utils.TrackError(fmt.Errorf("failed to parse Data Plane Operator Managed Identity ResourceID %s: %w", identityResourceIDStr, err)) + } + + replacementIdentity := &coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity{ + ResourceID: resourceID, + } + replacement.Status.DataPlaneOperatorsManagedIdentities.Identities[identityResourceIDStr] = replacementIdentity + + currentMI, err := userAssignedIdentitiesClient.Get(ctx, resourceID.ResourceGroupName, resourceID.Name, nil) + if azureclient.IsResourceNotFoundErr(err) { + // The identity is not found in Azure. Keep the entry so the ServiceProviderCluster + // still lists the customer-configured identity, but clear ClientID/PrincipalID and + // record why they are nil in RetrievalError. This is an expected, potentially + // transient state rather than a sync failure, so it is not accumulated into errs. + replacementIdentity.ClientID = nil + replacementIdentity.PrincipalID = nil + replacementIdentity.RetrievalError = truncateRetrievalError(err.Error()) + continue + } + if err != nil { + // On any other Get failure, clear ClientID/PrincipalID because the previously + // resolved values are no longer trustworthy, and record the (truncated) error in + // RetrievalError. Accumulate the failure and keep going so successfully resolved + // identities can still be persisted; the accumulated error is returned so the + // workqueue retry re-queries Azure. + replacementIdentity.ClientID = nil + replacementIdentity.PrincipalID = nil + replacementIdentity.RetrievalError = truncateRetrievalError(err.Error()) + errs = append(errs, utils.TrackError(fmt.Errorf("failed to get Data Plane Operator Managed Identity %s: %w", identityResourceIDStr, err))) + continue + } + + if currentMI.Properties == nil { + // The identity should always have properties. If it doesn't, we return an error instead of accumulating it, as this is unexpected and should not happen. + return utils.TrackError(fmt.Errorf("unexpected Data Plane Operator Managed Identity %s Properties is nil", identityResourceIDStr)) + } + + // 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 + } + + if !equality.Semantic.DeepEqual(replacement.Status.DataPlaneOperatorsManagedIdentities, existingServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities) { + _, err = c.resourcesDBClient.ServiceProviderClusters(existingCluster.ID.SubscriptionID, existingCluster.ID.ResourceGroupName, existingCluster.ID.Name).Replace(ctx, replacement, nil) + if cosmosstorageutils.IsPreconditionFailedError(err) { + // Status (including any new Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime) was not written. + // needsWork will still see the previously persisted value. + return errors.Join(errs...) + } + if err != nil { + // Same as precondition failure: Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime was not + // persisted, so needsWork will still see the previously persisted value. + return errors.Join(append(errs, utils.TrackError(fmt.Errorf("failed to replace ServiceProviderCluster: %w", err)))...) + } + } + + return errors.Join(errs...) +} + +// uniqueDataPlaneOperatorResourceIDs returns the unique lowercased ResourceID +// strings from desiredDataPlaneOperators. It returns nil if any ResourceID is nil. +func (c *fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer) uniqueDataPlaneOperatorResourceIDs(desiredDataPlaneOperators map[string]*azcorearm.ResourceID) map[string]struct{} { + unique := make(map[string]struct{}, len(desiredDataPlaneOperators)) + for _, resourceID := range desiredDataPlaneOperators { + if resourceID == nil { + // The desired set is not fully resolved yet. Return nil so callers + // (SyncOnce checks identitiesToSync == nil) fail safely and retry, + // as documented, instead of dereferencing a nil ResourceID. + return nil + } + unique[strings.ToLower(resourceID.String())] = struct{}{} + } + return unique +} + +// desiredDataPlaneOperatorResourceIDsMatchServiceProviderCluster reports whether the +// unique data plane operator managed identity ResourceIDs stored on the +// ServiceProviderCluster match desiredDataPlaneOperatorIdentities. +// desiredDataPlaneOperatorIdentities must already be keyed by lowercased ResourceID. +// Comparison is by ResourceID presence only; ClientID/PrincipalID are ignored. +func (c *fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer) desiredDataPlaneOperatorResourceIDsMatchServiceProviderCluster(desiredDataPlaneOperatorsResourceIDStrs map[string]struct{}, serviceProviderCluster *coreapi.ServiceProviderCluster) bool { + serviceProviderClusterIdentities := serviceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.Identities + if len(desiredDataPlaneOperatorsResourceIDStrs) != len(serviceProviderClusterIdentities) { + return false + } + + for resourceIDKey := range desiredDataPlaneOperatorsResourceIDStrs { + if _, ok := serviceProviderClusterIdentities[resourceIDKey]; !ok { + return false + } + } + + return true +} + +// truncateRetrievalError returns a pointer to errMsg truncated to at most +// maxRetrievalErrorLength runes so multi-byte UTF-8 sequences are never split. It bounds +// the size of the per-identity RetrievalError persisted on the ServiceProviderCluster. +func truncateRetrievalError(errMsg string) *string { + if runes := []rune(errMsg); len(runes) > maxRetrievalErrorLength { + errMsg = string(runes[:maxRetrievalErrorLength]) + } + return &errMsg +} diff --git a/backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info_test.go b/backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info_test.go new file mode 100644 index 00000000000..0a6aa5fc675 --- /dev/null +++ b/backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info_test.go @@ -0,0 +1,599 @@ +// Copyright 2026 Microsoft Corporation +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package identity + +import ( + "context" + "errors" + "io" + "net/http" + "net/url" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clocktesting "k8s.io/utils/clock/testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi" + + azureclient "github.com/Azure/ARO-HCP/backend/pkg/azure/client" + "github.com/Azure/ARO-HCP/backend/pkg/utils/controllerutils" + "github.com/Azure/ARO-HCP/internal/api/coreapi" + "github.com/Azure/ARO-HCP/internal/api/metadataapi" + "github.com/Azure/ARO-HCP/internal/database/cosmosstoragetesting/corecosmosstoragetesting" + "github.com/Azure/ARO-HCP/internal/database/listertesting/corelistertesting" +) + +func TestDesiredDataPlaneOperatorResourceIDsMatchServiceProviderCluster(t *testing.T) { + t.Parallel() + + identityA := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/identity-a")) + identityB := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/identity-b")) + mixedCaseIdentityA := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/Test-RG/providers/Microsoft.ManagedIdentity/userAssignedIdentities/Identity-A")) + + testCases := []struct { + name string + desiredResourceIDs map[string]struct{} + serviceProviderClusterIdentities map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity + expectedMatch bool + }{ + { + name: "both empty match", + desiredResourceIDs: map[string]struct{}{}, + serviceProviderClusterIdentities: map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity{}, + expectedMatch: true, + }, + { + name: "matching resource ID", + desiredResourceIDs: map[string]struct{}{ + strings.ToLower(identityA.String()): {}, + }, + serviceProviderClusterIdentities: map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity{ + strings.ToLower(identityA.String()): { + ResourceID: identityA, + }, + }, + expectedMatch: true, + }, + { + name: "matching ignores resource ID casing when already lowercased as key", + desiredResourceIDs: map[string]struct{}{ + strings.ToLower(mixedCaseIdentityA.String()): {}, + }, + serviceProviderClusterIdentities: map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity{ + strings.ToLower(identityA.String()): { + ResourceID: identityA, + }, + }, + expectedMatch: true, + }, + { + name: "unique identity count mismatch", + desiredResourceIDs: map[string]struct{}{ + strings.ToLower(identityA.String()): {}, + }, + serviceProviderClusterIdentities: map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity{ + strings.ToLower(identityA.String()): { + ResourceID: identityA, + }, + strings.ToLower(identityB.String()): { + ResourceID: identityB, + }, + }, + expectedMatch: false, + }, + { + name: "resource ID mismatch", + desiredResourceIDs: map[string]struct{}{ + strings.ToLower(identityA.String()): {}, + }, + serviceProviderClusterIdentities: map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity{ + strings.ToLower(identityB.String()): { + ResourceID: identityB, + }, + }, + expectedMatch: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + syncer := &fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer{} + serviceProviderCluster := &coreapi.ServiceProviderCluster{} + serviceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.Identities = tc.serviceProviderClusterIdentities + + assert.Equal(t, tc.expectedMatch, syncer.desiredDataPlaneOperatorResourceIDsMatchServiceProviderCluster(tc.desiredResourceIDs, serviceProviderCluster)) + }) + } +} + +func TestUniqueDataPlaneOperatorResourceIDs(t *testing.T) { + t.Parallel() + + identityA := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/identity-a")) + mixedCaseIdentityA := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/Test-RG/providers/Microsoft.ManagedIdentity/userAssignedIdentities/Identity-A")) + + syncer := &fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer{} + + t.Run("dedupes shared identity across operators", func(t *testing.T) { + t.Parallel() + unique := syncer.uniqueDataPlaneOperatorResourceIDs(map[string]*azcorearm.ResourceID{ + "operator-a": identityA, + "operator-b": identityA, + }) + require.NotNil(t, unique) + assert.Equal(t, map[string]struct{}{ + strings.ToLower(identityA.String()): {}, + }, unique) + }) + + t.Run("lowercases resource ID keys", func(t *testing.T) { + t.Parallel() + unique := syncer.uniqueDataPlaneOperatorResourceIDs(map[string]*azcorearm.ResourceID{ + "operator-a": mixedCaseIdentityA, + }) + require.NotNil(t, unique) + assert.Equal(t, map[string]struct{}{ + strings.ToLower(identityA.String()): {}, + }, unique) + }) + + t.Run("nil resource ID returns nil", func(t *testing.T) { + t.Parallel() + unique := syncer.uniqueDataPlaneOperatorResourceIDs(map[string]*azcorearm.ResourceID{ + "operator-a": nil, + }) + assert.Nil(t, unique) + }) +} + +func TestFetchDataPlaneOperatorsManagedIdentitiesInfoNeedsWork(t *testing.T) { + t.Parallel() + + now := time.Date(2026, 8, 14, 12, 0, 0, 0, time.UTC) + identityA := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/identity-a")) + identityB := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/identity-b")) + + matchingDesired := map[string]struct{}{ + strings.ToLower(identityA.String()): {}, + } + matchingServiceProviderClusterIdentities := map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity{ + strings.ToLower(identityA.String()): { + ResourceID: identityA, + }, + } + + testCases := []struct { + name string + desiredResourceIDs map[string]struct{} + serviceProviderClusterIdentities map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity + earliestRecheckTime *metav1.Time + expectedNeedsWork bool + }{ + { + name: "matching identities with future recheck skips work", + desiredResourceIDs: matchingDesired, + serviceProviderClusterIdentities: matchingServiceProviderClusterIdentities, + earliestRecheckTime: &metav1.Time{Time: now.Add(time.Hour)}, + expectedNeedsWork: false, + }, + { + name: "matching identities with past recheck needs work", + desiredResourceIDs: matchingDesired, + serviceProviderClusterIdentities: matchingServiceProviderClusterIdentities, + earliestRecheckTime: &metav1.Time{Time: now.Add(-time.Hour)}, + expectedNeedsWork: true, + }, + { + name: "matching identities with nil recheck needs work", + desiredResourceIDs: matchingDesired, + serviceProviderClusterIdentities: matchingServiceProviderClusterIdentities, + earliestRecheckTime: nil, + expectedNeedsWork: true, + }, + { + name: "mismatched identities ignore future recheck", + desiredResourceIDs: map[string]struct{}{ + strings.ToLower(identityB.String()): {}, + }, + serviceProviderClusterIdentities: matchingServiceProviderClusterIdentities, + earliestRecheckTime: &metav1.Time{Time: now.Add(time.Hour)}, + expectedNeedsWork: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + syncer := &fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer{ + clock: clocktesting.NewFakePassiveClock(now), + } + serviceProviderCluster := &coreapi.ServiceProviderCluster{} + serviceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.Identities = tc.serviceProviderClusterIdentities + serviceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime = tc.earliestRecheckTime + + require.Equal(t, tc.expectedNeedsWork, syncer.needsWork(serviceProviderCluster, tc.desiredResourceIDs)) + }) + } +} + +// fakeUserAssignedIdentitiesClient is a minimal hand-written fake implementing +// azureclient.UserAssignedIdentitiesClient. Only Get is exercised by the controller, +// so getResp/getErr make it configurable. CreateOrUpdate and Delete are never called by +// the controller and panic if invoked so an accidental use is caught immediately. +type fakeUserAssignedIdentitiesClient struct { + getResp armmsi.UserAssignedIdentitiesClientGetResponse + getErr error +} + +var _ azureclient.UserAssignedIdentitiesClient = (*fakeUserAssignedIdentitiesClient)(nil) + +func (f *fakeUserAssignedIdentitiesClient) Get(_ context.Context, _ string, _ string, _ *armmsi.UserAssignedIdentitiesClientGetOptions) (armmsi.UserAssignedIdentitiesClientGetResponse, error) { + return f.getResp, f.getErr +} + +func (f *fakeUserAssignedIdentitiesClient) CreateOrUpdate(_ context.Context, _ string, _ string, _ armmsi.Identity, _ *armmsi.UserAssignedIdentitiesClientCreateOrUpdateOptions) (armmsi.UserAssignedIdentitiesClientCreateOrUpdateResponse, error) { + panic("CreateOrUpdate not implemented in fakeUserAssignedIdentitiesClient") +} + +func (f *fakeUserAssignedIdentitiesClient) Delete(_ context.Context, _ string, _ string, _ *armmsi.UserAssignedIdentitiesClientDeleteOptions) (armmsi.UserAssignedIdentitiesClientDeleteResponse, error) { + panic("Delete not implemented in fakeUserAssignedIdentitiesClient") +} + +// newTestClusterWithIdentities builds an HCPOpenShiftCluster addressable by the mock +// ResourcesDBClient with the supplied ServiceManagedIdentity and data plane operator +// identities on its CustomerProperties. +func newTestClusterWithIdentities(t *testing.T, clusterName string, serviceManagedIdentity *azcorearm.ResourceID, dataPlaneOperators map[string]*azcorearm.ResourceID) *coreapi.HCPOpenShiftCluster { + t.Helper() + + resourceID := metadataapi.Must(azcorearm.ParseResourceID( + "/subscriptions/" + testSubscriptionID + + "/resourceGroups/" + testResourceGroupName + + "/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/" + clusterName, + )) + + cluster := &coreapi.HCPOpenShiftCluster{ + CosmosMetadata: coreapi.CosmosMetadata{ + ResourceID: resourceID, + PartitionKey: strings.ToLower(resourceID.SubscriptionID), + }, + TrackedResource: coreapi.TrackedResource{ + Resource: coreapi.Resource{ + ID: resourceID, + Name: clusterName, + Type: resourceID.ResourceType.String(), + }, + }, + } + cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity = serviceManagedIdentity + cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators = dataPlaneOperators + + return cluster +} + +// newTestServiceProviderClusterWithIdentities builds a ServiceProviderCluster addressable +// by the mock ResourcesDBClient with the supplied resolved identities and recheck time. +func newTestServiceProviderClusterWithIdentities(clusterName string, identities map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity, earliestRecheckTime *metav1.Time) *coreapi.ServiceProviderCluster { + resourceID := metadataapi.Must(azcorearm.ParseResourceID( + "/subscriptions/" + testSubscriptionID + + "/resourceGroups/" + testResourceGroupName + + "/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/" + clusterName + + "/" + coreapi.ServiceProviderClusterResourceTypeName + + "/" + coreapi.ServiceProviderClusterResourceName, + )) + + serviceProviderCluster := &coreapi.ServiceProviderCluster{ + CosmosMetadata: coreapi.CosmosMetadata{ + ResourceID: resourceID, + PartitionKey: strings.ToLower(resourceID.SubscriptionID), + }, + } + serviceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.Identities = identities + serviceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime = earliestRecheckTime + + return serviceProviderCluster +} + +// TestFetchDataPlaneOperatorsManagedIdentitiesInfoSyncOnceNilServiceManagedIdentity verifies +// that a nil cluster ServiceManagedIdentity is turned into a tracked error before the SMI +// client is built, rather than panicking inside the client builder (which dereferences +// smiResourceID.String()). +func TestFetchDataPlaneOperatorsManagedIdentitiesInfoSyncOnceNilServiceManagedIdentity(t *testing.T) { + t.Parallel() + + ctx := context.Background() + now := time.Date(2026, 8, 14, 12, 0, 0, 0, time.UTC) + + identityA := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/" + testSubscriptionID + "/resourceGroups/" + testResourceGroupName + "/providers/Microsoft.ManagedIdentity/userAssignedIdentities/identity-a")) + + // Cluster has a data plane operator identity but a nil ServiceManagedIdentity. + cluster := newTestClusterWithIdentities(t, testClusterName, nil, map[string]*azcorearm.ResourceID{ + "operator-a": identityA, + }) + // ServiceProviderCluster with no resolved identities so needsWork returns true and SyncOnce reaches the guard. + serviceProviderCluster := newTestServiceProviderClusterWithIdentities(testClusterName, nil, nil) + + mockResourcesDB, err := corecosmosstoragetesting.NewMockResourcesDBClientWithResources(ctx, []any{cluster, serviceProviderCluster}) + require.NoError(t, err) + + ctrl := gomock.NewController(t) + smiClientBuilder := azureclient.NewMockServiceManagedIdentityClientBuilder(ctrl) + // The builder must NOT be called: the nil-SMI guard returns before building the client. + smiClientBuilder.EXPECT(). + UserAssignedIdentitiesClient(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Times(0) + + syncer := &fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer{ + clock: clocktesting.NewFakePassiveClock(now), + clusterLister: &corelistertesting.DBClusterLister{ResourcesDBClient: mockResourcesDB}, + serviceProviderClusterLister: &corelistertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDB}, + resourcesDBClient: mockResourcesDB, + smiClientBuilder: smiClientBuilder, + } + + key := controllerutils.HCPClusterKey{ + SubscriptionID: testSubscriptionID, + ResourceGroupName: testResourceGroupName, + HCPClusterName: testClusterName, + } + + // SyncOnce must return a tracked error (not panic) when ServiceManagedIdentity is nil. + err = syncer.SyncOnce(ctx, key) + require.Error(t, err) + assert.Contains(t, err.Error(), "ServiceManagedIdentity is nil") +} + +// TestFetchDataPlaneOperatorsManagedIdentitiesInfoSyncOnceClearsEarliestRecheckTimeOnGetError +// verifies that when an Azure Get fails and the desired identity set changed, the persisted +// ServiceProviderCluster has EarliestRecheckTime cleared to nil (so needsWork returns true on the retry) even +// though the previously stored value was in the future. +func TestFetchDataPlaneOperatorsManagedIdentitiesInfoSyncOnceClearsEarliestRecheckTimeOnGetError(t *testing.T) { + t.Parallel() + + ctx := context.Background() + now := time.Date(2026, 8, 14, 12, 0, 0, 0, time.UTC) + + serviceManagedIdentity := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/" + testSubscriptionID + "/resourceGroups/" + testResourceGroupName + "/providers/Microsoft.ManagedIdentity/userAssignedIdentities/smi")) + identityA := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/" + testSubscriptionID + "/resourceGroups/" + testResourceGroupName + "/providers/Microsoft.ManagedIdentity/userAssignedIdentities/identity-a")) + identityB := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/" + testSubscriptionID + "/resourceGroups/" + testResourceGroupName + "/providers/Microsoft.ManagedIdentity/userAssignedIdentities/identity-b")) + + // Desired set is {B}; the ServiceProviderCluster currently stores {A} with a FUTURE recheck time. The desired + // set differs from ServiceProviderCluster, so needsWork returns true and SyncOnce re-queries Azure. + cluster := newTestClusterWithIdentities(t, testClusterName, serviceManagedIdentity, map[string]*azcorearm.ResourceID{ + "operator-b": identityB, + }) + futureRecheck := &metav1.Time{Time: now.Add(time.Hour)} + serviceProviderCluster := newTestServiceProviderClusterWithIdentities(testClusterName, map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity{ + strings.ToLower(identityA.String()): {ResourceID: identityA}, + }, futureRecheck) + + mockResourcesDB, err := corecosmosstoragetesting.NewMockResourcesDBClientWithResources(ctx, []any{cluster, serviceProviderCluster}) + require.NoError(t, err) + + // The fake Azure client returns a non-ResourceNotFound error so the Get failure is accumulated. + getErr := errors.New("simulated azure Get failure") + fakeClient := &fakeUserAssignedIdentitiesClient{getErr: getErr} + + ctrl := gomock.NewController(t) + smiClientBuilder := azureclient.NewMockServiceManagedIdentityClientBuilder(ctrl) + smiClientBuilder.EXPECT(). + UserAssignedIdentitiesClient(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(fakeClient, nil). + Times(1) + + syncer := &fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer{ + clock: clocktesting.NewFakePassiveClock(now), + clusterLister: &corelistertesting.DBClusterLister{ResourcesDBClient: mockResourcesDB}, + serviceProviderClusterLister: &corelistertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDB}, + resourcesDBClient: mockResourcesDB, + smiClientBuilder: smiClientBuilder, + } + + key := controllerutils.HCPClusterKey{ + SubscriptionID: testSubscriptionID, + ResourceGroupName: testResourceGroupName, + HCPClusterName: testClusterName, + } + + // SyncOnce should surface the accumulated Get error... + err = syncer.SyncOnce(ctx, key) + require.Error(t, err) + assert.Contains(t, err.Error(), "simulated azure Get failure") + + // ...and must clear EarliestRecheckTime so needsWork returns true on the workqueue retry. + updatedServiceProviderCluster, err := mockResourcesDB.ServiceProviderClusters(testSubscriptionID, testResourceGroupName, testClusterName).Get(ctx, coreapi.ServiceProviderClusterResourceName) + require.NoError(t, err) + assert.Nil(t, updatedServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime) + + // The desired identity (B) should be persisted; the stale one (A) pruned. + assert.Contains(t, updatedServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.Identities, strings.ToLower(identityB.String())) + assert.NotContains(t, updatedServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.Identities, strings.ToLower(identityA.String())) + + // On the Get error, identity B must have ClientID/PrincipalID cleared and the error recorded in RetrievalError. + identityBEntry := updatedServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.Identities[strings.ToLower(identityB.String())] + require.NotNil(t, identityBEntry) + assert.Nil(t, identityBEntry.ClientID) + assert.Nil(t, identityBEntry.PrincipalID) + require.NotNil(t, identityBEntry.RetrievalError) + assert.Contains(t, *identityBEntry.RetrievalError, "simulated azure Get failure") +} + +// TestFetchDataPlaneOperatorsManagedIdentitiesInfoSyncOnceClearsResolvedValuesOnGetError +// verifies that a non-ResourceNotFound Azure Get failure clears any previously resolved +// ClientID/PrincipalID (they are no longer trustworthy) and records the error in +// RetrievalError, while still surfacing the accumulated error. +func TestFetchDataPlaneOperatorsManagedIdentitiesInfoSyncOnceClearsResolvedValuesOnGetError(t *testing.T) { + t.Parallel() + + ctx := context.Background() + now := time.Date(2026, 8, 14, 12, 0, 0, 0, time.UTC) + + serviceManagedIdentity := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/" + testSubscriptionID + "/resourceGroups/" + testResourceGroupName + "/providers/Microsoft.ManagedIdentity/userAssignedIdentities/smi")) + identityA := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/" + testSubscriptionID + "/resourceGroups/" + testResourceGroupName + "/providers/Microsoft.ManagedIdentity/userAssignedIdentities/identity-a")) + + // Desired set is {A}; the ServiceProviderCluster already stores A with resolved + // ClientID/PrincipalID and a past recheck time so needsWork returns true. + cluster := newTestClusterWithIdentities(t, testClusterName, serviceManagedIdentity, map[string]*azcorearm.ResourceID{ + "operator-a": identityA, + }) + priorClientID := "prior-client-id" + priorPrincipalID := "prior-principal-id" + pastRecheck := &metav1.Time{Time: now.Add(-time.Hour)} + serviceProviderCluster := newTestServiceProviderClusterWithIdentities(testClusterName, map[string]*coreapi.ServiceProviderClusterDataPlaneOperatorManagedIdentity{ + strings.ToLower(identityA.String()): { + ResourceID: identityA, + ClientID: &priorClientID, + PrincipalID: &priorPrincipalID, + }, + }, pastRecheck) + + mockResourcesDB, err := corecosmosstoragetesting.NewMockResourcesDBClientWithResources(ctx, []any{cluster, serviceProviderCluster}) + require.NoError(t, err) + + getErr := errors.New("simulated azure Get failure") + fakeClient := &fakeUserAssignedIdentitiesClient{getErr: getErr} + + ctrl := gomock.NewController(t) + smiClientBuilder := azureclient.NewMockServiceManagedIdentityClientBuilder(ctrl) + smiClientBuilder.EXPECT(). + UserAssignedIdentitiesClient(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(fakeClient, nil). + Times(1) + + syncer := &fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer{ + clock: clocktesting.NewFakePassiveClock(now), + clusterLister: &corelistertesting.DBClusterLister{ResourcesDBClient: mockResourcesDB}, + serviceProviderClusterLister: &corelistertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDB}, + resourcesDBClient: mockResourcesDB, + smiClientBuilder: smiClientBuilder, + } + + key := controllerutils.HCPClusterKey{ + SubscriptionID: testSubscriptionID, + ResourceGroupName: testResourceGroupName, + HCPClusterName: testClusterName, + } + + err = syncer.SyncOnce(ctx, key) + require.Error(t, err) + assert.Contains(t, err.Error(), "simulated azure Get failure") + + updatedServiceProviderCluster, err := mockResourcesDB.ServiceProviderClusters(testSubscriptionID, testResourceGroupName, testClusterName).Get(ctx, coreapi.ServiceProviderClusterResourceName) + require.NoError(t, err) + + entry := updatedServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.Identities[strings.ToLower(identityA.String())] + require.NotNil(t, entry) + assert.Nil(t, entry.ClientID, "previously resolved ClientID must be cleared on Get error") + assert.Nil(t, entry.PrincipalID, "previously resolved PrincipalID must be cleared on Get error") + require.NotNil(t, entry.RetrievalError) + assert.Contains(t, *entry.RetrievalError, "simulated azure Get failure") + + // A Get failure also clears EarliestRecheckTime so needsWork retries immediately. + assert.Nil(t, updatedServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime) +} + +// TestFetchDataPlaneOperatorsManagedIdentitiesInfoSyncOnceResourceNotFoundSetsRetrievalError +// verifies that when Azure reports the identity as not found, the entry is kept with nil +// ClientID/PrincipalID and a RetrievalError explaining why, the sync is NOT failed (no error +// returned), and EarliestRecheckTime is set to a future value. +func TestFetchDataPlaneOperatorsManagedIdentitiesInfoSyncOnceResourceNotFoundSetsRetrievalError(t *testing.T) { + t.Parallel() + + ctx := context.Background() + now := time.Date(2026, 8, 14, 12, 0, 0, 0, time.UTC) + + serviceManagedIdentity := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/" + testSubscriptionID + "/resourceGroups/" + testResourceGroupName + "/providers/Microsoft.ManagedIdentity/userAssignedIdentities/smi")) + identityA := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/" + testSubscriptionID + "/resourceGroups/" + testResourceGroupName + "/providers/Microsoft.ManagedIdentity/userAssignedIdentities/identity-a")) + + cluster := newTestClusterWithIdentities(t, testClusterName, serviceManagedIdentity, map[string]*azcorearm.ResourceID{ + "operator-a": identityA, + }) + // ServiceProviderCluster with no resolved identities and nil recheck so needsWork returns true. + serviceProviderCluster := newTestServiceProviderClusterWithIdentities(testClusterName, nil, nil) + + mockResourcesDB, err := corecosmosstoragetesting.NewMockResourcesDBClientWithResources(ctx, []any{cluster, serviceProviderCluster}) + require.NoError(t, err) + + // The fake Azure client returns a ResourceNotFound error. + fakeClient := &fakeUserAssignedIdentitiesClient{getErr: resourceNotFoundResponseError()} + + ctrl := gomock.NewController(t) + smiClientBuilder := azureclient.NewMockServiceManagedIdentityClientBuilder(ctrl) + smiClientBuilder.EXPECT(). + UserAssignedIdentitiesClient(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(fakeClient, nil). + Times(1) + + syncer := &fetchDataPlaneOperatorsManagedIdentitiesInfoSyncer{ + clock: clocktesting.NewFakePassiveClock(now), + clusterLister: &corelistertesting.DBClusterLister{ResourcesDBClient: mockResourcesDB}, + serviceProviderClusterLister: &corelistertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDB}, + resourcesDBClient: mockResourcesDB, + smiClientBuilder: smiClientBuilder, + } + + key := controllerutils.HCPClusterKey{ + SubscriptionID: testSubscriptionID, + ResourceGroupName: testResourceGroupName, + HCPClusterName: testClusterName, + } + + // ResourceNotFound is not a sync failure. + err = syncer.SyncOnce(ctx, key) + require.NoError(t, err) + + updatedServiceProviderCluster, err := mockResourcesDB.ServiceProviderClusters(testSubscriptionID, testResourceGroupName, testClusterName).Get(ctx, coreapi.ServiceProviderClusterResourceName) + require.NoError(t, err) + + entry := updatedServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.Identities[strings.ToLower(identityA.String())] + require.NotNil(t, entry) + assert.Nil(t, entry.ClientID) + assert.Nil(t, entry.PrincipalID) + require.NotNil(t, entry.RetrievalError) + assert.Contains(t, *entry.RetrievalError, "ResourceNotFound") + + // A non-failing sync sets a future EarliestRecheckTime. + require.NotNil(t, updatedServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime) + assert.True(t, updatedServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime.After(now)) +} + +// resourceNotFoundResponseError returns an *azcore.ResponseError that +// azureclient.IsResourceNotFoundErr recognizes as a ResourceNotFound. The RawResponse is +// fully populated so err.Error() renders without panicking. +func resourceNotFoundResponseError() *azcore.ResponseError { + return &azcore.ResponseError{ + ErrorCode: "ResourceNotFound", + StatusCode: http.StatusNotFound, + RawResponse: &http.Response{ + Status: "404 Not Found", + StatusCode: http.StatusNotFound, + Body: io.NopCloser(strings.NewReader(`{"error":{"code":"ResourceNotFound","message":"The identity was not found."}}`)), + Request: &http.Request{ + Method: http.MethodGet, + URL: &url.URL{Scheme: "https", Host: "management.azure.com", Path: "/identity"}, + }, + }, + } +} diff --git a/docs/cosmos-data-flow.md b/docs/cosmos-data-flow.md index 2138918ac38..8e8002088b2 100644 --- a/docs/cosmos-data-flow.md +++ b/docs/cosmos-data-flow.md @@ -1136,6 +1136,22 @@ No writes to the Cosmos Resources container. | Read | Existing `ReadDesire` | | | **Write** | `ReadDesire` (kube-applier DB) | | +#### FetchDataPlaneOperatorsManagedIdentitiesInfo + +**File:** [fetch_data_plane_operators_managed_identities_info.go](../backend/pkg/controllers/cluster/identity/fetch_data_plane_operators_managed_identities_info.go) +**Trigger:** Cluster informer, 1-minute resync +**Gate (needsWork on ServiceProviderCluster):** +- Skipped entirely when `HCPOpenShiftCluster.ServiceProviderProperties.DeletionTimestamp` != nil +- Honors `ServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities.EarliestRecheckTime` only when the unique data plane operator ResourceID set on `Status.DataPlaneOperatorsManagedIdentities.Identities` still matches the desired set from `CustomerProperties`; returns true (query Azure) immediately on any mismatch +- When identities match: returns false while `EarliestRecheckTime` is in the future; true when `EarliestRecheckTime` is nil or already past + +| | Object | Fields | +|---|--------|--------| +| Read | `HCPOpenShiftCluster` | | +| Read | `ServiceProviderCluster` | | +| Read | Azure (UserAssignedIdentitiesClient) | | +| **Write** | **`ServiceProviderCluster`** | | + --- ## 3. Execution Order Digraphs @@ -1489,6 +1505,14 @@ Single writer, but tracks the namespace containing control plane pods (etcd, kub Single writer. Read by [ClusterIdentitySync](#clusteridentitysync) to populate `HCPOpenShiftCluster.Identity.UserAssignedIdentities`. +### `ServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities` + +| Actor | When | +|-------|------| +| [FetchDataPlaneOperatorsManagedIdentitiesInfo](#fetchdataplaneoperatorsmanagedidentitiesinfo) | Resolves each data plane operator identity's `ClientID`/`PrincipalID` from Azure (or clears them and sets `RetrievalError` on a Get failure), and sets `EarliestRecheckTime` for the next Azure recheck | + +Single writer. Mirrors the customer's data plane operator managed identities (`CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators`) into `Identities` keyed by lowercased Azure ResourceID, each carrying the Azure-resolved `ClientID`/`PrincipalID` or a `RetrievalError`. + ### `ServiceProviderCluster.Status.Validations` | Actor | When | diff --git a/internal/api/coreapi/types_serviceprovider_cluster.go b/internal/api/coreapi/types_serviceprovider_cluster.go index 4ddfaaeab73..15c4104511e 100644 --- a/internal/api/coreapi/types_serviceprovider_cluster.go +++ b/internal/api/coreapi/types_serviceprovider_cluster.go @@ -258,6 +258,16 @@ type ServiceProviderClusterStatus struct { // Additionally, this also tracks when Azure should next be re-queried for that info. // Written by: FetchMSIIdentitiesInfo MSIManagedIdentities ServiceProviderClusterMSIManagedIdentities `json:"msiManagedIdentities,omitempty"` + + // DataPlaneOperatorsManagedIdentities tracks resolved ClientID/PrincipalID for + // the Azure User Assigned Managed Identities associated with the cluster's data + // plane operators, plus when Azure should next be re-queried for that info. + // A cluster's data plane operator is a Kubernetes operator associated with the + // 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 with a Cluster. + // Written by: FetchDataPlaneOperatorsManagedIdentitiesInfoController + DataPlaneOperatorsManagedIdentities ServiceProviderClusterDataPlaneOperatorsManagedIdentities `json:"dataPlaneOperatorsManagedIdentities,omitempty"` } // ServiceProviderClusterMSIManagedIdentities holds Managed Service Identity (MSI) @@ -330,6 +340,62 @@ type ServiceProviderClusterServiceManagedIdentity struct { PrincipalID *string `json:"principalId,omitempty"` } +// ServiceProviderClusterDataPlaneOperatorsManagedIdentities holds the resolved +// managed-identity metadata for all data plane operators on a cluster, together +// with a single EarliestRecheckTime that applies to every entry in Identities. +type ServiceProviderClusterDataPlaneOperatorsManagedIdentities struct { + // Identities is a map containing resolved ClientID/PrincipalID for the Azure + // User Assigned Managed Identities associated with the cluster's data plane + // operators. The key is the fully lowercased Azure Resource ID of the + // identity. Which operators reference each identity is tracked on + // Cluster.CustomerProperties, not here. Multiple operators may share one + // identity entry. + // Written by: FetchDataPlaneOperatorsManagedIdentitiesInfoController + Identities map[string]*ServiceProviderClusterDataPlaneOperatorManagedIdentity `json:"identities,omitempty"` + // EarliestRecheckTime is the earliest time at which the controller should + // re-query Azure for ClientID/PrincipalID of Identities. Nil means recheck + // immediately. The same recheck time applies across all elements of Identities. + // This allows the controller to avoid repeatedly hitting an Azure API 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: FetchDataPlaneOperatorsManagedIdentitiesInfoController + EarliestRecheckTime *metav1.Time `json:"earliestRecheckTime,omitempty"` +} + +// ServiceProviderClusterDataPlaneOperatorManagedIdentity contains resolved +// ClientID/PrincipalID for an Azure User Assigned Managed Identity used by one +// or more of a cluster's data plane operators. +// A cluster's data plane operator is a customer operator associated with the cluster that runs in the cluster's data plane. +// Which operators reference this identity is tracked on +// Cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators. +type ServiceProviderClusterDataPlaneOperatorManagedIdentity struct { + // ResourceID is the Azure Resource ID of the Azure User Assigned Managed Identity. + // This field is an input: its value is mirrored from the Cluster's CustomerProperties + // (Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators) into this status. + // Written by: FetchDataPlaneOperatorsManagedIdentitiesInfoController + ResourceID *azcorearm.ResourceID `json:"resourceID,omitempty"` + // ClientID is the Client 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 + 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"` + // 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. + // This field is an output: it is written here by the controller. + // Written by: FetchDataPlaneOperatorsManagedIdentitiesInfoController + RetrievalError *string `json:"retrievalError,omitempty"` +} + // AzureResources groups the Azure resource references associated with a cluster. type AzureResources struct { // DenyAssignments tracks the deny assignments applied to the cluster's resources. diff --git a/internal/api/coreapi/zz_generated.deepcopy.go b/internal/api/coreapi/zz_generated.deepcopy.go index 2db58be428b..3adb35cc8b0 100644 --- a/internal/api/coreapi/zz_generated.deepcopy.go +++ b/internal/api/coreapi/zz_generated.deepcopy.go @@ -2026,6 +2026,76 @@ func (in *ServiceProviderClusterControlPlaneOperatorIdentity) DeepCopy() *Servic return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ServiceProviderClusterDataPlaneOperatorManagedIdentity) DeepCopyInto(out *ServiceProviderClusterDataPlaneOperatorManagedIdentity) { + *out = *in + if in.ResourceID != nil { + in, out := &in.ResourceID, &out.ResourceID + *out = DeepCopyResourceID(*in) + } + if in.ClientID != nil { + in, out := &in.ClientID, &out.ClientID + *out = new(string) + **out = **in + } + if in.PrincipalID != nil { + in, out := &in.PrincipalID, &out.PrincipalID + *out = new(string) + **out = **in + } + if in.RetrievalError != nil { + in, out := &in.RetrievalError, &out.RetrievalError + *out = new(string) + **out = **in + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceProviderClusterDataPlaneOperatorManagedIdentity. +func (in *ServiceProviderClusterDataPlaneOperatorManagedIdentity) DeepCopy() *ServiceProviderClusterDataPlaneOperatorManagedIdentity { + if in == nil { + return nil + } + out := new(ServiceProviderClusterDataPlaneOperatorManagedIdentity) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ServiceProviderClusterDataPlaneOperatorsManagedIdentities) DeepCopyInto(out *ServiceProviderClusterDataPlaneOperatorsManagedIdentities) { + *out = *in + if in.Identities != nil { + in, out := &in.Identities, &out.Identities + *out = make(map[string]*ServiceProviderClusterDataPlaneOperatorManagedIdentity, len(*in)) + for key, val := range *in { + var outVal *ServiceProviderClusterDataPlaneOperatorManagedIdentity + if val == nil { + (*out)[key] = nil + } else { + in, out := &val, &outVal + *out = new(ServiceProviderClusterDataPlaneOperatorManagedIdentity) + (*in).DeepCopyInto(*out) + } + (*out)[key] = outVal + } + } + if in.EarliestRecheckTime != nil { + in, out := &in.EarliestRecheckTime, &out.EarliestRecheckTime + *out = (*in).DeepCopy() + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceProviderClusterDataPlaneOperatorsManagedIdentities. +func (in *ServiceProviderClusterDataPlaneOperatorsManagedIdentities) DeepCopy() *ServiceProviderClusterDataPlaneOperatorsManagedIdentities { + if in == nil { + return nil + } + out := new(ServiceProviderClusterDataPlaneOperatorsManagedIdentities) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ServiceProviderClusterList) DeepCopyInto(out *ServiceProviderClusterList) { *out = *in @@ -2222,6 +2292,7 @@ func (in *ServiceProviderClusterStatus) DeepCopyInto(out *ServiceProviderCluster } in.AzureResources.DeepCopyInto(&out.AzureResources) in.MSIManagedIdentities.DeepCopyInto(&out.MSIManagedIdentities) + in.DataPlaneOperatorsManagedIdentities.DeepCopyInto(&out.DataPlaneOperatorsManagedIdentities) return }