Remove deprecated securityGroup client (#3853)

Azure · Sep 20, 2024 · 35b2881 · 35b2881
1 parent b0518e7
commit 35b2881
Show file tree

Hide file tree

Showing 3 changed files with 93 additions and 8 deletions.
diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go
@@ -85,7 +85,6 @@ type manager struct {
 	loadBalancers            network.LoadBalancersClient // TODO: use armLoadBalancers instead.
 	armLoadBalancers         armnetwork.LoadBalancersClient
 	armPrivateEndpoints      armnetwork.PrivateEndpointsClient
-	securityGroups           network.SecurityGroupsClient // TODO: use armSecurityGroups instead.
 	armSecurityGroups        armnetwork.SecurityGroupsClient
 	deployments              features.DeploymentsClient
 	resourceGroups           features.ResourceGroupsClient
@@ -264,7 +263,6 @@ func New(ctx context.Context, log *logrus.Entry, _env env.Interface, db database
 		loadBalancers:            network.NewLoadBalancersClient(_env.Environment(), r.SubscriptionID, fpAuthorizer),
 		armLoadBalancers:         armLoadBalancersClient,
 		armPrivateEndpoints:      armPrivateEndpoints,
-		securityGroups:           network.NewSecurityGroupsClient(_env.Environment(), r.SubscriptionID, fpAuthorizer),
 		armSecurityGroups:        armSecurityGroupsClient,
 		deployments:              features.NewDeploymentsClient(_env.Environment(), r.SubscriptionID, fpAuthorizer),
 		resourceGroups:           features.NewResourceGroupsClient(_env.Environment(), r.SubscriptionID, fpAuthorizer),

diff --git a/pkg/cluster/delete.go b/pkg/cluster/delete.go
@@ -70,17 +70,17 @@ func (m *manager) disconnectSecurityGroup(ctx context.Context, resourceID string
 		return err
 	}
 
-	nsg, err := m.securityGroups.Get(ctx, r.ResourceGroup, r.ResourceName, "")
+	resp, err := m.armSecurityGroups.Get(ctx, r.ResourceGroup, r.ResourceName, nil)
 	if err != nil {
 		return err
 	}
+	nsg := resp.SecurityGroup
 
-	if nsg.SecurityGroupPropertiesFormat == nil ||
-		nsg.SecurityGroupPropertiesFormat.Subnets == nil {
+	if nsg.Properties == nil || nsg.Properties.Subnets == nil {
 		return nil
 	}
 
-	for _, subnet := range *nsg.SecurityGroupPropertiesFormat.Subnets {
+	for _, subnet := range nsg.Properties.Subnets {
 		// Note: subnet only has value in the ID field,
 		// so we have to make another API request to get full subnet struct
 		// TODO: there is probably an undesirable race condition here - check if etags can help.
@@ -102,8 +102,7 @@ func (m *manager) disconnectSecurityGroup(ctx context.Context, resourceID string
 			}
 		}
 
-		if s.SubnetPropertiesFormat == nil ||
-			s.SubnetPropertiesFormat.NetworkSecurityGroup == nil ||
+		if s.SubnetPropertiesFormat == nil || s.SubnetPropertiesFormat.NetworkSecurityGroup == nil ||
 			!strings.EqualFold(*s.SubnetPropertiesFormat.NetworkSecurityGroup.ID, *nsg.ID) {
 			continue
 		}

diff --git a/pkg/cluster/delete_test.go b/pkg/cluster/delete_test.go
@@ -10,18 +10,23 @@ import (
 	"net/http"
 	"testing"
 
+	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v2"
 	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/stretchr/testify/assert"
+	"k8s.io/utils/ptr"
 
 	"github.com/Azure/ARO-RP/pkg/api"
+	mock_armnetwork "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/azuresdk/armnetwork"
 	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"
+	mock_subnet "github.com/Azure/ARO-RP/pkg/util/mocks/subnet"
 	utilerror "github.com/Azure/ARO-RP/test/util/error"
 )
 
@@ -274,3 +279,86 @@ func TestDeleteResourceGroup(t *testing.T) {
 		})
 	}
 }
+
+func TestDisconnectSecurityGroup(t *testing.T) {
+	subscription := "00000000-0000-0000-0000-000000000000"
+	resourceGroup := "test-rg"
+	nsgId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/networkSecurityGroups/test-nsg", subscription, resourceGroup)
+
+	tests := []struct {
+		name    string
+		mocks   func(*mock_armnetwork.MockSecurityGroupsClient, *mock_subnet.MockManager)
+		wantErr string
+	}{
+		{
+			name: "empty security group",
+			mocks: func(securityGroups *mock_armnetwork.MockSecurityGroupsClient, subnets *mock_subnet.MockManager) {
+				securityGroup := armnetwork.SecurityGroupsClientGetResponse{
+					SecurityGroup: armnetwork.SecurityGroup{
+						ID: ptr.To(nsgId),
+						Properties: &armnetwork.SecurityGroupPropertiesFormat{
+							Subnets: []*armnetwork.Subnet{},
+						},
+					},
+				}
+				securityGroups.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), nil).Return(securityGroup, nil)
+				subnets.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
+			},
+		},
+		{
+			name: "disconnects subnets",
+			mocks: func(securityGroups *mock_armnetwork.MockSecurityGroupsClient, subnets *mock_subnet.MockManager) {
+				subnetId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/test-subnet", subscription, resourceGroup)
+				securityGroup := armnetwork.SecurityGroupsClientGetResponse{
+					SecurityGroup: armnetwork.SecurityGroup{
+						ID: ptr.To(nsgId),
+						Properties: &armnetwork.SecurityGroupPropertiesFormat{
+							Subnets: []*armnetwork.Subnet{
+								{
+									ID: ptr.To(subnetId),
+								},
+							},
+						},
+					},
+				}
+				securityGroups.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), nil).Return(securityGroup, nil)
+				subnets.EXPECT().Get(gomock.Any(), subnetId).Times(1).Return(&mgmtnetwork.Subnet{
+					ID: ptr.To(subnetId),
+					SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{
+						NetworkSecurityGroup: &mgmtnetwork.SecurityGroup{
+							ID: ptr.To(nsgId),
+						},
+					},
+				}, nil)
+				subnets.EXPECT().CreateOrUpdate(gomock.Any(), subnetId, &mgmtnetwork.Subnet{
+					ID:                     ptr.To(subnetId),
+					SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{},
+				}).Times(1).Return(nil)
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			controller := gomock.NewController(t)
+			defer controller.Finish()
+
+			securityGroups := mock_armnetwork.NewMockSecurityGroupsClient(controller)
+			subnets := mock_subnet.NewMockManager(controller)
+
+			tt.mocks(securityGroups, subnets)
+
+			m := manager{
+				log:               logrus.NewEntry(logrus.StandardLogger()),
+				armSecurityGroups: securityGroups,
+				subnet:            subnets,
+			}
+
+			ctx := context.Background()
+			err := m.disconnectSecurityGroup(ctx, nsgId)
+			if tt.wantErr != "" {
+				assert.EqualError(t, err, tt.wantErr)
+			}
+		})
+	}
+}