Skip to content

Commit

Permalink
Refactor deleteResourcesAndResourceGroup to be simpler and more testable
Browse files Browse the repository at this point in the history
  • Loading branch information
@tsatam
tsatam committed Oct 10, 2023
1 parent 4b3a3ce commit 579d9d3
Show file tree
Hide file tree
Showing 2 changed files with 188 additions and 21 deletions.
52 changes: 31 additions & 21 deletions pkg/cluster/delete.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
}
Expand Down
157 changes: 157 additions & 0 deletions pkg/cluster/delete_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
})
}
}

0 comments on commit 579d9d3

Please sign in to comment.