From 280bee8164a3bcc0acb98c9215f33b42891404f8 Mon Sep 17 00:00:00 2001 From: David Eads Date: Mon, 2 Mar 2026 18:21:14 -0500 Subject: [PATCH 01/10] refactor to use only cosmos for cluster reads --- .../identity_migration.go | 16 +-- frontend/pkg/frontend/cluster.go | 125 ++---------------- internal/ocm/convert.go | 41 +----- 3 files changed, 24 insertions(+), 158 deletions(-) diff --git a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go index f520e0df9d1..ba319bce319 100644 --- a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go +++ b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go @@ -45,7 +45,7 @@ var _ controllerutils.ClusterSyncer = (*identityMigrationSyncer)(nil) // NewIdentityMigrationController creates a new controller that migrates identity information // from Cluster Service to Cosmos DB. // It periodically checks each cluster and populates the Identity.UserAssignedIdentities -// field if it is not set, using SetClusterServiceOnlyFieldsOnCluster to extract the identity data. +// field if it is not set, using GetClusterServiceUserAssignedIdentities to extract the identity data. func NewIdentityMigrationController( cosmosClient database.DBClient, clusterServiceClient ocm.ClusterServiceClientSpec, @@ -98,7 +98,7 @@ func (c *identityMigrationSyncer) NeedsWork(ctx context.Context, existingCluster // SyncOnce performs a single reconciliation of cluster identity information. // It checks if the Identity.UserAssignedIdentities field is unset, // and if so, fetches the values from Cluster Service using -// SetClusterServiceOnlyFieldsOnCluster and updates Cosmos with +// GetClusterServiceUserAssignedIdentities and updates Cosmos with // the Identity.UserAssignedIdentities only. func (c *identityMigrationSyncer) SyncOnce(ctx context.Context, key controllerutils.HCPClusterKey) error { logger := utils.LoggerFromContext(ctx) @@ -138,12 +138,10 @@ func (c *identityMigrationSyncer) SyncOnce(ctx context.Context, key controllerut return utils.TrackError(fmt.Errorf("failed to get cluster from Cluster Service: %w", err)) } - // Use SetClusterServiceOnlyFieldsOnCluster on a deep copy to extract identity data - clusterCopy := existingCluster.DeepCopy() - ocm.SetClusterServiceOnlyFieldsOnCluster(clusterCopy, csCluster) - - // nothing to set - if clusterCopy.Identity == nil || len(clusterCopy.Identity.UserAssignedIdentities) == 0 { + // Use GetClusterServiceUserAssignedIdentities on a deep copy to extract identity data + userAssignedIdentities := ocm.GetClusterServiceUserAssignedIdentities(csCluster) + if len(userAssignedIdentities) == 0 { + // nothing to set return nil } @@ -151,7 +149,7 @@ func (c *identityMigrationSyncer) SyncOnce(ctx context.Context, key controllerut if existingCluster.Identity == nil { existingCluster.Identity = &arm.ManagedServiceIdentity{} } - existingCluster.Identity.UserAssignedIdentities = clusterCopy.Identity.UserAssignedIdentities + existingCluster.Identity.UserAssignedIdentities = userAssignedIdentities // Write the updated cluster back to Cosmos if _, err := clusterCRUD.Replace(ctx, existingCluster, nil); err != nil { diff --git a/frontend/pkg/frontend/cluster.go b/frontend/pkg/frontend/cluster.go index 928979d0af9..66682f6806e 100644 --- a/frontend/pkg/frontend/cluster.go +++ b/frontend/pkg/frontend/cluster.go @@ -78,7 +78,6 @@ func (f *Frontend) GetHCPCluster(writer http.ResponseWriter, request *http.Reque func (f *Frontend) ArmResourceListClusters(writer http.ResponseWriter, request *http.Request) error { ctx := request.Context() - logger := utils.LoggerFromContext(ctx) versionedInterface, err := VersionFromContext(ctx) if err != nil { @@ -99,14 +98,22 @@ func (f *Frontend) ArmResourceListClusters(writer http.ResponseWriter, request * if err != nil { return utils.TrackError(err) } - clustersByClusterServiceID := make(map[string]*api.HCPOpenShiftCluster) for _, internalCluster := range internalClusterIterator.Items(ctx) { +<<<<<<< HEAD if internalCluster.ServiceProviderProperties.ClusterServiceID == nil { // TODO this will be removed during our switch to read only from cosmos. // we can still merge now since the value will never be nil until both the read path is fixed and this PR makes it to prod. continue } clustersByClusterServiceID[internalCluster.ServiceProviderProperties.ClusterServiceID.ID()] = internalCluster +======= + resultingExternalCluster := versionedInterface.NewHCPOpenShiftCluster(internalCluster) + jsonBytes, err := arm.MarshalJSON(resultingExternalCluster) + if err != nil { + return utils.TrackError(err) + } + pagedResponse.AddValue(jsonBytes) +>>>>>>> bc80f6968 (refactor to use only cosmos for cluster reads) } err = internalClusterIterator.GetError() if err != nil { @@ -118,37 +125,6 @@ func (f *Frontend) ArmResourceListClusters(writer http.ResponseWriter, request * return utils.TrackError(err) } - // Build a Cluster Service query that looks for - // the specific IDs returned by the Cosmos query. - queryIDs := make([]string, 0, len(clustersByClusterServiceID)) - for key := range clustersByClusterServiceID { - queryIDs = append(queryIDs, "'"+key+"'") - } - query := fmt.Sprintf("id in (%s)", strings.Join(queryIDs, ", ")) - logger.Info(fmt.Sprintf("Searching Cluster Service for %q", query)) - - csIterator := f.clusterServiceClient.ListClusters(query) - - for csCluster := range csIterator.Items(ctx) { - if internalCluster, ok := clustersByClusterServiceID[csCluster.ID()]; ok { - // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos - internalCluster, err = mergeToInternalCluster(csCluster, internalCluster, f.azureLocation) - if err != nil { - return utils.TrackError(err) - } - resultingExternalCluster := versionedInterface.NewHCPOpenShiftCluster(internalCluster) - jsonBytes, err := arm.MarshalJSON(resultingExternalCluster) - if err != nil { - return utils.TrackError(err) - } - pagedResponse.AddValue(jsonBytes) - } - } - // Check for iteration error. - if err := csIterator.GetError(); err != nil { - return utils.TrackError(err) - } - _, err = arm.WriteJSONResponse(writer, http.StatusOK, pagedResponse) if err != nil { return utils.TrackError(err) @@ -201,11 +177,6 @@ func (f *Frontend) CreateOrUpdateHCPCluster(writer http.ResponseWriter, request updating := oldInternalCluster != nil if updating { - // re-write oldInternalCluster for as long as cluster-service needs to be consulted for pre-existing state. - oldInternalCluster, err = f.readInternalClusterFromClusterService(ctx, oldInternalCluster) - if err != nil { - return utils.TrackError(err) - } // CheckForProvisioningStateConflict does not log conflict errors // but does log unexpected errors like database failures. if err := checkForProvisioningStateConflict(ctx, f.dbClient, database.OperationRequestUpdate, oldInternalCluster.ID, oldInternalCluster.ServiceProviderProperties.ProvisioningState); err != nil { @@ -424,11 +395,6 @@ func (f *Frontend) createHCPCluster(writer http.ResponseWriter, request *http.Re return fmt.Errorf("unexpected type %T", resultingUncastInternalCluster) } - // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos - resultingInternalCluster, err = mergeToInternalCluster(resultingClusterServiceCluster, resultingInternalCluster, f.azureLocation) - if err != nil { - return utils.TrackError(err) - } responseBytes, err := arm.MarshalJSON(versionedInterface.NewHCPOpenShiftCluster(resultingInternalCluster)) if err != nil { return utils.TrackError(err) @@ -711,11 +677,6 @@ func (f *Frontend) updateHCPClusterInCosmos(ctx context.Context, writer http.Res } resultingInternalCluster := resultingUncastObj.(*api.HCPOpenShiftCluster) - // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos - resultingInternalCluster, err = mergeToInternalCluster(resultingClusterServiceCluster, resultingInternalCluster, f.azureLocation) - if err != nil { - return utils.TrackError(err) - } responseBytes, err := arm.MarshalJSON(versionedInterface.NewHCPOpenShiftCluster(resultingInternalCluster)) if err != nil { return utils.TrackError(err) @@ -873,72 +834,6 @@ func (f *Frontend) addDeleteClusterToTransaction(ctx context.Context, writer htt return nil } -// mergeToInternalCluster renders a CS Cluster object in JSON format, applying -// the necessary conversions for the API version of the request. -// TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos -// TODO remove the azure location once we have migrated every record to store the location -func mergeToInternalCluster(csCluster *arohcpv1alpha1.Cluster, internalCluster *api.HCPOpenShiftCluster, azureLocation string) (*api.HCPOpenShiftCluster, error) { - if csCluster == nil { - return nil, utils.TrackError(fmt.Errorf("cannot merge nil cluster")) - } - if len(internalCluster.CustomerProperties.Version.ChannelGroup) == 0 { - // if we hit this branch, then we have old data that exists from before we stored all the content of the requested cluster - return legacyMergeToInternalCluster(csCluster, internalCluster, azureLocation) - } - - // otherwise use as much from cosmos as possible, so we have a clear list of what remains in cluster-service - ocm.SetClusterServiceOnlyFieldsOnCluster(internalCluster, csCluster) - - return internalCluster, nil -} - -func legacyMergeToInternalCluster(csCluster *arohcpv1alpha1.Cluster, internalCluster *api.HCPOpenShiftCluster, azureLocation string) (*api.HCPOpenShiftCluster, error) { - clusterServiceBasedInternalCluster, err := ocm.LegacyCreateInternalClusterFromClusterService(internalCluster.ID, azureLocation, csCluster) - if err != nil { - return nil, utils.TrackError(err) - } - - // this does not use conversion.CopyReadOnly* because some ServiceProvider properties come from cluster-service-only or live reads - clusterServiceBasedInternalCluster.SystemData = internalCluster.SystemData.DeepCopy() - clusterServiceBasedInternalCluster.Tags = maps.Clone(internalCluster.Tags) - clusterServiceBasedInternalCluster.ServiceProviderProperties.ExistingCosmosUID = internalCluster.ServiceProviderProperties.ExistingCosmosUID - clusterServiceBasedInternalCluster.ServiceProviderProperties.ProvisioningState = internalCluster.ServiceProviderProperties.ProvisioningState - clusterServiceBasedInternalCluster.ServiceProviderProperties.ActiveOperationID = internalCluster.ServiceProviderProperties.ActiveOperationID - clusterServiceBasedInternalCluster.ServiceProviderProperties.ClusterServiceID = internalCluster.ServiceProviderProperties.ClusterServiceID - if clusterServiceBasedInternalCluster.Identity == nil { - clusterServiceBasedInternalCluster.Identity = &arm.ManagedServiceIdentity{} - } - - if internalCluster.Identity != nil { - clusterServiceBasedInternalCluster.Identity.PrincipalID = internalCluster.Identity.PrincipalID - clusterServiceBasedInternalCluster.Identity.TenantID = internalCluster.Identity.TenantID - clusterServiceBasedInternalCluster.Identity.Type = internalCluster.Identity.Type - } - - return clusterServiceBasedInternalCluster, nil -} - -// readInternalClusterFromClusterService takes an internal Cluster read from cosmos, retrieves the corresponding cluster-service data, -// merges the states together, and returns the internal representation. -// TODO remove the header it takes and collapse that to some general error handling. -func (f *Frontend) readInternalClusterFromClusterService(ctx context.Context, oldInternalCluster *api.HCPOpenShiftCluster) (*api.HCPOpenShiftCluster, error) { - if oldInternalCluster.ServiceProviderProperties.ClusterServiceID == nil { - return nil, utils.TrackError(errors.New("clusterServiceID is nil")) - } - oldClusterServiceCluster, err := f.clusterServiceClient.GetCluster(ctx, *oldInternalCluster.ServiceProviderProperties.ClusterServiceID) - if err != nil { - return nil, utils.TrackError(err) - } - - // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos - oldInternalCluster, err = mergeToInternalCluster(oldClusterServiceCluster, oldInternalCluster, f.azureLocation) - if err != nil { - return nil, utils.TrackError(err) - } - - return oldInternalCluster, nil -} - func (f *Frontend) getInternalClusterFromStorage(ctx context.Context, resourceID *azcorearm.ResourceID) (*api.HCPOpenShiftCluster, error) { internalCluster, err := f.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).Get(ctx, resourceID.Name) if database.IsNotFoundError(err) { @@ -967,7 +862,7 @@ func (f *Frontend) getInternalClusterFromStorage(ctx context.Context, resourceID } internalCluster.ID = resourceID - return f.readInternalClusterFromClusterService(ctx, internalCluster) + return internalCluster, nil } // ensureSystemData tries to use the src systemData diff --git a/internal/ocm/convert.go b/internal/ocm/convert.go index e91168ac5af..f2c63ba76ba 100644 --- a/internal/ocm/convert.go +++ b/internal/ocm/convert.go @@ -701,60 +701,33 @@ func LegacyCreateInternalClusterFromClusterService(resourceID *azcorearm.Resourc return hcpcluster, nil } -// SetClusterServiceOnlyFieldsOnCluster converts a CS Cluster object into an HCPOpenShiftCluster object. -func SetClusterServiceOnlyFieldsOnCluster(internalCluster *api.HCPOpenShiftCluster, clusterServiceCluster *arohcpv1alpha1.Cluster) { - // this is defaulted if the user doesn't specify, so it isn't always known to to frontend. We will eventually have to - // choose when and where we set this. - if len(internalCluster.CustomerProperties.DNS.BaseDomainPrefix) == 0 { - internalCluster.CustomerProperties.DNS.BaseDomainPrefix = clusterServiceCluster.DomainPrefix() - } - - // this is defaulted if the user doesn't specify, so it isn't always known to to frontend. We will eventually have to - // choose when and where we set this. - if len(internalCluster.CustomerProperties.Platform.ManagedResourceGroup) == 0 { - internalCluster.CustomerProperties.Platform.ManagedResourceGroup = clusterServiceCluster.Azure().ManagedResourceGroupName() - } - - internalCluster.ServiceProviderProperties.DNS.BaseDomain = clusterServiceCluster.DNS().BaseDomain() - internalCluster.ServiceProviderProperties.Console.URL = clusterServiceCluster.Console().URL() - internalCluster.ServiceProviderProperties.API.URL = clusterServiceCluster.API().URL() - internalCluster.ServiceProviderProperties.Platform.IssuerURL = clusterServiceCluster.Azure().OidcIssuerUrl() +// GetClusterServiceUserAssignedIdentities converts a CS Cluster object into an HCPOpenShiftCluster object. +func GetClusterServiceUserAssignedIdentities(clusterServiceCluster *arohcpv1alpha1.Cluster) map[string]*arm.UserAssignedIdentity { + ret := make(map[string]*arm.UserAssignedIdentity) // the clientID and principalID are currently only known to cluster-service. We'll need to determine them somewhere else. if clusterServiceCluster.Azure().OperatorsAuthentication() != nil { if mi, ok := clusterServiceCluster.Azure().OperatorsAuthentication().GetManagedIdentities(); ok { for _, operatorIdentity := range mi.ControlPlaneOperatorsManagedIdentities() { - if internalCluster.Identity == nil { - internalCluster.Identity = &arm.ManagedServiceIdentity{} - } - if internalCluster.Identity.UserAssignedIdentities == nil { - internalCluster.Identity.UserAssignedIdentities = make(map[string]*arm.UserAssignedIdentity) - } - clientID, _ := operatorIdentity.GetClientID() principalID, _ := operatorIdentity.GetPrincipalID() - internalCluster.Identity.UserAssignedIdentities[operatorIdentity.ResourceID()] = &arm.UserAssignedIdentity{ + ret[operatorIdentity.ResourceID()] = &arm.UserAssignedIdentity{ ClientID: &clientID, PrincipalID: &principalID, } } if len(mi.ServiceManagedIdentity().ResourceID()) > 0 { - if internalCluster.Identity == nil { - internalCluster.Identity = &arm.ManagedServiceIdentity{} - } - if internalCluster.Identity.UserAssignedIdentities == nil { - internalCluster.Identity.UserAssignedIdentities = make(map[string]*arm.UserAssignedIdentity) - } - clientID, _ := mi.ServiceManagedIdentity().GetClientID() principalID, _ := mi.ServiceManagedIdentity().GetPrincipalID() - internalCluster.Identity.UserAssignedIdentities[mi.ServiceManagedIdentity().ResourceID()] = &arm.UserAssignedIdentity{ + ret[mi.ServiceManagedIdentity().ResourceID()] = &arm.UserAssignedIdentity{ ClientID: &clientID, PrincipalID: &principalID, } } } } + + return ret } func convertRpAutoscalarToCSBuilder(in *api.ClusterAutoscalingProfile) (*arohcpv1alpha1.ClusterAutoscalerBuilder, error) { From c178a48d5c616116fd2d2fd3ef6c63bb86df4b13 Mon Sep 17 00:00:00 2001 From: David Eads Date: Mon, 2 Mar 2026 18:26:28 -0500 Subject: [PATCH 02/10] remove unnecessary code now that we've migrated away from cluster-service for read path --- backend/pkg/app/backend.go | 7 - .../cluster_customer_properties_migration.go | 151 ------- ...ster_customer_properties_migration_test.go | 267 ------------- .../identity_migration_test.go | 1 + frontend/pkg/frontend/cluster.go | 23 +- .../convert_defaults_consistency_test.go | 58 --- internal/ocm/convert.go | 370 ----------------- internal/ocm/convert_test.go | 376 ------------------ 8 files changed, 3 insertions(+), 1250 deletions(-) delete mode 100644 backend/pkg/controllers/clusterpropertiescontroller/cluster_customer_properties_migration.go delete mode 100644 backend/pkg/controllers/clusterpropertiescontroller/cluster_customer_properties_migration_test.go diff --git a/backend/pkg/app/backend.go b/backend/pkg/app/backend.go index db32f83d5db..ad855c5baec 100644 --- a/backend/pkg/app/backend.go +++ b/backend/pkg/app/backend.go @@ -436,12 +436,6 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context, activeOperationLister, backendInformers, ) - clusterServiceMigrationController := clusterpropertiescontroller.NewClusterCustomerPropertiesMigrationController( - b.options.CosmosDBClient, - b.options.ClustersServiceClient, - activeOperationLister, - backendInformers, - ) identityMigrationController := clusterpropertiescontroller.NewIdentityMigrationController( b.options.CosmosDBClient, b.options.ClustersServiceClient, @@ -569,7 +563,6 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context, go controlPlaneDesiredVersionController.Run(ctx, 20) go triggerControlPlaneUpgradeController.Run(ctx, 20) go clusterPropertiesSyncController.Run(ctx, 20) - go clusterServiceMigrationController.Run(ctx, 20) go identityMigrationController.Run(ctx, 20) go azureRPRegistrationValidationController.Run(ctx, 20) go azureClusterResourceGroupExistenceValidationController.Run(ctx, 20) diff --git a/backend/pkg/controllers/clusterpropertiescontroller/cluster_customer_properties_migration.go b/backend/pkg/controllers/clusterpropertiescontroller/cluster_customer_properties_migration.go deleted file mode 100644 index 7e398c7e932..00000000000 --- a/backend/pkg/controllers/clusterpropertiescontroller/cluster_customer_properties_migration.go +++ /dev/null @@ -1,151 +0,0 @@ -// 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 clusterpropertiescontroller - -import ( - "context" - "fmt" - "time" - - "github.com/Azure/ARO-HCP/backend/pkg/controllers/controllerutils" - "github.com/Azure/ARO-HCP/backend/pkg/informers" - "github.com/Azure/ARO-HCP/backend/pkg/listers" - "github.com/Azure/ARO-HCP/internal/api" - "github.com/Azure/ARO-HCP/internal/database" - "github.com/Azure/ARO-HCP/internal/ocm" - "github.com/Azure/ARO-HCP/internal/utils" -) - -// clusterCustomerPropertiesMigrationController is a Cluster controller that migrates customerProperties from cluster-service -// to cosmos DB. It uses the Version.ID and Version.ChannelGroup fields to know that customerProperties are missing. -// Old records will lack those fields and once we read from cluster-service, we'll have the information we need. -type clusterCustomerPropertiesMigrationController struct { - cooldownChecker controllerutils.CooldownChecker - - clusterLister listers.ClusterLister - cosmosClient database.DBClient - clusterServiceClient ocm.ClusterServiceClientSpec -} - -var _ controllerutils.ClusterSyncer = (*clusterCustomerPropertiesMigrationController)(nil) - -func NewClusterCustomerPropertiesMigrationController( - cosmosClient database.DBClient, - clusterServiceClient ocm.ClusterServiceClientSpec, - activeOperationLister listers.ActiveOperationLister, - informers informers.BackendInformers, -) controllerutils.Controller { - _, clusterLister := informers.Clusters() - - syncer := &clusterCustomerPropertiesMigrationController{ - cooldownChecker: controllerutils.DefaultActiveOperationPrioritizingCooldown(activeOperationLister), - clusterLister: clusterLister, - cosmosClient: cosmosClient, - clusterServiceClient: clusterServiceClient, - } - - controller := controllerutils.NewClusterWatchingController( - "ClusterServiceMigration", - cosmosClient, - informers, - 60*time.Minute, // Check every 60 minutes - syncer, - ) - - return controller -} - -func (c *clusterCustomerPropertiesMigrationController) CooldownChecker() controllerutils.CooldownChecker { - return c.cooldownChecker -} - -func (c *clusterCustomerPropertiesMigrationController) NeedsWork(ctx context.Context, existingCluster *api.HCPOpenShiftCluster) bool { - // Check if we have a cluster service ID to query. We will lack this information for newly created records when we - // transition to async cluster-service creation. - if existingCluster.ServiceProviderProperties.ClusterServiceID == nil || len(existingCluster.ServiceProviderProperties.ClusterServiceID.String()) == 0 { - return false - } - - // Check if version information needs to be migrated - // Records that have this information already, then we have all the other info stored in cosmos, so we don't need to do anything with them - // Records that don't have this information need to be migrated - needsVersionID := len(existingCluster.CustomerProperties.Version.ID) == 0 - needsChannelGroup := len(existingCluster.CustomerProperties.Version.ChannelGroup) == 0 - if !needsVersionID && !needsChannelGroup { - return false - } - - return true -} - -func (c *clusterCustomerPropertiesMigrationController) 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 database.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.cosmosClient.HCPClusters(key.SubscriptionID, key.ResourceGroupName) - existingCluster, err := clusterCRUD.Get(ctx, key.HCPClusterName) - if database.IsNotFoundError(err) { - return nil // cluster doesn't exist, no work to do - } - if err != nil { - return utils.TrackError(fmt.Errorf("failed to get Cluster: %w", err)) - } - // check if we need to do work again. Sometimes the live data is more fresh than the cache and obviates the need to any work - if !c.NeedsWork(ctx, existingCluster) { - return nil - } - - // Fetch the cluster from Cluster Service - csCluster, err := c.clusterServiceClient.GetCluster(ctx, *existingCluster.ServiceProviderProperties.ClusterServiceID) - if err != nil { - return utils.TrackError(fmt.Errorf("failed to get cluster from Cluster Service: %w", err)) - } - - // Use LegacyCreateInternalClusterFromClusterService to convert the cluster and extract the CustomerProperties - convertedCluster, err := ocm.LegacyCreateInternalClusterFromClusterService( - existingCluster.ID, - existingCluster.Location, - csCluster, - ) - if err != nil { - return utils.TrackError(fmt.Errorf("failed to convert cluster from Cluster Service: %w", err)) - } - - // Update only the CustomerProperties from the converted cluster - existingCluster.CustomerProperties = convertedCluster.CustomerProperties - - // Write the updated cluster back to Cosmos - if _, err := clusterCRUD.Replace(ctx, existingCluster, nil); err != nil { - return utils.TrackError(fmt.Errorf("failed to replace Cluster: %w", err)) - } - - logger.Info("migrated customer properties from Cluster Service") - return nil -} diff --git a/backend/pkg/controllers/clusterpropertiescontroller/cluster_customer_properties_migration_test.go b/backend/pkg/controllers/clusterpropertiescontroller/cluster_customer_properties_migration_test.go deleted file mode 100644 index 6e2b5d450b7..00000000000 --- a/backend/pkg/controllers/clusterpropertiescontroller/cluster_customer_properties_migration_test.go +++ /dev/null @@ -1,267 +0,0 @@ -// 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 clusterpropertiescontroller - -import ( - "context" - "fmt" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "go.uber.org/mock/gomock" - - arohcpv1alpha1 "github.com/openshift-online/ocm-sdk-go/arohcp/v1alpha1" - - "github.com/Azure/ARO-HCP/backend/pkg/controllers/controllerutils" - "github.com/Azure/ARO-HCP/backend/pkg/listertesting" - "github.com/Azure/ARO-HCP/internal/api" - "github.com/Azure/ARO-HCP/internal/databasetesting" - "github.com/Azure/ARO-HCP/internal/ocm" -) - -const ( - testLocation = "eastus" - testVersionID = "4.15" - testChannelGroup = "stable" - testSubnetID = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/test-subnet" -) - -func TestClusterServiceMigrationSyncer_SyncOnce(t *testing.T) { - testCases := []struct { - name string - cachedCluster *api.HCPOpenShiftCluster // cluster in cache, nil means use same as existingCluster - existingCluster *api.HCPOpenShiftCluster // cluster in cosmos - csCluster *arohcpv1alpha1.Cluster - csError error - expectCosmosGet bool - expectCSCall bool - expectCosmosUpdate bool - expectError bool - expectedVersionID string - expectedChannelGroup string - }{ - { - name: "cache indicates no work needed - early return without cosmos lookup", - cachedCluster: newTestClusterForMigration(func(c *api.HCPOpenShiftCluster) { - c.CustomerProperties.Version.ID = testVersionID - c.CustomerProperties.Version.ChannelGroup = testChannelGroup - }), - existingCluster: newTestClusterForMigration(func(c *api.HCPOpenShiftCluster) { - c.CustomerProperties.Version.ID = testVersionID - c.CustomerProperties.Version.ChannelGroup = testChannelGroup - }), - expectCosmosGet: false, - expectCSCall: false, - expectCosmosUpdate: false, - expectError: false, - expectedVersionID: testVersionID, - expectedChannelGroup: testChannelGroup, - }, - { - name: "cache says work needed but live data says no work needed", - cachedCluster: newTestClusterForMigration(), // cache has no version info - existingCluster: newTestClusterForMigration(func(c *api.HCPOpenShiftCluster) { - // cosmos has the version info (cache is stale) - c.CustomerProperties.Version.ID = testVersionID - c.CustomerProperties.Version.ChannelGroup = testChannelGroup - }), - expectCosmosGet: true, - expectCSCall: false, - expectCosmosUpdate: false, - expectError: false, - expectedVersionID: testVersionID, - expectedChannelGroup: testChannelGroup, - }, - { - name: "no work to do - both versionID and channelGroup already set", - existingCluster: newTestClusterForMigration(func(c *api.HCPOpenShiftCluster) { - c.CustomerProperties.Version.ID = testVersionID - c.CustomerProperties.Version.ChannelGroup = testChannelGroup - }), - expectCosmosGet: false, - expectCSCall: false, - expectCosmosUpdate: false, - expectError: false, - expectedVersionID: testVersionID, - expectedChannelGroup: testChannelGroup, - }, - { - name: "error reading from cluster-service", - existingCluster: newTestClusterForMigration(), - csError: fmt.Errorf("connection refused"), - expectCosmosGet: true, - expectCSCall: true, - expectCosmosUpdate: false, - expectError: true, - expectedVersionID: "", - expectedChannelGroup: "", - }, - { - name: "success - migrate version when both fields missing", - existingCluster: newTestClusterForMigration(), - csCluster: buildFullCSCluster(testVersionID, testChannelGroup), - expectCosmosGet: true, - expectCSCall: true, - expectCosmosUpdate: true, - expectError: false, - expectedVersionID: testVersionID, - expectedChannelGroup: testChannelGroup, - }, - { - name: "success - migrate version when only ID missing", - existingCluster: newTestClusterForMigration(func(c *api.HCPOpenShiftCluster) { - c.CustomerProperties.Version.ChannelGroup = testChannelGroup - }), - csCluster: buildFullCSCluster(testVersionID, testChannelGroup), - expectCosmosGet: true, - expectCSCall: true, - expectCosmosUpdate: true, - expectError: false, - expectedVersionID: testVersionID, - expectedChannelGroup: testChannelGroup, - }, - { - name: "success - migrate version when only ChannelGroup missing", - existingCluster: newTestClusterForMigration(func(c *api.HCPOpenShiftCluster) { - c.CustomerProperties.Version.ID = testVersionID - }), - csCluster: buildFullCSCluster(testVersionID, testChannelGroup), - expectCosmosGet: true, - expectCSCall: true, - expectCosmosUpdate: true, - expectError: false, - expectedVersionID: testVersionID, - expectedChannelGroup: testChannelGroup, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - ctx := context.Background() - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - // Setup mock DB - mockDB := databasetesting.NewMockDBClient() - - // Create the cluster in the mock DB (cosmos) - clusterCRUD := mockDB.HCPClusters(testSubscriptionID, testResourceGroupName) - _, err := clusterCRUD.Create(ctx, tc.existingCluster, nil) - require.NoError(t, err) - - // Setup slice cluster lister (cache) - // If cachedCluster is nil, use the same as existingCluster - cachedCluster := tc.cachedCluster - if cachedCluster == nil { - cachedCluster = tc.existingCluster - } - sliceClusterLister := &listertesting.SliceClusterLister{ - Clusters: []*api.HCPOpenShiftCluster{cachedCluster}, - } - - // Setup mock CS client - mockCSClient := ocm.NewMockClusterServiceClientSpec(ctrl) - - if tc.expectCSCall { - mockCSClient.EXPECT(). - GetCluster(gomock.Any(), api.Must(api.NewInternalID(testClusterServiceIDStr))). - Return(tc.csCluster, tc.csError) - } - - // Create syncer - syncer := &clusterCustomerPropertiesMigrationController{ - cooldownChecker: &alwaysSyncCooldownChecker{}, - clusterLister: sliceClusterLister, - cosmosClient: mockDB, - clusterServiceClient: mockCSClient, - } - - // Execute - key := controllerutils.HCPClusterKey{ - SubscriptionID: testSubscriptionID, - ResourceGroupName: testResourceGroupName, - HCPClusterName: testClusterName, - } - err = syncer.SyncOnce(ctx, key) - - if tc.expectError { - require.Error(t, err) - } else { - require.NoError(t, err) - } - - // Verify the cluster state in Cosmos - updatedCluster, err := clusterCRUD.Get(ctx, testClusterName) - require.NoError(t, err) - - assert.Equal(t, tc.expectedVersionID, updatedCluster.CustomerProperties.Version.ID) - assert.Equal(t, tc.expectedChannelGroup, updatedCluster.CustomerProperties.Version.ChannelGroup) - }) - } -} - -// newTestClusterForMigration creates a test HCPOpenShiftCluster with default values -// including Location for use with LegacyCreateInternalClusterFromClusterService. -func newTestClusterForMigration(opts ...func(*api.HCPOpenShiftCluster)) *api.HCPOpenShiftCluster { - cluster := newTestCluster(opts...) - cluster.Location = testLocation - return cluster -} - -// buildFullCSCluster creates a mock Cluster Service cluster with all required fields -// for LegacyCreateInternalClusterFromClusterService. -func buildFullCSCluster(versionID, channelGroup string) *arohcpv1alpha1.Cluster { - cluster, err := arohcpv1alpha1.NewCluster(). - API(arohcpv1alpha1.NewClusterAPI(). - Listening(arohcpv1alpha1.ListeningMethodExternal)). - Azure(arohcpv1alpha1.NewAzure(). - EtcdEncryption(arohcpv1alpha1.NewAzureEtcdEncryption(). - DataEncryption(arohcpv1alpha1.NewAzureEtcdDataEncryption(). - KeyManagementMode("platform_managed"))). - ManagedResourceGroupName("managed-rg"). - NetworkSecurityGroupResourceID(""). - NodesOutboundConnectivity(arohcpv1alpha1.NewAzureNodesOutboundConnectivity(). - OutboundType("load_balancer")). - OperatorsAuthentication(arohcpv1alpha1.NewAzureOperatorsAuthentication(). - ManagedIdentities(arohcpv1alpha1.NewAzureOperatorsAuthenticationManagedIdentities(). - ControlPlaneOperatorsManagedIdentities(make(map[string]*arohcpv1alpha1.AzureControlPlaneManagedIdentityBuilder)). - DataPlaneOperatorsManagedIdentities(make(map[string]*arohcpv1alpha1.AzureDataPlaneManagedIdentityBuilder)). - ManagedIdentitiesDataPlaneIdentityUrl(""))). - SubnetResourceID(testSubnetID)). - Console(arohcpv1alpha1.NewClusterConsole().URL(testConsoleURL)). - DNS(arohcpv1alpha1.NewDNS().BaseDomain(testBaseDomain)). - DomainPrefix(testBaseDomainPrefix). - Network(arohcpv1alpha1.NewNetwork(). - HostPrefix(23). - MachineCIDR("10.0.0.0/16"). - PodCIDR("10.128.0.0/14"). - ServiceCIDR("172.30.0.0/16"). - Type("OVNKubernetes")). - Autoscaler(arohcpv1alpha1.NewClusterAutoscaler(). - PodPriorityThreshold(-10). - MaxNodeProvisionTime("15m"). - MaxPodGracePeriod(600)). - Version(arohcpv1alpha1.NewVersion(). - ID("openshift-v" + versionID + ".0"). - ChannelGroup(channelGroup)). - ImageRegistry(arohcpv1alpha1.NewClusterImageRegistry(). - State("enabled")). - Build() - if err != nil { - panic(err) - } - return cluster -} diff --git a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration_test.go b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration_test.go index 2be9490e0d0..06b5a163483 100644 --- a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration_test.go +++ b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration_test.go @@ -37,6 +37,7 @@ const ( testIdentityResourceID = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/test-identity" testClientID = "client-id-123" testPrincipalID = "principal-id-456" + testLocation = "test-location" ) func TestIdentityMigrationSyncer_SyncOnce(t *testing.T) { diff --git a/frontend/pkg/frontend/cluster.go b/frontend/pkg/frontend/cluster.go index 66682f6806e..9e30677667e 100644 --- a/frontend/pkg/frontend/cluster.go +++ b/frontend/pkg/frontend/cluster.go @@ -30,7 +30,6 @@ import ( azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" "github.com/Azure/azure-sdk-for-go/sdk/data/azcosmos" - arohcpv1alpha1 "github.com/openshift-online/ocm-sdk-go/arohcp/v1alpha1" ocmerrors "github.com/openshift-online/ocm-sdk-go/errors" "github.com/Azure/ARO-HCP/internal/admission" @@ -99,21 +98,12 @@ func (f *Frontend) ArmResourceListClusters(writer http.ResponseWriter, request * return utils.TrackError(err) } for _, internalCluster := range internalClusterIterator.Items(ctx) { -<<<<<<< HEAD - if internalCluster.ServiceProviderProperties.ClusterServiceID == nil { - // TODO this will be removed during our switch to read only from cosmos. - // we can still merge now since the value will never be nil until both the read path is fixed and this PR makes it to prod. - continue - } - clustersByClusterServiceID[internalCluster.ServiceProviderProperties.ClusterServiceID.ID()] = internalCluster -======= resultingExternalCluster := versionedInterface.NewHCPOpenShiftCluster(internalCluster) jsonBytes, err := arm.MarshalJSON(resultingExternalCluster) if err != nil { return utils.TrackError(err) } pagedResponse.AddValue(jsonBytes) ->>>>>>> bc80f6968 (refactor to use only cosmos for cluster reads) } err = internalClusterIterator.GetError() if err != nil { @@ -607,7 +597,6 @@ func (f *Frontend) updateHCPClusterInCosmos(ctx context.Context, writer http.Res tenantID = *subscription.Properties.TenantId } - var resultingClusterServiceCluster *arohcpv1alpha1.Cluster if oldInternalCluster.ServiceProviderProperties.ClusterServiceID != nil { oldClusterServiceCluster, err := f.clusterServiceClient.GetCluster(ctx, *oldInternalCluster.ServiceProviderProperties.ClusterServiceID) if err != nil { @@ -619,19 +608,11 @@ func (f *Frontend) updateHCPClusterInCosmos(ctx context.Context, writer http.Res } logger.Info(fmt.Sprintf("updating resource %s", oldInternalCluster.ID)) - resultingClusterServiceAutoscaler, err := f.clusterServiceClient.UpdateClusterAutoscaler(ctx, *oldInternalCluster.ServiceProviderProperties.ClusterServiceID, newClusterServiceAutoscalerBuilder) - if err != nil { - return utils.TrackError(err) - } - resultingClusterServiceCluster, err = f.clusterServiceClient.UpdateCluster(ctx, *oldInternalCluster.ServiceProviderProperties.ClusterServiceID, newClusterServiceClusterBuilder) + _, err = f.clusterServiceClient.UpdateClusterAutoscaler(ctx, *oldInternalCluster.ServiceProviderProperties.ClusterServiceID, newClusterServiceAutoscalerBuilder) if err != nil { return utils.TrackError(err) } - // Merge the autoscaler model into the cluster model. - resultingClusterServiceCluster, err = arohcpv1alpha1.NewCluster(). - Copy(resultingClusterServiceCluster). - Autoscaler(arohcpv1alpha1.NewClusterAutoscaler().Copy(resultingClusterServiceAutoscaler)). - Build() + _, err = f.clusterServiceClient.UpdateCluster(ctx, *oldInternalCluster.ServiceProviderProperties.ClusterServiceID, newClusterServiceClusterBuilder) if err != nil { return utils.TrackError(err) } diff --git a/internal/database/convert_defaults_consistency_test.go b/internal/database/convert_defaults_consistency_test.go index 72cad39a30b..0090a7ebb6b 100644 --- a/internal/database/convert_defaults_consistency_test.go +++ b/internal/database/convert_defaults_consistency_test.go @@ -197,64 +197,6 @@ func TestEnsureDefaultsConsistencyCluster(t *testing.T) { }) } -// TestCSToRPDefaultsConsistencyCluster verifies that when Cluster Service -// returns the default values for canonically-defaulted fields, the CS→RP -// conversion produces the same values as canonical defaults. -// See docs/api-version-defaults-and-storage.md. -func TestCSToRPDefaultsConsistencyCluster(t *testing.T) { - resourceID := api.Must(azcorearm.ParseResourceID( - "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/cluster", - )) - - // Build a CS cluster with the default values for each canonically-defaulted field. - // These are the CS-side representations of the default values. - csCluster, err := arohcpv1alpha1.NewCluster(). - API(arohcpv1alpha1.NewClusterAPI(). - Listening(arohcpv1alpha1.ListeningMethodExternal)). - Network(arohcpv1alpha1.NewNetwork(). - Type("OVNKubernetes")). - Azure(arohcpv1alpha1.NewAzure(). - NodesOutboundConnectivity(arohcpv1alpha1.NewAzureNodesOutboundConnectivity(). - OutboundType("load_balancer")). - EtcdEncryption(arohcpv1alpha1.NewAzureEtcdEncryption(). - DataEncryption(arohcpv1alpha1.NewAzureEtcdDataEncryption(). - KeyManagementMode("platform_managed")))). - ImageRegistry(arohcpv1alpha1.NewClusterImageRegistry(). - State("enabled")). - Build() - if err != nil { - t.Fatalf("failed to build CS cluster: %v", err) - } - - rpCluster, err := ocm.LegacyCreateInternalClusterFromClusterService(resourceID, "eastus", csCluster) - if err != nil { - t.Fatalf("LegacyCreateInternalClusterFromClusterService failed: %v", err) - } - - ensuredDefault := &api.HCPOpenShiftCluster{} - ensuredDefault.EnsureDefaults() - - checks := []struct { - name string - csToRPVal string - canonicalVal string - }{ - {"NetworkType", string(rpCluster.CustomerProperties.Network.NetworkType), string(ensuredDefault.CustomerProperties.Network.NetworkType)}, - {"Visibility", string(rpCluster.CustomerProperties.API.Visibility), string(ensuredDefault.CustomerProperties.API.Visibility)}, - {"OutboundType", string(rpCluster.CustomerProperties.Platform.OutboundType), string(ensuredDefault.CustomerProperties.Platform.OutboundType)}, - {"ClusterImageRegistry.State", string(rpCluster.CustomerProperties.ClusterImageRegistry.State), string(ensuredDefault.CustomerProperties.ClusterImageRegistry.State)}, - {"Etcd.DataEncryption.KeyManagementMode", string(rpCluster.CustomerProperties.Etcd.DataEncryption.KeyManagementMode), string(ensuredDefault.CustomerProperties.Etcd.DataEncryption.KeyManagementMode)}, - {"Version.ID", rpCluster.CustomerProperties.Version.ID, ensuredDefault.CustomerProperties.Version.ID}, - } - for _, c := range checks { - t.Run(c.name, func(t *testing.T) { - if c.csToRPVal != c.canonicalVal { - t.Errorf("CS→RP default = %q, canonical default = %q", c.csToRPVal, c.canonicalVal) - } - }) - } -} - // TestCSToRPDefaultsConsistencyNodePool verifies that when Cluster Service // returns the default value for DiskStorageAccountType, the CS→RP conversion // produces the same value as the canonical default. diff --git a/internal/ocm/convert.go b/internal/ocm/convert.go index f2c63ba76ba..f82b2beac47 100644 --- a/internal/ocm/convert.go +++ b/internal/ocm/convert.go @@ -90,23 +90,6 @@ func conversionError[T any](v any) error { return fmt.Errorf("cannot convert %T(%q) to %T: %w", v, v, *new(T), ErrUnknownValue) } -func convertListeningToVisibility(listening arohcpv1alpha1.ListeningMethod) (api.Visibility, error) { - switch listening { - case "": - // We convert illegal values because zero-value is the state for an object and while the value may not be valid - // We need to convert it and let validation worry about whether it is legal or illegal in the context of its usage. - // Zero values are preserved through round tripping these, the difference we're seeing here is the expression of unset - // in ocm-api-model is different than the expression of unset in canonical golang. - return "", nil - case arohcpv1alpha1.ListeningMethodExternal: - return api.VisibilityPublic, nil - case arohcpv1alpha1.ListeningMethodInternal: - return api.VisibilityPrivate, nil - default: - return "", conversionError[api.Visibility](listening) - } -} - func convertVisibilityToListening(visibility api.Visibility) (arohcpv1alpha1.ListeningMethod, error) { switch visibility { case api.VisibilityPublic: @@ -129,21 +112,6 @@ func convertKeyVaultVisibilityRPToCS(visibility api.KeyVaultVisibility) (arohcpv } } -func convertOutboundTypeCSToRP(outboundTypeCS string) (api.OutboundType, error) { - switch outboundTypeCS { - case "": - // We convert illegal values because zero-value is the state for an object and while the value may not be valid - // We need to convert it and let validation worry about whether it is legal or illegal in the context of its usage. - // Zero values are preserved through round tripping these, the difference we're seeing here is the expression of unset - // in ocm-api-model is different than the expression of unset in canonical golang. - return "", nil - case csOutboundType: - return api.OutboundTypeLoadBalancer, nil - default: - return "", conversionError[api.OutboundType](outboundTypeCS) - } -} - func convertOutboundTypeRPToCS(outboundTypeRP api.OutboundType) (string, error) { switch outboundTypeRP { case api.OutboundTypeLoadBalancer: @@ -217,15 +185,6 @@ func convertDiskTypeRPToCS(diskType api.OsDiskType) (string, error) { } } -func convertCustomerManagedEncryptionTypeCSToRP(encryptionTypeCS string) (api.CustomerManagedEncryptionType, error) { - switch encryptionTypeCS { - case csCustomerManagedEncryptionTypeKms: - return api.CustomerManagedEncryptionTypeKMS, nil - default: - return "", conversionError[api.CustomerManagedEncryptionType](encryptionTypeCS) - } -} - func convertCustomerManagedEncryptionTypeRPToCS(encryptionTypeRP api.CustomerManagedEncryptionType) (string, error) { switch encryptionTypeRP { case api.CustomerManagedEncryptionTypeKMS: @@ -261,17 +220,6 @@ func convertUsernameClaimPrefixPolicyRPToCS(prefixPolicyRP api.UsernameClaimPref } } -func convertAuthorizedCidrs(clusterAPI *arohcpv1alpha1.ClusterAPI) []string { - cidrAccess := clusterAPI.CIDRBlockAccess() - if cidrAccess.Empty() { - return nil - } - if cidr := cidrAccess.Allow(); cidr != nil { - return cidr.Values() - } - return nil -} - func convertEnableEncryptionAtHostToCSBuilder(in api.NodePoolPlatformProfile) *arohcpv1alpha1.AzureNodePoolEncryptionAtHostBuilder { var state string @@ -295,43 +243,6 @@ func convertClusterImageRegistryStateRPToCS(in api.ClusterImageRegistryProfile) } } -func convertClusterImageRegistryStateCSToRP(state string) (api.ClusterImageRegistryState, error) { - switch state { - case "": - // We convert illegal values because zero-value is the state for an object and while the value may not be valid - // We need to convert it and let validation worry about whether it is legal or illegal in the context of its usage. - // Zero values are preserved through round tripping these, the difference we're seeing here is the expression of unset - // in ocm-api-model is different than the expression of unset in canonical golang. - return "", nil - case csImageRegistryStateDisabled: - return api.ClusterImageRegistryStateDisabled, nil - case csImageRegistryStateEnabled: - return api.ClusterImageRegistryStateEnabled, nil - default: - return "", conversionError[api.ClusterImageRegistryState](state) - } -} - -func convertNodeDrainTimeoutCSToRP(in *arohcpv1alpha1.Cluster) int32 { - if nodeDrainGracePeriod, ok := in.GetNodeDrainGracePeriod(); ok { - if unit, ok := nodeDrainGracePeriod.GetUnit(); ok && unit == csNodeDrainGracePeriodUnit { - return int32(nodeDrainGracePeriod.Value()) - } - } - return 0 -} - -func convertKeyManagementModeTypeCSToRP(keyManagementModeCS string) (api.EtcdDataEncryptionKeyManagementModeType, error) { - switch keyManagementModeCS { - case csKeyManagementModePlatformManaged: - return api.EtcdDataEncryptionKeyManagementModeTypePlatformManaged, nil - case csKeyManagementModeCustomerManaged: - return api.EtcdDataEncryptionKeyManagementModeTypeCustomerManaged, nil - default: - return "", conversionError[api.EtcdDataEncryptionKeyManagementModeType](keyManagementModeCS) - } -} - func convertKeyManagementModeTypeRPToCS(keyManagementModeRP api.EtcdDataEncryptionKeyManagementModeType) (string, error) { switch keyManagementModeRP { case api.EtcdDataEncryptionKeyManagementModeTypePlatformManaged: @@ -365,89 +276,6 @@ func convertExternalAuthClientTypeRPToCS(externalAuthClientTypeRP api.ExternalAu } } -func convertCustomerManagedEncryptionCSToRP(in *arohcpv1alpha1.AzureEtcdDataEncryption) (*api.CustomerManagedEncryptionProfile, error) { - if customerManaged, ok := in.GetCustomerManaged(); ok { - encryptionType, err := convertCustomerManagedEncryptionTypeCSToRP(customerManaged.EncryptionType()) - if err != nil { - return nil, err - } - - kms, err := convertKmsEncryptionCSToRP(in.CustomerManaged()) - if err != nil { - return nil, err - } - - // Validate discriminated union: when encryptionType is KMS, Kms field must be present - if encryptionType == api.CustomerManagedEncryptionTypeKMS && kms == nil { - return nil, fmt.Errorf("cluster Service reported customer-managed encryption type KMS but did not provide KMS configuration") - } - - return &api.CustomerManagedEncryptionProfile{ - EncryptionType: encryptionType, - Kms: kms, - }, nil - } - - return nil, nil -} - -func convertKmsEncryptionCSToRP(in *arohcpv1alpha1.AzureEtcdDataEncryptionCustomerManaged) (*api.KmsEncryptionProfile, error) { - if kms, ok := in.GetKms(); ok { - // Only return a KmsEncryptionProfile if we have an activeKey - // to avoid creating invalid profiles with empty key fields - if activeKey, ok := kms.GetActiveKey(); ok { - kmsProfile := &api.KmsEncryptionProfile{ - ActiveKey: api.KmsKey{ - Name: activeKey.KeyName(), - VaultName: activeKey.KeyVaultName(), - Version: activeKey.KeyVersion(), - }, - } - - // Parse visibility if present - if visibility, ok := kms.GetVisibility(); ok { - switch visibility { - case arohcpv1alpha1.AzureKmsEncryptionVisibilityPublic: - kmsProfile.Visibility = api.KeyVaultVisibilityPublic - case arohcpv1alpha1.AzureKmsEncryptionVisibilityPrivate: - kmsProfile.Visibility = api.KeyVaultVisibilityPrivate - default: - return nil, fmt.Errorf("unknown KMS visibility value from Cluster Service: %q", visibility) - } - } - - return kmsProfile, nil - } - } - return nil, nil -} - -func convertAutoscalarCSToRP(in *arohcpv1alpha1.ClusterAutoscaler) (api.ClusterAutoscalingProfile, error) { - if in == nil { - return api.ClusterAutoscalingProfile{}, nil - } - - var maxNodeProvisionTime int32 - if len(in.MaxNodeProvisionTime()) > 0 { - // maxNodeProvisionTime (string) - minutes e.g - “15m” - // https://gitlab.cee.redhat.com/service/uhc-clusters-service/-/blob/master/pkg/api/autoscaler.go?ref_type=heads#L30-42 - maxNodeProvisionTimeDuration, err := time.ParseDuration(in.MaxNodeProvisionTime()) - if err != nil { - return api.ClusterAutoscalingProfile{}, err - } - maxNodeProvisionTime = int32(maxNodeProvisionTimeDuration.Seconds()) - } - - return api.ClusterAutoscalingProfile{ - MaxNodesTotal: int32(in.ResourceLimits().MaxNodesTotal()), - // MaxPodGracePeriod (int) - seconds e.g - 300 - // https://gitlab.cee.redhat.com/service/uhc-clusters-service/-/blob/master/pkg/api/autoscaler.go?ref_type=heads#L30-42 - MaxPodGracePeriodSeconds: int32(in.MaxPodGracePeriod()), - MaxNodeProvisionTimeSeconds: maxNodeProvisionTime, - PodPriorityThreshold: int32(in.PodPriorityThreshold()), - }, nil -} - func convertEtcdRPToCS(in api.EtcdProfile) (*arohcpv1alpha1.AzureEtcdEncryptionBuilder, error) { keyManagementMode, err := convertKeyManagementModeTypeRPToCS(in.DataEncryption.KeyManagementMode) if err != nil { @@ -503,204 +331,6 @@ func convertCIDRBlockAllowAccessRPToCS(in api.CustomerAPIProfile) (*arohcpv1alph return arohcpv1alpha1.NewCIDRBlockAccess().Allow(cidrBlockAllowAccess), nil } -// LegacyCreateInternalClusterFromClusterService this exists only for clusters that were created before we held all -// customer desired state in cosmos. -func LegacyCreateInternalClusterFromClusterService(resourceID *azcorearm.ResourceID, azureLocation string, cluster *arohcpv1alpha1.Cluster) (*api.HCPOpenShiftCluster, error) { - // A word about ProvisioningState: - // ProvisioningState is stored in Cosmos and is applied to the - // HCPOpenShiftCluster struct along with the ARM metadata that - // is also stored in Cosmos. We could convert the ClusterState - // from Cluster Service to a ProvisioningState, but instead we - // defer that to the backend pod so that the ProvisioningState - // stays consistent with the Status of any active non-terminal - // operation on the cluster. - - apiVisibility, err := convertListeningToVisibility(cluster.API().Listening()) - if err != nil { - return nil, utils.TrackError(err) - } - outboundType, err := convertOutboundTypeCSToRP(cluster.Azure().NodesOutboundConnectivity().OutboundType()) - if err != nil { - return nil, utils.TrackError(err) - } - clusterImageRegistryState, err := convertClusterImageRegistryStateCSToRP(cluster.ImageRegistry().State()) - if err != nil { - return nil, utils.TrackError(err) - } - clusterAutoscaler, err := convertAutoscalarCSToRP(cluster.Autoscaler()) - if err != nil { - return nil, utils.TrackError(err) - } - var subnetResourceID *azcorearm.ResourceID - if len(cluster.Azure().SubnetResourceID()) > 0 { - subnetResourceID, err = azcorearm.ParseResourceID(cluster.Azure().SubnetResourceID()) - if err != nil { - return nil, utils.TrackError(err) - } - } - var networkSecurityGroupID *azcorearm.ResourceID - if len(cluster.Azure().NetworkSecurityGroupResourceID()) > 0 { - networkSecurityGroupID, err = azcorearm.ParseResourceID(cluster.Azure().NetworkSecurityGroupResourceID()) - if err != nil { - return nil, utils.TrackError(err) - } - } - var vnetIntegrationSubnetID *azcorearm.ResourceID - if len(cluster.Azure().VnetIntegrationSubnetResourceID()) > 0 { - vnetIntegrationSubnetID, err = azcorearm.ParseResourceID(cluster.Azure().VnetIntegrationSubnetResourceID()) - if err != nil { - return nil, utils.TrackError(err) - } - } - - hcpcluster := &api.HCPOpenShiftCluster{ - TrackedResource: arm.TrackedResource{ - Resource: arm.Resource{ - ID: resourceID, - Name: resourceID.Name, - Type: resourceID.ResourceType.String(), - }, - Location: azureLocation, - }, - CustomerProperties: api.HCPOpenShiftClusterCustomerProperties{ - Version: api.VersionProfile{ - ID: NewOpenShiftVersionXY(cluster.Version().ID()), - ChannelGroup: cluster.Version().ChannelGroup(), - }, - DNS: api.CustomerDNSProfile{ - BaseDomainPrefix: cluster.DomainPrefix(), - }, - Network: api.NetworkProfile{ - NetworkType: api.NetworkType(cluster.Network().Type()), - PodCIDR: cluster.Network().PodCIDR(), - ServiceCIDR: cluster.Network().ServiceCIDR(), - MachineCIDR: cluster.Network().MachineCIDR(), - HostPrefix: int32(cluster.Network().HostPrefix()), - }, - - API: api.CustomerAPIProfile{ - Visibility: apiVisibility, - AuthorizedCIDRs: convertAuthorizedCidrs(cluster.API()), - }, - Platform: api.CustomerPlatformProfile{ - ManagedResourceGroup: cluster.Azure().ManagedResourceGroupName(), - SubnetID: subnetResourceID, - VnetIntegrationSubnetID: vnetIntegrationSubnetID, - OutboundType: outboundType, - NetworkSecurityGroupID: networkSecurityGroupID, - }, - Autoscaling: clusterAutoscaler, - NodeDrainTimeoutMinutes: convertNodeDrainTimeoutCSToRP(cluster), - ClusterImageRegistry: api.ClusterImageRegistryProfile{ - State: clusterImageRegistryState, - }, - }, - ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{ - DNS: api.ServiceProviderDNSProfile{ - BaseDomain: cluster.DNS().BaseDomain(), - }, - Console: api.ServiceProviderConsoleProfile{ - URL: cluster.Console().URL(), - }, - API: api.ServiceProviderAPIProfile{ - URL: cluster.API().URL(), - }, - Platform: api.ServiceProviderPlatformProfile{ - IssuerURL: cluster.Azure().OidcIssuerUrl(), - }, - }, - } - - // Only set etcd encryption settings if they exist in the cluster service response - if cluster.Azure().EtcdEncryption() != nil { - dataEncryption := cluster.Azure().EtcdEncryption().DataEncryption() - if dataEncryption != nil { - customerManaged, err := convertCustomerManagedEncryptionCSToRP(dataEncryption) - if err != nil { - return nil, err - } - keyManagementMode, err := convertKeyManagementModeTypeCSToRP(dataEncryption.KeyManagementMode()) - if err != nil { - return nil, err - } - - hcpcluster.CustomerProperties.Etcd = api.EtcdProfile{ - DataEncryption: api.EtcdDataEncryptionProfile{ - CustomerManaged: customerManaged, - KeyManagementMode: keyManagementMode, - }, - } - } - } - - // Each managed identity retrieved from Cluster Service needs to be added - // to the HCPOpenShiftCluster in two places: - // - The top-level Identity.UserAssignedIdentities map will need both the - // resourceID (as keys) and principal+client IDs (as values). - // - The operator-specific maps under OperatorsAuthentication mimics the - // Cluster Service maps but just has operator-to-resourceID pairings. - if cluster.Azure().OperatorsAuthentication() != nil { - if mi, ok := cluster.Azure().OperatorsAuthentication().GetManagedIdentities(); ok { - miDPURL := mi.ManagedIdentitiesDataPlaneIdentityUrl() - hcpcluster.ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL = miDPURL - - for operatorName, operatorIdentity := range mi.ControlPlaneOperatorsManagedIdentities() { - if hcpcluster.Identity == nil { - hcpcluster.Identity = &arm.ManagedServiceIdentity{} - } - if hcpcluster.Identity.UserAssignedIdentities == nil { - hcpcluster.Identity.UserAssignedIdentities = make(map[string]*arm.UserAssignedIdentity) - } - if hcpcluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators == nil { - hcpcluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators = make(map[string]*azcorearm.ResourceID) - } - - operatorIdentityResourceID, err := azcorearm.ParseResourceID(operatorIdentity.ResourceID()) - if err != nil { - return nil, utils.TrackError(err) - } - clientID, _ := operatorIdentity.GetClientID() - principalID, _ := operatorIdentity.GetPrincipalID() - hcpcluster.Identity.UserAssignedIdentities[operatorIdentity.ResourceID()] = &arm.UserAssignedIdentity{ClientID: &clientID, - PrincipalID: &principalID} - hcpcluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators[operatorName] = operatorIdentityResourceID - } - for operatorName, operatorIdentity := range mi.DataPlaneOperatorsManagedIdentities() { - if hcpcluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators == nil { - hcpcluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators = make(map[string]*azcorearm.ResourceID) - } - - // Skip adding to hcpcluster.Identity.UserAssignedIdentities map as it is not needed for the dataplane operator MIs. - operatorIdentityResourceID, err := azcorearm.ParseResourceID(operatorIdentity.ResourceID()) - if err != nil { - return nil, utils.TrackError(err) - } - hcpcluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators[operatorName] = operatorIdentityResourceID - } - if len(mi.ServiceManagedIdentity().ResourceID()) > 0 { - if hcpcluster.Identity == nil { - hcpcluster.Identity = &arm.ManagedServiceIdentity{} - } - if hcpcluster.Identity.UserAssignedIdentities == nil { - hcpcluster.Identity.UserAssignedIdentities = make(map[string]*arm.UserAssignedIdentity) - } - - serviceManagedIdentityResourceID, err := azcorearm.ParseResourceID(mi.ServiceManagedIdentity().ResourceID()) - if err != nil { - return nil, utils.TrackError(err) - } - clientID, _ := mi.ServiceManagedIdentity().GetClientID() - principalID, _ := mi.ServiceManagedIdentity().GetPrincipalID() - hcpcluster.Identity.UserAssignedIdentities[mi.ServiceManagedIdentity().ResourceID()] = &arm.UserAssignedIdentity{ClientID: &clientID, - PrincipalID: &principalID} - hcpcluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity = serviceManagedIdentityResourceID - } - } - } - - return hcpcluster, nil -} - // GetClusterServiceUserAssignedIdentities converts a CS Cluster object into an HCPOpenShiftCluster object. func GetClusterServiceUserAssignedIdentities(clusterServiceCluster *arohcpv1alpha1.Cluster) map[string]*arm.UserAssignedIdentity { ret := make(map[string]*arm.UserAssignedIdentity) diff --git a/internal/ocm/convert_test.go b/internal/ocm/convert_test.go index fb2682e5be1..4d54d8f2655 100644 --- a/internal/ocm/convert_test.go +++ b/internal/ocm/convert_test.go @@ -56,382 +56,6 @@ pAqEAuV4DNoxQKKWmhVv+J0ptMWD25Pnpxeq5sXzghfJnslJlQND var dummyAudiences = []string{"audience1", "audience2"} -func TestConvertCStoHCPOpenShiftCluster(t *testing.T) { - resourceID, err := azcorearm.ParseResourceID(api.TestClusterResourceID) - require.NoError(t, err) - - testCases := []struct { - name string - ocmClusterTweaks *arohcpv1alpha1.ClusterBuilder - hcpClusterTweaks *api.HCPOpenShiftCluster - }{ - { - name: "zero", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(), - hcpClusterTweaks: &api.HCPOpenShiftCluster{}, - }, - { - name: "converts nodeDrainGracePeriod to nodeDrainTimeoutMinutes", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - NodeDrainGracePeriod(arohcpv1alpha1.NewValue(). - Unit(csNodeDrainGracePeriodUnit). - Value(42), - ), - hcpClusterTweaks: &api.HCPOpenShiftCluster{ - CustomerProperties: api.HCPOpenShiftClusterCustomerProperties{ - NodeDrainTimeoutMinutes: 42, - }, - }, - }, - { - name: "converts EtcdEncryption for only default PlatformManaged", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - Azure(arohcpv1alpha1.NewAzure(). - EtcdEncryption(arohcpv1alpha1.NewAzureEtcdEncryption(). - DataEncryption(arohcpv1alpha1.NewAzureEtcdDataEncryption(). - KeyManagementMode(csKeyManagementModePlatformManaged), - ), - ), - ), - hcpClusterTweaks: &api.HCPOpenShiftCluster{ - CustomerProperties: api.HCPOpenShiftClusterCustomerProperties{ - Etcd: api.EtcdProfile{ - DataEncryption: api.EtcdDataEncryptionProfile{ - KeyManagementMode: api.EtcdDataEncryptionKeyManagementModeTypePlatformManaged, - CustomerManaged: nil, - }, - }, - }, - }, - }, - { - name: "converts EtcdEncryption for CustomerManaged (without visibility for backwards compat)", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - Azure(arohcpv1alpha1.NewAzure(). - EtcdEncryption(arohcpv1alpha1.NewAzureEtcdEncryption(). - DataEncryption(arohcpv1alpha1.NewAzureEtcdDataEncryption(). - KeyManagementMode(csKeyManagementModeCustomerManaged). - CustomerManaged(arohcpv1alpha1.NewAzureEtcdDataEncryptionCustomerManaged(). - EncryptionType("kms"). - Kms(arohcpv1alpha1.NewAzureKmsEncryption(). - ActiveKey(arohcpv1alpha1.NewAzureKmsKey(). - KeyName("test"). - KeyVaultName("test"). - KeyVersion("test-version"), - ), - ), - ), - ), - ), - ), - hcpClusterTweaks: &api.HCPOpenShiftCluster{ - CustomerProperties: api.HCPOpenShiftClusterCustomerProperties{ - Etcd: api.EtcdProfile{ - DataEncryption: api.EtcdDataEncryptionProfile{ - CustomerManaged: &api.CustomerManagedEncryptionProfile{ - EncryptionType: "KMS", - Kms: &api.KmsEncryptionProfile{ - ActiveKey: api.KmsKey{ - Name: "test", - VaultName: "test", - Version: "test-version", - }, - }, - }, - KeyManagementMode: api.EtcdDataEncryptionKeyManagementModeTypeCustomerManaged, - }, - }, - }, - }, - }, - { - name: "converts EtcdEncryption with KMS Public visibility", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - Azure(arohcpv1alpha1.NewAzure(). - EtcdEncryption(arohcpv1alpha1.NewAzureEtcdEncryption(). - DataEncryption(arohcpv1alpha1.NewAzureEtcdDataEncryption(). - KeyManagementMode(csKeyManagementModeCustomerManaged). - CustomerManaged(arohcpv1alpha1.NewAzureEtcdDataEncryptionCustomerManaged(). - EncryptionType("kms"). - Kms(arohcpv1alpha1.NewAzureKmsEncryption(). - Visibility(arohcpv1alpha1.AzureKmsEncryptionVisibilityPublic). - ActiveKey(arohcpv1alpha1.NewAzureKmsKey(). - KeyName("test-key"). - KeyVaultName("test-vault"). - KeyVersion("test-version"), - ), - ), - ), - ), - ), - ), - hcpClusterTweaks: &api.HCPOpenShiftCluster{ - CustomerProperties: api.HCPOpenShiftClusterCustomerProperties{ - Etcd: api.EtcdProfile{ - DataEncryption: api.EtcdDataEncryptionProfile{ - CustomerManaged: &api.CustomerManagedEncryptionProfile{ - EncryptionType: "KMS", - Kms: &api.KmsEncryptionProfile{ - Visibility: api.KeyVaultVisibilityPublic, - ActiveKey: api.KmsKey{ - Name: "test-key", - VaultName: "test-vault", - Version: "test-version", - }, - }, - }, - KeyManagementMode: api.EtcdDataEncryptionKeyManagementModeTypeCustomerManaged, - }, - }, - }, - }, - }, - { - name: "converts EtcdEncryption with KMS Private visibility", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - Azure(arohcpv1alpha1.NewAzure(). - EtcdEncryption(arohcpv1alpha1.NewAzureEtcdEncryption(). - DataEncryption(arohcpv1alpha1.NewAzureEtcdDataEncryption(). - KeyManagementMode(csKeyManagementModeCustomerManaged). - CustomerManaged(arohcpv1alpha1.NewAzureEtcdDataEncryptionCustomerManaged(). - EncryptionType("kms"). - Kms(arohcpv1alpha1.NewAzureKmsEncryption(). - Visibility(arohcpv1alpha1.AzureKmsEncryptionVisibilityPrivate). - ActiveKey(arohcpv1alpha1.NewAzureKmsKey(). - KeyName("test-key"). - KeyVaultName("test-vault"). - KeyVersion("test-version"), - ), - ), - ), - ), - ), - ), - hcpClusterTweaks: &api.HCPOpenShiftCluster{ - CustomerProperties: api.HCPOpenShiftClusterCustomerProperties{ - Etcd: api.EtcdProfile{ - DataEncryption: api.EtcdDataEncryptionProfile{ - CustomerManaged: &api.CustomerManagedEncryptionProfile{ - EncryptionType: "KMS", - Kms: &api.KmsEncryptionProfile{ - Visibility: api.KeyVaultVisibilityPrivate, - ActiveKey: api.KmsKey{ - Name: "test-key", - VaultName: "test-vault", - Version: "test-version", - }, - }, - }, - KeyManagementMode: api.EtcdDataEncryptionKeyManagementModeTypeCustomerManaged, - }, - }, - }, - }, - }, - { - name: "converts OIDC issuer URL from CS Azure", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - Azure(arohcpv1alpha1.NewAzure(). - OidcIssuerUrl("https://storage.z1.web.core.windows.net/tenant-id/cluster-id")), - hcpClusterTweaks: &api.HCPOpenShiftCluster{ - ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{ - Platform: api.ServiceProviderPlatformProfile{ - IssuerURL: "https://storage.z1.web.core.windows.net/tenant-id/cluster-id", - }, - }, - }, - }, - { - name: "converts CS ClusterImageRegistry to ClusterImageRegistryProfile", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - ImageRegistry(arohcpv1alpha1.NewClusterImageRegistry(). - State(string(csImageRegistryStateDisabled)), - ), - hcpClusterTweaks: &api.HCPOpenShiftCluster{ - CustomerProperties: api.HCPOpenShiftClusterCustomerProperties{ - ClusterImageRegistry: api.ClusterImageRegistryProfile{ - State: api.ClusterImageRegistryStateDisabled, - }, - }, - }, - }, - { - name: "converts stable version from CS to RP (X.Y.Z to X.Y)", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - Version(arohcpv1alpha1.NewVersion(). - ID("openshift-v4.20.17"). - ChannelGroup("stable")), - hcpClusterTweaks: &api.HCPOpenShiftCluster{ - CustomerProperties: api.HCPOpenShiftClusterCustomerProperties{ - Version: api.VersionProfile{ - ID: "4.20", - ChannelGroup: "stable", - }, - }, - }, - }, - { - name: "converts nightly version from CS to RP (strips channel suffix)", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - Version(arohcpv1alpha1.NewVersion(). - ID("openshift-v4.21.0-0.nightly-2025-01-01-nightly"). - ChannelGroup("nightly")), - hcpClusterTweaks: &api.HCPOpenShiftCluster{ - CustomerProperties: api.HCPOpenShiftClusterCustomerProperties{ - Version: api.VersionProfile{ - ID: "4.21", - ChannelGroup: "nightly", - }, - }, - }, - }, - { - name: "converts candidate version from CS to RP", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - Version(arohcpv1alpha1.NewVersion(). - ID("openshift-v4.21.1-candidate"). - ChannelGroup("candidate")), - hcpClusterTweaks: &api.HCPOpenShiftCluster{ - CustomerProperties: api.HCPOpenShiftClusterCustomerProperties{ - Version: api.VersionProfile{ - ID: "4.21", - ChannelGroup: "candidate", - }, - }, - }, - }, - } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - csCluster := ocmCluster(t, ocmClusterDefaults(api.TestLocation), tc.ocmClusterTweaks) - expectHcpCluster := api.ClusterTestCase(t, tc.hcpClusterTweaks) - // SystemData is not set by OCM conversion (it comes from ARM), so clear it from the expected result - expectHcpCluster.SystemData = nil - // Legacy conversion from CS doesn't set ClusterUID (RP-only field) - expectHcpCluster.ServiceProviderProperties.ClusterUID = "" - - actualHcpCluster, err := LegacyCreateInternalClusterFromClusterService(resourceID, api.TestLocation, csCluster) - require.NoError(t, err) - - assert.Equal(t, expectHcpCluster, actualHcpCluster) - }) - } -} - -func TestConvertCStoHCPOpenShiftCluster_ErrorCases(t *testing.T) { - resourceID, err := azcorearm.ParseResourceID(api.TestClusterResourceID) - require.NoError(t, err) - - testCases := []struct { - name string - ocmClusterTweaks *arohcpv1alpha1.ClusterBuilder - expectedError string - }{ - { - name: "error when encryptionType is KMS but activeKey is missing", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - Azure(arohcpv1alpha1.NewAzure(). - EtcdEncryption(arohcpv1alpha1.NewAzureEtcdEncryption(). - DataEncryption(arohcpv1alpha1.NewAzureEtcdDataEncryption(). - KeyManagementMode(csKeyManagementModeCustomerManaged). - CustomerManaged(arohcpv1alpha1.NewAzureEtcdDataEncryptionCustomerManaged(). - EncryptionType("kms"), - // Note: No Kms field set, so no activeKey - ), - ), - ), - ), - expectedError: "cluster Service reported customer-managed encryption type KMS but did not provide KMS configuration", - }, - { - name: "error when KMS has unknown visibility value", - ocmClusterTweaks: arohcpv1alpha1.NewCluster(). - Azure(arohcpv1alpha1.NewAzure(). - EtcdEncryption(arohcpv1alpha1.NewAzureEtcdEncryption(). - DataEncryption(arohcpv1alpha1.NewAzureEtcdDataEncryption(). - KeyManagementMode(csKeyManagementModeCustomerManaged). - CustomerManaged(arohcpv1alpha1.NewAzureEtcdDataEncryptionCustomerManaged(). - EncryptionType("kms"). - Kms(arohcpv1alpha1.NewAzureKmsEncryption(). - Visibility("InvalidVisibility"). // Unknown visibility value - ActiveKey(arohcpv1alpha1.NewAzureKmsKey(). - KeyName("test-key"). - KeyVaultName("test-vault"). - KeyVersion("test-version"), - ), - ), - ), - ), - ), - ), - expectedError: "unknown KMS visibility value from Cluster Service: \"InvalidVisibility\"", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - csCluster := ocmCluster(t, ocmClusterDefaults(api.TestLocation), tc.ocmClusterTweaks) - - _, err := LegacyCreateInternalClusterFromClusterService(resourceID, api.TestLocation, csCluster) - require.Error(t, err) - assert.Contains(t, err.Error(), tc.expectedError) - }) - } -} - -func TestSetClusterServiceOnlyFieldsOnCluster(t *testing.T) { - testCases := []struct { - name string - csCluster *arohcpv1alpha1.ClusterBuilder - initialCluster *api.HCPOpenShiftCluster - expectCluster func(*api.HCPOpenShiftCluster) - }{ - { - name: "populates OIDC issuer URL", - csCluster: arohcpv1alpha1.NewCluster(). - Azure(arohcpv1alpha1.NewAzure(). - OidcIssuerUrl("https://storage.z1.web.core.windows.net/tenant-id/cluster-id")), - initialCluster: &api.HCPOpenShiftCluster{}, - expectCluster: func(c *api.HCPOpenShiftCluster) { - assert.Equal(t, "https://storage.z1.web.core.windows.net/tenant-id/cluster-id", c.ServiceProviderProperties.Platform.IssuerURL) - }, - }, - { - name: "populates DNS, Console, API, and OIDC issuer URL", - csCluster: arohcpv1alpha1.NewCluster(). - DNS(arohcpv1alpha1.NewDNS().BaseDomain("example.com")). - Console(arohcpv1alpha1.NewClusterConsole().URL("https://console.example.com")). - API(arohcpv1alpha1.NewClusterAPI().URL("https://api.example.com")). - Azure(arohcpv1alpha1.NewAzure(). - OidcIssuerUrl("https://oidc.example.com/tenant/cluster")), - initialCluster: &api.HCPOpenShiftCluster{}, - expectCluster: func(c *api.HCPOpenShiftCluster) { - assert.Equal(t, "example.com", c.ServiceProviderProperties.DNS.BaseDomain) - assert.Equal(t, "https://console.example.com", c.ServiceProviderProperties.Console.URL) - assert.Equal(t, "https://api.example.com", c.ServiceProviderProperties.API.URL) - assert.Equal(t, "https://oidc.example.com/tenant/cluster", c.ServiceProviderProperties.Platform.IssuerURL) - }, - }, - { - name: "empty OIDC issuer URL when not set in CS", - csCluster: arohcpv1alpha1.NewCluster(), - initialCluster: &api.HCPOpenShiftCluster{}, - expectCluster: func(c *api.HCPOpenShiftCluster) { - assert.Empty(t, c.ServiceProviderProperties.Platform.IssuerURL) - }, - }, - } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - csCluster, err := tc.csCluster.Build() - require.NoError(t, err) - - SetClusterServiceOnlyFieldsOnCluster(tc.initialCluster, csCluster) - tc.expectCluster(tc.initialCluster) - }) - } -} - func TestWithImmutableAttributes(t *testing.T) { testCases := []struct { name string From 5d2ad09c69445e6d74debbf5988c16ae72384cd0 Mon Sep 17 00:00:00 2001 From: David Eads Date: Sat, 7 Mar 2026 15:19:36 -0500 Subject: [PATCH 03/10] modify manifests for not having any cluster-service read for frontend --- .../noop-update/04-httpGet-cluster/cluster.json | 8 +------- .../patch-identity/03-httpPatch-cluster/patch.json | 10 ++-------- .../patch-identity/04-httpGet-cluster/cluster.json | 12 +----------- .../patch-tags/01-httpCreate-cluster/cluster.json | 13 +------------ .../patch-tags/04-httpGet-cluster/cluster.json | 13 +------------ .../02-loadCosmos-cluster/cosmos-01-cluster.json | 11 +++++++++-- .../02-loadCosmos-cluster/cosmos-01-cluster.json | 11 +++++++++-- 7 files changed, 24 insertions(+), 54 deletions(-) diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/noop-update/04-httpGet-cluster/cluster.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/noop-update/04-httpGet-cluster/cluster.json index 9b486eb0de8..e49c72ebce3 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/noop-update/04-httpGet-cluster/cluster.json +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/noop-update/04-httpGet-cluster/cluster.json @@ -1,13 +1,7 @@ { "id": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/resourceGroupName/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/noop-update", "identity": { - "type": "UserAssigned", - "userAssignedIdentities": { - "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/different-resource-group/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity": { - "clientId": "service-managed-identity_fake-client-id", - "principalId": "service-managed-identity_fake-principal-id" - } - } + "type": "UserAssigned" }, "location": "fake-location", "name": "noop-update", diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/03-httpPatch-cluster/patch.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/03-httpPatch-cluster/patch.json index 4b50e22f283..e91cd68b80b 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/03-httpPatch-cluster/patch.json +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/03-httpPatch-cluster/patch.json @@ -2,14 +2,8 @@ "identity": { "type": "UserAssigned", "userAssignedIdentities": { - "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/cluster-api-azure": { - "clientId": "some-key_fake-client-id", - "principalId": "some-key_fake-principal-id" - }, - "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity": { - "clientId": "service-managed-identity_fake-client-id", - "principalId": "service-managed-identity_fake-principal-id" - } + "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/cluster-api-azure": {}, + "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity": {} } } } diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/04-httpGet-cluster/cluster.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/04-httpGet-cluster/cluster.json index 71cfc0fea8b..09d43b9360d 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/04-httpGet-cluster/cluster.json +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/04-httpGet-cluster/cluster.json @@ -1,17 +1,7 @@ { "id": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/resourceGroupName/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/patch-identity", "identity": { - "type": "UserAssigned", - "userAssignedIdentities": { - "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/cluster-api-azure": { - "clientId": "some-key_fake-client-id", - "principalId": "some-key_fake-principal-id" - }, - "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity": { - "clientId": "service-managed-identity_fake-client-id", - "principalId": "service-managed-identity_fake-principal-id" - } - } + "type": "UserAssigned" }, "location": "fake-location", "name": "patch-identity", diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-tags/01-httpCreate-cluster/cluster.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-tags/01-httpCreate-cluster/cluster.json index 731f78d5510..a87a85170da 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-tags/01-httpCreate-cluster/cluster.json +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-tags/01-httpCreate-cluster/cluster.json @@ -1,12 +1,6 @@ { "identity": { - "type": "UserAssigned", - "userAssignedIdentities": { - "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity": { - "clientId": "the-service-managed-client", - "principalId": "the-service-managed-principal" - } - } + "type": "UserAssigned" }, "name": "patch-tags", "properties": { @@ -43,11 +37,6 @@ "platform": { "managedResourceGroup": "managed-resource-group-name", "networkSecurityGroupId": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.Network/networkSecurityGroups/nsg", - "operatorsAuthentication": { - "userAssignedIdentities": { - "serviceManagedIdentity": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity" - } - }, "outboundType": "LoadBalancer", "subnetId": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.Network/virtualNetworks/vnet/subnets/subnet" }, diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-tags/04-httpGet-cluster/cluster.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-tags/04-httpGet-cluster/cluster.json index 4d7af140780..7fdc622b564 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-tags/04-httpGet-cluster/cluster.json +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-tags/04-httpGet-cluster/cluster.json @@ -1,13 +1,7 @@ { "id": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/resourceGroupName/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/patch-tags", "identity": { - "type": "UserAssigned", - "userAssignedIdentities": { - "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity": { - "clientId": "service-managed-identity_fake-client-id", - "principalId": "service-managed-identity_fake-principal-id" - } - } + "type": "UserAssigned" }, "location": "fake-location", "name": "patch-tags", @@ -48,11 +42,6 @@ "platform": { "managedResourceGroup": "managed-resource-group-name", "networkSecurityGroupId": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.Network/networkSecurityGroups/nsg", - "operatorsAuthentication": { - "userAssignedIdentities": { - "serviceManagedIdentity": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity" - } - }, "outboundType": "LoadBalancer", "subnetId": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.Network/virtualNetworks/vnet/subnets/subnet" }, diff --git a/test-integration/frontend/artifacts/FrontendCRUD/NodePool/cluster-creating/02-loadCosmos-cluster/cosmos-01-cluster.json b/test-integration/frontend/artifacts/FrontendCRUD/NodePool/cluster-creating/02-loadCosmos-cluster/cosmos-01-cluster.json index e9e81a58a89..c07863564d4 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/NodePool/cluster-creating/02-loadCosmos-cluster/cosmos-01-cluster.json +++ b/test-integration/frontend/artifacts/FrontendCRUD/NodePool/cluster-creating/02-loadCosmos-cluster/cosmos-01-cluster.json @@ -2,7 +2,6 @@ "id": "d9dc2060-758a-540b-bb6e-bab0900ea591", "partitionKey": "0465bc32-c654-41b8-8d87-9815d7abe8f6", "properties": { - "customerDesiredState": null, "intermediateResourceDoc": { "identity": { "principalId": "the-principal", @@ -16,7 +15,15 @@ "foo": "bar" } }, - "serviceProviderState": null + "internalState": { + "internalAPI": { + "customerProperties": { + "version": { + "channelGroup": "stable" + } + } + } + } }, "resourceID": "/subscriptions/0465bc32-c654-41b8-8d87-9815d7abe8f6/resourceGroups/some-resource-group/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/cluster-creating", "resourceType": "microsoft.redhatopenshift/hcpopenshiftclusters" diff --git a/test-integration/frontend/artifacts/FrontendCRUD/NodePool/cluster-deleting/02-loadCosmos-cluster/cosmos-01-cluster.json b/test-integration/frontend/artifacts/FrontendCRUD/NodePool/cluster-deleting/02-loadCosmos-cluster/cosmos-01-cluster.json index 9d9dbdf5e3d..db4c434351b 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/NodePool/cluster-deleting/02-loadCosmos-cluster/cosmos-01-cluster.json +++ b/test-integration/frontend/artifacts/FrontendCRUD/NodePool/cluster-deleting/02-loadCosmos-cluster/cosmos-01-cluster.json @@ -2,7 +2,6 @@ "id": "24d2a0af-ce42-50a3-80c4-dbfb99e5b460", "partitionKey": "0465bc32-c654-41b8-8d87-9815d7abe8f6", "properties": { - "customerDesiredState": null, "intermediateResourceDoc": { "identity": { "principalId": "the-principal", @@ -16,7 +15,15 @@ "foo": "bar" } }, - "serviceProviderState": null + "internalState": { + "internalAPI": { + "customerProperties": { + "version": { + "channelGroup": "stable" + } + } + } + } }, "resourceID": "/subscriptions/0465bc32-c654-41b8-8d87-9815d7abe8f6/resourceGroups/some-resource-group/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/cluster-deleting", "resourceType": "microsoft.redhatopenshift/hcpopenshiftclusters" From 2bc2994fe0d6c17930dc8f2a25fa391b7fea1e0b Mon Sep 17 00:00:00 2001 From: David Eads Date: Wed, 25 Mar 2026 15:42:44 -0400 Subject: [PATCH 04/10] run the controller for identity for every mismatch --- .../identity_migration.go | 31 +++++++++++++++++++ .../identity_migration_test.go | 14 +++++++++ internal/database/convert_cluster.go | 12 ++++--- 3 files changed, 52 insertions(+), 5 deletions(-) diff --git a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go index ba319bce319..4e209f0c278 100644 --- a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go +++ b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go @@ -19,6 +19,8 @@ import ( "fmt" "time" + "k8s.io/utils/ptr" + "github.com/Azure/ARO-HCP/backend/pkg/controllers/controllerutils" "github.com/Azure/ARO-HCP/backend/pkg/informers" "github.com/Azure/ARO-HCP/backend/pkg/listers" @@ -92,6 +94,35 @@ func (c *identityMigrationSyncer) NeedsWork(ctx context.Context, existingCluster return true } + for operatorIdentityResourceIDString, userAssignedIdentity := range existingCluster.Identity.UserAssignedIdentities { + if userAssignedIdentity == nil || len(ptr.Deref(userAssignedIdentity.ClientID, "")) == 0 || len(ptr.Deref(userAssignedIdentity.PrincipalID, "")) == 0 { + // try to fill in the information. + return true + } + + controlPlaneExists := false + for _, resourceID := range existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators { + if resourceID != nil && resourceID.String() == operatorIdentityResourceIDString { + controlPlaneExists = true + break + } + } + if !controlPlaneExists { + // need to prune + return true + } + } + + for _, operatorIdentityResourceID := range existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators { + userAssignedIdentity, ok := existingCluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()] + if !ok { + return true + } + if len(ptr.Deref(userAssignedIdentity.ClientID, "")) == 0 || len(ptr.Deref(userAssignedIdentity.PrincipalID, "")) == 0 { + return true + } + } + return false } diff --git a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration_test.go b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration_test.go index 06b5a163483..a016792f818 100644 --- a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration_test.go +++ b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration_test.go @@ -23,6 +23,8 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + arohcpv1alpha1 "github.com/openshift-online/ocm-sdk-go/arohcp/v1alpha1" "github.com/Azure/ARO-HCP/backend/pkg/controllers/controllerutils" @@ -66,6 +68,9 @@ func TestIdentityMigrationSyncer_SyncOnce(t *testing.T) { }, }, } + c.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators = map[string]*azcorearm.ResourceID{ + "test-operator": api.Must(azcorearm.ParseResourceID(testIdentityResourceID)), + } }), existingCluster: newTestClusterForIdentityMigration(func(c *api.HCPOpenShiftCluster) { c.Identity = &arm.ManagedServiceIdentity{ @@ -76,6 +81,9 @@ func TestIdentityMigrationSyncer_SyncOnce(t *testing.T) { }, }, } + c.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators = map[string]*azcorearm.ResourceID{ + "test-operator": api.Must(azcorearm.ParseResourceID(testIdentityResourceID)), + } }), expectCosmosGet: false, expectCSCall: false, @@ -98,6 +106,9 @@ func TestIdentityMigrationSyncer_SyncOnce(t *testing.T) { }, }, } + c.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators = map[string]*azcorearm.ResourceID{ + "test-operator": api.Must(azcorearm.ParseResourceID(testIdentityResourceID)), + } }), expectCosmosGet: true, expectCSCall: false, @@ -118,6 +129,9 @@ func TestIdentityMigrationSyncer_SyncOnce(t *testing.T) { }, }, } + c.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators = map[string]*azcorearm.ResourceID{ + "test-operator": api.Must(azcorearm.ParseResourceID(testIdentityResourceID)), + } }), expectCosmosGet: false, expectCSCall: false, diff --git a/internal/database/convert_cluster.go b/internal/database/convert_cluster.go index 0f1776b0931..8c9e0b04dbd 100644 --- a/internal/database/convert_cluster.go +++ b/internal/database/convert_cluster.go @@ -79,12 +79,14 @@ func toCosmosIdentity(src *arm.ManagedServiceIdentity) *arm.ManagedServiceIdenti return nil } tempIdentity := *src - // we only keep the keys of the UserAssignedIdentities. - // the values are looked up on azure somehow on demand if src.UserAssignedIdentities != nil { - tempIdentity.UserAssignedIdentities = map[string]*arm.UserAssignedIdentity{} - for k := range src.UserAssignedIdentities { - tempIdentity.UserAssignedIdentities[k] = nil + tempIdentity.UserAssignedIdentities = make(map[string]*arm.UserAssignedIdentity, len(src.UserAssignedIdentities)) + for k, v := range src.UserAssignedIdentities { + if v != nil { + tempIdentity.UserAssignedIdentities[k] = v.DeepCopy() + } else { + tempIdentity.UserAssignedIdentities[k] = nil + } } } return &tempIdentity From b22fce0431a47897b7832550e898f21f57f71419 Mon Sep 17 00:00:00 2001 From: David Eads Date: Wed, 25 Mar 2026 16:04:33 -0400 Subject: [PATCH 05/10] Adjust frontend to build valid, empty default for Identity on cluster mutation Rather than clearing entirely, this change has the frontend create a valid default. We also default on reading from storage so the return value is always valid for the RP. --- frontend/pkg/frontend/cluster.go | 85 +++++++++++++++---- .../04-httpGet-cluster/cluster.json | 5 +- .../05-httpGet-cluster/00-key.json | 3 + .../05-httpGet-cluster/cluster.json | 73 ++++++++++++++++ .../00-key.json | 0 .../00-key.json | 0 .../patch.json | 0 .../00-key.json | 0 .../cluster.json | 6 +- .../01-httpCreate-cluster/expected-error.txt | 10 +-- 10 files changed, 158 insertions(+), 24 deletions(-) create mode 100644 test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/00-key.json create mode 100644 test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/cluster.json rename test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/{02-completeOperation-op => 10-completeOperation-op}/00-key.json (100%) rename test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/{03-httpPatch-cluster => 15-httpPatch-cluster}/00-key.json (100%) rename test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/{03-httpPatch-cluster => 15-httpPatch-cluster}/patch.json (100%) rename test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/{04-httpGet-cluster => 20-httpGet-cluster}/00-key.json (100%) rename test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/{04-httpGet-cluster => 20-httpGet-cluster}/cluster.json (85%) diff --git a/frontend/pkg/frontend/cluster.go b/frontend/pkg/frontend/cluster.go index 9e30677667e..765581640c7 100644 --- a/frontend/pkg/frontend/cluster.go +++ b/frontend/pkg/frontend/cluster.go @@ -25,6 +25,7 @@ import ( "time" "k8s.io/apimachinery/pkg/api/operation" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/ptr" azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" @@ -223,6 +224,7 @@ func decodeDesiredClusterCreate(ctx context.Context, azureLocation string, reque if err != nil { return nil, utils.TrackError(err) } + // Backstop for fields unknown to this API version's SetDefaultValues*. // See docs/api-version-defaults-and-storage.md. newInternalCluster.EnsureDefaults() @@ -240,11 +242,6 @@ func decodeDesiredClusterCreate(ctx context.Context, azureLocation string, reque // http header 'X-Ms-Identity-Url'. newInternalCluster.ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL = requestHeader.Get(arm.HeaderNameIdentityURL) - // Clear the user-assigned identities map since that is reconstructed from Cluster Service data. - // TODO we'd like to have the instance complete when we go to validate it. Right now validation fails if we clear this. - // TODO we probably update validation to require this field is cleared. - //newInternalCluster.Identity.UserAssignedIdentities = nil - return newInternalCluster, nil } @@ -305,9 +302,11 @@ func (f *Frontend) createHCPCluster(writer http.ResponseWriter, request *http.Re return utils.TrackError(err) } - // Now that validation is done we clear the user-assigned identities map since that is reconstructed from Cluster Service data - // TODO this is bad, see above TODOs. We want to validate what we store. + // we must validate using user provided .Identity.UserAssignedIdentities because that is the intent expressed by the user to allow + // us to use these identities. The information contained in those key is not trusted to be accurate, so we clear this field and set to + // a valid, but empty set of information newInternalCluster.Identity.UserAssignedIdentities = nil + completeClusterIdentity(newInternalCluster, nil) var tenantID string if subscription.Properties != nil && subscription.Properties.TenantId != nil { @@ -463,10 +462,8 @@ func decodeDesiredClusterReplace(ctx context.Context, oldInternalCluster *api.HC newInternalCluster.Tags = maps.Clone(oldInternalCluster.Tags) } - // Clear the user-assigned identities map since that is reconstructed from Cluster Service data. - // TODO we'd like to have the instance complete when we go to validate it. Right now validation fails if we clear this. - // TODO we probably update validation to require this field is cleared. - //newInternalCluster.Identity.UserAssignedIdentities = nil + // set any missing defaults + newInternalCluster.EnsureDefaults() return newInternalCluster, nil } @@ -528,10 +525,6 @@ func decodeDesiredClusterPatch(ctx context.Context, oldInternalCluster *api.HCPO // validation errors on status fields that the user isn't trying to modify. conversion.CopyReadOnlyClusterValues(newInternalCluster, oldInternalCluster) newInternalCluster.SystemData = ensureSystemData(systemData, oldInternalCluster.SystemData) - // Clear the user-assigned identities map since that is reconstructed from Cluster Service data. - // TODO we'd like to have the instance complete when we go to validate it. Right now validation fails if we clear this. - // TODO we probably update validation to require this field is cleared. - //newInternalCluster.Identity.UserAssignedIdentities = nil // Here the difference between a nil map and an empty map is significant. // If the Tags map is nil, that means it was omitted from the request body, @@ -542,6 +535,9 @@ func decodeDesiredClusterPatch(ctx context.Context, oldInternalCluster *api.HCPO newInternalCluster.Tags = maps.Clone(oldInternalCluster.Tags) } + // set any missing defaults + newInternalCluster.EnsureDefaults() + return newInternalCluster, nil } @@ -588,9 +584,12 @@ func (f *Frontend) updateHCPClusterInCosmos(ctx context.Context, writer http.Res if err := arm.CloudErrorFromFieldErrors(validationErrs); err != nil { return utils.TrackError(err) } - // Now that validation is done we clear the user-assigned identities map since that is reconstructed from Cluster Service data - // TODO this is bad, see above TODOs. We want to validate what we store. + + // we must validate using user provided .Identity.UserAssignedIdentities because that is the intent expressed by the user to allow + // us to use these identities. The information contained in those key is not trusted to be accurate, so we clear this field and set to + // a valid, but empty set of information newInternalCluster.Identity.UserAssignedIdentities = nil + completeClusterIdentity(newInternalCluster, oldInternalCluster.Identity.UserAssignedIdentities) var tenantID string if subscription.Properties != nil && subscription.Properties.TenantId != nil { @@ -843,6 +842,11 @@ func (f *Frontend) getInternalClusterFromStorage(ctx context.Context, resourceID } internalCluster.ID = resourceID + // this allows partial information to be provided by a controller. + completeClusterIdentity(internalCluster, nil) + + // temporarily fill in info if we have something missing. + return internalCluster, nil } @@ -881,5 +885,52 @@ func ensureSystemData(newObj, oldObj *arm.SystemData) *arm.SystemData { } return ret +} + +// completeClusterIdentity fills in any missing cluster.Identity.UserAssignedIdentities and removes any extra cluster.Identity.UserAssignedIdentities keys. +func completeClusterIdentity(cluster *api.HCPOpenShiftCluster, existingUserAssignedIdentity map[string]*arm.UserAssignedIdentity) { + allExpectedKeys := sets.Set[string]{} + + // set default .Identity.UserAssignedIdentities if none exist for required entry. + for _, operatorIdentityResourceID := range cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators { + allExpectedKeys.Insert(operatorIdentityResourceID.String()) + if cluster.Identity == nil { + cluster.Identity = &arm.ManagedServiceIdentity{} + } + if cluster.Identity.UserAssignedIdentities == nil { + cluster.Identity.UserAssignedIdentities = make(map[string]*arm.UserAssignedIdentity) + } + + if val, ok := cluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()]; !ok || val == nil { + if existingValue, hasExisting := existingUserAssignedIdentity[operatorIdentityResourceID.String()]; hasExisting { + cluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()] = existingValue.DeepCopy() + } else { + cluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()] = &arm.UserAssignedIdentity{} + } + } + } + if serviceManagedIdentity := cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity; serviceManagedIdentity != nil { + allExpectedKeys.Insert(serviceManagedIdentity.String()) + + if cluster.Identity == nil { + cluster.Identity = &arm.ManagedServiceIdentity{} + } + if cluster.Identity.UserAssignedIdentities == nil { + cluster.Identity.UserAssignedIdentities = make(map[string]*arm.UserAssignedIdentity) + } + + if val, ok := cluster.Identity.UserAssignedIdentities[serviceManagedIdentity.String()]; !ok || val == nil { + if existingValue, hasExisting := existingUserAssignedIdentity[serviceManagedIdentity.String()]; hasExisting { + cluster.Identity.UserAssignedIdentities[serviceManagedIdentity.String()] = existingValue.DeepCopy() + } else { + cluster.Identity.UserAssignedIdentities[serviceManagedIdentity.String()] = &arm.UserAssignedIdentity{} + } + } + } + for key := range cluster.Identity.UserAssignedIdentities { + if !allExpectedKeys.Has(key) { + delete(cluster.Identity.UserAssignedIdentities, key) + } + } } diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/noop-update/04-httpGet-cluster/cluster.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/noop-update/04-httpGet-cluster/cluster.json index e49c72ebce3..ea13499080f 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/noop-update/04-httpGet-cluster/cluster.json +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/noop-update/04-httpGet-cluster/cluster.json @@ -1,7 +1,10 @@ { "id": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/resourceGroupName/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/noop-update", "identity": { - "type": "UserAssigned" + "type": "UserAssigned", + "userAssignedIdentities": { + "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/different-resource-group/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity": {} + } }, "location": "fake-location", "name": "noop-update", diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/00-key.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/00-key.json new file mode 100644 index 00000000000..2c3f269d2b1 --- /dev/null +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/00-key.json @@ -0,0 +1,3 @@ +{ + "resourceID": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/resourceGroupName/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/patch-identity" +} diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/cluster.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/cluster.json new file mode 100644 index 00000000000..9f9963e6652 --- /dev/null +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/cluster.json @@ -0,0 +1,73 @@ +{ + "id": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/resourceGroupName/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/patch-identity", + "identity": { + "type": "UserAssigned", + "userAssignedIdentities": { + "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/cluster-api-azure": {}, + "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity": {} + } + }, + "location": "fake-location", + "name": "patch-identity", + "properties": { + "api": { + "visibility": "Public" + }, + "autoscaling": { + "maxNodeProvisionTimeSeconds": 900, + "maxPodGracePeriodSeconds": 600, + "podPriorityThreshold": -10 + }, + "clusterImageRegistry": { + "state": "Disabled" + }, + "etcd": { + "dataEncryption": { + "customerManaged": { + "encryptionType": "KMS", + "kms": { + "activeKey": { + "name": "encryptionKeyName", + "vaultName": "keyVaultName", + "version": "2024-12-01-preview" + } + } + }, + "keyManagementMode": "CustomerManaged" + } + }, + "network": { + "hostPrefix": 23, + "machineCidr": "10.0.0.0/16", + "networkType": "OVNKubernetes", + "podCidr": "10.128.0.0/14", + "serviceCidr": "172.30.0.0/16" + }, + "platform": { + "managedResourceGroup": "managed-resource-group-name", + "networkSecurityGroupId": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.Network/networkSecurityGroups/nsg", + "operatorsAuthentication": { + "userAssignedIdentities": { + "controlPlaneOperators": { + "some-key": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/cluster-api-azure" + }, + "serviceManagedIdentity": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity" + } + }, + "outboundType": "LoadBalancer", + "subnetId": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.Network/virtualNetworks/vnet/subnets/subnet" + }, + "provisioningState": "Accepted", + "version": { + "id": "4.20", + "channelGroup": "stable" + } + }, + "type": "Microsoft.RedHatOpenShift/hcpOpenShiftClusters", + "systemData": { + "createdBy": "Unknown-ARO-HCP-frontend", + "createdByType": "Application", + "lastModifiedBy": "Unknown-ARO-HCP-frontend", + "lastModifiedByType": "Application" + } +} diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/02-completeOperation-op/00-key.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/10-completeOperation-op/00-key.json similarity index 100% rename from test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/02-completeOperation-op/00-key.json rename to test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/10-completeOperation-op/00-key.json diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/03-httpPatch-cluster/00-key.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/15-httpPatch-cluster/00-key.json similarity index 100% rename from test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/03-httpPatch-cluster/00-key.json rename to test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/15-httpPatch-cluster/00-key.json diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/03-httpPatch-cluster/patch.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/15-httpPatch-cluster/patch.json similarity index 100% rename from test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/03-httpPatch-cluster/patch.json rename to test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/15-httpPatch-cluster/patch.json diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/04-httpGet-cluster/00-key.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/20-httpGet-cluster/00-key.json similarity index 100% rename from test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/04-httpGet-cluster/00-key.json rename to test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/20-httpGet-cluster/00-key.json diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/04-httpGet-cluster/cluster.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/20-httpGet-cluster/cluster.json similarity index 85% rename from test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/04-httpGet-cluster/cluster.json rename to test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/20-httpGet-cluster/cluster.json index 09d43b9360d..4259b204188 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/04-httpGet-cluster/cluster.json +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/20-httpGet-cluster/cluster.json @@ -1,7 +1,11 @@ { "id": "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/resourceGroupName/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/patch-identity", "identity": { - "type": "UserAssigned" + "type": "UserAssigned", + "userAssignedIdentities": { + "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/cluster-api-azure": {}, + "/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/bar/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-managed-identity": {} + } }, "location": "fake-location", "name": "patch-identity", diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/peer-field-validation/01-httpCreate-cluster/expected-error.txt b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/peer-field-validation/01-httpCreate-cluster/expected-error.txt index 0078ca720fc..43e12c5b6f2 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/peer-field-validation/01-httpCreate-cluster/expected-error.txt +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/peer-field-validation/01-httpCreate-cluster/expected-error.txt @@ -18,6 +18,11 @@ "message": "Invalid value: \"/subscriptions/different-sub/resourceGroups/some-resource-group/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/test-subnet\": must be in the same Azure subscription: \"6b690bec-0c16-4ecb-8f67-781caf40bba7\"", "target": "properties.platform.subnetId" } + { + "code": "InvalidRequestContent", + "message": "Invalid value: \"/subscriptions/different-sub/resourceGroups/some-resource-group/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/test-subnet\": must not be the same resource group name: \"some-resource-group\"", + "target": "properties.platform.subnetId" + } { "code": "InvalidRequestContent", "message": "Invalid value: \"/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/some-resource-group/providers/Microsoft.ManagedIdentity/userAssignedIdentities/shared-identity\": must not be the same resource group name: \"some-resource-group\"", @@ -43,11 +48,6 @@ "message": "Invalid value: \"/subscriptions/different-sub/resourceGroups/some-resource-group/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-identity\": must not be the same resource group name: \"some-resource-group\"", "target": "properties.platform.operatorsAuthentication.userAssignedIdentities.serviceManagedIdentity" } - { - "code": "InvalidRequestContent", - "message": "Invalid value: \"/subscriptions/different-sub/resourceGroups/some-resource-group/providers/Microsoft.ManagedIdentity/userAssignedIdentities/service-identity\": identity is not assigned to this resource", - "target": "properties.platform.operatorsAuthentication.userAssignedIdentities.serviceManagedIdentity" - } { "code": "InvalidRequestContent", "message": "Invalid value: \"/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/some-resource-group/providers/Microsoft.ManagedIdentity/userAssignedIdentities/shared-identity\": identity is used multiple times", From 74879b9f890c0fe17d673aebcb8f3003fec2afb4 Mon Sep 17 00:00:00 2001 From: David Eads Date: Tue, 31 Mar 2026 11:56:32 -0400 Subject: [PATCH 06/10] Only create valid userassignedidentities If the strings aren't longer than zero, they aren't valid and confuse clients. --- internal/ocm/convert.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/ocm/convert.go b/internal/ocm/convert.go index f82b2beac47..84251edca6c 100644 --- a/internal/ocm/convert.go +++ b/internal/ocm/convert.go @@ -341,17 +341,21 @@ func GetClusterServiceUserAssignedIdentities(clusterServiceCluster *arohcpv1alph for _, operatorIdentity := range mi.ControlPlaneOperatorsManagedIdentities() { clientID, _ := operatorIdentity.GetClientID() principalID, _ := operatorIdentity.GetPrincipalID() - ret[operatorIdentity.ResourceID()] = &arm.UserAssignedIdentity{ - ClientID: &clientID, - PrincipalID: &principalID, + if len(clientID) > 0 && len(principalID) > 0 { + ret[operatorIdentity.ResourceID()] = &arm.UserAssignedIdentity{ + ClientID: &clientID, + PrincipalID: &principalID, + } } } if len(mi.ServiceManagedIdentity().ResourceID()) > 0 { clientID, _ := mi.ServiceManagedIdentity().GetClientID() principalID, _ := mi.ServiceManagedIdentity().GetPrincipalID() - ret[mi.ServiceManagedIdentity().ResourceID()] = &arm.UserAssignedIdentity{ - ClientID: &clientID, - PrincipalID: &principalID, + if len(clientID) > 0 && len(principalID) > 0 { + ret[mi.ServiceManagedIdentity().ResourceID()] = &arm.UserAssignedIdentity{ + ClientID: &clientID, + PrincipalID: &principalID, + } } } } From 83ab41e4bec8f623c3249f7fa176ef49b310f8c4 Mon Sep 17 00:00:00 2001 From: David Eads Date: Wed, 22 Apr 2026 09:23:09 -0400 Subject: [PATCH 07/10] formating --- .../patch-identity/05-httpGet-cluster/cluster.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/cluster.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/cluster.json index 9f9963e6652..4259b204188 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/cluster.json +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/patch-identity/05-httpGet-cluster/cluster.json @@ -59,15 +59,15 @@ }, "provisioningState": "Accepted", "version": { - "id": "4.20", - "channelGroup": "stable" + "channelGroup": "stable", + "id": "4.20" } }, - "type": "Microsoft.RedHatOpenShift/hcpOpenShiftClusters", "systemData": { "createdBy": "Unknown-ARO-HCP-frontend", "createdByType": "Application", "lastModifiedBy": "Unknown-ARO-HCP-frontend", "lastModifiedByType": "Application" - } + }, + "type": "Microsoft.RedHatOpenShift/hcpOpenShiftClusters" } From 068ef18cff770be25a6fa551bd5b3d9387744081 Mon Sep 17 00:00:00 2001 From: David Eads Date: Thu, 30 Apr 2026 15:27:57 -0400 Subject: [PATCH 08/10] address a few nil checks that aren't a practical problem, but trip the scanner --- .../identity_migration.go | 22 ++++++++++------ frontend/pkg/frontend/cluster.go | 25 ++++++++++++++----- internal/ocm/convert.go | 2 +- 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go index 4e209f0c278..e523af3eff8 100644 --- a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go +++ b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go @@ -94,26 +94,32 @@ func (c *identityMigrationSyncer) NeedsWork(ctx context.Context, existingCluster return true } + expectedIdentityResourceIDs := map[string]struct{}{} + for _, resourceID := range existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators { + if resourceID != nil { + expectedIdentityResourceIDs[resourceID.String()] = struct{}{} + } + } + if serviceManagedIdentity := existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity; serviceManagedIdentity != nil { + expectedIdentityResourceIDs[serviceManagedIdentity.String()] = struct{}{} + } + for operatorIdentityResourceIDString, userAssignedIdentity := range existingCluster.Identity.UserAssignedIdentities { if userAssignedIdentity == nil || len(ptr.Deref(userAssignedIdentity.ClientID, "")) == 0 || len(ptr.Deref(userAssignedIdentity.PrincipalID, "")) == 0 { // try to fill in the information. return true } - controlPlaneExists := false - for _, resourceID := range existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators { - if resourceID != nil && resourceID.String() == operatorIdentityResourceIDString { - controlPlaneExists = true - break - } - } - if !controlPlaneExists { + if _, ok := expectedIdentityResourceIDs[operatorIdentityResourceIDString]; !ok { // need to prune return true } } for _, operatorIdentityResourceID := range existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators { + if operatorIdentityResourceID == nil { + return true + } userAssignedIdentity, ok := existingCluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()] if !ok { return true diff --git a/frontend/pkg/frontend/cluster.go b/frontend/pkg/frontend/cluster.go index 765581640c7..fcda24ca726 100644 --- a/frontend/pkg/frontend/cluster.go +++ b/frontend/pkg/frontend/cluster.go @@ -305,7 +305,9 @@ func (f *Frontend) createHCPCluster(writer http.ResponseWriter, request *http.Re // we must validate using user provided .Identity.UserAssignedIdentities because that is the intent expressed by the user to allow // us to use these identities. The information contained in those key is not trusted to be accurate, so we clear this field and set to // a valid, but empty set of information - newInternalCluster.Identity.UserAssignedIdentities = nil + if newInternalCluster.Identity != nil { + newInternalCluster.Identity.UserAssignedIdentities = nil + } completeClusterIdentity(newInternalCluster, nil) var tenantID string @@ -588,8 +590,14 @@ func (f *Frontend) updateHCPClusterInCosmos(ctx context.Context, writer http.Res // we must validate using user provided .Identity.UserAssignedIdentities because that is the intent expressed by the user to allow // us to use these identities. The information contained in those key is not trusted to be accurate, so we clear this field and set to // a valid, but empty set of information - newInternalCluster.Identity.UserAssignedIdentities = nil - completeClusterIdentity(newInternalCluster, oldInternalCluster.Identity.UserAssignedIdentities) + if newInternalCluster.Identity != nil { + newInternalCluster.Identity.UserAssignedIdentities = nil + } + var existingUserAssignedIdentities map[string]*arm.UserAssignedIdentity + if oldInternalCluster.Identity != nil { + existingUserAssignedIdentities = oldInternalCluster.Identity.UserAssignedIdentities + } + completeClusterIdentity(newInternalCluster, existingUserAssignedIdentities) var tenantID string if subscription.Properties != nil && subscription.Properties.TenantId != nil { @@ -893,6 +901,9 @@ func completeClusterIdentity(cluster *api.HCPOpenShiftCluster, existingUserAssig // set default .Identity.UserAssignedIdentities if none exist for required entry. for _, operatorIdentityResourceID := range cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators { + if operatorIdentityResourceID == nil { + continue + } allExpectedKeys.Insert(operatorIdentityResourceID.String()) if cluster.Identity == nil { cluster.Identity = &arm.ManagedServiceIdentity{} @@ -928,9 +939,11 @@ func completeClusterIdentity(cluster *api.HCPOpenShiftCluster, existingUserAssig } } - for key := range cluster.Identity.UserAssignedIdentities { - if !allExpectedKeys.Has(key) { - delete(cluster.Identity.UserAssignedIdentities, key) + if cluster.Identity != nil { + for key := range cluster.Identity.UserAssignedIdentities { + if !allExpectedKeys.Has(key) { + delete(cluster.Identity.UserAssignedIdentities, key) + } } } } diff --git a/internal/ocm/convert.go b/internal/ocm/convert.go index 84251edca6c..758bbd85cf7 100644 --- a/internal/ocm/convert.go +++ b/internal/ocm/convert.go @@ -331,7 +331,7 @@ func convertCIDRBlockAllowAccessRPToCS(in api.CustomerAPIProfile) (*arohcpv1alph return arohcpv1alpha1.NewCIDRBlockAccess().Allow(cidrBlockAllowAccess), nil } -// GetClusterServiceUserAssignedIdentities converts a CS Cluster object into an HCPOpenShiftCluster object. +// GetClusterServiceUserAssignedIdentities extracts user-assigned identities from a CS Cluster object, keyed by resource ID. func GetClusterServiceUserAssignedIdentities(clusterServiceCluster *arohcpv1alpha1.Cluster) map[string]*arm.UserAssignedIdentity { ret := make(map[string]*arm.UserAssignedIdentity) From 2dba438492cb03103be5360c42377e53123650ab Mon Sep 17 00:00:00 2001 From: David Eads Date: Mon, 4 May 2026 09:37:06 -0400 Subject: [PATCH 09/10] return empty, but valid identities --- internal/ocm/convert.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/ocm/convert.go b/internal/ocm/convert.go index 758bbd85cf7..b34a42a3aba 100644 --- a/internal/ocm/convert.go +++ b/internal/ocm/convert.go @@ -346,6 +346,8 @@ func GetClusterServiceUserAssignedIdentities(clusterServiceCluster *arohcpv1alph ClientID: &clientID, PrincipalID: &principalID, } + } else { + ret[operatorIdentity.ResourceID()] = &arm.UserAssignedIdentity{} // empty, but valid } } if len(mi.ServiceManagedIdentity().ResourceID()) > 0 { @@ -356,6 +358,8 @@ func GetClusterServiceUserAssignedIdentities(clusterServiceCluster *arohcpv1alph ClientID: &clientID, PrincipalID: &principalID, } + } else { + ret[mi.ServiceManagedIdentity().ResourceID()] = &arm.UserAssignedIdentity{} // empty, but valid } } } From daf3b71091090f6aa51f20192a9c15d7470aa6b6 Mon Sep 17 00:00:00 2001 From: David Eads Date: Mon, 4 May 2026 16:59:15 -0400 Subject: [PATCH 10/10] correct more small comments --- .../identity_migration.go | 20 ++++++++++++++++--- frontend/pkg/frontend/cluster.go | 15 ++++++++------ .../validation/hcpopenshiftcluster_test.go | 4 ---- internal/validation/validate_cluster.go | 4 +++- .../validate_cluster_comprehensive_test.go | 1 - .../01-httpCreate-cluster/expected-error.txt | 5 ----- 6 files changed, 29 insertions(+), 20 deletions(-) diff --git a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go index e523af3eff8..ef05cafbe92 100644 --- a/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go +++ b/backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go @@ -120,11 +120,12 @@ func (c *identityMigrationSyncer) NeedsWork(ctx context.Context, existingCluster if operatorIdentityResourceID == nil { return true } - userAssignedIdentity, ok := existingCluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()] - if !ok { + if needsWorkForIdentityKey(existingCluster.Identity.UserAssignedIdentities, operatorIdentityResourceID.String()) { return true } - if len(ptr.Deref(userAssignedIdentity.ClientID, "")) == 0 || len(ptr.Deref(userAssignedIdentity.PrincipalID, "")) == 0 { + } + if serviceManagedIdentity := existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity; serviceManagedIdentity != nil { + if needsWorkForIdentityKey(existingCluster.Identity.UserAssignedIdentities, serviceManagedIdentity.String()) { return true } } @@ -132,6 +133,19 @@ func (c *identityMigrationSyncer) NeedsWork(ctx context.Context, existingCluster return false } +// needsWorkForIdentityKey returns true when the identity at key is missing or has empty +// client/principal IDs, signalling that the migration controller should fill it in. +func needsWorkForIdentityKey(userAssignedIdentities map[string]*arm.UserAssignedIdentity, key string) bool { + identity, ok := userAssignedIdentities[key] + if !ok || identity == nil { + return true + } + if len(ptr.Deref(identity.ClientID, "")) == 0 || len(ptr.Deref(identity.PrincipalID, "")) == 0 { + return true + } + return false +} + // SyncOnce performs a single reconciliation of cluster identity information. // It checks if the Identity.UserAssignedIdentities field is unset, // and if so, fetches the values from Cluster Service using diff --git a/frontend/pkg/frontend/cluster.go b/frontend/pkg/frontend/cluster.go index fcda24ca726..f45c1248904 100644 --- a/frontend/pkg/frontend/cluster.go +++ b/frontend/pkg/frontend/cluster.go @@ -89,10 +89,9 @@ func (f *Frontend) ArmResourceListClusters(writer http.ResponseWriter, request * pagedResponse := arm.NewPagedResponse() - // Even though the bulk of the list content comes from Cluster Service, - // we start by querying Cosmos DB because its continuation token meets - // the requirements of a skipToken for ARM pagination. We then query - // Cluster Service for the exact set of IDs returned by Cosmos. + // Cluster list is served entirely from Cosmos DB. Cosmos's continuation token also meets + // the requirements of a skipToken for ARM pagination, so it is used directly as the + // nextLink token below. internalClusterIterator, err := f.dbClient.HCPClusters(subscriptionID, resourceGroupName).List(ctx, dbListOptionsFromRequest(request)) if err != nil { @@ -913,7 +912,10 @@ func completeClusterIdentity(cluster *api.HCPOpenShiftCluster, existingUserAssig } if val, ok := cluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()]; !ok || val == nil { - if existingValue, hasExisting := existingUserAssignedIdentity[operatorIdentityResourceID.String()]; hasExisting { + // existing entries can be present-but-nil (older Cosmos records that stored only + // the keys), so a nil existingValue is treated as "no existing details" instead of + // being copied through to a nil map entry that would serialize as `null`. + if existingValue := existingUserAssignedIdentity[operatorIdentityResourceID.String()]; existingValue != nil { cluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()] = existingValue.DeepCopy() } else { cluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()] = &arm.UserAssignedIdentity{} @@ -931,7 +933,8 @@ func completeClusterIdentity(cluster *api.HCPOpenShiftCluster, existingUserAssig } if val, ok := cluster.Identity.UserAssignedIdentities[serviceManagedIdentity.String()]; !ok || val == nil { - if existingValue, hasExisting := existingUserAssignedIdentity[serviceManagedIdentity.String()]; hasExisting { + // Same nil-existing handling as the control plane operators path above. + if existingValue := existingUserAssignedIdentity[serviceManagedIdentity.String()]; existingValue != nil { cluster.Identity.UserAssignedIdentities[serviceManagedIdentity.String()] = existingValue.DeepCopy() } else { cluster.Identity.UserAssignedIdentities[serviceManagedIdentity.String()] = &arm.UserAssignedIdentity{} diff --git a/internal/validation/hcpopenshiftcluster_test.go b/internal/validation/hcpopenshiftcluster_test.go index 3dfd00f41a7..761d4a91bf2 100644 --- a/internal/validation/hcpopenshiftcluster_test.go +++ b/internal/validation/hcpopenshiftcluster_test.go @@ -831,10 +831,6 @@ func TestClusterValidate(t *testing.T) { message: "must be in the same Azure subscription: \"11111111-1111-1111-1111-111111111111\"", fieldPath: "customerProperties.platform.vnetIntegrationSubnetId", }, - { - message: "must not be the same resource group name: \"MRG\"", - fieldPath: "customerProperties.platform.subnetId", - }, }, }, { diff --git a/internal/validation/validate_cluster.go b/internal/validation/validate_cluster.go index 591a598e883..7756cf53794 100644 --- a/internal/validation/validate_cluster.go +++ b/internal/validation/validate_cluster.go @@ -561,7 +561,9 @@ func validateCustomerPlatformProfile(ctx context.Context, op operation.Operation errs = append(errs, validate.RequiredPointer(ctx, op, fldPath.Child("subnetId"), newObj.SubnetID, safe.Field(oldObj, toPlatformSubnetID))...) errs = append(errs, immutableByReflect(ctx, op, fldPath.Child("subnetId"), newObj.SubnetID, safe.Field(oldObj, toPlatformSubnetID))...) errs = append(errs, RestrictedResourceIDWithResourceGroup(ctx, op, fldPath.Child("subnetId"), newObj.SubnetID, safe.Field(oldObj, toPlatformSubnetID), "Microsoft.Network/virtualNetworks/subnets")...) - errs = append(errs, DifferentResourceGroupNameFromResourceID(ctx, op, fldPath.Child("subnetId"), newObj.SubnetID, nil, newObj.ManagedResourceGroup)...) + // Note: DifferentResourceGroupNameFromResourceID for subnetId is performed at the cluster + // peer-field level (it's a cross-field check against ManagedResourceGroup); duplicating it + // here would emit the same error twice for the same input. // VnetIntegrationSubnetID *azcorearm.ResourceID `json:"vnetIntegrationSubnetId,omitempty"` // vnetIntegrationSubnetId was added in v20251223preview, so it's optional for backwards compatibility diff --git a/internal/validation/validate_cluster_comprehensive_test.go b/internal/validation/validate_cluster_comprehensive_test.go index 4255192b699..61a3666b7af 100644 --- a/internal/validation/validate_cluster_comprehensive_test.go +++ b/internal/validation/validate_cluster_comprehensive_test.go @@ -745,7 +745,6 @@ func TestValidateClusterCreate(t *testing.T) { {message: "must not be the same resource group name", fieldPath: "customerProperties.platform.subnetId"}, {message: "must not be the same resource group name", fieldPath: "customerProperties.platform.vnetIntegrationSubnetId"}, {message: "must not be the same resource group name", fieldPath: "customerProperties.platform.managedResourceGroup"}, - {message: "must not be the same resource group name", fieldPath: "customerProperties.platform.subnetId"}, {message: "must not be the same resource group name", fieldPath: "customerProperties.platform.operatorsAuthentication.userAssignedIdentities.controlPlaneOperators[test-operator]"}, }, }, diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/peer-field-validation/01-httpCreate-cluster/expected-error.txt b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/peer-field-validation/01-httpCreate-cluster/expected-error.txt index 43e12c5b6f2..7dfbd0fd5cc 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/peer-field-validation/01-httpCreate-cluster/expected-error.txt +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/peer-field-validation/01-httpCreate-cluster/expected-error.txt @@ -18,11 +18,6 @@ "message": "Invalid value: \"/subscriptions/different-sub/resourceGroups/some-resource-group/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/test-subnet\": must be in the same Azure subscription: \"6b690bec-0c16-4ecb-8f67-781caf40bba7\"", "target": "properties.platform.subnetId" } - { - "code": "InvalidRequestContent", - "message": "Invalid value: \"/subscriptions/different-sub/resourceGroups/some-resource-group/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/test-subnet\": must not be the same resource group name: \"some-resource-group\"", - "target": "properties.platform.subnetId" - } { "code": "InvalidRequestContent", "message": "Invalid value: \"/subscriptions/6b690bec-0c16-4ecb-8f67-781caf40bba7/resourceGroups/some-resource-group/providers/Microsoft.ManagedIdentity/userAssignedIdentities/shared-identity\": must not be the same resource group name: \"some-resource-group\"",