diff --git a/backend/pkg/app/backend.go b/backend/pkg/app/backend.go index 2320bf21e88..d33ac418ce9 100644 --- a/backend/pkg/app/backend.go +++ b/backend/pkg/app/backend.go @@ -38,6 +38,7 @@ import ( azureclient "github.com/Azure/ARO-HCP/backend/pkg/azure/client" azureconfig "github.com/Azure/ARO-HCP/backend/pkg/azure/config" "github.com/Azure/ARO-HCP/backend/pkg/controllers/billing" + clusterazureresources "github.com/Azure/ARO-HCP/backend/pkg/controllers/cluster/azureresources" clusterbackups "github.com/Azure/ARO-HCP/backend/pkg/controllers/cluster/backups" clustercreation "github.com/Azure/ARO-HCP/backend/pkg/controllers/cluster/creation" credentialrequestcreation "github.com/Azure/ARO-HCP/backend/pkg/controllers/cluster/credentialrequest/creation" @@ -800,6 +801,15 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context, backendInformers, ) + observeManagedResourceGroupController := clusterazureresources.NewManagedResourceGroupController( + b.options.ResourcesDBClient, + serviceProviderClusterLister, + subscriptionLister, + b.options.FPAClientBuilder, + backendInformers, + unionKubeApplierInformers, + ) + virtualMachineResourceSKUsCachedReaderController := cachedreader.NewFPAVirtualMachineResourceSKUsCachedReaderController( b.options.FPAClientBuilder, b.options.AzureLocation, @@ -1103,6 +1113,7 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context, go createServiceProviderClusterController.Run(ctx, 20) go createServiceProviderNodePoolController.Run(ctx, 20) go cleanOrphanedClusterManagedResourceGroupController.Run(ctx, 20) + go observeManagedResourceGroupController.Run(ctx, 20) go triggerNodePoolUpgradeController.Run(ctx, 20) go nodePoolDeletionClusterServiceDeleteDispatchController.Run(ctx, 20) go nodePoolClusterServiceIDClearerController.Run(ctx, 20) diff --git a/backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go b/backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go new file mode 100644 index 00000000000..a42ed9c26da --- /dev/null +++ b/backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go @@ -0,0 +1,357 @@ +// 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 azureresources + +import ( + "context" + "fmt" + "time" + + azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + + azureclient "github.com/Azure/ARO-HCP/backend/pkg/azure/client" + "github.com/Azure/ARO-HCP/backend/pkg/utils/controllerutils" + "github.com/Azure/ARO-HCP/internal/api/coreapi" + controllerutil "github.com/Azure/ARO-HCP/internal/controllerutils" + "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/corecosmosstorage" + "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/cosmosstorageutils" + "github.com/Azure/ARO-HCP/internal/database/informers/coreinformers" + "github.com/Azure/ARO-HCP/internal/database/listers/corelisters" + unionkubeapplierinformers "github.com/Azure/ARO-HCP/internal/database/unioninformers/kubeapplier" + "github.com/Azure/ARO-HCP/internal/utils" +) + +// ManagedResourceGroupControllerName is the single source of truth for this +// controller's name. It is used for the workqueue name (a Prometheus label), +// context/logger controller name, and log fields. +const ManagedResourceGroupControllerName = "ObserveManagedResourceGroup" + +// managedResourceGroupSyncer OBSERVES the cluster's managed resource group (MRG) +// in Azure and reflects its existence onto +// ServiceProviderCluster.Status.AzureResources.ManagedResourceGroup. +// +// Cluster Service is the actor that creates and deletes the MRG. This controller +// is strictly read-only against Azure: it never calls CreateOrUpdate or +// BeginDelete on a resource group. Its only job is to mirror the observed state +// so that DB-consuming code (for example the cluster child-resources cleanup +// gate) can reason about the MRG without reaching into Azure directly. +type managedResourceGroupSyncer struct { + resourcesDBClient corecosmosstorage.ResourcesDBClient + clusterLister corelisters.ClusterLister + serviceProviderClusterLister corelisters.ServiceProviderClusterLister + subscriptionLister corelisters.SubscriptionLister + azureFPAClientBuilder azureclient.FirstPartyApplicationClientBuilder +} + +var _ controllerutils.ClusterSyncer = (*managedResourceGroupSyncer)(nil) + +// NewManagedResourceGroupController creates a cluster-watching controller that +// keeps ServiceProviderCluster.Status.AzureResources.ManagedResourceGroup in sync +// with the observed existence of the cluster's managed resource group in Azure. +func NewManagedResourceGroupController( + resourcesDBClient corecosmosstorage.ResourcesDBClient, + serviceProviderClusterLister corelisters.ServiceProviderClusterLister, + subscriptionLister corelisters.SubscriptionLister, + azureFPAClientBuilder azureclient.FirstPartyApplicationClientBuilder, + informers coreinformers.BackendInformers, + kubeApplierInformers *unionkubeapplierinformers.UnionKubeApplierInformers, +) controllerutils.Controller { + _, clusterLister := informers.Clusters() + + syncer := &managedResourceGroupSyncer{ + resourcesDBClient: resourcesDBClient, + clusterLister: clusterLister, + serviceProviderClusterLister: serviceProviderClusterLister, + subscriptionLister: subscriptionLister, + azureFPAClientBuilder: azureFPAClientBuilder, + } + + return controllerutils.NewClusterWatchingController( + ManagedResourceGroupControllerName, + resourcesDBClient, + informers, + kubeApplierInformers, + 5*time.Minute, + syncer, + ) +} + +// NeedsWork reports whether SyncOnce has anything to do. +// +// - While the cluster is being deleted there is work only while a reference is +// still set: we re-check Azure until the managed resource group is gone, then +// clear the references (opening the deletion gate) and have nothing more to do. +// - While the cluster is not being deleted there is work only until the managed +// resource group is confirmed as AzureResource; it is immutable, so once +// confirmed there is nothing new to observe. +func (c *managedResourceGroupSyncer) NeedsWork(cluster *coreapi.HCPOpenShiftCluster, serviceProviderCluster *coreapi.ServiceProviderCluster) bool { + managedResourceGroup := serviceProviderCluster.Status.AzureResources.ManagedResourceGroup + if cluster.ServiceProviderProperties.DeletionTimestamp != nil { + return managedResourceGroup.PendingAzureResource != nil || managedResourceGroup.AzureResource != nil + } + return managedResourceGroup.AzureResource == nil +} + +// SyncOnce reads the cluster and ServiceProviderCluster from the informer caches, +// short-circuits via NeedsWork, and then dispatches to the deletion or +// non-deletion (reconcile) path. This controller never creates or deletes the +// resource group. +func (c *managedResourceGroupSyncer) SyncOnce(ctx context.Context, key controllerutils.HCPClusterKey) error { + cluster, err := c.clusterLister.Get(ctx, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName) + if cosmosstorageutils.IsNotFoundError(err) { + return nil + } + if err != nil { + return utils.TrackError(fmt.Errorf("failed to get Cluster: %w", err)) + } + + existingServiceProviderCluster, err := c.serviceProviderClusterLister.Get(ctx, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName) + if cosmosstorageutils.IsNotFoundError(err) { + // CreateServiceProviderCluster will populate it; we'll be re-enqueued via + // the ServiceProviderCluster informer. + return nil + } + if err != nil { + return utils.TrackError(fmt.Errorf("failed to get ServiceProviderCluster: %w", err)) + } + + // Short-circuit on the cheap lister reads before doing anything fallible. + if !c.NeedsWork(cluster, existingServiceProviderCluster) { + return nil + } + + if cluster.ServiceProviderProperties.DeletionTimestamp != nil { + return c.deleteManagedResourceGroup(ctx, cluster, existingServiceProviderCluster) + } + return c.reconcileManagedResourceGroup(ctx, cluster, existingServiceProviderCluster) +} + +// reconcileManagedResourceGroup observes the managed resource group for a cluster +// that is not being deleted and reflects its state onto the ServiceProviderCluster. +// +// It first records the resource group as PendingAzureResource and persists that +// intent BEFORE querying Azure, so that a Get failure - or a resource group that +// does not exist yet - still leaves a durable pending marker (keeping the deletion +// gate closed) rather than an empty reference. It then queries Azure and: +// +// - not found: does nothing, leaving the pending marker in place (Cluster Service +// owns creation; this controller is observe-only). +// - other error: returns the error so the sync retries. +// - exists: if the resource group is owned by another cluster (its ManagedBy is +// set and does not equal this cluster's ID) it returns an error; otherwise it +// clears the pending marker and records the resource group as AzureResource. +func (c *managedResourceGroupSyncer) reconcileManagedResourceGroup(ctx context.Context, cluster *coreapi.HCPOpenShiftCluster, existingServiceProviderCluster *coreapi.ServiceProviderCluster) error { + // A cluster should always have a managed resource group name recorded on its + // CustomerProperties. If it is empty, something is wrong upstream; return a hard + // error so the syncer retries rather than silently skipping. + managedResourceGroupName := cluster.CustomerProperties.Platform.ManagedResourceGroup + if len(managedResourceGroupName) == 0 { + return utils.TrackError(fmt.Errorf("managed resource group name is empty for cluster %q", cluster.ID.String())) + } + + managedResourceGroupID, err := coreapi.ToResourceGroupResourceID(cluster.ID.SubscriptionID, managedResourceGroupName) + if err != nil { + return utils.TrackError(fmt.Errorf("failed to build managed resource group resource ID: %w", err)) + } + + // Set pending before Get: persist the intent so a subsequent Get failure or a + // not-yet-created resource group still leaves a durable pending marker. + if existingServiceProviderCluster.Status.AzureResources.ManagedResourceGroup.PendingAzureResource == nil { + replacement := existingServiceProviderCluster.DeepCopy() + replacement.Status.AzureResources.ManagedResourceGroup.PendingAzureResource = managedResourceGroupID + existingServiceProviderCluster, err = c.persistIfChanged(ctx, cluster, existingServiceProviderCluster, replacement) + if err != nil { + return utils.TrackError(err) + } + } + + rgClient, err := c.resourceGroupsClient(ctx, cluster.ID.SubscriptionID) + if err != nil { + return utils.TrackError(err) + } + + getResponse, getErr := rgClient.Get(ctx, managedResourceGroupID.Name, nil) + switch { + case isNotFound(getErr): + // The managed resource group does not exist yet. Cluster Service owns its + // creation; leave the pending marker in place and wait for a later pass. + // TODO: create the managed resource group. + return nil + case getErr != nil: + return utils.TrackError(fmt.Errorf("failed to get managed resource group %q: %w", managedResourceGroupID.Name, getErr)) + default: + // The managed resource group exists. getResponse is a value type; its + // ManagedBy (*string) is only meaningful here, where the Get succeeded. + if ownedByAnotherCluster(getResponse.ManagedBy, cluster.ID) { + return utils.TrackError(fmt.Errorf("managed resource group %q is owned by another cluster (ManagedBy=%q), not %q", + managedResourceGroupID.Name, managedByValue(getResponse.ManagedBy), cluster.ID.String())) + } + replacement := existingServiceProviderCluster.DeepCopy() + reference := &replacement.Status.AzureResources.ManagedResourceGroup + reference.PendingAzureResource = nil + reference.AzureResource = managedResourceGroupID + _, err = c.persistIfChanged(ctx, cluster, existingServiceProviderCluster, replacement) + return utils.TrackError(err) + } +} + +// deleteManagedResourceGroup observes the managed resource group while the cluster +// is being deleted and reflects its state so the cluster child-resources cleanup +// gate can decide when it is safe to remove the ServiceProviderCluster document. +// +// NeedsWork guarantees a reference is still set when we reach here, so we derive the +// managed resource group ID from that reference, query Azure and: +// +// - not found: clear both references so the deletion gate opens. +// - other error: return the error so the gate stays closed until we can positively +// determine the resource group state. +// - exists but owned by another cluster: a foreign / pre-existing resource group that +// Cluster Service will not delete on our behalf, so clear both references to open the +// deletion gate rather than blocking cluster deletion forever. +// - exists and owned by this cluster: do nothing and leave the reference in place so the +// gate stays closed. Cluster Service owns the resource group's deletion. TODO: begin +// deletion. +func (c *managedResourceGroupSyncer) deleteManagedResourceGroup(ctx context.Context, cluster *coreapi.HCPOpenShiftCluster, existingServiceProviderCluster *coreapi.ServiceProviderCluster) error { + // A reference is guaranteed set here (see NeedsWork). Prefer the confirmed + // AzureResource, falling back to the PendingAzureResource marker. + currentReference := existingServiceProviderCluster.Status.AzureResources.ManagedResourceGroup + managedResourceGroupID := currentReference.AzureResource + if managedResourceGroupID == nil { + managedResourceGroupID = currentReference.PendingAzureResource + } + + rgClient, err := c.resourceGroupsClient(ctx, cluster.ID.SubscriptionID) + if err != nil { + return utils.TrackError(err) + } + + getResponse, getErr := rgClient.Get(ctx, managedResourceGroupID.Name, nil) + switch { + case isNotFound(getErr): + // The managed resource group is gone: clear both references so the deletion + // gate opens and the ServiceProviderCluster document can be removed. + return utils.TrackError(c.clearManagedResourceGroupReferences(ctx, cluster, existingServiceProviderCluster)) + case getErr != nil: + return utils.TrackError(getErr) + case ownedByAnotherCluster(getResponse.ManagedBy, cluster.ID): + // The managed resource group exists but is owned by another cluster: a + // foreign / pre-existing resource group that Cluster Service will not delete + // on our behalf. It is not ours to wait on, so clear both references to open + // the deletion gate rather than blocking cluster deletion forever. + return utils.TrackError(c.clearManagedResourceGroupReferences(ctx, cluster, existingServiceProviderCluster)) + default: + // The managed resource group still exists and is owned by this cluster. + // Cluster Service owns its deletion; leave the reference in place so the + // deletion gate stays closed. + // TODO: begin deletion of the managed resource group. + return nil + } +} + +// clearManagedResourceGroupReferences clears both the pending and confirmed managed +// resource group references on the ServiceProviderCluster and persists the change, +// opening the cluster deletion gate. persistIfChanged makes this a no-op write when the +// references are already clear. +func (c *managedResourceGroupSyncer) clearManagedResourceGroupReferences(ctx context.Context, cluster *coreapi.HCPOpenShiftCluster, existingServiceProviderCluster *coreapi.ServiceProviderCluster) error { + replacement := existingServiceProviderCluster.DeepCopy() + reference := &replacement.Status.AzureResources.ManagedResourceGroup + reference.PendingAzureResource = nil + reference.AzureResource = nil + _, err := c.persistIfChanged(ctx, cluster, existingServiceProviderCluster, replacement) + return err +} + +// persistIfChanged replaces the ServiceProviderCluster when replacement differs +// from existing and returns the object to use for any subsequent write (the freshly +// persisted document on success, or existing when nothing changed). A Cosmos +// precondition conflict is treated as success (another writer updated the document +// first; we'll be re-enqueued and retry). +func (c *managedResourceGroupSyncer) persistIfChanged(ctx context.Context, cluster *coreapi.HCPOpenShiftCluster, existing, replacement *coreapi.ServiceProviderCluster) (*coreapi.ServiceProviderCluster, error) { + if !controllerutil.NeedsUpdate(existing, replacement) { + return existing, nil + } + + logger := utils.LoggerFromContext(ctx) + managedResourceGroup := replacement.Status.AzureResources.ManagedResourceGroup + logger.Info("reflecting managed resource group state onto ServiceProviderCluster", + "azureResource", resourceIDString(managedResourceGroup.AzureResource), + "pendingAzureResource", resourceIDString(managedResourceGroup.PendingAzureResource)) + + updated, err := c.resourcesDBClient.ServiceProviderClusters(cluster.ID.SubscriptionID, cluster.ID.ResourceGroupName, cluster.ID.Name).Replace(ctx, replacement, nil) + if cosmosstorageutils.IsPreconditionFailedError(err) { + return existing, nil + } + if err != nil { + return nil, utils.TrackError(fmt.Errorf("failed to replace ServiceProviderCluster: %w", err)) + } + return updated, nil +} + +// isNotFound reports whether err indicates the managed resource group does not +// exist in Azure. +func isNotFound(err error) bool { + return azureclient.IsResourceGroupNotFoundErr(err) +} + +// ownedByAnotherCluster reports whether a resource group's ManagedBy value refers +// to a cluster other than clusterID. An empty ManagedBy means the resource group is +// unclaimed and is therefore not owned by another cluster. A ManagedBy that is set +// but does not parse as a resource ID is treated as owned by another cluster: it is +// demonstrably not this cluster, so we fail closed and surface an error. +func ownedByAnotherCluster(managedBy *string, clusterID *azcorearm.ResourceID) bool { + if managedBy == nil || len(*managedBy) == 0 { + return false + } + managedByID, err := azcorearm.ParseResourceID(*managedBy) + if err != nil { + return true + } + return !controllerutil.ResourceIDsEqual(managedByID, clusterID) +} + +// managedByValue safely dereferences a resource group's ManagedBy for logging. +func managedByValue(managedBy *string) string { + if managedBy == nil { + return "" + } + return *managedBy +} + +// resourceIDString renders an optional resource ID for structured logging without +// panicking on a nil pointer (azcorearm.ResourceID.String has a pointer receiver). +func resourceIDString(id *azcorearm.ResourceID) string { + if id == nil { + return "" + } + return id.String() +} + +// resourceGroupsClient builds an FPA-credentialed Azure ResourceGroups client for +// the given subscription, resolving the tenant ID from the subscription document. +func (c *managedResourceGroupSyncer) resourceGroupsClient(ctx context.Context, subscriptionID string) (azureclient.ResourceGroupsClient, error) { + subscription, err := c.subscriptionLister.Get(ctx, subscriptionID) + if err != nil { + return nil, fmt.Errorf("failed to get subscription %q: %w", subscriptionID, err) + } + if subscription.Properties == nil || subscription.Properties.TenantId == nil { + return nil, fmt.Errorf("subscription %q has no tenant ID", subscriptionID) + } + + rgClient, err := c.azureFPAClientBuilder.ResourceGroupsClient(*subscription.Properties.TenantId, subscriptionID) + if err != nil { + return nil, fmt.Errorf("failed to build resource groups client: %w", err) + } + return rgClient, nil +} diff --git a/backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller_test.go b/backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller_test.go new file mode 100644 index 00000000000..09275fdf750 --- /dev/null +++ b/backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller_test.go @@ -0,0 +1,570 @@ +// 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 azureresources + +import ( + "context" + "errors" + "io" + "net/http" + "net/url" + "strings" + "testing" + "time" + + "github.com/go-logr/logr/testr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" + + azureclient "github.com/Azure/ARO-HCP/backend/pkg/azure/client" + "github.com/Azure/ARO-HCP/backend/pkg/utils/controllerutils" + "github.com/Azure/ARO-HCP/internal/api/coreapi" + "github.com/Azure/ARO-HCP/internal/api/metadataapi" + "github.com/Azure/ARO-HCP/internal/database/cosmosstoragetesting/corecosmosstoragetesting" + "github.com/Azure/ARO-HCP/internal/database/listertesting/corelistertesting" + "github.com/Azure/ARO-HCP/internal/utils" +) + +const ( + testSubscriptionID = "00000000-0000-0000-0000-000000000000" + testResourceGroupName = "test-rg" + testClusterName = "test-cluster" + testTenantID = "test-tenant-id" + testManagedRGName = "test-managed-rg" +) + +// testManagedResourceGroupID returns the resource ID the controller derives from +// the cluster's CustomerProperties.Platform.ManagedResourceGroup. +func testManagedResourceGroupID(t *testing.T) *azcorearm.ResourceID { + t.Helper() + return metadataapi.Must(coreapi.ToResourceGroupResourceID(testSubscriptionID, testManagedRGName)) +} + +// newTestCluster builds an HCPOpenShiftCluster addressable by the mock +// ResourcesDBClient with the given managed resource group name and deletion state. +func newTestCluster(deleting bool) *coreapi.HCPOpenShiftCluster { + resourceID := metadataapi.Must(azcorearm.ParseResourceID( + "/subscriptions/" + testSubscriptionID + + "/resourceGroups/" + testResourceGroupName + + "/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/" + testClusterName, + )) + + cluster := &coreapi.HCPOpenShiftCluster{ + CosmosMetadata: coreapi.CosmosMetadata{ + ResourceID: resourceID, + PartitionKey: strings.ToLower(resourceID.SubscriptionID), + }, + TrackedResource: coreapi.TrackedResource{ + Resource: coreapi.Resource{ + ID: resourceID, + Name: testClusterName, + Type: resourceID.ResourceType.String(), + }, + }, + } + cluster.CustomerProperties.Platform.ManagedResourceGroup = testManagedRGName + if deleting { + cluster.ServiceProviderProperties.DeletionTimestamp = &metav1.Time{Time: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)} + } + return cluster +} + +// newTestServiceProviderCluster builds a ServiceProviderCluster addressable by the +// mock ResourcesDBClient with the given managed resource group reference. +func newTestServiceProviderCluster(reference coreapi.AzureReference) *coreapi.ServiceProviderCluster { + resourceID := metadataapi.Must(azcorearm.ParseResourceID( + "/subscriptions/" + testSubscriptionID + + "/resourceGroups/" + testResourceGroupName + + "/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/" + testClusterName + + "/" + coreapi.ServiceProviderClusterResourceTypeName + + "/" + coreapi.ServiceProviderClusterResourceName, + )) + + serviceProviderCluster := &coreapi.ServiceProviderCluster{ + CosmosMetadata: coreapi.CosmosMetadata{ + ResourceID: resourceID, + PartitionKey: strings.ToLower(resourceID.SubscriptionID), + }, + } + serviceProviderCluster.Status.AzureResources.ManagedResourceGroup = reference + return serviceProviderCluster +} + +// newTestSubscription builds a Subscription for the SliceSubscriptionLister with +// the given tenant ID. +func newTestSubscription(tenantID *string) *coreapi.Subscription { + subscriptionResourceID := metadataapi.Must(azcorearm.ParseResourceID("/subscriptions/" + testSubscriptionID)) + return &coreapi.Subscription{ + CosmosMetadata: coreapi.CosmosMetadata{ + ResourceID: subscriptionResourceID, + PartitionKey: strings.ToLower(subscriptionResourceID.SubscriptionID), + }, + ResourceID: subscriptionResourceID, + Properties: &coreapi.SubscriptionProperties{ + TenantId: tenantID, + }, + } +} + +// resourceGroupNotFoundError returns an *azcore.ResponseError that +// azureclient.IsResourceGroupNotFoundErr recognizes as a missing resource group. +func resourceGroupNotFoundError() *azcore.ResponseError { + return &azcore.ResponseError{ + ErrorCode: "ResourceGroupNotFound", + StatusCode: http.StatusNotFound, + RawResponse: &http.Response{ + Status: "404 Not Found", + StatusCode: http.StatusNotFound, + Body: io.NopCloser(strings.NewReader(`{"error":{"code":"ResourceGroupNotFound","message":"Resource group not found."}}`)), + Request: &http.Request{ + Method: http.MethodGet, + URL: &url.URL{Scheme: "https", Host: "management.azure.com", Path: "/rg"}, + }, + }, + } +} + +// resourceGroupPresentResponse returns a Get response describing an existing +// managed resource group whose ManagedBy is set to the given owner resource ID. +func resourceGroupPresentResponse(managedBy string) armresources.ResourceGroupsClientGetResponse { + return armresources.ResourceGroupsClientGetResponse{ + ResourceGroup: armresources.ResourceGroup{ + Name: ptr.To(testManagedRGName), + ManagedBy: ptr.To(managedBy), + Properties: &armresources.ResourceGroupProperties{ + ProvisioningState: ptr.To("Succeeded"), + }, + }, + } +} + +// resourceGroupPresentResponseUnclaimed returns a Get response describing an +// existing managed resource group with no ManagedBy set (unclaimed by any cluster). +func resourceGroupPresentResponseUnclaimed() armresources.ResourceGroupsClientGetResponse { + return armresources.ResourceGroupsClientGetResponse{ + ResourceGroup: armresources.ResourceGroup{ + Name: ptr.To(testManagedRGName), + Properties: &armresources.ResourceGroupProperties{ + ProvisioningState: ptr.To("Succeeded"), + }, + }, + } +} + +// TestManagedResourceGroupSyncerSyncOnce exercises the switch-based reconcile and +// deletion paths end to end through the mock Cosmos DB, listers, and Azure client. +func TestManagedResourceGroupSyncerSyncOnce(t *testing.T) { + t.Parallel() + + mrgID := testManagedResourceGroupID(t) + // ownerClusterID is the ManagedBy value that marks the resource group as owned + // by this cluster. + ownerClusterID := newTestCluster(false).ID.String() + // differentOwnerID is a valid but foreign ManagedBy value (owned by a different + // cluster) used to exercise the "exists but owned by another cluster" error path. + differentOwnerID := "/subscriptions/" + testSubscriptionID + + "/resourceGroups/" + testResourceGroupName + + "/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/other-cluster" + // sameOwnerDifferentCasingID is this cluster's own ID as Azure returns it: + // identical to ownerClusterID except for the provider-namespace casing + // ("Microsoft.RedHatOpenshift" vs "Microsoft.RedHatOpenShift"). ARM IDs are + // case-insensitive, so this must be treated as owned by THIS cluster (regression + // guard for the observe-controller hot loop). + sameOwnerDifferentCasingID := strings.Replace(ownerClusterID, "Microsoft.RedHatOpenShift", "Microsoft.RedHatOpenshift", 1) + + testCases := []struct { + name string + deleting bool + initialReference coreapi.AzureReference + getResponse armresources.ResourceGroupsClientGetResponse + getErr error + expectErr bool + expectErrContains string + expectAzure *azcorearm.ResourceID + expectPending *azcorearm.ResourceID + }{ + { + // Set-pending-before-Get: the pending marker persisted before the Get stays + // in place when the resource group does not exist yet, and no actual is set. + name: "not deleting and resource group missing keeps pending and no actual", + deleting: false, + initialReference: coreapi.AzureReference{}, + getResponse: armresources.ResourceGroupsClientGetResponse{}, + getErr: resourceGroupNotFoundError(), + expectAzure: nil, + expectPending: mrgID, + }, + { + name: "not deleting and resource group present and owned sets actual and clears pending", + deleting: false, + initialReference: coreapi.AzureReference{}, + getResponse: resourceGroupPresentResponse(ownerClusterID), + getErr: nil, + expectAzure: mrgID, + expectPending: nil, + }, + { + // An existing resource group with no ManagedBy is treated as ours (Cluster + // Service does not always stamp ManagedBy): actual is set, pending cleared. + name: "not deleting and resource group present and unclaimed sets actual and clears pending", + deleting: false, + initialReference: coreapi.AzureReference{}, + getResponse: resourceGroupPresentResponseUnclaimed(), + getErr: nil, + expectAzure: mrgID, + expectPending: nil, + }, + { + // Owned-by-another: returns an error and does NOT set actual; the pending + // marker recorded before the Get remains. + name: "not deleting and resource group owned by another cluster errors and does not set actual", + deleting: false, + initialReference: coreapi.AzureReference{}, + getResponse: resourceGroupPresentResponse(differentOwnerID), + getErr: nil, + expectErr: true, + expectErrContains: "owned by another cluster", + expectAzure: nil, + expectPending: mrgID, + }, + { + // Owned-by-this-cluster despite provider-namespace casing differences: + // Azure returns ManagedBy with "Microsoft.RedHatOpenshift" while this + // cluster's ID uses "Microsoft.RedHatOpenShift". ARM IDs are + // case-insensitive, so this is NOT owned by another cluster: actual is + // set and pending cleared, with no error. Regression guard against the + // observe-controller hot loop. + name: "not deleting and resource group owned by this cluster with different provider casing sets actual and clears pending", + deleting: false, + initialReference: coreapi.AzureReference{}, + getResponse: resourceGroupPresentResponse(sameOwnerDifferentCasingID), + getErr: nil, + expectAzure: mrgID, + expectPending: nil, + }, + { + name: "deleting and resource group gone clears both", + deleting: true, + initialReference: coreapi.AzureReference{AzureResource: mrgID}, + getResponse: armresources.ResourceGroupsClientGetResponse{}, + getErr: resourceGroupNotFoundError(), + expectAzure: nil, + expectPending: nil, + }, + { + // The resource group exists and is owned by THIS cluster: Cluster Service + // owns its deletion, so the reference is left in place and the deletion gate + // stays closed until the resource group is actually gone. + name: "deleting and resource group present and owned by this cluster leaves reference in place", + deleting: true, + initialReference: coreapi.AzureReference{AzureResource: mrgID}, + getResponse: resourceGroupPresentResponse(ownerClusterID), + getErr: nil, + expectAzure: mrgID, + expectPending: nil, + }, + { + // The resource group exists but is owned by ANOTHER cluster (a foreign / + // pre-existing resource group Cluster Service will not delete on our behalf). + // It is not ours to wait on, so both references are cleared to open the + // deletion gate rather than blocking cluster deletion forever. + name: "deleting and resource group owned by another cluster clears both to unblock deletion", + deleting: true, + initialReference: coreapi.AzureReference{AzureResource: mrgID}, + getResponse: resourceGroupPresentResponse(differentOwnerID), + getErr: nil, + expectAzure: nil, + expectPending: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + + cluster := newTestCluster(tc.deleting) + serviceProviderCluster := newTestServiceProviderCluster(tc.initialReference) + + mockResourcesDB, err := corecosmosstoragetesting.NewMockResourcesDBClientWithResources(ctx, []any{cluster, serviceProviderCluster}) + require.NoError(t, err) + + ctrl := gomock.NewController(t) + mockRGClient := azureclient.NewMockResourceGroupsClient(ctrl) + mockRGClient.EXPECT(). + Get(gomock.Any(), mrgID.Name, nil). + Return(tc.getResponse, tc.getErr). + Times(1) + fpaClientBuilder := azureclient.NewMockFirstPartyApplicationClientBuilder(ctrl) + fpaClientBuilder.EXPECT(). + ResourceGroupsClient(testTenantID, testSubscriptionID). + Return(mockRGClient, nil). + Times(1) + + syncer := &managedResourceGroupSyncer{ + resourcesDBClient: mockResourcesDB, + clusterLister: &corelistertesting.DBClusterLister{ResourcesDBClient: mockResourcesDB}, + serviceProviderClusterLister: &corelistertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDB}, + subscriptionLister: &corelistertesting.SliceSubscriptionLister{Subscriptions: []*coreapi.Subscription{newTestSubscription(ptr.To(testTenantID))}}, + azureFPAClientBuilder: fpaClientBuilder, + } + + key := controllerutils.HCPClusterKey{ + SubscriptionID: testSubscriptionID, + ResourceGroupName: testResourceGroupName, + HCPClusterName: testClusterName, + } + + err = syncer.SyncOnce(ctx, key) + if tc.expectErr { + require.Error(t, err) + if tc.expectErrContains != "" { + assert.Contains(t, err.Error(), tc.expectErrContains) + } + } else { + require.NoError(t, err) + } + + updated, err := mockResourcesDB.ServiceProviderClusters(testSubscriptionID, testResourceGroupName, testClusterName).Get(ctx, coreapi.ServiceProviderClusterResourceName) + require.NoError(t, err) + + gotReference := updated.Status.AzureResources.ManagedResourceGroup + assertResourceIDEqual(t, tc.expectAzure, gotReference.AzureResource, "AzureResource") + assertResourceIDEqual(t, tc.expectPending, gotReference.PendingAzureResource, "PendingAzureResource") + }) + } +} + +// TestManagedResourceGroupSyncerNeedsWork verifies the NeedsWork short-circuit: +// a not-deleting cluster has work only until the managed resource group is reflected +// as AzureResource; a deleting cluster has work only while a reference is still set. +func TestManagedResourceGroupSyncerNeedsWork(t *testing.T) { + t.Parallel() + + mrgID := testManagedResourceGroupID(t) + + testCases := []struct { + name string + deleting bool + reference coreapi.AzureReference + expect bool + }{ + { + name: "not deleting and already reflected has no work", + deleting: false, + reference: coreapi.AzureReference{AzureResource: mrgID}, + expect: false, + }, + { + name: "not deleting and only pending needs work", + deleting: false, + reference: coreapi.AzureReference{PendingAzureResource: mrgID}, + expect: true, + }, + { + name: "not deleting and empty reference needs work", + deleting: false, + reference: coreapi.AzureReference{}, + expect: true, + }, + { + name: "deleting and confirmed reference needs work", + deleting: true, + reference: coreapi.AzureReference{AzureResource: mrgID}, + expect: true, + }, + { + name: "deleting and only pending needs work", + deleting: true, + reference: coreapi.AzureReference{PendingAzureResource: mrgID}, + expect: true, + }, + { + name: "deleting and empty reference has no work", + deleting: true, + reference: coreapi.AzureReference{}, + expect: false, + }, + } + + syncer := &managedResourceGroupSyncer{} + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + cluster := newTestCluster(tc.deleting) + serviceProviderCluster := newTestServiceProviderCluster(tc.reference) + assert.Equal(t, tc.expect, syncer.NeedsWork(cluster, serviceProviderCluster)) + }) + } +} + +// TestManagedResourceGroupSyncerSyncOnceEmptyManagedResourceGroupName verifies that +// a cluster without a managed resource group name is a hard error (a cluster should +// always have one) and that Azure is never queried. +func TestManagedResourceGroupSyncerSyncOnceEmptyManagedResourceGroupName(t *testing.T) { + t.Parallel() + + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + + cluster := newTestCluster(false) + cluster.CustomerProperties.Platform.ManagedResourceGroup = "" + serviceProviderCluster := newTestServiceProviderCluster(coreapi.AzureReference{}) + + mockResourcesDB, err := corecosmosstoragetesting.NewMockResourcesDBClientWithResources(ctx, []any{cluster, serviceProviderCluster}) + require.NoError(t, err) + + ctrl := gomock.NewController(t) + fpaClientBuilder := azureclient.NewMockFirstPartyApplicationClientBuilder(ctrl) + fpaClientBuilder.EXPECT(). + ResourceGroupsClient(gomock.Any(), gomock.Any()). + Times(0) + + syncer := &managedResourceGroupSyncer{ + resourcesDBClient: mockResourcesDB, + clusterLister: &corelistertesting.DBClusterLister{ResourcesDBClient: mockResourcesDB}, + serviceProviderClusterLister: &corelistertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDB}, + subscriptionLister: &corelistertesting.SliceSubscriptionLister{Subscriptions: []*coreapi.Subscription{newTestSubscription(ptr.To(testTenantID))}}, + azureFPAClientBuilder: fpaClientBuilder, + } + + key := controllerutils.HCPClusterKey{ + SubscriptionID: testSubscriptionID, + ResourceGroupName: testResourceGroupName, + HCPClusterName: testClusterName, + } + + err = syncer.SyncOnce(ctx, key) + require.Error(t, err) + assert.Contains(t, err.Error(), "managed resource group name is empty") +} + +// TestManagedResourceGroupSyncerSyncOnceSkipsAzureWhenAlreadyReflected verifies +// the NeedsWork short-circuit end to end: when the cluster is not being deleted and +// the managed resource group is already reflected as AzureResource, the controller +// neither builds the FPA client nor queries Azure, and makes no write. +func TestManagedResourceGroupSyncerSyncOnceSkipsAzureWhenAlreadyReflected(t *testing.T) { + t.Parallel() + + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + + mrgID := testManagedResourceGroupID(t) + + cluster := newTestCluster(false) + serviceProviderCluster := newTestServiceProviderCluster(coreapi.AzureReference{AzureResource: mrgID}) + + mockResourcesDB, err := corecosmosstoragetesting.NewMockResourcesDBClientWithResources(ctx, []any{cluster, serviceProviderCluster}) + require.NoError(t, err) + + ctrl := gomock.NewController(t) + fpaClientBuilder := azureclient.NewMockFirstPartyApplicationClientBuilder(ctrl) + fpaClientBuilder.EXPECT(). + ResourceGroupsClient(gomock.Any(), gomock.Any()). + Times(0) + + syncer := &managedResourceGroupSyncer{ + resourcesDBClient: mockResourcesDB, + clusterLister: &corelistertesting.DBClusterLister{ResourcesDBClient: mockResourcesDB}, + serviceProviderClusterLister: &corelistertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDB}, + subscriptionLister: &corelistertesting.SliceSubscriptionLister{Subscriptions: []*coreapi.Subscription{newTestSubscription(ptr.To(testTenantID))}}, + azureFPAClientBuilder: fpaClientBuilder, + } + + key := controllerutils.HCPClusterKey{ + SubscriptionID: testSubscriptionID, + ResourceGroupName: testResourceGroupName, + HCPClusterName: testClusterName, + } + + require.NoError(t, syncer.SyncOnce(ctx, key)) + + updated, err := mockResourcesDB.ServiceProviderClusters(testSubscriptionID, testResourceGroupName, testClusterName).Get(ctx, coreapi.ServiceProviderClusterResourceName) + require.NoError(t, err) + gotReference := updated.Status.AzureResources.ManagedResourceGroup + assertResourceIDEqual(t, mrgID, gotReference.AzureResource, "AzureResource") + assertResourceIDEqual(t, nil, gotReference.PendingAzureResource, "PendingAzureResource") +} + +// TestManagedResourceGroupSyncerSyncOnceDeletingKeepsGateClosedOnGetError verifies +// that during deletion, when a reference already holds the gate closed and the +// Azure Get fails with a non-404 error, the controller returns the error and leaves +// the reference unchanged (the gate stays closed). +func TestManagedResourceGroupSyncerSyncOnceDeletingKeepsGateClosedOnGetError(t *testing.T) { + t.Parallel() + + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + + mrgID := testManagedResourceGroupID(t) + + cluster := newTestCluster(true) // deleting + serviceProviderCluster := newTestServiceProviderCluster(coreapi.AzureReference{AzureResource: mrgID}) + + mockResourcesDB, err := corecosmosstoragetesting.NewMockResourcesDBClientWithResources(ctx, []any{cluster, serviceProviderCluster}) + require.NoError(t, err) + + ctrl := gomock.NewController(t) + mockRGClient := azureclient.NewMockResourceGroupsClient(ctrl) + mockRGClient.EXPECT(). + Get(gomock.Any(), mrgID.Name, nil). + Return(armresources.ResourceGroupsClientGetResponse{}, errors.New("transient azure error")). + Times(1) + fpaClientBuilder := azureclient.NewMockFirstPartyApplicationClientBuilder(ctrl) + fpaClientBuilder.EXPECT(). + ResourceGroupsClient(testTenantID, testSubscriptionID). + Return(mockRGClient, nil). + Times(1) + + syncer := &managedResourceGroupSyncer{ + resourcesDBClient: mockResourcesDB, + clusterLister: &corelistertesting.DBClusterLister{ResourcesDBClient: mockResourcesDB}, + serviceProviderClusterLister: &corelistertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDB}, + subscriptionLister: &corelistertesting.SliceSubscriptionLister{Subscriptions: []*coreapi.Subscription{newTestSubscription(ptr.To(testTenantID))}}, + azureFPAClientBuilder: fpaClientBuilder, + } + + key := controllerutils.HCPClusterKey{ + SubscriptionID: testSubscriptionID, + ResourceGroupName: testResourceGroupName, + HCPClusterName: testClusterName, + } + + require.Error(t, syncer.SyncOnce(ctx, key)) + + // The reference must be unchanged so the deletion gate stays closed. + updated, err := mockResourcesDB.ServiceProviderClusters(testSubscriptionID, testResourceGroupName, testClusterName).Get(ctx, coreapi.ServiceProviderClusterResourceName) + require.NoError(t, err) + gotReference := updated.Status.AzureResources.ManagedResourceGroup + assertResourceIDEqual(t, mrgID, gotReference.AzureResource, "AzureResource") + assertResourceIDEqual(t, nil, gotReference.PendingAzureResource, "PendingAzureResource") +} + +// assertResourceIDEqual compares two optional resource IDs by their canonical string form. +func assertResourceIDEqual(t *testing.T, expected, actual *azcorearm.ResourceID, field string) { + t.Helper() + if expected == nil { + assert.Nil(t, actual, "%s should be nil", field) + return + } + require.NotNil(t, actual, "%s should not be nil", field) + assert.Equal(t, expected.String(), actual.String(), "%s resource ID mismatch", field) +} diff --git a/backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go b/backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go index 977d8a3fe65..3dfe7e01d67 100644 --- a/backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go +++ b/backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go @@ -264,6 +264,23 @@ func (c *clusterChildResourcesCleanupController) extraDeleteGateShouldDeleteServ return false, utils.TrackError(fmt.Errorf("failed to get ServiceProviderCluster: %w", err)) } + // Do not delete the ServiceProviderCluster while the cluster's managed + // resource group is still reflected as present (either confirmed or pending). + // The ObserveManagedResourceGroup controller clears both references once the + // MRG is gone in Azure; until then we keep the ServiceProviderCluster document + // alive so that reflected state remains available. + managedResourceGroup := spc.Status.AzureResources.ManagedResourceGroup + if managedResourceGroup.AzureResource != nil || managedResourceGroup.PendingAzureResource != nil { + mrgID := managedResourceGroup.AzureResource + if mrgID == nil { + mrgID = managedResourceGroup.PendingAzureResource + } + logger.Info("waiting for the managed resource group to be deleted before removing the ServiceProviderCluster document", + "serviceProviderClusterResourceID", spc.ResourceID.String(), + "managedResourceGroupID", mrgID.String()) + return false, nil + } + // Check if there are any Maestro readonly bundles remaining. if len(spc.Status.MaestroReadonlyBundles) > 0 { logger.Info("waiting for cluster-scoped Maestro readonly bundles to be deleted before removing Cosmos entry", diff --git a/backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_mrg_gate_test.go b/backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_mrg_gate_test.go new file mode 100644 index 00000000000..fdd8dfef9f3 --- /dev/null +++ b/backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_mrg_gate_test.go @@ -0,0 +1,101 @@ +// 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 deletion + +import ( + "context" + "strings" + "testing" + + "github.com/go-logr/logr/testr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + + "github.com/Azure/ARO-HCP/internal/api/coreapi" + "github.com/Azure/ARO-HCP/internal/api/metadataapi" + "github.com/Azure/ARO-HCP/internal/database/cosmosstoragetesting/corecosmosstoragetesting" + "github.com/Azure/ARO-HCP/internal/utils" +) + +// TestExtraDeleteGateShouldDeleteServiceProviderClusterManagedResourceGroup verifies +// that the ServiceProviderCluster delete gate blocks deletion while the managed +// resource group is still reflected (AzureResource or PendingAzureResource set) and +// allows it once both references are cleared. +func TestExtraDeleteGateShouldDeleteServiceProviderClusterManagedResourceGroup(t *testing.T) { + const ( + subscriptionID = "00000000-0000-0000-0000-000000000000" + resourceGroupName = "test-rg" + clusterName = "test-cluster" + managedRGName = "test-managed-rg" + ) + + managedResourceGroupID := metadataapi.Must(coreapi.ToResourceGroupResourceID(subscriptionID, managedRGName)) + serviceProviderClusterResourceID := metadataapi.Must(azcorearm.ParseResourceID( + "/subscriptions/" + subscriptionID + + "/resourceGroups/" + resourceGroupName + + "/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/" + clusterName + + "/" + coreapi.ServiceProviderClusterResourceTypeName + + "/" + coreapi.ServiceProviderClusterResourceName, + )) + + testCases := []struct { + name string + reference coreapi.AzureReference + expectShouldDelete bool + }{ + { + name: "azure resource set blocks deletion", + reference: coreapi.AzureReference{AzureResource: managedResourceGroupID}, + expectShouldDelete: false, + }, + { + name: "pending azure resource set blocks deletion", + reference: coreapi.AzureReference{PendingAzureResource: managedResourceGroupID}, + expectShouldDelete: false, + }, + { + name: "both references nil allows deletion", + reference: coreapi.AzureReference{}, + expectShouldDelete: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + + serviceProviderCluster := &coreapi.ServiceProviderCluster{ + CosmosMetadata: coreapi.CosmosMetadata{ + ResourceID: serviceProviderClusterResourceID, + PartitionKey: strings.ToLower(serviceProviderClusterResourceID.SubscriptionID), + }, + } + serviceProviderCluster.Status.AzureResources.ManagedResourceGroup = tc.reference + + mockResourcesDB, err := corecosmosstoragetesting.NewMockResourcesDBClientWithResources(ctx, []any{serviceProviderCluster}) + require.NoError(t, err) + + controller := &clusterChildResourcesCleanupController{ + resourcesDBClient: mockResourcesDB, + } + + shouldDelete, err := controller.extraDeleteGateShouldDeleteServiceProviderCluster(ctx, serviceProviderClusterResourceID) + require.NoError(t, err) + assert.Equal(t, tc.expectShouldDelete, shouldDelete) + }) + } +} diff --git a/docs/cosmos-data-flow.md b/docs/cosmos-data-flow.md index 8e8002088b2..a9b3437a97e 100644 --- a/docs/cosmos-data-flow.md +++ b/docs/cosmos-data-flow.md @@ -559,10 +559,10 @@ No Cosmos writes. Dispatches updates to Cluster Service via PATCH. | | Object | Fields | |---|--------|--------| | Read | `HCPOpenShiftCluster` | | -| Read | `ServiceProviderCluster` | | +| Read | `ServiceProviderCluster` | | | Read | Child NodePools | | | Read | Child ExternalAuths | | -| **Write** | Child Cosmos docs | | +| **Write** | Child Cosmos docs | | #### ClusterDeletionController @@ -1152,6 +1152,25 @@ No writes to the Cosmos Resources container. | Read | Azure (UserAssignedIdentitiesClient) | | | **Write** | **`ServiceProviderCluster`** | | +#### ObserveManagedResourceGroup + +**File:** [managed_resource_group_controller.go](../backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go) +**Trigger:** Cluster informer, 5-minute resync +**Behavior:** Observe-only — never creates or deletes the managed resource group. A `NeedsWork` gate skips the sync when there is nothing to do: while the cluster is not being deleted, only until the managed resource group is confirmed as `AzureResource` (it is immutable, so a confirmed reference never needs re-checking); while the cluster is being deleted, only while a reference is still set. +- Not deleting (reconcile): records the managed resource group as `PendingAzureResource` and persists that intent **before** querying Azure ("set pending before Get"), so a Get failure — or a resource group that does not exist yet — still leaves a durable pending marker (keeping the deletion gate closed) rather than an empty reference. It then queries Azure and switches on the result: + - **not found** → does nothing, leaving the pending marker in place (Cluster Service owns creation; this controller is observe-only). + - **other error** → returns the error so the sync retries. + - **exists** → if the resource group is owned by another cluster (its `ManagedBy` is set and does not equal this cluster's ID via the `ResourceIDsEqual` helper) it returns an error and does **not** set `AzureResource`; otherwise (owned by this cluster, or `ManagedBy` absent) it clears `PendingAzureResource` and records the resource group as `AzureResource`. +- Deleting: derives the resource group ID from the reference still on the document (guaranteed set by `NeedsWork`), queries Azure and switches on the result: once the resource group is gone it clears both references so the deletion gate opens; on any other error it returns the error so the gate stays closed until the state is known; while the resource group still exists it does nothing (it does not set `AzureResource`, write a pending marker, or perform the ownership check). + +| | Object | Fields | +|---|--------|--------| +| Read | `HCPOpenShiftCluster` | | +| Read | `Subscription` | | +| Read | `ServiceProviderCluster` | | +| Read | Azure (ResourceGroupsClient) | | +| **Write** | **`ServiceProviderCluster`** | | + --- ## 3. Execution Order Digraphs @@ -1513,6 +1532,14 @@ Single writer. Read by [ClusterIdentitySync](#clusteridentitysync) to populate ` Single writer. Mirrors the customer's data plane operator managed identities (`CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators`) into `Identities` keyed by lowercased Azure ResourceID, each carrying the Azure-resolved `ClientID`/`PrincipalID` or a `RetrievalError`. +### `ServiceProviderCluster.Status.AzureResources.ManagedResourceGroup` + +| Actor | When | +|-------|------| +| [ObserveManagedResourceGroup](#observemanagedresourcegroup) | Observe-only: while the cluster is not being deleted, records `PendingAzureResource` before querying Azure, then sets `AzureResource` (clearing pending) when the resource group exists and is not owned by another cluster, leaves the pending marker when it is missing, and returns an error when it is owned by another cluster (`ManagedBy` set to a different cluster ID); while the cluster is being deleted, clears both references once the resource group is gone and otherwise does nothing | + +Single writer. Read by [ClusterChildResourcesCleanupController](#clusterchildresourcescleanupcontroller) to gate deletion of the `ServiceProviderCluster` document until the managed resource group is gone. + ### `ServiceProviderCluster.Status.Validations` | Actor | When | diff --git a/internal/api/coreapi/types_serviceprovider_cluster.go b/internal/api/coreapi/types_serviceprovider_cluster.go index 15c4104511e..9fc08284461 100644 --- a/internal/api/coreapi/types_serviceprovider_cluster.go +++ b/internal/api/coreapi/types_serviceprovider_cluster.go @@ -401,6 +401,7 @@ type AzureResources struct { // DenyAssignments tracks the deny assignments applied to the cluster's resources. DenyAssignments AzureMultiReference `json:"denyAssignments,omitempty"` // ManagedResourceGroup tracks the managed resource group for the cluster. + // Written by: ObserveManagedResourceGroup ManagedResourceGroup AzureReference `json:"managedResourceGroup,omitempty"` } diff --git a/internal/controllerutils/needs_update.go b/internal/controllerutils/needs_update.go index e12fe59689e..95c86ecfe27 100644 --- a/internal/controllerutils/needs_update.go +++ b/internal/controllerutils/needs_update.go @@ -17,6 +17,7 @@ package controllerutils import ( "bytes" "encoding/json" + "strings" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/conversion" @@ -116,11 +117,17 @@ var needsUpdateEqualities = func() conversion.Equalities { // canonical string form. Both may be nil; non-nil values are compared by // String(), so independently-parsed instances with different parent pointer // chains still compare equal when they represent the same ARM ID. +// +// The comparison is case-insensitive: ARM resource IDs are case-insensitive for +// their provider namespaces and resource types (for example Azure may return +// "Microsoft.RedHatOpenshift" where our internal types use +// "Microsoft.RedHatOpenShift"), so two IDs that differ only by casing represent +// the same resource and must compare equal. func ResourceIDsEqual(a, b *azcorearm.ResourceID) bool { if a == nil || b == nil { return a == b } - return a.String() == b.String() + return strings.EqualFold(a.String(), b.String()) } // NeedsUpdate reports whether `desired` differs from `existing` in any way that should cause us to diff --git a/internal/controllerutils/needs_update_test.go b/internal/controllerutils/needs_update_test.go index f087910d94d..fb246b0fd35 100644 --- a/internal/controllerutils/needs_update_test.go +++ b/internal/controllerutils/needs_update_test.go @@ -116,6 +116,69 @@ func TestNeedsUpdate_ResourceID(t *testing.T) { } } +func TestResourceIDsEqual(t *testing.T) { + const ( + // idOpenShift uses the provider-namespace casing our internal (coreapi) + // types produce. + idOpenShift = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/c" + // idOpenshift is the same ARM ID as Azure returns it, differing only by + // provider-namespace casing ("Openshift" vs "OpenShift"). + idOpenshift = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.RedHatOpenshift/hcpOpenShiftClusters/c" + // idDifferentName is a genuinely different resource (different name). + idDifferentName = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/d" + ) + + tests := []struct { + name string + a *azcorearm.ResourceID + b *azcorearm.ResourceID + want bool + }{ + { + name: "identical IDs are equal", + a: mustParseRID(t, idOpenShift), + b: mustParseRID(t, idOpenShift), + want: true, + }, + { + name: "IDs differing only by provider-namespace casing are equal", + a: mustParseRID(t, idOpenShift), + b: mustParseRID(t, idOpenshift), + want: true, + }, + { + name: "genuinely different resource names are not equal", + a: mustParseRID(t, idOpenShift), + b: mustParseRID(t, idDifferentName), + want: false, + }, + { + name: "both nil are equal", + a: nil, + b: nil, + want: true, + }, + { + name: "non-nil vs nil are not equal", + a: mustParseRID(t, idOpenShift), + b: nil, + want: false, + }, + { + name: "nil vs non-nil are not equal", + a: nil, + b: mustParseRID(t, idOpenShift), + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, ResourceIDsEqual(tt.a, tt.b)) + }) + } +} + func TestNeedsUpdate_InternalID(t *testing.T) { idA, err := metadataapi.NewInternalID("/api/aro_hcp/v1alpha1/provision_shards/abc") require.NoError(t, err)