OCPBUGS-63720: orphan machines when managed identity is invalid on clus…

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{}

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")
+				}
+			}
+		})
+	}
+}