From 579d9d3c3d39ed9a56fb39e010a9b4bbdb4e56cd Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Tue, 10 Oct 2023 17:05:13 -0400 Subject: [PATCH] Refactor deleteResourcesAndResourceGroup to be simpler and more testable --- pkg/cluster/delete.go | 52 +++++++----- pkg/cluster/delete_test.go | 157 +++++++++++++++++++++++++++++++++++++ 2 files changed, 188 insertions(+), 21 deletions(-) diff --git a/pkg/cluster/delete.go b/pkg/cluster/delete.go index c83357f01fa..a944bee8fe5 100644 --- a/pkg/cluster/delete.go +++ b/pkg/cluster/delete.go @@ -376,22 +376,10 @@ func (m *manager) deleteGateway(ctx context.Context) error { func (m *manager) deleteResourcesAndResourceGroup(ctx context.Context) error { resourceGroup := stringutils.LastTokenByte(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID, '/') - // In edge case of CRG not being managedBy ARO, we have a different delete path - // we will assume normal case and set rgManagedByARO to true CRG is managedBy ARO - rgManagedByARO := true - rg, err := m.resourceGroups.Get(ctx, resourceGroup) - if err != nil { - m.log.Warnf("failed to get resourceGroup %s", err) - } else if !m.env.IsLocalDevelopmentMode() { - if rg.ManagedBy == nil || *rg.ManagedBy == "" || !strings.EqualFold(*rg.ManagedBy, m.doc.OpenShiftCluster.ID) { - rgManagedByARO = false - m.log.Infof("cluster resource group not managed by aro %s", *rg.Name) - } - } - // Do not delete the resource group if it is not managed by ARO - if !rgManagedByARO { - return nil + shouldDelete, err := m.shouldDeleteResourceGroup(ctx, resourceGroup) + if err != nil || !shouldDelete { + return err } m.log.Printf("deleting resources") @@ -401,13 +389,35 @@ func (m *manager) deleteResourcesAndResourceGroup(ctx context.Context) error { } m.log.Printf("deleting resource group %s", resourceGroup) - err = m.resourceGroups.DeleteAndWait(ctx, resourceGroup) - detailedErr, ok := err.(autorest.DetailedError) - if ok && (detailedErr.StatusCode == http.StatusNotFound) { - err = nil + return m.deleteResourceGroup(ctx, resourceGroup) +} + +func (m *manager) shouldDeleteResourceGroup(ctx context.Context, name string) (bool, error) { + resourceGroup, err := m.resourceGroups.Get(ctx, name) + if err != nil { + detailedErr, isDetailedErr := err.(autorest.DetailedError) + if azureerrors.ResourceGroupNotFound(err) || (isDetailedErr && detailedErr.StatusCode == http.StatusNotFound) { + m.log.Infof("managed resource group %s not found, skipping deletion", name) + err = nil + } + return false, err } - if azureerrors.ResourceGroupNotFound(err) { - err = nil + + rgManagedByCluster := resourceGroup.ManagedBy != nil && strings.EqualFold(*resourceGroup.ManagedBy, m.doc.OpenShiftCluster.ID) + if !rgManagedByCluster { + m.log.Infof("managed resource group %s not managed by cluster, skipping deletion", *resourceGroup.Name) + return false, nil + } + + return true, nil +} + +func (m *manager) deleteResourceGroup(ctx context.Context, name string) error { + err := m.resourceGroups.DeleteAndWait(ctx, name) + + detailedErr, isDetailedErr := err.(autorest.DetailedError) + if azureerrors.ResourceGroupNotFound(err) || (isDetailedErr && (detailedErr.StatusCode == http.StatusNotFound)) { + return nil } return err } diff --git a/pkg/cluster/delete_test.go b/pkg/cluster/delete_test.go index f6cf817488e..f0647bfbdf9 100644 --- a/pkg/cluster/delete_test.go +++ b/pkg/cluster/delete_test.go @@ -5,16 +5,21 @@ package cluster import ( "context" + "errors" "fmt" "net/http" "testing" mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network" + mgmtfeatures "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-07-01/features" "github.com/Azure/go-autorest/autorest" + "github.com/Azure/go-autorest/autorest/azure" + "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" "github.com/sirupsen/logrus" "github.com/Azure/ARO-RP/pkg/api" + mock_features "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/features" mock_network "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/network" mock_env "github.com/Azure/ARO-RP/pkg/util/mocks/env" utilerror "github.com/Azure/ARO-RP/test/util/error" @@ -117,3 +122,155 @@ func TestDeleteNic(t *testing.T) { }) } } + +func TestShouldDeleteResourceGroup(t *testing.T) { + ctx := context.Background() + subscription := "00000000-0000-0000-0000-000000000000" + clusterName := "cluster" + clusterRGName := "aro-cluster" + clusterResourceId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.RedHatOpenShift/openShiftClusters/%s", subscription, clusterRGName, clusterName) + managedRGName := "aro-managed-rg" + + errNotFound := autorest.DetailedError{ + StatusCode: http.StatusNotFound, + Original: &azure.ServiceError{ + Code: "ResourceGroupNotFound", + }, + } + + tests := []struct { + name string + getResourceGroup mgmtfeatures.ResourceGroup + getErr error + wantShouldDelete bool + wantErr string + }{ + { + name: "get resource group - not found", + getErr: errNotFound, + wantShouldDelete: false, + }, + { + name: "get resource group - other error", + getErr: errors.New("generic err"), + wantShouldDelete: false, + wantErr: "generic err", + }, + { + name: "resource group not managed (nil)", + getResourceGroup: mgmtfeatures.ResourceGroup{Name: &managedRGName, ManagedBy: nil}, + wantShouldDelete: false, + }, + { + name: "resource group not managed (empty string)", + getResourceGroup: mgmtfeatures.ResourceGroup{Name: &managedRGName, ManagedBy: to.StringPtr("")}, + wantShouldDelete: false, + }, + { + name: "resource group not managed by cluster", + getResourceGroup: mgmtfeatures.ResourceGroup{Name: &managedRGName, ManagedBy: to.StringPtr("/somethingelse")}, + wantShouldDelete: false, + }, + { + name: "resource group managed by cluster", + getResourceGroup: mgmtfeatures.ResourceGroup{Name: &managedRGName, ManagedBy: to.StringPtr(clusterResourceId)}, + wantShouldDelete: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + controller := gomock.NewController(t) + defer controller.Finish() + + resourceGroups := mock_features.NewMockResourceGroupsClient(controller) + resourceGroups.EXPECT().Get(gomock.Any(), gomock.Eq(managedRGName)).Return(tt.getResourceGroup, tt.getErr) + + m := manager{ + log: logrus.NewEntry(logrus.StandardLogger()), + doc: &api.OpenShiftClusterDocument{ + OpenShiftCluster: &api.OpenShiftCluster{ + ID: clusterResourceId, + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + ResourceGroupID: fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", subscription, managedRGName), + }, + }, + }, + }, + resourceGroups: resourceGroups, + } + + shouldDelete, err := m.shouldDeleteResourceGroup(ctx, managedRGName) + + if shouldDelete != tt.wantShouldDelete { + t.Errorf("wanted shouldDelete: %v but got %v", tt.wantShouldDelete, shouldDelete) + } + + utilerror.AssertErrorMessage(t, err, tt.wantErr) + }) + } +} + +func TestDeleteResourceGroup(t *testing.T) { + ctx := context.Background() + subscription := "00000000-0000-0000-0000-000000000000" + clusterName := "cluster" + clusterRGName := "aro-cluster" + clusterResourceId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.RedHatOpenShift/openShiftClusters/%s", subscription, clusterRGName, clusterName) + managedRGName := "aro-managed-rg" + + errNotFound := autorest.DetailedError{ + StatusCode: http.StatusNotFound, + Original: &azure.ServiceError{ + Code: "ResourceGroupNotFound", + }, + } + + tests := []struct { + name string + deleteErr error + wantErr string + }{ + { + name: "not found", + deleteErr: errNotFound, + }, + { + name: "other error", + deleteErr: errors.New("generic err"), + wantErr: "generic err", + }, + { + name: "success", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + controller := gomock.NewController(t) + defer controller.Finish() + + resourceGroups := mock_features.NewMockResourceGroupsClient(controller) + resourceGroups.EXPECT().DeleteAndWait(gomock.Any(), gomock.Eq(managedRGName)).Times(1).Return(tt.deleteErr) + + m := manager{ + log: logrus.NewEntry(logrus.StandardLogger()), + doc: &api.OpenShiftClusterDocument{ + OpenShiftCluster: &api.OpenShiftCluster{ + ID: clusterResourceId, + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + ResourceGroupID: fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", subscription, managedRGName), + }, + }, + }, + }, + resourceGroups: resourceGroups, + } + + err := m.deleteResourceGroup(ctx, managedRGName) + utilerror.AssertErrorMessage(t, err, tt.wantErr) + }) + } +}