Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions backend/pkg/app/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
credentialrevocationdeletion "github.com/Azure/ARO-HCP/backend/pkg/controllers/cluster/credentialrevocation/deletion"
credentialrevocationoperations "github.com/Azure/ARO-HCP/backend/pkg/controllers/cluster/credentialrevocation/operations"
clusterdeletion "github.com/Azure/ARO-HCP/backend/pkg/controllers/cluster/deletion"
clusteridentity "github.com/Azure/ARO-HCP/backend/pkg/controllers/cluster/identity"
"github.com/Azure/ARO-HCP/backend/pkg/controllers/cluster/legacycredentialrequest"
clusteroperations "github.com/Azure/ARO-HCP/backend/pkg/controllers/cluster/operations"
clusterplacement "github.com/Azure/ARO-HCP/backend/pkg/controllers/cluster/placement"
Expand Down Expand Up @@ -695,9 +696,8 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context,
unionKubeApplierInformers,
unionReadDesireLister,
)
identityMigrationController := clusterproperties.NewIdentityMigrationController(
clusterIdentitySyncController := clusteridentity.NewClusterIdentitySyncController(
b.options.ResourcesDBClient,
b.options.ClustersServiceClient,
backendInformers,
unionKubeApplierInformers,
)
Expand Down Expand Up @@ -1006,6 +1006,13 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context,
backendInformers,
)

fetchMSIIdentitiesInfoController := clusteridentity.NewFetchMSIIdentitiesInfoController(
b.clock,
b.options.ResourcesDBClient,
backendInformers,
b.options.FPAMIDataplaneClientBuilder,
)

leaderElectionConfig := leaderelection.LeaderElectionConfig{
Lock: b.options.LeaderElectionLock,
LeaseDuration: sharedleaderelection.RecommendedLeaseDuration,
Expand Down Expand Up @@ -1069,7 +1076,7 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context,
go triggerControlPlaneUpgradeController.Run(ctx, 20)
go clusterBaseDomainPrefixSyncController.Run(ctx, 20)
go clusterPropertiesSyncController.Run(ctx, 20)
go identityMigrationController.Run(ctx, 20)
go clusterIdentitySyncController.Run(ctx, 20)
go clusterDegradedAggregatorController.Run(ctx, 20)
go clusterRequirementsValidAggregatorController.Run(ctx, 20)
go nodePoolDegradedAggregatorController.Run(ctx, 20)
Expand Down Expand Up @@ -1117,6 +1124,7 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context,
go cosmosMigrationController.Run(ctx, 5)
go virtualMachineResourceSKUsCachedReaderController.Run(ctx, 20)
go backupScheduleController.Run(ctx, 20)
go fetchMSIIdentitiesInfoController.Run(ctx, 20)
},
OnStoppedLeading: func() {
// This needs to be defined even though it does nothing.
Expand Down
204 changes: 204 additions & 0 deletions backend/pkg/controllers/cluster/identity/cluster_identity_sync.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
// 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"
"fmt"
"strings"
"time"

"k8s.io/apimachinery/pkg/api/equality"

"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"
unionkubeapplierinformers "github.com/Azure/ARO-HCP/internal/database/unioninformers/kubeapplier"
"github.com/Azure/ARO-HCP/internal/utils"
)

const ClusterIdentitySyncControllerName = "ClusterIdentitySync"

// clusterIdentitySyncer keeps ClientID/PrincipalID on
// HCPOpenShiftCluster.Identity.UserAssignedIdentities in sync with
// ServiceProviderCluster.Status.MSIManagedIdentities. It iterates the existing
// Identity map keys (preserving casing) and looks up each one in SPC by
// lowercased resource ID.
type clusterIdentitySyncer struct {
clusterLister corelisters.ClusterLister
serviceProviderClusterLister corelisters.ServiceProviderClusterLister
resourcesDBClient corecosmosstorage.ResourcesDBClient
}

var _ controllerutils.ClusterSyncer = (*clusterIdentitySyncer)(nil)

// NewClusterIdentitySyncController creates a new controller that continuously
// syncs Identity.UserAssignedIdentities ClientID/PrincipalID from
// ServiceProviderCluster.Status.MSIManagedIdentities.
//
// It compares Cluster.Identity against SPC and updates when ClientID/PrincipalID
// would change (including nil values returned when an identity does not exist).
// Map keys in Identity keep the casing from CustomerProperties; SPC lookups use
// lowercased resource IDs. Keys remain even when SPC does not yet have a matching
// identity entry. Deleting clusters are skipped.
func NewClusterIdentitySyncController(
resourcesDBClient corecosmosstorage.ResourcesDBClient,
informers coreinformers.BackendInformers,
kubeApplierInformers *unionkubeapplierinformers.UnionKubeApplierInformers,
) controllerutils.Controller {
_, clusterLister := informers.Clusters()
_, serviceProviderClusterLister := informers.ServiceProviderClusters()

syncer := &clusterIdentitySyncer{
clusterLister: clusterLister,
serviceProviderClusterLister: serviceProviderClusterLister,
resourcesDBClient: resourcesDBClient,
}

controller := controllerutils.NewClusterWatchingController(
ClusterIdentitySyncControllerName,
resourcesDBClient,
informers,
kubeApplierInformers,
60*time.Minute, // Check every 60 minutes
syncer,
)

return controller
}

func (c *clusterIdentitySyncer) NeedsWork(ctx context.Context, existingCluster *coreapi.HCPOpenShiftCluster) bool {
if existingCluster.ServiceProviderProperties.DeletionTimestamp != nil {
return false
}

if existingCluster.Identity == nil || len(existingCluster.Identity.UserAssignedIdentities) == 0 {
return false
}

return true
}

