OCPBUGS-86690: fix Azure cluster deletion hanging when resource groups are already deleted

cmd/cluster/azure/destroy.go

@@ -2,7 +2,9 @@ package azure
 
 import (
 	"context"
+	stderrors "errors"
 	"fmt"
+	"net/http"
 	"os"
 	"os/signal"
 	"syscall"
@@ -22,6 +24,7 @@ import (
 
 	"k8s.io/apimachinery/pkg/util/errors"
 
+	"github.com/go-logr/logr"
 	"github.com/spf13/cobra"
 )
 
@@ -139,8 +142,8 @@ func DestroyCluster(ctx context.Context, o *core.DestroyOptions) error {
 			return fmt.Errorf("failed to create new resource groups client: %w", err)
 		}
 
-		if _, err = resourceGroupClient.Get(ctx, o.AzurePlatform.ResourceGroupName, nil); err != nil {
-			return fmt.Errorf("failed to get resource group name, '%s': %w", o.AzurePlatform.ResourceGroupName, err)
+		if err := checkResourceGroup(ctx, resourceGroupClient, o.AzurePlatform.ResourceGroupName, o.Log); err != nil {
+			return err
 		}
 	} else {
 		o.AzurePlatform.ResourceGroupName = o.Name + "-" + o.InfraID
@@ -149,6 +152,29 @@ func DestroyCluster(ctx context.Context, o *core.DestroyOptions) error {
 	return core.DestroyCluster(ctx, hostedCluster, o, destroyPlatformSpecifics)
 }
 
+type resourceGroupClient interface {
+	Get(context.Context, string, *armresources.ResourceGroupsClientGetOptions) (armresources.ResourceGroupsClientGetResponse, error)
+}
+
+func checkResourceGroup(ctx context.Context, client resourceGroupClient, resourceGroupName string, logger logr.Logger) error {
+	if _, err := client.Get(ctx, resourceGroupName, nil); err != nil {
+		if isResourceGroupNotFound(err) {
+			logger.Info("Resource group not found, continuing with cluster deletion", "resourceGroup", resourceGroupName)
+		} else {
+			return fmt.Errorf("failed to get resource group name, '%s': %w", resourceGroupName, err)
+		}
+	}
+	return nil
+}
+
+// isResourceGroupNotFound returns true if err is an Azure 404 response, indicating the
+// resource group was already deleted out-of-band (e.g. via the Azure portal or an expired
+// credential's cleanup process).
+func isResourceGroupNotFound(err error) bool {
+	var respErr *azcore.ResponseError
+	return stderrors.As(err, &respErr) && respErr.StatusCode == http.StatusNotFound
+}
+
 func destroyPlatformSpecifics(ctx context.Context, o *core.DestroyOptions) error {
 	// Clean up role assignments before destroying infrastructure to avoid orphans.
 	// Match the create path resource-group names: {name}-nsg and {name}-vnet.

cmd/cluster/azure/destroy_test.go

@@ -1,6 +1,9 @@
 package azure
 
 import (
+	"context"
+	"fmt"
+	"net/http"
 	"testing"
 	"time"
 
@@ -10,6 +13,9 @@ import (
 	"github.com/openshift/hypershift/cmd/cluster/core"
 	"github.com/openshift/hypershift/cmd/log"
 	"github.com/openshift/hypershift/support/config"
+
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources"
 )
 
 func TestDestroyClusterSetsCloudFromHostedCluster(t *testing.T) {
@@ -170,3 +176,74 @@ func TestDestroyClusterSetsGracePeriodFromTopology(t *testing.T) {
 		})
 	}
 }
+
+func TestIsResourceGroupNotFound(t *testing.T) {
+	tests := map[string]struct {
+		err      error
+		expected bool
+	}{
+		"When err is an azcore.ResponseError with 404 status, it should return true": {
+			err:      &azcore.ResponseError{StatusCode: http.StatusNotFound},
+			expected: true,
+		},
+		"When err is an azcore.ResponseError with a non-404 status, it should return false": {
+			err:      &azcore.ResponseError{StatusCode: http.StatusForbidden},
+			expected: false,
+		},
+		"When err is a generic error, it should return false": {
+			err:      fmt.Errorf("some unrelated failure"),
+			expected: false,
+		},
+		"When err wraps an azcore.ResponseError with 404 status, it should return true": {
+			err:      fmt.Errorf("wrapped: %w", &azcore.ResponseError{StatusCode: http.StatusNotFound}),
+			expected: true,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			g.Expect(isResourceGroupNotFound(test.err)).To(Equal(test.expected))
+		})
+	}
+}
+
+type fakeResourceGroupClient struct {
+	err error
+}
+
+func (f fakeResourceGroupClient) Get(context.Context, string, *armresources.ResourceGroupsClientGetOptions) (armresources.ResourceGroupsClientGetResponse, error) {
+	return armresources.ResourceGroupsClientGetResponse{}, f.err
+}
+
+func TestCheckResourceGroup(t *testing.T) {
+	tests := map[string]struct {
+		err         error
+		expectError bool
+	}{
+		"When the resource group returns 404, it should continue": {
+			err: &azcore.ResponseError{StatusCode: http.StatusNotFound},
+		},
+		"When the resource group returns a wrapped 404, it should continue": {
+			err: fmt.Errorf("wrapped: %w", &azcore.ResponseError{StatusCode: http.StatusNotFound}),
+		},
+		"When the resource group returns a non-404 error, it should return an error": {
+			err:         &azcore.ResponseError{StatusCode: http.StatusForbidden},
+			expectError: true,
+		},
+		"When the resource group lookup succeeds, it should continue": {},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			err := checkResourceGroup(context.Background(), fakeResourceGroupClient{err: test.err}, "test-resource-group", log.Log)
+			if test.expectError {
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err.Error()).To(ContainSubstring("failed to get resource group name, 'test-resource-group'"))
+				return
+			}
+			g.Expect(err).ToNot(HaveOccurred())
+		})
+	}
+}

hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go

@@ -1054,3 +1054,68 @@ func TestCAPIProviderDeploymentSpec(t *testing.T) {
 		})
 	}
 }
+
+func TestHasDeletionFailedCondition(t *testing.T) {
+	testCases := []struct {
+		name     string
+		machine  capiazure.AzureMachine
+		expected bool
+	}{
+		{
+			name: "When Ready is False with DeletionFailed reason, it should return true",
+			machine: capiazure.AzureMachine{
+				Status: capiazure.AzureMachineStatus{
+					Conditions: capiv1.Conditions{
+						{
+							Type:   capiv1.ReadyCondition,
+							Status: corev1.ConditionFalse,
+							Reason: capiazure.DeletionFailedReason,
+						},
+					},
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "When Ready is True, it should return false",
+			machine: capiazure.AzureMachine{
+				Status: capiazure.AzureMachineStatus{
+					Conditions: capiv1.Conditions{
+						{
+							Type:   capiv1.ReadyCondition,
+							Status: corev1.ConditionTrue,
+						},
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "When Ready is False with a different reason, it should return false",
+			machine: capiazure.AzureMachine{
+				Status: capiazure.AzureMachineStatus{
+					Conditions: capiv1.Conditions{
+						{
+							Type:   capiv1.ReadyCondition,
+							Status: corev1.ConditionFalse,
+							Reason: "SomeOtherReason",
+						},
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name:     "When there are no conditions, it should return false",
+			machine:  capiazure.AzureMachine{},
+			expected: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			g.Expect(hasDeletionFailedCondition(&tc.machine)).To(Equal(tc.expected))
+		})
+	}
+}

hypershift-operator/controllers/hostedcluster/internal/platform/platform.go

@@ -36,13 +36,14 @@ const (
 )
 
 var (
-	_ Platform = aws.AWS{}
-	_ Platform = azure.Azure{}
-	_ Platform = ibmcloud.IBMCloud{}
-	_ Platform = none.None{}
-	_ Platform = agent.Agent{}
-	_ Platform = kubevirt.Kubevirt{}
-	_ Platform = gcp.GCP{}
+	_ Platform      = aws.AWS{}
+	_ Platform      = azure.Azure{}
+	_ Platform      = ibmcloud.IBMCloud{}
+	_ Platform      = none.None{}
+	_ Platform      = agent.Agent{}
+	_ Platform      = kubevirt.Kubevirt{}
+	_ Platform      = gcp.GCP{}
+	_ OrphanDeleter = &azure.Azure{}
 )
 
 type Platform interface {