Watch MachineSets for worker subnet changes instead of Machines (#3280)

https://issues.redhat.com/browse/ARO-4632

pkg/operator/controllers/storageaccounts/storageaccount_controller.go

@@ -9,7 +9,6 @@ import (
 	"github.com/Azure/go-autorest/autorest/azure"
 	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	"github.com/sirupsen/logrus"
-	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
@@ -116,11 +115,15 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 	aroClusterPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool {
 		return o.GetName() == arov1alpha1.SingletonClusterName
 	})
+	masterMachinePredicate := predicate.NewPredicateFuncs(func(o client.Object) bool {
+		role, ok := o.GetLabels()["machine.openshift.io/cluster-api-machine-role"]
+		return ok && role == "master"
+	})
 
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&arov1alpha1.Cluster{}, builder.WithPredicates(aroClusterPredicate)).
-		Watches(&source.Kind{Type: &machinev1beta1.Machine{}}, &handler.EnqueueRequestForObject{}). // to reconcile on machine replacement
-		Watches(&source.Kind{Type: &corev1.Node{}}, &handler.EnqueueRequestForObject{}).            // to reconcile on node status change
+		Watches(&source.Kind{Type: &machinev1beta1.Machine{}}, &handler.EnqueueRequestForObject{}, builder.WithPredicates(masterMachinePredicate)). // to reconcile on master machine replacement
+		Watches(&source.Kind{Type: &machinev1beta1.MachineSet{}}, &handler.EnqueueRequestForObject{}).                                              // to reconcile on worker machinesets
 		Named(ControllerName).
 		Complete(r)
 }

pkg/operator/controllers/storageaccounts/storageaccounts.go

@@ -13,6 +13,7 @@ import (
 	imageregistryv1 "github.com/openshift/api/imageregistry/v1"
 	"k8s.io/apimachinery/pkg/types"
 
+	"github.com/Azure/ARO-RP/pkg/util/azureerrors"
 	"github.com/Azure/ARO-RP/pkg/util/stringutils"
 )
 