// SyncOnce performs a single reconciliation of cluster identity information.
// It iterates Identity.UserAssignedIdentities, looks up each key (lowercased)
// in ServiceProviderCluster.Status.MSIManagedIdentities, and updates
// ClientID/PrincipalID when SPC has a match. Keys that are absent from SPC
// remain unchanged.
func (c *clusterIdentitySyncer) SyncOnce(ctx context.Context, key controllerutils.HCPClusterKey) error {
logger := utils.LoggerFromContext(ctx)

// do the super cheap cache check first
cachedCluster, err := c.clusterLister.Get(ctx, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName)
if cosmosstorageutils.IsNotFoundError(err) {
// we'll be re-fired if it is created again
return nil
}
if err != nil {
return utils.TrackError(fmt.Errorf("failed to get cluster from cache: %w", err))
}
if !c.NeedsWork(ctx, cachedCluster) {
// if the cache doesn't need work, then we'll be retriggered if those values change when the cache updates.
// if the values don't change, then we still have no work to do.
return nil
}

// Get the cluster from Cosmos
clusterCRUD := c.resourcesDBClient.HCPClusters(key.SubscriptionID, key.ResourceGroupName)
existingCluster, err := clusterCRUD.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))
}
// check if we need to do work again. Sometimes the live data is ahead of the cache and obviates the need to do any work
if !c.NeedsWork(ctx, existingCluster) {
return nil
}
Comment on lines +120 to +132

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no reason for live gets


existingSPC, err := c.serviceProviderClusterLister.Get(ctx, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName)
if cosmosstorageutils.IsNotFoundError(err) {
// SPC may not exist yet; nothing to copy into Identity.
return nil
}
if err != nil {
return utils.TrackError(fmt.Errorf("failed to get ServiceProviderCluster from cache: %w", err))
}

replacement := existingCluster.DeepCopy()
c.updateIdentityUserAssignedIdentitiesFromSPC(
replacement.Identity.UserAssignedIdentities,
existingSPC.Status.MSIManagedIdentities.ControlPlaneOperatorsIdentities,
existingSPC.Status.MSIManagedIdentities.ServiceManagedIdentity,
)

if equality.Semantic.DeepEqual(existingCluster.Identity, replacement.Identity) {
return nil
}

// Write the updated cluster back to Cosmos
_, err = clusterCRUD.Replace(ctx, replacement, nil)
if cosmosstorageutils.IsPreconditionFailedError(err) {
// if we have a conflict error, then we're guaranteed that our informer will eventually see an update and trigger us again.
return nil
}
if err != nil {
return utils.TrackError(fmt.Errorf("failed to replace Cluster: %w", err))
}

logger.Info("synced identity information from ServiceProviderCluster")
return nil
}

// updateIdentityUserAssignedIdentitiesFromSPC walks the existing Identity map and, for
// each key, looks up the lowercased resource ID in SPC control-plane operator
// identities or the service managed identity. When found, ClientID and
// PrincipalID are updated in place. Keys missing from SPC are left as-is.
func (c *clusterIdentitySyncer) updateIdentityUserAssignedIdentitiesFromSPC(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't abbreviate SPC

identityUserAssignedIdentities map[string]*coreapi.UserAssignedIdentity,
spcControlPlaneOperatorsIdentities map[string]*coreapi.ServiceProviderClusterControlPlaneOperatorIdentity,
spcServiceManagedIdentity *coreapi.ServiceProviderClusterServiceManagedIdentity,
) {
for identityResourceIDStr := range identityUserAssignedIdentities {
lowerResourceIDStr := strings.ToLower(identityResourceIDStr)

var clientID, principalID *string

// If we found the identity in the SPC control plane operators identities, we use the ClientID and PrincipalID from the SPC.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't abbrevaite spc throughout

if spcIdentity, ok := spcControlPlaneOperatorsIdentities[lowerResourceIDStr]; ok && spcIdentity != nil {
clientID = spcIdentity.ClientID
principalID = spcIdentity.PrincipalID
// If we found the identity in the SPC service managed identity, we use the ClientID and PrincipalID from the SPC.
} else if spcServiceManagedIdentity != nil && spcServiceManagedIdentity.ResourceID != nil &&
strings.ToLower(spcServiceManagedIdentity.ResourceID.String()) == lowerResourceIDStr {
clientID = spcServiceManagedIdentity.ClientID
principalID = spcServiceManagedIdentity.PrincipalID
Comment on lines +187 to +190

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is some kind of a default? Really, that's shocking.

} else { // otherwise, we leave the identity as-is
continue

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't like this. We should clear it. There was a way to express empty before. I don't recall if it was nil or zero-value reference, but we allowed it. Leave the key, but clear the value.

}
Comment on lines +183 to +193

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactor to switch/case please


// We initialize the identity if it does not exist, to avoid nil dereferencing
if identityUserAssignedIdentities[identityResourceIDStr] == nil {
identityUserAssignedIdentities[identityResourceIDStr] = &coreapi.UserAssignedIdentity{}
}
// We update the identity with the ClientID and PrincipalID values the SPC. Those values themselves
// maybe be nil or empty strings.
identityUserAssignedIdentities[identityResourceIDStr].ClientID = clientID
identityUserAssignedIdentities[identityResourceIDStr].PrincipalID = principalID
Comment on lines +195 to +202

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just always do

identityUserAssignedIdentities[identityResourceIDStr] = &coreapi.UserAssignedIdentity{
clientID: clientID, 
principlalID: principalID,
}

Also, inline into the switch/case above.

}
}
Loading