diff --git a/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller.go b/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller.go index 5181c30c4b6..a1a2c52719b 100644 --- a/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller.go +++ b/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller.go @@ -17,7 +17,6 @@ package clustercreation import ( "context" "fmt" - "strings" "time" arohcpv1alpha1 "github.com/openshift-online/ocm-sdk-go/arohcp/v1alpha1" @@ -126,7 +125,7 @@ func (c *clusterClusterServiceCreateSyncer) SyncOnce(ctx context.Context, key co tenantID := *subscription.Properties.TenantId mrg := cluster.CustomerProperties.Platform.ManagedResourceGroup - csCluster, err := c.findAROHCPClusterByAzureInfo(ctx, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName, tenantID, mrg) + csCluster, err := ocm.FindClusterByAzureInfo(ctx, c.clustersServiceClient, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName, tenantID, mrg) if err != nil { return utils.TrackError(err) } @@ -169,69 +168,6 @@ func (c *clusterClusterServiceCreateSyncer) createPreconditionDesiredVersionReso return false, nil } -// findAROHCPClusterByAzureInfo returns the Cluster Service cluster whose Azure -// metadata matches the given subscription, resource group, ARM resource name, -// tenant ID, and managed resource group name (MRG). -// It returns (nil, nil) when no such cluster exists. -// An error is returned if more than one cluster is returned matching the Azure metadata, as it should be unique. -func (c *clusterClusterServiceCreateSyncer) findAROHCPClusterByAzureInfo(ctx context.Context, subscriptionID, resourceGroupName, resourceName, tenantID, managedResourceGroupName string) (*arohcpv1alpha1.Cluster, error) { - // Subscription ID, resource group, and cluster name are lowercased when building the Cluster Service - // cluster (see withImmutableAttributes in convert.go). - wantSub := strings.ToLower(subscriptionID) - wantRG := strings.ToLower(resourceGroupName) - wantName := strings.ToLower(resourceName) - // Tenant ID and managed resource group are not lowercased in the OCM CS - // builder (see withImmutableAttributes in convert.go), we keep the casing as it is. - wantTenant := tenantID - wantMRG := managedResourceGroupName - search := c.clustersServiceClusterByAzureInfoSearchString(wantSub, wantRG, wantName, wantTenant, wantMRG) - matches, err := c.csClustersMatchingClusterByAzureInfo(ctx, c.clustersServiceClient.ListClusters(search), wantSub, wantRG, wantName, wantTenant, wantMRG) - if err != nil { - return nil, err - } - if len(matches) > 1 { - return nil, fmt.Errorf( - "cluster service returned %d clusters for one Azure resource (expected exactly 1): "+ - "subscription_id=%q resource_group=%q resource_name=%q tenant_id=%q managed_resource_group=%q", - len(matches), wantSub, wantRG, wantName, wantTenant, wantMRG, - ) - } - if len(matches) == 1 { - return matches[0], nil - } - return nil, nil -} - -func (c *clusterClusterServiceCreateSyncer) clustersServiceClusterByAzureInfoSearchString(wantSub, wantRG, wantName, wantTenant, wantMRG string) string { - return fmt.Sprintf( - "azure.subscription_id = '%s' and azure.resource_group_name = '%s' and azure.resource_name = '%s' and "+ - "azure.tenant_id = '%s' and azure.managed_resource_group_name = '%s'", - wantSub, wantRG, wantName, wantTenant, wantMRG, - ) -} - -func (c *clusterClusterServiceCreateSyncer) csClustersMatchingClusterByAzureInfo(ctx context.Context, it ocm.ClusterListIterator, wantSub, wantRG, wantName, wantTenant, wantMRG string) ([]*arohcpv1alpha1.Cluster, error) { - var res []*arohcpv1alpha1.Cluster - for csCluster := range it.Items(ctx) { - az := csCluster.Azure() - if az == nil { - continue - } - if az.SubscriptionID() != wantSub || - az.ResourceGroupName() != wantRG || - az.ResourceName() != wantName || - az.TenantID() != wantTenant || - az.ManagedResourceGroupName() != wantMRG { - continue - } - res = append(res, csCluster) - } - if err := it.GetError(); err != nil { - return nil, err - } - return res, nil -} - func (c *clusterClusterServiceCreateSyncer) createClusterServiceCluster(ctx context.Context, cluster *api.HCPOpenShiftCluster, serviceProviderCluster *api.ServiceProviderCluster, tenantID string) (*arohcpv1alpha1.Cluster, error) { logger := utils.LoggerFromContext(ctx) diff --git a/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go b/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go index bb631db9742..2f694cbc53d 100644 --- a/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go +++ b/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go @@ -201,7 +201,7 @@ func TestClusterClusterServiceCreate_SyncOnce(t *testing.T) { setupMockCS: func(ctrl *gomock.Controller) ocm.ClusterServiceClientSpec { mockCS := ocm.NewMockClusterServiceClientSpec(ctrl) // Build the CS cluster with Azure fields matching the test cluster so it - // passes the csClustersMatchingClusterByAzureInfo filter. + // passes the FindClusterByAzureInfo filter. csCluster, err := arohcpv1alpha1.NewCluster(). HREF(testClusterServiceIDStr). Azure(arohcpv1alpha1.NewAzure(). @@ -276,7 +276,7 @@ func TestClusterClusterServiceCreate_SyncOnce(t *testing.T) { } } -func TestClusterClusterServiceCreate_findAROHCPClusterByAzureInfo(t *testing.T) { +func TestFindClusterByAzureInfo(t *testing.T) { azureTestCluster := func(t *testing.T, sub, rg, name, tenant, mrg string) *arohcpv1alpha1.Cluster { t.Helper() c, err := arohcpv1alpha1.NewCluster(). @@ -300,7 +300,9 @@ func TestClusterClusterServiceCreate_findAROHCPClusterByAzureInfo(t *testing.T) defaultMRG := "arohcp-mycluster-uuid" searchString := func(sub, rg, name, tenant, mrg string) string { - return (&clusterClusterServiceCreateSyncer{}).clustersServiceClusterByAzureInfoSearchString( + return fmt.Sprintf( + "azure.subscription_id = '%s' and azure.resource_group_name = '%s' and azure.resource_name = '%s' and "+ + "azure.tenant_id = '%s' and azure.managed_resource_group_name = '%s'", strings.ToLower(sub), strings.ToLower(rg), strings.ToLower(name), tenant, mrg, ) } @@ -451,6 +453,23 @@ func TestClusterClusterServiceCreate_findAROHCPClusterByAzureInfo(t *testing.T) return mock, nil }, }, + { + name: "found among non-matching clusters", + setupMockCS: func(t *testing.T, ctrl *gomock.Controller, wantSearch string) (ocm.ClusterServiceClientSpec, *arohcpv1alpha1.Cluster) { + wantSub := strings.ToLower(defaultSub) + wantRG := strings.ToLower(defaultRG) + wantName := strings.ToLower(defaultResName) + match := azureTestCluster(t, wantSub, wantRG, wantName, defaultTenant, defaultMRG) + differentSub := azureTestCluster(t, "00000000-0000-0000-0000-000000000099", wantRG, wantName, defaultTenant, defaultMRG) + differentRG := azureTestCluster(t, wantSub, "other-rg", wantName, defaultTenant, defaultMRG) + differentName := azureTestCluster(t, wantSub, wantRG, "other-cluster", defaultTenant, defaultMRG) + mock := ocm.NewMockClusterServiceClientSpec(ctrl) + mock.EXPECT(). + ListClusters(wantSearch). + Return(ocm.NewSimpleClusterListIterator([]*arohcpv1alpha1.Cluster{differentSub, differentRG, match, differentName}, nil)) + return mock, match + }, + }, { name: "multiple matches error", setupMockCS: func(t *testing.T, ctrl *gomock.Controller, wantSearch string) (ocm.ClusterServiceClientSpec, *arohcpv1alpha1.Cluster) { @@ -496,8 +515,7 @@ func TestClusterClusterServiceCreate_findAROHCPClusterByAzureInfo(t *testing.T) ctrl := gomock.NewController(t) mockCS, want := tt.setupMockCS(t, ctrl, wantSearch) - s := &clusterClusterServiceCreateSyncer{clustersServiceClient: mockCS} - got, err := s.findAROHCPClusterByAzureInfo(ctx, sub, rg, resName, tenant, mrg) + got, err := ocm.FindClusterByAzureInfo(ctx, mockCS, sub, rg, resName, tenant, mrg) if tt.wantErr { require.Error(t, err) diff --git a/frontend/pkg/frontend/cluster.go b/frontend/pkg/frontend/cluster.go index 3652e88b648..997d09ac3d3 100644 --- a/frontend/pkg/frontend/cluster.go +++ b/frontend/pkg/frontend/cluster.go @@ -369,24 +369,45 @@ func (f *Frontend) createHCPCluster(writer http.ResponseWriter, request *http.Re tenantID = *subscription.Properties.TenantId } - initialClusterProperties := map[string]string{} - if len(f.clusterServiceProvisionShard) != 0 { - initialClusterProperties[ocm.CSPropertyProvisionShardID] = f.clusterServiceProvisionShard - } - if f.clusterServiceNoopProvision { - initialClusterProperties[ocm.CSPropertyNoopProvision] = ocm.CSPropertyEnabled - } - if f.clusterServiceNoopDeprovision { - initialClusterProperties[ocm.CSPropertyNoopDeprovision] = ocm.CSPropertyEnabled - } - newClusterServiceClusterBuilder, newClusterServiceAutoscalerBuilder, err := ocm.BuildCSCluster(newInternalCluster.ID, tenantID, newInternalCluster, initialClusterProperties, nil, nil) + // Check if CS already has a cluster with matching Azure metadata. This + // handles the retry-after-partial-failure scenario: a prior attempt may + // have created the CS cluster but failed to write the Cosmos documents, + // leaving an orphan in CS that would cause PostCluster to reject with + // "Duplicate ARO-HCP cluster name" (see ARO-27951). + resultingClusterServiceCluster, err := ocm.FindClusterByAzureInfo( + ctx, f.clusterServiceClient, + newInternalCluster.ID.SubscriptionID, + newInternalCluster.ID.ResourceGroupName, + newInternalCluster.Name, + tenantID, + newInternalCluster.CustomerProperties.Platform.ManagedResourceGroup, + ) if err != nil { return utils.TrackError(err) } - logger.Info(fmt.Sprintf("creating resource %s", newInternalCluster.ID)) - resultingClusterServiceCluster, err := f.clusterServiceClient.PostCluster(ctx, newClusterServiceClusterBuilder, newClusterServiceAutoscalerBuilder) - if err != nil { - return utils.TrackError(err) + + if resultingClusterServiceCluster == nil { + logger.Info(fmt.Sprintf("creating resource %s", newInternalCluster.ID)) + initialClusterProperties := map[string]string{} + if len(f.clusterServiceProvisionShard) != 0 { + initialClusterProperties[ocm.CSPropertyProvisionShardID] = f.clusterServiceProvisionShard + } + if f.clusterServiceNoopProvision { + initialClusterProperties[ocm.CSPropertyNoopProvision] = ocm.CSPropertyEnabled + } + if f.clusterServiceNoopDeprovision { + initialClusterProperties[ocm.CSPropertyNoopDeprovision] = ocm.CSPropertyEnabled + } + newClusterServiceClusterBuilder, newClusterServiceAutoscalerBuilder, err := ocm.BuildCSCluster(newInternalCluster.ID, tenantID, newInternalCluster, initialClusterProperties, nil, nil) + if err != nil { + return utils.TrackError(err) + } + resultingClusterServiceCluster, err = f.clusterServiceClient.PostCluster(ctx, newClusterServiceClusterBuilder, newClusterServiceAutoscalerBuilder) + if err != nil { + return utils.TrackError(err) + } + } else { + logger.Info("Reusing existing Cluster Service cluster found by Azure metadata", "csClusterHREF", resultingClusterServiceCluster.HREF()) } csID, err := api.NewInternalID(resultingClusterServiceCluster.HREF()) diff --git a/internal/ocm/client.go b/internal/ocm/client.go index 32fc69ae56a..4e4fc03b7ed 100644 --- a/internal/ocm/client.go +++ b/internal/ocm/client.go @@ -845,3 +845,53 @@ func ConvertOpenShiftVersionAddPrefix(v string) string { } return v } + +// FindClusterByAzureInfo searches Cluster Service for a cluster matching the +// given Azure metadata (subscription, resource group, resource name, tenant, +// managed resource group). Returns (nil, nil) when no match exists. Returns an +// error if more than one matching cluster is found. +func FindClusterByAzureInfo(ctx context.Context, client ClusterServiceClientSpec, subscriptionID, resourceGroupName, resourceName, tenantID, managedResourceGroupName string) (*arohcpv1alpha1.Cluster, error) { + wantSub := strings.ToLower(subscriptionID) + wantRG := strings.ToLower(resourceGroupName) + wantName := strings.ToLower(resourceName) + wantTenant := tenantID + wantMRG := managedResourceGroupName + + search := fmt.Sprintf( + "azure.subscription_id = '%s' and azure.resource_group_name = '%s' and azure.resource_name = '%s' and "+ + "azure.tenant_id = '%s' and azure.managed_resource_group_name = '%s'", + wantSub, wantRG, wantName, wantTenant, wantMRG, + ) + + it := client.ListClusters(search) + var matches []*arohcpv1alpha1.Cluster + for csCluster := range it.Items(ctx) { + az := csCluster.Azure() + if az == nil { + continue + } + if az.SubscriptionID() != wantSub || + az.ResourceGroupName() != wantRG || + az.ResourceName() != wantName || + az.TenantID() != wantTenant || + az.ManagedResourceGroupName() != wantMRG { + continue + } + matches = append(matches, csCluster) + } + if err := it.GetError(); err != nil { + return nil, err + } + + if len(matches) > 1 { + return nil, fmt.Errorf( + "cluster service returned %d clusters for one Azure resource (expected exactly 1): "+ + "subscription_id=%q resource_group=%q resource_name=%q tenant_id=%q managed_resource_group=%q", + len(matches), wantSub, wantRG, wantName, wantTenant, wantMRG, + ) + } + if len(matches) == 1 { + return matches[0], nil + } + return nil, nil +}