@@ -32,6 +33,10 @@ func (r *reconcileManager) reconcileAccounts(ctx context.Context) error {
 	for _, subnet := range subnets {
 		mgmtSubnet, err := r.subnets.Get(ctx, subnet.ResourceID)
 		if err != nil {
+			if azureerrors.IsNotFoundError(err) {
+				r.log.Infof("Subnet %s not found, skipping", subnet.ResourceID)
+				break
+			}
 			return err
 		}
 

pkg/operator/controllers/storageaccounts/storageaccounts_test.go

@@ -5,11 +5,13 @@ package storageaccounts
 
 import (
 	"context"
+	"net/http"
 	"strconv"
 	"testing"
 
 	mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
 	mgmtstorage "github.com/Azure/azure-sdk-for-go/services/storage/mgmt/2019-06-01/storage"
+	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/golang/mock/gomock"
 	imageregistryv1 "github.com/openshift/api/imageregistry/v1"
@@ -212,6 +214,56 @@ func TestReconcileManager(t *testing.T) {
 				storage.EXPECT().Update(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, updated)
 			},
 		},
+		{
+			name:         "Operator Flag enabled - not found error on getting worker subnet skips subnet",
+			operatorFlag: true,
+			mocks: func(storage *mock_storage.MockAccountsClient, kubeSubnet *mock_subnet.MockKubeManager, mgmtSubnet *mock_subnet.MockManager) {
+				// Azure subnets
+				masterSubnet := getValidSubnet(resourceIdMaster)
+
+				notFoundErr := autorest.DetailedError{
+					StatusCode: http.StatusNotFound,
+				}
+
+				mgmtSubnet.EXPECT().Get(gomock.Any(), resourceIdMaster).Return(masterSubnet, nil)
+				mgmtSubnet.EXPECT().Get(gomock.Any(), resourceIdWorker).Return(nil, notFoundErr)
+
+				// cluster subnets
+				kubeSubnet.EXPECT().List(gomock.Any()).Return([]subnet.Subnet{
+					{
+						ResourceID: resourceIdMaster,
+						IsMaster:   true,
+					},
+					{
+						ResourceID: resourceIdWorker,
+						IsMaster:   false,
+					},
+				}, nil)
+
+				// storage objects in azure
+				result := getValidAccount([]string{})
+				updated := mgmtstorage.AccountUpdateParameters{
+					AccountPropertiesUpdateParameters: &mgmtstorage.AccountPropertiesUpdateParameters{
+						NetworkRuleSet: getValidAccount([]string{resourceIdMaster}).NetworkRuleSet,
+					},
+				}
+
+				storage.EXPECT().GetProperties(gomock.Any(), clusterResourceGroupName, clusterStorageAccountName, gomock.Any()).Return(*result, nil)
+				storage.EXPECT().Update(gomock.Any(), clusterResourceGroupName, clusterStorageAccountName, updated)
+
+				// we can't reuse these from above due to fact how gomock handles objects.
+				// they are modified by the functions so they are not the same anymore
+				result = getValidAccount([]string{})
+				updated = mgmtstorage.AccountUpdateParameters{
+					AccountPropertiesUpdateParameters: &mgmtstorage.AccountPropertiesUpdateParameters{
+						NetworkRuleSet: getValidAccount([]string{resourceIdMaster}).NetworkRuleSet,
+					},
+				}
+
+				storage.EXPECT().GetProperties(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, gomock.Any()).Return(*result, nil)
+				storage.EXPECT().Update(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, updated)
+			},
+		},
 		{
 			name:         "Operator flag enabled - worker subnet rule to all accounts because storage service endpoint on worker subnet",
 			operatorFlag: true,

pkg/operator/controllers/subnets/subnet_controller_test.go

@@ -5,11 +5,13 @@ package subnets
 
 import (
 	"context"
+	"net/http"
 	"reflect"
 	"strconv"
 	"testing"
 
 	mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
+	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/golang/mock/gomock"
 	"github.com/sirupsen/logrus"
@@ -34,11 +36,12 @@ var (
 	subnetNameWorker         = "worker"
 	subnetNameMaster         = "master"
 
-	nsgv1NodeResourceId    = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGNodeSuffixV1
-	nsgv1MasterResourceId  = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGControlPlaneSuffixV1
-	nsgv2ResourceId        = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGSuffixV2
-	subnetResourceIdMaster = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameMaster
-	subnetResourceIdWorker = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker
+	nsgv1NodeResourceId           = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGNodeSuffixV1
+	nsgv1MasterResourceId         = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGControlPlaneSuffixV1
+	nsgv2ResourceId               = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGSuffixV2
+	subnetResourceIdMaster        = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameMaster
+	subnetResourceIdWorker        = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker
+	subnetResourceIdWorkerInvalid = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker + "-invalid"
 )
 
 func getValidClusterInstance(operatorFlagEnabled bool, operatorFlagNSG bool, operatorFlagServiceEndpoint bool) *arov1alpha1.Cluster {
@@ -177,6 +180,52 @@ func TestReconcileManager(t *testing.T) {
 				mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdWorker, subnetObjectWorkerUpdate).Return(nil)
 			},
 		},
+		{
+			name:                        "Architecture V1 - skips invalid/not found subnets",
+			operatorFlagEnabled:         true,
+			operatorFlagNSG:             true,
+			operatorFlagServiceEndpoint: true,
+			wantAnnotationsUpdated:      true,
+			subnetMock: func(mock *mock_subnet.MockManager, kmock *mock_subnet.MockKubeManager) {
+				kmock.EXPECT().List(gomock.Any()).Return([]subnet.Subnet{
+					{
+						ResourceID: subnetResourceIdMaster,
+						IsMaster:   true,
+					},
+					{
+						ResourceID: subnetResourceIdWorker,
+						IsMaster:   false,
+					},
+					{
+						ResourceID: subnetResourceIdWorkerInvalid,
+						IsMaster:   false,
+					},
+				}, nil)
+
+				subnetObjectMaster := getValidSubnet()
+				subnetObjectMaster.NetworkSecurityGroup.ID = to.StringPtr(nsgv1MasterResourceId + "new")
+				mock.EXPECT().Get(gomock.Any(), subnetResourceIdMaster).Return(subnetObjectMaster, nil).MaxTimes(2)
+
+				subnetObjectMasterUpdate := getValidSubnet()
+				subnetObjectMasterUpdate.NetworkSecurityGroup.ID = to.StringPtr(nsgv1MasterResourceId)
+				mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdMaster, subnetObjectMasterUpdate).Return(nil)
+
+				subnetObjectWorker := getValidSubnet()
+				subnetObjectWorker.NetworkSecurityGroup.ID = to.StringPtr(nsgv1NodeResourceId + "new")
+				mock.EXPECT().Get(gomock.Any(), subnetResourceIdWorker).Return(subnetObjectWorker, nil).MaxTimes(2)
+
+				subnetObjectWorkerUpdate := getValidSubnet()
+				subnetObjectWorkerUpdate.NetworkSecurityGroup.ID = to.StringPtr(nsgv1NodeResourceId)
+				mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdWorker, subnetObjectWorkerUpdate).Return(nil)
+
+				notFoundErr := autorest.DetailedError{
+					StatusCode: http.StatusNotFound,
+				}
+
+				mock.EXPECT().Get(gomock.Any(), subnetResourceIdWorkerInvalid).Return(nil, notFoundErr).AnyTimes()
+				mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdWorkerInvalid, gomock.Any()).Times(0)
+			},
+		},
 		{
 			name:                        "Architecture V1 - node only fixup",
 			operatorFlagEnabled:         true,
@@ -275,6 +324,55 @@ func TestReconcileManager(t *testing.T) {
 				instace.Spec.ArchitectureVersion = int(api.ArchitectureVersionV2)
 			},
 		},
