diff --git a/hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go b/hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go index 6d633c16e840..8cd8b3567b0b 100644 --- a/hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go +++ b/hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go @@ -8,6 +8,7 @@ import ( "os" "path" "strings" + "time" hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/cloud/azure" @@ -24,11 +25,14 @@ import ( rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/utils/ptr" capiazure "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" capiv1 "sigs.k8s.io/cluster-api/api/core/v1beta1" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/blang/semver" ) @@ -355,6 +359,71 @@ func (a Azure) DeleteCredentials(ctx context.Context, c client.Client, hcluster return nil } +// deletionFailedThreshold is the minimum duration a machine must have had a non-zero +// DeletionTimestamp before it is considered permanently stuck and eligible for orphaning. +// This guards against orphaning machines that hit transient failures (e.g. rate limiting). +const deletionFailedThreshold = 10 * time.Minute + +// DeleteOrphanedMachines removes the finalizer from AzureMachines that are stuck in deletion +// due to credential failures. This is detected by checking each AzureMachine's Status.Conditions +// for a Ready=False condition with Reason=DeletionFailed. Orphaning the machine allows management +// cluster cleanup to proceed without requiring valid cloud credentials. +func (Azure) DeleteOrphanedMachines(ctx context.Context, c client.Client, hc *hyperv1.HostedCluster, controlPlaneNamespace string) error { + // This orphaning behavior is intended for managed-identity cleanup flow. + if hc.Spec.Platform.Azure.AzureAuthenticationConfig.ManagedIdentities == nil { + return nil + } + + azureMachineList := capiazure.AzureMachineList{} + if err := c.List(ctx, &azureMachineList, client.InNamespace(controlPlaneNamespace)); err != nil { + return fmt.Errorf("failed to list AzureMachines in %s: %w", controlPlaneNamespace, err) + } + + logger := ctrl.LoggerFrom(ctx) + var errs []error + + for i := range azureMachineList.Items { + azureMachine := &azureMachineList.Items[i] + if azureMachine.DeletionTimestamp.IsZero() { + continue + } + if time.Since(azureMachine.DeletionTimestamp.Time) < deletionFailedThreshold { + continue + } + if !hasDeletionFailedCondition(azureMachine) { + continue + } + // Remove the AzureMachine finalizer to orphan the machine, leaving Azure + // infrastructure intact rather than attempting cloud API calls with invalid credentials. + if removed := controllerutil.RemoveFinalizer(azureMachine, capiazure.MachineFinalizer); !removed { + continue + } + if err := c.Update(ctx, azureMachine); err != nil { + errs = append(errs, fmt.Errorf("failed to orphan machine %s/%s: %w", + azureMachine.Namespace, azureMachine.Name, err)) + continue + } + logger.Info("orphaning azuremachine stuck in deletion due to credential failure", + "machine", client.ObjectKeyFromObject(azureMachine)) + } + + return utilerrors.NewAggregate(errs) +} + +// hasDeletionFailedCondition returns true if the AzureMachine has a Ready condition with +// Status=False and Reason=DeletionFailed, indicating the cloud provider could not delete +// the underlying VM (e.g., due to invalid or expired credentials). +func hasDeletionFailedCondition(azureMachine *capiazure.AzureMachine) bool { + for _, condition := range azureMachine.Status.Conditions { + if condition.Type == capiv1.ReadyCondition && + condition.Status == corev1.ConditionFalse && + condition.Reason == capiazure.DeletionFailedReason { + return true + } + } + return false +} + func reconcileAzureCluster(azureCluster *capiazure.AzureCluster, hcluster *hyperv1.HostedCluster, apiEndpoint hyperv1.APIEndpoint, azureClusterIdentity *capiazure.AzureClusterIdentity, _ string) error { if azureCluster.Annotations == nil { azureCluster.Annotations = map[string]string{} diff --git a/hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go b/hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go index b71468ea2374..7417e08373cf 100644 --- a/hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go +++ b/hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "testing" + "time" . "github.com/onsi/gomega" @@ -17,6 +18,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" capiazure "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" + capiv1 "sigs.k8s.io/cluster-api/api/core/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -527,3 +529,198 @@ func TestReconcileKMSConfigSecret(t *testing.T) { }) } } + +func TestDeleteOrphanedMachines(t *testing.T) { + controlPlaneNamespace := "test-cp-namespace" + + managedIdentitiesHC := &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{ + Azure: &hyperv1.AzurePlatformSpec{ + AzureAuthenticationConfig: hyperv1.AzureAuthenticationConfiguration{ + ManagedIdentities: &hyperv1.AzureResourceManagedIdentities{}, + }, + }, + }, + }, + } + + // staleDeletionTimestamp simulates a machine that has been pending deletion beyond the threshold. + staleDeletionTimestamp := metav1.NewTime(time.Now().Add(-(deletionFailedThreshold + time.Minute))) + recentDeletionTimestamp := metav1.NewTime(time.Now()) + + deletionFailedConditions := capiv1.Conditions{ + { + Type: capiv1.ReadyCondition, + Status: corev1.ConditionFalse, + Reason: capiazure.DeletionFailedReason, + }, + } + + testCases := []struct { + name string + hostedCluster *hyperv1.HostedCluster + azureMachines []capiazure.AzureMachine + expectedFinalizersRemoved bool + expectedError bool + }{ + { + name: "when ManagedIdentities is nil it should return early without modifying machines", + hostedCluster: &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{ + Azure: &hyperv1.AzurePlatformSpec{ + AzureAuthenticationConfig: hyperv1.AzureAuthenticationConfiguration{}, + }, + }, + }, + }, + azureMachines: []capiazure.AzureMachine{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: controlPlaneNamespace, + Finalizers: []string{capiazure.MachineFinalizer}, + DeletionTimestamp: &staleDeletionTimestamp, + }, + Status: capiazure.AzureMachineStatus{ + Conditions: deletionFailedConditions, + }, + }, + }, + expectedFinalizersRemoved: false, + expectedError: false, + }, + { + name: "when there are no machines it should succeed", + hostedCluster: managedIdentitiesHC, + azureMachines: []capiazure.AzureMachine{}, + expectedFinalizersRemoved: false, + expectedError: false, + }, + { + name: "when a machine has a stale DeletionTimestamp with DeletionFailed condition it should remove finalizers", + hostedCluster: managedIdentitiesHC, + azureMachines: []capiazure.AzureMachine{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: controlPlaneNamespace, + Finalizers: []string{capiazure.MachineFinalizer}, + DeletionTimestamp: &staleDeletionTimestamp, + }, + Status: capiazure.AzureMachineStatus{ + Conditions: deletionFailedConditions, + }, + }, + }, + expectedFinalizersRemoved: true, + expectedError: false, + }, + { + name: "when a machine has a recent DeletionTimestamp with DeletionFailed condition it should not remove finalizers", + hostedCluster: managedIdentitiesHC, + azureMachines: []capiazure.AzureMachine{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: controlPlaneNamespace, + Finalizers: []string{capiazure.MachineFinalizer}, + DeletionTimestamp: &recentDeletionTimestamp, + }, + Status: capiazure.AzureMachineStatus{ + Conditions: deletionFailedConditions, + }, + }, + }, + expectedFinalizersRemoved: false, + expectedError: false, + }, + { + name: "when a machine has a stale DeletionTimestamp without DeletionFailed condition it should not remove finalizers", + hostedCluster: managedIdentitiesHC, + azureMachines: []capiazure.AzureMachine{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: controlPlaneNamespace, + Finalizers: []string{capiazure.MachineFinalizer}, + DeletionTimestamp: &staleDeletionTimestamp, + }, + Status: capiazure.AzureMachineStatus{ + Conditions: capiv1.Conditions{ + { + Type: capiv1.ReadyCondition, + Status: corev1.ConditionTrue, + }, + }, + }, + }, + }, + expectedFinalizersRemoved: false, + expectedError: false, + }, + { + name: "when a machine is not pending deletion it should not remove finalizers regardless of conditions", + hostedCluster: managedIdentitiesHC, + azureMachines: []capiazure.AzureMachine{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: controlPlaneNamespace, + Finalizers: []string{capiazure.MachineFinalizer}, + }, + Status: capiazure.AzureMachineStatus{ + Conditions: deletionFailedConditions, + }, + }, + }, + expectedFinalizersRemoved: false, + expectedError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + ctx := context.Background() + + objects := make([]client.Object, len(tc.azureMachines)) + for i := range tc.azureMachines { + objects[i] = &tc.azureMachines[i] + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(objects...). + WithStatusSubresource(objects...). + Build() + + azure := Azure{} + + err := azure.DeleteOrphanedMachines(ctx, fakeClient, tc.hostedCluster, controlPlaneNamespace) + + if tc.expectedError { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + + azureMachineList := &capiazure.AzureMachineList{} + g.Expect(fakeClient.List(ctx, azureMachineList, client.InNamespace(controlPlaneNamespace))).To(Succeed()) + + if tc.expectedFinalizersRemoved { + for _, machine := range azureMachineList.Items { + if !machine.DeletionTimestamp.IsZero() { + g.Expect(machine.Finalizers).To(BeEmpty(), "finalizers should be removed for machines with DeletionFailed condition") + } + } + } else { + for _, machine := range azureMachineList.Items { + g.Expect(machine.Finalizers).To(Equal(tc.azureMachines[0].Finalizers), "finalizers should not be modified") + } + } + }) + } +}