adding NSG controller in ARO Operator

2021-03-31 13:09:25 +02:00 · 2021-03-31 13:09:25 +02:00 · 144606bb00
--- a/cmd/aro/operator.go
+++ b/cmd/aro/operator.go
@ -23,6 +23,7 @@ import (
 	aroclient "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/alertwebhook"
+	"github.com/Azure/ARO-RP/pkg/operator/controllers/azurensg"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/checker"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/clusteroperatoraro"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/dnsmasq"
@ -156,6 +157,11 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 			kubernetescli)).SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("unable to create controller Node: %v", err)
 		}
+		if err = (azurensg.NewReconciler(
+			log.WithField("controller", controllers.AzureNSGControllerName),
+			arocli, maocli, kubernetescli)).SetupWithManager(mgr); err != nil {
+			return fmt.Errorf("unable to create controller AzureNSG: %v", err)
+		}
 	}

 	if err = (checker.NewReconciler(
--- a/pkg/operator/README.md
+++ b/pkg/operator/README.md
@ -25,6 +25,9 @@ There will be use cases where we may want to remediate end user decisions
 automatically. Carrying out remediation locally is advantageous because it is
 likely to be simpler, more reliable, and with a shorter time to remediate.

+Remediations in place:
+* periodically reset NSGs in the master and worker subnets to the defaults (controlled by the reconcileNSGs feature flag)
+
 ### End user warnings

 * [TODO] see https://docs.openshift.com/container-platform/4.4/web_console/customizing-the-web-console.html#creating-custom-notification-banners_customizing-web-console
--- a/pkg/operator/apis/aro.openshift.io/v1alpha1/cluster_types.go
+++ b/pkg/operator/apis/aro.openshift.io/v1alpha1/cluster_types.go
@ -41,22 +41,26 @@ type InternetCheckerSpec struct {
 // ClusterSpec defines the desired state of Cluster
 type ClusterSpec struct {
 	// ResourceID is the Azure resourceId of the cluster
-	ResourceID      string              `json:"resourceId,omitempty"`
-	Domain          string              `json:"domain,omitempty"`
-	ACRDomain       string              `json:"acrDomain,omitempty"`
-	AZEnvironment   string              `json:"azEnvironment,omitempty"`
-	Location        string              `json:"location,omitempty"`
-	GenevaLogging   GenevaLoggingSpec   `json:"genevaLogging,omitempty"`
-	InternetChecker InternetCheckerSpec `json:"internetChecker,omitempty"`
-	VnetID          string              `json:"vnetId,omitempty"`
-	APIIntIP        string              `json:"apiIntIP,omitempty"`
-	IngressIP       string              `json:"ingressIP,omitempty"`
+	ResourceID          string              `json:"resourceId,omitempty"`
+	Domain              string              `json:"domain,omitempty"`
+	ACRDomain           string              `json:"acrDomain,omitempty"`
+	AZEnvironment       string              `json:"azEnvironment,omitempty"`
+	Location            string              `json:"location,omitempty"`
+	InfraID             string              `json:"infraId,omitempty"`
+	ArchitectureVersion int                 `json:"architectureVersion,omitempty"`
+	GenevaLogging       GenevaLoggingSpec   `json:"genevaLogging,omitempty"`
+	InternetChecker     InternetCheckerSpec `json:"internetChecker,omitempty"`
+	VnetID              string              `json:"vnetId,omitempty"`
+	APIIntIP            string              `json:"apiIntIP,omitempty"`
+	IngressIP           string              `json:"ingressIP,omitempty"`

 	Features FeaturesSpec `json:"features,omitempty"`
 }

 // FeaturesSpec defines ARO operator feature gates
-type FeaturesSpec struct{}
+type FeaturesSpec struct {
+	ReconcileNSGs bool `json:"reconcileNSGs,omitempty"`
+}

 // ClusterStatus defines the observed state of Cluster
 type ClusterStatus struct {
--- a/pkg/operator/controllers/azurensg/azurensg_controller.go
+++ b/pkg/operator/controllers/azurensg/azurensg_controller.go
@ -0,0 +1,103 @@
+package azurensg
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+
+	"github.com/Azure/go-autorest/autorest/azure"
+	machinev1beta1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
+	maoclient "github.com/openshift/machine-api-operator/pkg/generated/clientset/versioned"
+	"github.com/sirupsen/logrus"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/builder"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/handler"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+	"sigs.k8s.io/controller-runtime/pkg/source"
+
+	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
+	aroclient "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned"
+	"github.com/Azure/ARO-RP/pkg/operator/controllers"
+	"github.com/Azure/ARO-RP/pkg/util/aad"
+	"github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/network"
+	"github.com/Azure/ARO-RP/pkg/util/clusterauthorizer"
+)
+
+// AzureNSGReconciler is the controller struct
+type AzureNSGReconciler struct {
+	arocli        aroclient.Interface
+	maocli        maoclient.Interface
+	kubernetescli kubernetes.Interface
+	log           *logrus.Entry
+}
+
+// NewReconciler creates a new Reconciler
+func NewReconciler(log *logrus.Entry, arocli aroclient.Interface, maocli maoclient.Interface, kubernetescli kubernetes.Interface) *AzureNSGReconciler {
+	return &AzureNSGReconciler{
+		arocli:        arocli,
+		maocli:        maocli,
+		kubernetescli: kubernetescli,
+		log:           log,
+	}
+}
+
+//Reconcile fixes the Network Security Groups
+func (r *AzureNSGReconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) {
+	instance, err := r.arocli.AroV1alpha1().Clusters().Get(ctx, arov1alpha1.SingletonClusterName, metav1.GetOptions{})
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+
+	if !instance.Spec.Features.ReconcileNSGs {
+		// reconciling NSGs is disabled
+		return reconcile.Result{}, nil
+	}
+
+	// Get endpoints from operator
+	azEnv, err := azure.EnvironmentFromName(instance.Spec.AZEnvironment)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+	// Grab azure-credentials from secret
+	credentials, err := clusterauthorizer.AzCredentials(ctx, r.kubernetescli)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+	resource, err := azure.ParseResourceID(instance.Spec.ResourceID)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+	// create service principal token from azure-credentials
+	token, err := aad.GetToken(ctx, r.log, string(credentials.ClientID), string(credentials.ClientSecret), string(credentials.TenantID), azEnv.ActiveDirectoryEndpoint, azEnv.ResourceManagerEndpoint)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+	// create refreshable authorizer from token
+	authorizer, err := clusterauthorizer.NewAzRefreshableAuthorizer(token)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+	subnetsClient := network.NewSubnetsClient(&azEnv, resource.SubscriptionID, authorizer)
+
+	return reconcile.Result{}, r.reconcileSubnetNSG(ctx, instance, resource.SubscriptionID, subnetsClient)
+}
+
+// SetupWithManager creates the controller
+func (r *AzureNSGReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	aroClusterPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool {
+		return o.GetName() == arov1alpha1.SingletonClusterName
+	})
+
+	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
+		Named(controllers.AzureNSGControllerName).
+		Complete(r)
+}
--- a/pkg/operator/controllers/azurensg/const.go
+++ b/pkg/operator/controllers/azurensg/const.go
@ -0,0 +1,8 @@
+package azurensg
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+const (
+	machineSetsNamespace = "openshift-machine-api"
+)
--- a/pkg/operator/controllers/azurensg/subnetnsg.go
+++ b/pkg/operator/controllers/azurensg/subnetnsg.go
@ -0,0 +1,116 @@
+package azurensg
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"strings"
+
+	mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-07-01/network"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	azureproviderv1beta1 "sigs.k8s.io/cluster-api-provider-azure/pkg/apis/azureprovider/v1beta1"
+
+	"github.com/Azure/ARO-RP/pkg/api"
+	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
+	"github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/network"
+	"github.com/Azure/ARO-RP/pkg/util/subnet"
+)
+
+type subnetDescriptor struct {
+	resourceGroup string
+	vnetName      string
+	subnetName    string
+}
+
+func (r *AzureNSGReconciler) reconcileSubnetNSG(ctx context.Context, instance *arov1alpha1.Cluster, subscriptionID string, subnetsClient network.SubnetsClient) error {
+	// the main logic starts here
+	subnets, masterResourceGroup, err := r.getSubnets(ctx)
+	if err != nil {
+		return err
+	}
+	for ws := range subnets {
+		err = r.ensureSubnetNSG(ctx, subnetsClient, subscriptionID, masterResourceGroup, instance.Spec.InfraID, api.ArchitectureVersion(instance.Spec.ArchitectureVersion), ws.resourceGroup, ws.vnetName, ws.subnetName, subnets[ws])
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (r *AzureNSGReconciler) ensureSubnetNSG(ctx context.Context, subnetsClient network.SubnetsClient, subscriptionID, resourcesResourceGroup, infraID string, architectureVersion api.ArchitectureVersion, vnetResourceGroup, vnetName, subnetName string, isWorkerSubnet bool) error {
+	subnetObject, err := subnetsClient.Get(ctx, vnetResourceGroup, vnetName, subnetName, "")
+	if err != nil {
+		return err
+	}
+	if subnetObject.SubnetPropertiesFormat == nil || subnetObject.SubnetPropertiesFormat.NetworkSecurityGroup == nil {
+		return fmt.Errorf("received nil, expected a value in SubnetProperties when trying to Get subnet %s/%s in resource group %s", vnetName, subnetName, vnetResourceGroup)
+	}
+	correctNSGResourceID, err := subnet.NetworkSecurityGroupIDExpanded(architectureVersion, resourcesResourceGroup, infraID, isWorkerSubnet)
+	if err != nil {
+		return err
+	}
+	correctNSGResourceID = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", subscriptionID, correctNSGResourceID)
+
+	if !strings.EqualFold(*subnetObject.NetworkSecurityGroup.ID, correctNSGResourceID) {
+		r.log.Infof("Fixing NSG from %s to %s", *subnetObject.NetworkSecurityGroup.ID, correctNSGResourceID)
+		// NSG doesn't match - fixing
+		subnetObject.NetworkSecurityGroup = &mgmtnetwork.SecurityGroup{ID: &correctNSGResourceID}
+		err = subnetsClient.CreateOrUpdateAndWait(ctx, vnetResourceGroup, vnetName, subnetName, subnetObject)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (r *AzureNSGReconciler) getSubnets(ctx context.Context) (map[subnetDescriptor]bool, string, error) {
+	subnetMap := make(map[subnetDescriptor]bool) // bool is true for master subnets
+	var masterResourceGroup *string
+	// select all workers by the  machine.openshift.io/cluster-api-machine-role: not equal to master Label
+	machines, err := r.maocli.MachineV1beta1().Machines(machineSetsNamespace).List(ctx, metav1.ListOptions{LabelSelector: "machine.openshift.io/cluster-api-machine-role!=master"})
+	if err != nil {
+		return nil, "", err
+	}
+
+	for _, machine := range machines.Items {
+		_, subnetDesc, err := r.getDescriptorFromProviderSpec(machine.Spec.ProviderSpec.Value)
+		if err != nil {
+			return nil, "", err
+		}
+
+		subnetMap[*subnetDesc] = false
+	}
+	machines, err = r.maocli.MachineV1beta1().Machines(machineSetsNamespace).List(ctx, metav1.ListOptions{LabelSelector: "machine.openshift.io/cluster-api-machine-role=master"})
+	if err != nil {
+		return nil, "", err
+	}
+	for _, machine := range machines.Items {
+		var subnetDesc *subnetDescriptor // declared here due to := rescoping of the masterResourceGroup variable below
+		masterResourceGroup, subnetDesc, err = r.getDescriptorFromProviderSpec(machine.Spec.ProviderSpec.Value)
+		if err != nil {
+			return nil, "", err
+		}
+		subnetMap[*subnetDesc] = true
+	}
+	if masterResourceGroup == nil {
+		return nil, "", fmt.Errorf("master resource group not found")
+	}
+	return subnetMap, *masterResourceGroup, nil
+}
+
+func (r *AzureNSGReconciler) getDescriptorFromProviderSpec(providerSpec *runtime.RawExtension) (*string, *subnetDescriptor, error) {
+	var spec azureproviderv1beta1.AzureMachineProviderSpec
+	err := json.Unmarshal(providerSpec.Raw, &spec)
+	if err != nil {
+		return nil, nil, err
+	}
+	return &spec.ResourceGroup, &subnetDescriptor{
+		resourceGroup: spec.NetworkResourceGroup,
+		vnetName:      spec.Vnet,
+		subnetName:    spec.Subnet,
+	}, nil
+}
--- a/pkg/operator/controllers/azurensg/subnetnsg_test.go
+++ b/pkg/operator/controllers/azurensg/subnetnsg_test.go
@ -0,0 +1,219 @@
+package azurensg
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"fmt"
+	"strings"
+	"testing"
+
+	mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-07-01/network"
+	"github.com/Azure/go-autorest/autorest/azure"
+	"github.com/Azure/go-autorest/autorest/to"
+	"github.com/golang/mock/gomock"
+	machinev1beta1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
+	maofake "github.com/openshift/machine-api-operator/pkg/generated/clientset/versioned/fake"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+
+	"github.com/Azure/ARO-RP/pkg/api"
+	utillog "github.com/Azure/ARO-RP/pkg/util/log"
+	mock_network "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/network"
+)
+
+var (
+	vnet = azure.Resource{
+		SubscriptionID: "0000000-0000-0000-0000-000000000000",
+		ResourceGroup:  "vnet-rg",
+		ResourceName:   "vnet-name",
+	}
+)
+
+func TestEnsureSubnetNSG(t *testing.T) {
+	r := AzureNSGReconciler{log: utillog.GetLogger()}
+	for _, tt := range []struct {
+		name                string
+		nsgname             string
+		architectureVersion api.ArchitectureVersion
+		expectedValue       string
+		modifySubnet        func(*mgmtnetwork.Subnet)
+		expectedErr         error
+		expectUpdate        bool
+	}{
+		{
+			name:                "mismatched NSG in Architecture v1",
+			nsgname:             "someotherv1-nsg",
+			architectureVersion: api.ArchitectureVersionV1,
+			expectedValue:       "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/resourcegroup/providers/Microsoft.Network/networkSecurityGroups/infraid-node-nsg",
+			expectUpdate:        true,
+		},
+		{
+			name:                "correct NSG in Architecture v2",
+			nsgname:             "infraid-nsg",
+			architectureVersion: api.ArchitectureVersionV2,
+			expectedValue:       "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/resourcegroup/providers/Microsoft.Network/networkSecurityGroups/infraid-nsg",
+			expectUpdate:        false,
+		},
+		{
+			name:                "mismatched NSG in Architecture v2",
+			nsgname:             "someotherv2-nsg",
+			architectureVersion: api.ArchitectureVersionV2,
+			expectedValue:       "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/resourcegroup/providers/Microsoft.Network/networkSecurityGroups/infraid-nsg",
+			expectUpdate:        true,
+		},
+		{
+			name:                "missing fields",
+			nsgname:             "someotherv2-nsg",
+			architectureVersion: api.ArchitectureVersionV2,
+			expectedValue:       "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/resourcegroup/providers/Microsoft.Network/networkSecurityGroups/infraid-nsg",
+			modifySubnet: func(subnet *mgmtnetwork.Subnet) {
+				subnet.SubnetPropertiesFormat = nil
+			},
+			expectedErr:  fmt.Errorf("received nil, expected a value in SubnetProperties when trying to Get subnet vnet-name/subnet in resource group vnet-rg"),
+			expectUpdate: false,
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			controller := gomock.NewController(t)
+			defer controller.Finish()
+			fakeSubnet := mgmtnetwork.Subnet{SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{
+				NetworkSecurityGroup: &mgmtnetwork.SecurityGroup{
+					ID: to.StringPtr("/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/resourcegroup/providers/Microsoft.Network/networkSecurityGroups/" + tt.nsgname),
+				},
+			}}
+			if tt.modifySubnet != nil {
+				tt.modifySubnet(&fakeSubnet)
+			}
+			subnetsClient := mock_network.NewMockSubnetsClient(controller)
+			subnetsClient.EXPECT().Get(context.Background(), "vnet-rg", "vnet-name", "subnet", "").
+				Return(fakeSubnet, nil)
+			if tt.expectUpdate {
+				subnetsClient.EXPECT().CreateOrUpdateAndWait(context.Background(), "vnet-rg", "vnet-name", "subnet", fakeSubnet).
+					Return(nil)
+			}
+			err := r.ensureSubnetNSG(context.Background(), subnetsClient, vnet.SubscriptionID, "resourcegroup", "infraid", tt.architectureVersion, vnet.ResourceGroup, vnet.ResourceName, "subnet", true)
+			if err != nil {
+				if tt.expectedErr == nil {
+					t.Fatal(err)
+				}
+				if !strings.EqualFold(tt.expectedErr.Error(), err.Error()) {
+					t.Errorf("Expected Error %s, got %s when processing %s testcase", tt.expectedErr.Error(), err.Error(), tt.name)
+				}
+				return
+			}
+			if !strings.EqualFold(tt.expectedValue, *fakeSubnet.SubnetPropertiesFormat.NetworkSecurityGroup.ID) {
+				t.Errorf("Expected NSG ID %s, got %s when processing %s testcase", tt.expectedValue, *fakeSubnet.SubnetPropertiesFormat.NetworkSecurityGroup.ID, tt.name)
+			}
+		})
+	}
+}
+
+func TestGetSubnets(t *testing.T) {
+	r := AzureNSGReconciler{log: utillog.GetLogger()}
+	for _, tt := range []struct {
+		name             string
+		machinelabel     string
+		expectedMap      map[subnetDescriptor]bool
+		expectedMasterRG string
+		modify           func(*machinev1beta1.Machine, *machinev1beta1.Machine)
+		expectedErr      error
+	}{
+		{
+			name: "main path",
+			expectedMap: map[subnetDescriptor]bool{
+				{
+					resourceGroup: "netRG",
+					vnetName:      "workerVnet",
+					subnetName:    "workerSubnet",
+				}: false,
+				{
+					resourceGroup: "netRG",
+					vnetName:      "masterVnet",
+					subnetName:    "masterSubnet",
+				}: true,
+			},
+			expectedMasterRG: "masterRG",
+			modify:           func(worker *machinev1beta1.Machine, master *machinev1beta1.Machine) {},
+		},
+		{
+			name:             "missing providerSpec",
+			expectedMap:      nil,
+			expectedMasterRG: "",
+			modify: func(worker *machinev1beta1.Machine, master *machinev1beta1.Machine) {
+				master.Spec.ProviderSpec.Value.Raw = []byte("")
+			},
+			expectedErr: fmt.Errorf("unexpected end of JSON input"),
+		},
+		{
+			name:             "missing master nodes",
+			expectedMap:      nil,
+			expectedMasterRG: "masterRG",
+			modify: func(worker *machinev1beta1.Machine, master *machinev1beta1.Machine) {
+				master.Labels = map[string]string{}
+			},
+			expectedErr: fmt.Errorf("master resource group not found"),
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			masterMachine := machinev1beta1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "master-0",
+					Namespace: "openshift-machine-api",
+					Labels:    map[string]string{"machine.openshift.io/cluster-api-machine-role": "master"},
+				},
+				Spec: machinev1beta1.MachineSpec{
+					ProviderSpec: machinev1beta1.ProviderSpec{
+						Value: &runtime.RawExtension{
+							Raw: []byte("{\"resourceGroup\":\"masterRG\",\"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\":\"netRG\",\"vnet\":\"masterVnet\",\"subnet\":\"masterSubnet\"}"),
+						},
+					},
+				},
+			}
+			workerMachine := machinev1beta1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "worker-0",
+					Namespace: "openshift-machine-api",
+					Labels:    map[string]string{"machine.openshift.io/cluster-api-machine-role": "worker"},
+				},
+				Spec: machinev1beta1.MachineSpec{
+					ProviderSpec: machinev1beta1.ProviderSpec{
+						Value: &runtime.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\":\"netRG\",\"vnet\":\"workerVnet\",\"subnet\":\"workerSubnet\"}"),
+						},
+					},
+				},
+			}
+			tt.modify(&workerMachine, &masterMachine)
+			r.maocli = maofake.NewSimpleClientset(&workerMachine, &masterMachine)
+			subnetMap, masterRG, err := r.getSubnets(context.Background())
+			if err != nil {
+				if tt.expectedErr == nil {
+					t.Fatal(err)
+				}
+				if !strings.EqualFold(err.Error(), tt.expectedErr.Error()) {
+					t.Errorf("Expected Error %s, got %s when processing %s testcase", tt.expectedErr.Error(), err.Error(), tt.name)
+				}
+				return
+			}
+			if !strings.EqualFold(tt.expectedMasterRG, masterRG) {
+				t.Errorf("Expected Master Resource Group %s, got %s when processing %s testcase", tt.expectedMasterRG, masterRG, tt.name)
+			}
+			if tt.expectedMap != nil {
+				if len(tt.expectedMap) != len(subnetMap) {
+					t.Errorf("Expected Map length %d, doesn't match result map length %d when processing %s testcase", len(tt.expectedMap), len(subnetMap), tt.name)
+				}
+				for subnet := range tt.expectedMap {
+					value, present := subnetMap[subnet]
+					if !present {
+						t.Errorf("Subnet %s, %s, %s expected but not present in result when processing %s testcase", subnet.resourceGroup, subnet.vnetName, subnet.subnetName, tt.name)
+					}
+					if tt.expectedMap[subnet] != value {
+						t.Errorf("Value of isMaster boolean doesn't match for subnet %s, %s, %s when processing %s testcase", subnet.resourceGroup, subnet.vnetName, subnet.subnetName, tt.name)
+					}
+				}
+			}
+		})
+	}
+}
--- a/pkg/operator/controllers/checker/const.go
+++ b/pkg/operator/controllers/checker/const.go
@ -4,7 +4,5 @@ package checker
 // Licensed under the Apache License 2.0.

 const (
-	azureCredentialSecretNamespace = "kube-system"
-	azureCredentialSecretName      = "azure-credentials"
-	machineSetsNamespace           = "openshift-machine-api"
+	machineSetsNamespace = "openshift-machine-api"
 )