+		{
+			name:                        "Architecture V2 - skips invalid/not found subnets",
+			operatorFlagEnabled:         true,
+			operatorFlagNSG:             true,
+			operatorFlagServiceEndpoint: true,
+			wantAnnotationsUpdated:      true,
+			subnetMock: func(mock *mock_subnet.MockManager, kmock *mock_subnet.MockKubeManager) {
+				kmock.EXPECT().List(gomock.Any()).Return([]subnet.Subnet{
+					{
+						ResourceID: subnetResourceIdMaster,
+						IsMaster:   true,
+					},
+					{
+						ResourceID: subnetResourceIdWorker,
+						IsMaster:   false,
+					},
+					{
+						ResourceID: subnetResourceIdWorkerInvalid,
+						IsMaster:   false,
+					},
+				}, nil)
+
+				subnetObjectMaster := getValidSubnet()
+				subnetObjectMaster.NetworkSecurityGroup.ID = to.StringPtr(nsgv2ResourceId + "new")
+				mock.EXPECT().Get(gomock.Any(), subnetResourceIdMaster).Return(subnetObjectMaster, nil).MaxTimes(2)
+
+				subnetObjectMasterUpdate := getValidSubnet()
+				subnetObjectMasterUpdate.NetworkSecurityGroup.ID = to.StringPtr(nsgv2ResourceId)
+				mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdMaster, subnetObjectMasterUpdate).Return(nil)
+
+				subnetObjectWorker := getValidSubnet()
+				subnetObjectWorker.NetworkSecurityGroup.ID = to.StringPtr(nsgv2ResourceId + "new")
+				mock.EXPECT().Get(gomock.Any(), subnetResourceIdWorker).Return(subnetObjectWorker, nil).MaxTimes(2)
+
+				subnetObjectWorkerUpdate := getValidSubnet()
+				subnetObjectWorkerUpdate.NetworkSecurityGroup.ID = to.StringPtr(nsgv2ResourceId)
+				mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdWorker, subnetObjectWorkerUpdate).Return(nil)
+
+				notFoundErr := autorest.DetailedError{
+					StatusCode: http.StatusNotFound,
+				}
+
+				mock.EXPECT().Get(gomock.Any(), subnetResourceIdWorkerInvalid).Return(nil, notFoundErr).AnyTimes()
+				mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdWorkerInvalid, gomock.Any()).Times(0)
+			},
+			instance: func(instace *arov1alpha1.Cluster) {
+				instace.Spec.ArchitectureVersion = int(api.ArchitectureVersionV2)
+			},
+		},
 		{
 			name:                        "Architecture V2 - endpoint fixup",
 			operatorFlagEnabled:         true,

pkg/operator/controllers/subnets/subnet_nsg.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/Azure/ARO-RP/pkg/api"
 	apisubnet "github.com/Azure/ARO-RP/pkg/api/util/subnet"
+	"github.com/Azure/ARO-RP/pkg/util/azureerrors"
 	"github.com/Azure/ARO-RP/pkg/util/subnet"
 )
 
