diff --git a/cmd/cluster/azure/destroy.go b/cmd/cluster/azure/destroy.go index baf63d29acb4..806ccd96c67b 100644 --- a/cmd/cluster/azure/destroy.go +++ b/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. diff --git a/cmd/cluster/azure/destroy_test.go b/cmd/cluster/azure/destroy_test.go index 6ecd539d77ce..30bd1917c486 100644 --- a/cmd/cluster/azure/destroy_test.go +++ b/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()) + }) + } +} 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 14512d5e4271..80af16f44b01 100644 --- a/hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go +++ b/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)) + }) + } +} diff --git a/hypershift-operator/controllers/hostedcluster/internal/platform/platform.go b/hypershift-operator/controllers/hostedcluster/internal/platform/platform.go index 9e59108c4e10..91482db99c96 100644 --- a/hypershift-operator/controllers/hostedcluster/internal/platform/platform.go +++ b/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 {