--- a/pkg/operator/controllers/checker/serviceprincipalchecker.go
+++ b/pkg/operator/controllers/checker/serviceprincipalchecker.go
@ -20,6 +20,7 @@ import (
 	aroclient "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers"
 	"github.com/Azure/ARO-RP/pkg/util/aad"
+	"github.com/Azure/ARO-RP/pkg/util/clusterauthorizer"
 )

 type ServicePrincipalChecker struct {
@ -67,12 +68,12 @@ func (r *ServicePrincipalChecker) Check(ctx context.Context) error {
 		return err
 	}

-	azCred, err := azCredentials(ctx, r.kubernetescli)
+	azCred, err := clusterauthorizer.AzCredentials(ctx, r.kubernetescli)
 	if err != nil {
 		return err
 	}

-	_, err = aad.GetToken(ctx, r.log, azCred.clientID, azCred.clientSecret, azCred.tenantID, azEnv.ActiveDirectoryEndpoint, azEnv.ResourceManagerEndpoint)
+	_, err = aad.GetToken(ctx, r.log, string(azCred.ClientID), string(azCred.ClientSecret), string(azCred.TenantID), azEnv.ActiveDirectoryEndpoint, azEnv.ResourceManagerEndpoint)
 	if err != nil {
 		updateFailedCondition(cond, err)
 	}
@ -82,7 +83,7 @@ func (r *ServicePrincipalChecker) Check(ctx context.Context) error {
 		return err
 	}

-	err = spDynamic.ValidateServicePrincipal(ctx, azCred.clientID, azCred.clientSecret, azCred.tenantID)
+	err = spDynamic.ValidateServicePrincipal(ctx, string(azCred.ClientID), string(azCred.ClientSecret), string(azCred.TenantID))
 	if err != nil {
 		updateFailedCondition(cond, err)
 	}
@ -90,27 +91,6 @@ func (r *ServicePrincipalChecker) Check(ctx context.Context) error {
 	return controllers.SetCondition(ctx, r.arocli, cond, r.role)
 }

-type credentials struct {
-	clientID     string
-	clientSecret string
-	tenantID     string
-}
-
-func azCredentials(ctx context.Context, kubernetescli kubernetes.Interface) (*credentials, error) {
-	var creds credentials
-
-	mysec, err := kubernetescli.CoreV1().Secrets(azureCredentialSecretNamespace).Get(ctx, azureCredentialSecretName, metav1.GetOptions{})
-	if err != nil {
-		return nil, err
-	}
-
-	creds.clientID = string(mysec.Data["azure_client_id"])
-	creds.clientSecret = string(mysec.Data["azure_client_secret"])
-	creds.tenantID = string(mysec.Data["azure_tenant_id"])
-
-	return &creds, nil
-}
-
 func updateFailedCondition(cond *status.Condition, err error) {
 	cond.Status = corev1.ConditionFalse
 	if tErr, ok := err.(*api.CloudError); ok {
--- a/pkg/operator/controllers/const.go
+++ b/pkg/operator/controllers/const.go
@ -5,6 +5,7 @@ package controllers

 const (
 	AlertwebhookControllerName             = "Alertwebhook"
+	AzureNSGControllerName                 = "AzureNSG"
 	ClusterOperatorAROName                 = "ClusterOperatorARO"
 	GenevaLoggingControllerName            = "GenevaLogging"
 	PullSecretControllerName               = "PullSecret"
--- a/pkg/operator/deploy/bindata.go
+++ b/pkg/operator/deploy/bindata.go
--- a/pkg/operator/deploy/deploy.go
+++ b/pkg/operator/deploy/deploy.go
@ -168,12 +168,14 @@ func (o *operator) resources() ([]runtime.Object, error) {
 				Name: arov1alpha1.SingletonClusterName,
 			},
 			Spec: arov1alpha1.ClusterSpec{
-				ResourceID:    o.oc.ID,
-				Domain:        domain,
-				ACRDomain:     o.env.ACRDomain(),
-				AZEnvironment: o.env.Environment().Name,
-				Location:      o.env.Location(),
-				VnetID:        vnetID,
+				ResourceID:          o.oc.ID,
+				Domain:              domain,
+				ACRDomain:           o.env.ACRDomain(),
+				AZEnvironment:       o.env.Environment().Name,
+				Location:            o.env.Location(),
+				InfraID:             o.oc.Properties.InfraID,
+				ArchitectureVersion: int(o.oc.Properties.ArchitectureVersion),
+				VnetID:              vnetID,
 				GenevaLogging: arov1alpha1.GenevaLoggingSpec{
 					ConfigVersion:            o.env.ClusterGenevaLoggingConfigVersion(),
 					MonitoringGCSEnvironment: o.env.ClusterGenevaLoggingEnvironment(),
--- a/pkg/operator/deploy/staticresources/aro.openshift.io_clusters.yaml
+++ b/pkg/operator/deploy/staticresources/aro.openshift.io_clusters.yaml
@ -40,12 +40,17 @@ spec:
                type: string
              apiIntIP:
                type: string
+              architectureVersion:
+                type: integer
              azEnvironment:
                type: string
              domain:
                type: string
              features:
                description: FeaturesSpec defines ARO operator feature gates
+                properties:
+                  reconcileNSGs:
+                    type: boolean
                type: object
              genevaLogging:
                properties:
@ -58,6 +63,8 @@ spec:
                    - Test
                    type: string
                type: object
+              infraId:
+                type: string
              ingressIP:
                type: string
              internetChecker:
--- a/pkg/util/clusterauthorizer/authorizer.go
+++ b/pkg/util/clusterauthorizer/authorizer.go
@ -0,0 +1,55 @@
+package clusterauthorizer
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/Azure/go-autorest/autorest/adal"
+	"github.com/form3tech-oss/jwt-go"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes"
+
+	"github.com/Azure/ARO-RP/pkg/util/azureclaim"
+	"github.com/Azure/ARO-RP/pkg/util/refreshable"
+)
+
+type Credentials struct {
+	ClientID     []byte
+	ClientSecret []byte
+	TenantID     []byte
+}
+
+// NewAzRefreshableAuthorizer returns a new refreshable authorizer based on an auth token (see GetToken in /pkg/util/aad)
+func NewAzRefreshableAuthorizer(token *adal.ServicePrincipalToken) (refreshable.Authorizer, error) {
+	p := &jwt.Parser{}
+	c := &azureclaim.AzureClaim{}
+	_, _, err := p.ParseUnverified(token.OAuthToken(), c)
+	if err != nil {
+		return nil, err
+	}
+
+	return refreshable.NewAuthorizer(token), nil
+}
+
+// AzCredentials gets Cluster Service Principal credentials from the Kubernetes secrets
+func AzCredentials(ctx context.Context, kubernetescli kubernetes.Interface) (*Credentials, error) {
+	mysec, err := kubernetescli.CoreV1().Secrets(azureCredentialSecretNameSpace).Get(ctx, azureCredentialSecretName, metav1.GetOptions{})
+	if err != nil {
+		return nil, err
+	}
+
+	for _, key := range []string{"azure_client_id", "azure_client_secret", "azure_tenant_id"} {
+		if _, ok := mysec.Data[key]; !ok {
+			return nil, fmt.Errorf("%s does not exist in the secret", key)
+		}
+	}
+
+	return &Credentials{
+		ClientID:     mysec.Data["azure_client_id"],
+		ClientSecret: mysec.Data["azure_client_secret"],
+		TenantID:     mysec.Data["azure_tenant_id"],
+	}, nil
+}
--- a/pkg/util/clusterauthorizer/const.go
+++ b/pkg/util/clusterauthorizer/const.go
@ -0,0 +1,9 @@
+package clusterauthorizer
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+const (
+	azureCredentialSecretName      = "azure-credentials"
+	azureCredentialSecretNameSpace = "kube-system"
+)
--- a/pkg/util/subnet/subnet.go
+++ b/pkg/util/subnet/subnet.go
@ -132,28 +132,35 @@ func NetworkSecurityGroupID(oc *api.OpenShiftCluster, subnetID string) (string,
 	if infraID == "" {
 		infraID = "aro"
 	}
-
-	switch oc.Properties.ArchitectureVersion {
-	case api.ArchitectureVersionV1:
-		return networkSecurityGroupIDV1(oc, subnetID, infraID), nil
-	case api.ArchitectureVersionV2:
-		return networkSecurityGroupIDV2(oc, subnetID, infraID), nil
-	default:
-		return "", fmt.Errorf("unknown architecture version %d", oc.Properties.ArchitectureVersion)
-	}
-
-}
-
-func networkSecurityGroupIDV1(oc *api.OpenShiftCluster, subnetID, infraID string) string {
+	isWorkerSubnet := false
 	for _, s := range oc.Properties.WorkerProfiles {
 		if strings.EqualFold(subnetID, s.SubnetID) {
-			return oc.Properties.ClusterProfile.ResourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/" + infraID + NSGNodeSuffixV1
+			isWorkerSubnet = true
+			break
 		}
-
 	}
-	return oc.Properties.ClusterProfile.ResourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/" + infraID + NSGControlPlaneSuffixV1
+	return NetworkSecurityGroupIDExpanded(oc.Properties.ArchitectureVersion, oc.Properties.ClusterProfile.ResourceGroupID, infraID, isWorkerSubnet)
 }

-func networkSecurityGroupIDV2(oc *api.OpenShiftCluster, subnetID, infraID string) string {
-	return oc.Properties.ClusterProfile.ResourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/" + infraID + NSGSuffixV2
+// NetworkSecurityGroupIDExpanded returns the NetworkSecurityGroup ID for a given subnetID, without the OpenShift Cluster document
+func NetworkSecurityGroupIDExpanded(architectureVersion api.ArchitectureVersion, resourceGroupID, infraID string, isWorkerSubnet bool) (string, error) {
+	switch architectureVersion {
+	case api.ArchitectureVersionV1:
+		return networkSecurityGroupIDV1(resourceGroupID, infraID, isWorkerSubnet), nil
+	case api.ArchitectureVersionV2:
+		return networkSecurityGroupIDV2(resourceGroupID, infraID), nil
+	default:
+		return "", fmt.Errorf("unknown architecture version %d", architectureVersion)
+	}
+}
+
+func networkSecurityGroupIDV1(resourceGroupID, infraID string, isWorkerSubnet bool) string {
+	if isWorkerSubnet {
+		return resourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/" + infraID + NSGNodeSuffixV1
+	}
+	return resourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/" + infraID + NSGControlPlaneSuffixV1
+}
+
+func networkSecurityGroupIDV2(resourceGroupID, infraID string) string {
+	return resourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/" + infraID + NSGSuffixV2
 }