@@ -25,6 +26,10 @@ func (r *reconcileManager) ensureSubnetNSG(ctx context.Context, s subnet.Subnet)
 
 	subnetObject, err := r.subnets.Get(ctx, s.ResourceID)
 	if err != nil {
+		if azureerrors.IsNotFoundError(err) {
+			r.log.Infof("Subnet %s not found, skipping", s.ResourceID)
+			return nil
+		}
 		return err
 	}
 	if subnetObject.SubnetPropertiesFormat == nil {

pkg/operator/controllers/subnets/subnet_serviceendpoint.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/Azure/ARO-RP/pkg/api"
 	"github.com/Azure/ARO-RP/pkg/operator"
+	"github.com/Azure/ARO-RP/pkg/util/azureerrors"
 	"github.com/Azure/ARO-RP/pkg/util/subnet"
 )
 
@@ -22,6 +23,10 @@ func (r *reconcileManager) ensureSubnetServiceEndpoints(ctx context.Context, s s
 
 		subnetObject, err := r.subnets.Get(ctx, s.ResourceID)
 		if err != nil {
+			if azureerrors.IsNotFoundError(err) {
+				r.log.Infof("Subnet %s not found, skipping. err: %v", s.ResourceID, err)
+				return nil
+			}
 			return err
 		}
 

pkg/operator/controllers/subnets/subnets_controller.go

@@ -11,7 +11,6 @@ import (
 	"github.com/Azure/go-autorest/autorest/azure"
 	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	"github.com/sirupsen/logrus"
-	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
@@ -155,11 +154,15 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 	aroClusterPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool {
 		return o.GetName() == arov1alpha1.SingletonClusterName
 	})
+	masterMachinePredicate := predicate.NewPredicateFuncs(func(o client.Object) bool {
+		role, ok := o.GetLabels()["machine.openshift.io/cluster-api-machine-role"]
+		return ok && role == "master"
+	})
 
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&arov1alpha1.Cluster{}, builder.WithPredicates(aroClusterPredicate)).
-		Watches(&source.Kind{Type: &machinev1beta1.Machine{}}, &handler.EnqueueRequestForObject{}). // to reconcile on machine replacement
-		Watches(&source.Kind{Type: &corev1.Node{}}, &handler.EnqueueRequestForObject{}).            // to reconcile on node status change
+		Watches(&source.Kind{Type: &machinev1beta1.Machine{}}, &handler.EnqueueRequestForObject{}, builder.WithPredicates(masterMachinePredicate)). // to reconcile on master machine replacement
+		Watches(&source.Kind{Type: &machinev1beta1.MachineSet{}}, &handler.EnqueueRequestForObject{}).                                              // to reconcile on worker machinesets
 		Named(ControllerName).
 		Complete(r)
 }

pkg/util/azureerrors/error.go

@@ -5,6 +5,7 @@ package azureerrors
 
 import (
 	"encoding/json"
+	"net/http"
 	"strings"
 
 	"github.com/Azure/go-autorest/autorest"
@@ -71,6 +72,12 @@ func IsDeploymentActiveError(err error) bool {
 	return false
 }
 
+func IsNotFoundError(err error) bool {
+	detailedErr, ok := err.(autorest.DetailedError)
+
+	return ok && detailedErr.StatusCode == http.StatusNotFound
+}
+
 // IsInvalidSecretError returns if errors is InvalidCredentials error
 // Example: (adal.tokenRefreshError) adal: Refresh request failed. Status Code = '401'.
 // Response body: {"error":"invalid_client","error_description":"AADSTS7000215:

pkg/util/subnet/cluster_subnets.go

@@ -45,8 +45,8 @@ func (m *kubeManager) List(ctx context.Context) ([]Subnet, error) {
 
 	// select all workers by the  machine.openshift.io/cluster-api-machine-role: not equal to master Label
 	selector, _ := labels.Parse("machine.openshift.io/cluster-api-machine-role!=master")
-	machines := &machinev1beta1.MachineList{}
-	err := m.client.List(ctx, machines, &client.ListOptions{
+	machineSets := &machinev1beta1.MachineSetList{}
+	err := m.client.List(ctx, machineSets, &client.ListOptions{
 		Namespace:     machineSetsNamespace,
 		LabelSelector: selector,
 	})
@@ -54,8 +54,8 @@ func (m *kubeManager) List(ctx context.Context) ([]Subnet, error) {
 		return nil, err
 	}
 
-	for _, machine := range machines.Items {
-		subnetDesc, err := m.getDescriptorFromProviderSpec(machine.Spec.ProviderSpec.Value)
+	for _, machineSet := range machineSets.Items {
+		subnetDesc, err := m.getDescriptorFromProviderSpec(machineSet.Spec.Template.Spec.ProviderSpec.Value)
 		if err != nil {
 			return nil, err
 		}
@@ -63,7 +63,7 @@ func (m *kubeManager) List(ctx context.Context) ([]Subnet, error) {
 	}
 
 	selector, _ = labels.Parse("machine.openshift.io/cluster-api-machine-role=master")
-	machines = &machinev1beta1.MachineList{}
+	machines := &machinev1beta1.MachineList{}
 	err = m.client.List(ctx, machines, &client.ListOptions{
 		Namespace:     machineSetsNamespace,
 		LabelSelector: selector,

pkg/util/subnet/cluster_subnets_test.go

@@ -30,7 +30,7 @@ func TestListFromCluster(t *testing.T) {
 		name         string
 		machinelabel string
 		expect       []Subnet
-		modify       func(*machinev1beta1.Machine, *machinev1beta1.Machine)
+		modify       func(*machinev1beta1.MachineSet, *machinev1beta1.Machine)
 		wantErr      string
 	}{
 		{
@@ -48,7 +48,7 @@ func TestListFromCluster(t *testing.T) {
 		{
 			name:   "master missing providerSpec",
 			expect: nil,
-			modify: func(worker *machinev1beta1.Machine, master *machinev1beta1.Machine) {
+			modify: func(worker *machinev1beta1.MachineSet, master *machinev1beta1.Machine) {
 				master.Spec.ProviderSpec.Value.Raw = []byte("")
 			},
 			wantErr: "json: error calling MarshalJSON for type *runtime.RawExtension: unexpected end of JSON input",
@@ -56,8 +56,8 @@ func TestListFromCluster(t *testing.T) {
 		{
 			name:   "worker missing providerSpec",
 			expect: nil,
-			modify: func(worker *machinev1beta1.Machine, master *machinev1beta1.Machine) {
-				worker.Spec.ProviderSpec.Value.Raw = []byte("")
+			modify: func(worker *machinev1beta1.MachineSet, master *machinev1beta1.Machine) {
+				worker.Spec.Template.Spec.ProviderSpec.Value.Raw = []byte("")
 			},
 			wantErr: "json: error calling MarshalJSON for type *runtime.RawExtension: unexpected end of JSON input",
 		},
@@ -77,26 +77,34 @@ func TestListFromCluster(t *testing.T) {
 					},
 				},
 			}
-			workerMachine := machinev1beta1.Machine{
+			workerMachineSet := machinev1beta1.MachineSet{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "worker-0",
+					Name:      "worker",
 					Namespace: "openshift-machine-api",
 					Labels:    map[string]string{"machine.openshift.io/cluster-api-machine-role": "worker"},
 				},
-				Spec: machinev1beta1.MachineSpec{
-					ProviderSpec: machinev1beta1.ProviderSpec{
-						Value: &kruntime.RawExtension{
-							Raw: []byte("{\"resourceGroup\":\"workerRG\",\"publicIP\":false,\"osDisk\":{\"diskSizeGB\": 1024,\"managedDisk\":{\"storageAccountType\": \"Premium_LRS\"},\"osType\":\"Linux\"},\"image\":{\"offer\": \"aro4\",\"publisher\": \"azureopenshift\", \"resourceID\": \"\", \"sku\": \"aro_43\", \"version\": \"43.81.20200311\"},\"networkResourceGroup\":\"vnet-rg\",\"vnet\":\"vnet\",\"subnet\":\"workerSubnet\"}"),
+				Spec: machinev1beta1.MachineSetSpec{
+					Template: machinev1beta1.MachineTemplateSpec{
+						ObjectMeta: machinev1beta1.ObjectMeta{
+							Labels: map[string]string{"machine.openshift.io/cluster-api-machine-role": "worker"},
+						},
+						Spec: machinev1beta1.MachineSpec{
+							ProviderSpec: machinev1beta1.ProviderSpec{
+								Value: &kruntime.RawExtension{
+									Raw: []byte("{\"resourceGroup\":\"workerRG\",\"publicIP\":false,\"osDisk\":{\"diskSizeGB\": 1024,\"managedDisk\":{\"storageAccountType\": \"Premium_LRS\"},\"osType\":\"Linux\"},\"image\":{\"offer\": \"aro4\",\"publisher\": \"azureopenshift\", \"resourceID\": \"\", \"sku\": \"aro_43\", \"version\": \"43.81.20200311\"},\"networkResourceGroup\":\"vnet-rg\",\"vnet\":\"vnet\",\"subnet\":\"workerSubnet\"}"),
+								},
+							},
 						},
 					},
 				},
 			}
+
 			if tt.modify != nil {
-				tt.modify(&workerMachine, &masterMachine)
+				tt.modify(&workerMachineSet, &masterMachine)
 			}
 
 			m := kubeManager{
-				client:         fake.NewClientBuilder().WithObjects(&workerMachine, &masterMachine).Build(),
+				client:         fake.NewClientBuilder().WithObjects(&workerMachineSet, &masterMachine).Build(),
 				subscriptionID: subscriptionId,
 			}
 

test/e2e/operator.go

@@ -19,6 +19,7 @@ import (
 	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/ghodss/yaml"
 	configv1 "github.com/openshift/api/config/v1"
+	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	cov1Helpers "github.com/openshift/library-go/pkg/config/clusteroperator/v1helpers"
 	mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
 	"github.com/ugorji/go/codec"
@@ -302,6 +303,7 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
 	var testnsg mgmtnetwork.SecurityGroup
 
 	const nsg = "e2e-nsg"
+	const emptyMachineSet = "e2e-test-machineset"
 
 	gatherNetworkInfo := func(ctx context.Context) {
 		By("gathering vnet name, resource group, location, and adds master/worker subnets to list to reconcile")
@@ -309,8 +311,9 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
 		Expect(err).NotTo(HaveOccurred())
 		location = *oc.Location
 
-		vnet, masterSubnet, err := apisubnet.Split(*oc.OpenShiftClusterProperties.MasterProfile.SubnetID)
+		vnet, masterSubnet, err := apisubnet.Split((*oc.OpenShiftClusterProperties.MasterProfile.SubnetID))
 		Expect(err).NotTo(HaveOccurred())
+
 		_, workerSubnet, err := apisubnet.Split((*(*oc.OpenShiftClusterProperties.WorkerProfiles)[0].SubnetID))
 		Expect(err).NotTo(HaveOccurred())
 
@@ -323,6 +326,32 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
 		Expect(err).NotTo(HaveOccurred())
 		resourceGroup = r.ResourceGroup
 		vnetName = r.ResourceName
+
+		// Store the existing NSGs for the cluster before the test runs, in order to ensure we clean up
+		// after the test finishes, success or failure.
+		// This is expensive but will prevent flakes.
+		By("gathering existing subnet NSGs")
+		for subnet := range subnetsToReconcile {
+			subnetObject, err := clients.Subnet.Get(ctx, resourceGroup, vnetName, subnet, "")
+			Expect(err).NotTo(HaveOccurred())
+
+			subnetsToReconcile[subnet] = subnetObject.NetworkSecurityGroup.ID
+		}
+	}
+
+	cleanUpSubnetNSGs := func(ctx context.Context) {
+		By("cleaning up subnet NSGs")
+		for subnet := range subnetsToReconcile {
+			subnetObject, err := clients.Subnet.Get(ctx, resourceGroup, vnetName, subnet, "")
+			Expect(err).NotTo(HaveOccurred())
+
+			if subnetObject.NetworkSecurityGroup.ID != subnetsToReconcile[subnet] {
+				subnetObject.NetworkSecurityGroup.ID = subnetsToReconcile[subnet]
+
+				err = clients.Subnet.CreateOrUpdateAndWait(ctx, resourceGroup, vnetName, subnet, subnetObject)
+				Expect(err).NotTo(HaveOccurred())
+			}
+		}
 	}
 
 	createE2ENSG := func(ctx context.Context) {
@@ -355,11 +384,20 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
 		createE2ENSG(ctx)
 	})
 	AfterEach(func(ctx context.Context) {
+		cleanUpSubnetNSGs(ctx)
+
 		By("deleting test NSG")
 		err := clients.NetworkSecurityGroups.DeleteAndWait(ctx, resourceGroup, nsg)
 		if err != nil {
 			log.Warn(err)
 		}
+
+		By("deleting test machineset if it still exists")
+		err = clients.MachineAPI.MachineV1beta1().MachineSets("openshift-machine-api").Delete(ctx, emptyMachineSet, metav1.DeleteOptions{})
+		Expect(err).To(SatisfyAny(
+			Not(HaveOccurred()),
+			MatchError(kerrors.IsNotFound),
+		))
 	})
 	It("must reconcile list of subnets when NSG is changed", func(ctx context.Context) {
 		for subnet := range subnetsToReconcile {
@@ -367,13 +405,34 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
 			// Gets current subnet NSG and then updates it to testnsg.
 			subnetObject, err := clients.Subnet.Get(ctx, resourceGroup, vnetName, subnet, "")
 			Expect(err).NotTo(HaveOccurred())
-			// Updates the value to the original NSG in our subnetsToReconcile map
-			subnetsToReconcile[subnet] = subnetObject.NetworkSecurityGroup.ID
+
 			subnetObject.NetworkSecurityGroup = &testnsg
+
 			err = clients.Subnet.CreateOrUpdateAndWait(ctx, resourceGroup, vnetName, subnet, subnetObject)
 			Expect(err).NotTo(HaveOccurred())
 		}
 
+		By("creating an empty MachineSet to force a reconcile")
+		Eventually(func(g Gomega, ctx context.Context) {
+			machinesets, err := clients.MachineAPI.MachineV1beta1().MachineSets("openshift-machine-api").List(ctx, metav1.ListOptions{})
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(machinesets.Items).To(Not(BeEmpty()))
+
+			newMachineSet := machinesets.Items[0].DeepCopy()
+			newMachineSet.Status = machinev1beta1.MachineSetStatus{}
+			newMachineSet.ObjectMeta = metav1.ObjectMeta{
+				Name:        emptyMachineSet,
+				Namespace:   "openshift-machine-api",
+				Annotations: newMachineSet.ObjectMeta.Annotations,
+				Labels:      newMachineSet.ObjectMeta.Labels,
+			}
+			newMachineSet.Name = emptyMachineSet
+			newMachineSet.Spec.Replicas = to.Int32Ptr(0)
+
+			_, err = clients.MachineAPI.MachineV1beta1().MachineSets("openshift-machine-api").Create(ctx, newMachineSet, metav1.CreateOptions{})
+			g.Expect(err).NotTo(HaveOccurred())
+		}).WithContext(ctx).WithTimeout(DefaultEventuallyTimeout).Should(Succeed())
+
 		for subnet, correctNSG := range subnetsToReconcile {
 			By(fmt.Sprintf("waiting for the subnet %q to be reconciled so it includes the original cluster NSG", subnet))
 			Eventually(func(g Gomega, ctx context.Context) {