Addressing feedback

FIx
Azure · Feb 23, 2022 · 7b68ab7 · 7b68ab7
1 parent bf5ce15
commit 7b68ab7
Show file tree

Hide file tree

Showing 11 changed files with 101 additions and 113 deletions.
diff --git a/cmd/aro/operator.go b/cmd/aro/operator.go
@@ -101,7 +101,7 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 	if err != nil {
 		return err
 	}
-	imageregistryclient, err := imageregistryclient.NewForConfig(restConfig)
+	imageregistrycli, err := imageregistryclient.NewForConfig(restConfig)
 	if err != nil {
 		return err
 	}
@@ -199,7 +199,7 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 		}
 		if err = (storageaccounts.NewReconciler(
 			log.WithField("controller", controllers.StorageAccountsControllerName),
-			arocli, maocli, kubernetescli, imageregistryclient)).SetupWithManager(mgr); err != nil {
+			arocli, maocli, kubernetescli, imageregistrycli)).SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("unable to create controller %s: %v", controllers.StorageAccountsControllerName, err)
 		}
 	}

diff --git a/pkg/api/defaults.go b/pkg/api/defaults.go
@@ -49,40 +49,25 @@ const FLAG_FALSE string = "false"
 
 // Default flags for new clusters & ones that have not been AdminUpdated
 var DefaultOperatorFlags OperatorFlags = OperatorFlags{
-	"aro.alertwebhook.enabled": FLAG_TRUE,
-
+	"aro.alertwebhook.enabled":                 FLAG_TRUE,
 	"aro.azuresubnets.enabled":                 FLAG_TRUE,
 	"aro.azuresubnets.nsg.managed":             FLAG_TRUE,
 	"aro.azuresubnets.serviceendpoint.managed": FLAG_TRUE,
-
-	"aro.banner.enabled": FLAG_FALSE,
-
-	"aro.checker.enabled": FLAG_TRUE,
-
-	"aro.dnsmasq.enabled": FLAG_TRUE,
-
-	"aro.genevalogging.enabled": FLAG_TRUE,
-
-	"aro.imageconfig.enabled": FLAG_TRUE,
-
-	"aro.machine.enabled": FLAG_TRUE,
-
-	"aro.machineset.enabled": FLAG_TRUE,
-
-	"aro.monitoring.enabled": FLAG_TRUE,
-
-	"aro.nodedrainer.enabled": FLAG_TRUE,
-
-	"aro.pullsecret.enabled": FLAG_TRUE,
-	"aro.pullsecret.managed": FLAG_TRUE,
-
-	"aro.rbac.enabled": FLAG_TRUE,
-
-	"aro.routefix.enabled": FLAG_TRUE,
-
-	"aro.storageaccounts.enabled": FLAG_TRUE,
-
-	"aro.workaround.enabled": FLAG_TRUE,
+	"aro.banner.enabled":                       FLAG_FALSE,
+	"aro.checker.enabled":                      FLAG_TRUE,
+	"aro.dnsmasq.enabled":                      FLAG_TRUE,
+	"aro.genevalogging.enabled":                FLAG_TRUE,
+	"aro.imageconfig.enabled":                  FLAG_TRUE,
+	"aro.machine.enabled":                      FLAG_TRUE,
+	"aro.machineset.enabled":                   FLAG_TRUE,
+	"aro.monitoring.enabled":                   FLAG_TRUE,
+	"aro.nodedrainer.enabled":                  FLAG_TRUE,
+	"aro.pullsecret.enabled":                   FLAG_TRUE,
+	"aro.pullsecret.managed":                   FLAG_TRUE,
+	"aro.rbac.enabled":                         FLAG_TRUE,
+	"aro.routefix.enabled":                     FLAG_TRUE,
+	"aro.storageaccounts.enabled":              FLAG_TRUE,
+	"aro.workaround.enabled":                   FLAG_TRUE,
 }
 
 func (d OperatorFlags) Copy() OperatorFlags {

diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go
@@ -80,16 +80,16 @@ type manager struct {
 	subnet  subnet.Manager
 	graph   graph.Manager
 
-	kubernetescli       kubernetes.Interface
-	extensionscli       extensionsclient.Interface
-	maocli              maoclient.Interface
-	mcocli              mcoclient.Interface
-	operatorcli         operatorclient.Interface
-	configcli           configclient.Interface
-	samplescli          samplesclient.Interface
-	securitycli         securityclient.Interface
-	arocli              aroclient.Interface
-	imageregistryclient imageregistryclient.Interface
+	kubernetescli    kubernetes.Interface
+	extensionscli    extensionsclient.Interface
+	maocli           maoclient.Interface
+	mcocli           mcoclient.Interface
+	operatorcli      operatorclient.Interface
+	configcli        configclient.Interface
+	samplescli       samplesclient.Interface
+	securitycli      securityclient.Interface
+	arocli           aroclient.Interface
+	imageregistrycli imageregistryclient.Interface
 }
 
 // New returns a cluster manager

diff --git a/pkg/cluster/deploystorage_resources.go b/pkg/cluster/deploystorage_resources.go
@@ -99,6 +99,8 @@ func (m *manager) storageAccount(name, region string, encrypted bool) *arm.Resou
 	}
 
 	// Prod includes a gateway rule as well
+	// Once we reach a PLS limit (1000) within a vnet , we may need some refactoring here
+	// https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/azure-subscription-service-limits#private-link-limits
 	if !m.env.IsLocalDevelopmentMode() {
 		virtualNetworkRules = append(virtualNetworkRules, mgmtstorage.VirtualNetworkRule{
 			VirtualNetworkResourceID: to.StringPtr("/subscriptions/" + m.env.SubscriptionID() + "/resourceGroups/" + m.env.GatewayResourceGroup() + "/providers/Microsoft.Network/virtualNetworks/gateway-vnet/subnets/gateway-subnet"),
@@ -127,13 +129,15 @@ func (m *manager) storageAccount(name, region string, encrypted bool) *arm.Resou
 	}
 
 	// In development API calls originates from user laptop so we allow all.
-	// TODO(mjudeikis): Move to development on VPN so we can make this IPRule
+	// TODO: Move to development on VPN so we can make this IPRule.  Will be done as part of Simply secure v2 work
 	if m.env.IsLocalDevelopmentMode() {
 		sa.NetworkRuleSet.DefaultAction = mgmtstorage.DefaultActionAllow
 	}
-	// When migrating storage accounts for old cluster we are not able to change
-	// encryption. For this we have this encryption flag. We not gonna add this
+	// When migrating storage accounts for old clusters we are not able to change
+	// encryption which is why we have this encryption flag. We will not add this
 	// retrospectively to old clusters
+	// If a storage account already has encryption enabled and the encrypted
+	// bool is set to false, it will still maintain the encryption on the storage account.
 	if encrypted {
 		sa.AccountProperties.Encryption = &mgmtstorage.Encryption{
 			RequireInfrastructureEncryption: to.BoolPtr(true),

diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go
@@ -292,7 +292,7 @@ func (m *manager) initializeKubernetesClients(ctx context.Context) error {
 		return err
 	}
 
-	m.imageregistryclient, err = imageregistryclient.NewForConfig(restConfig)
+	m.imageregistrycli, err = imageregistryclient.NewForConfig(restConfig)
 	return err
 }
 

diff --git a/pkg/cluster/storageaccounts.go b/pkg/cluster/storageaccounts.go
@@ -89,7 +89,7 @@ func (m *manager) populateRegistryStorageAccountName(ctx context.Context) error
 		return nil
 	}
 
-	rc, err := m.imageregistryclient.ImageregistryV1().Configs().Get(ctx, "cluster", metav1.GetOptions{})
+	rc, err := m.imageregistrycli.ImageregistryV1().Configs().Get(ctx, "cluster", metav1.GetOptions{})
 	if err != nil {
 		return err
 	}

diff --git a/pkg/operator/controllers/storageaccounts/storageaccount_controller.go b/pkg/operator/controllers/storageaccounts/storageaccount_controller.go
@@ -40,10 +40,10 @@ const (
 type Reconciler struct {
 	log *logrus.Entry
 
-	arocli              aroclient.Interface
-	kubernetescli       kubernetes.Interface
-	maocli              maoclient.Interface
-	imageregistryclient imageregistryclient.Interface
+	arocli           aroclient.Interface
+	kubernetescli    kubernetes.Interface
+	maocli           maoclient.Interface
+	imageregistrycli imageregistryclient.Interface
 }
 
 // reconcileManager is instance of manager instantiated per request
@@ -53,19 +53,19 @@ type reconcileManager struct {
 	instance       *arov1alpha1.Cluster
 	subscriptionID string
 
-	imageregistryclient imageregistryclient.Interface
-	kubeSubnets         subnet.KubeManager
-	storage             storage.AccountsClient
+	imageregistrycli imageregistryclient.Interface
+	kubeSubnets      subnet.KubeManager
+	storage          storage.AccountsClient
 }
 
 // NewReconciler creates a new Reconciler
-func NewReconciler(log *logrus.Entry, arocli aroclient.Interface, maocli maoclient.Interface, kubernetescli kubernetes.Interface, imageregistryclient imageregistryclient.Interface) *Reconciler {
+func NewReconciler(log *logrus.Entry, arocli aroclient.Interface, maocli maoclient.Interface, kubernetescli kubernetes.Interface, imageregistrycli imageregistryclient.Interface) *Reconciler {
 	return &Reconciler{
-		log:                 log,
-		arocli:              arocli,
-		kubernetescli:       kubernetescli,
-		imageregistryclient: imageregistryclient,
-		maocli:              maocli,
+		log:              log,
+		arocli:           arocli,
+		kubernetescli:    kubernetescli,
+		imageregistrycli: imageregistrycli,
+		maocli:           maocli,
 	}
 }
 
@@ -103,9 +103,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
 		instance:       instance,
 		subscriptionID: resource.SubscriptionID,
 
-		imageregistryclient: r.imageregistryclient,
-		kubeSubnets:         subnet.NewKubeManager(r.maocli, resource.SubscriptionID),
-		storage:             storage.NewAccountsClient(&azEnv, resource.SubscriptionID, authorizer),
+		imageregistrycli: r.imageregistrycli,
+		kubeSubnets:      subnet.NewKubeManager(r.maocli, resource.SubscriptionID),
+		storage:          storage.NewAccountsClient(&azEnv, resource.SubscriptionID, authorizer),
 	}
 
 	return reconcile.Result{}, manager.reconcileAccounts(ctx)

diff --git a/pkg/operator/controllers/storageaccounts/storageaccounts.go b/pkg/operator/controllers/storageaccounts/storageaccounts.go
@@ -22,7 +22,7 @@ func (r *reconcileManager) reconcileAccounts(ctx context.Context) error {
 		return err
 	}
 
-	rc, err := r.imageregistryclient.ImageregistryV1().Configs().Get(ctx, "cluster", metav1.GetOptions{})
+	rc, err := r.imageregistrycli.ImageregistryV1().Configs().Get(ctx, "cluster", metav1.GetOptions{})
 	if err != nil {
 		return err
 	}

diff --git a/pkg/operator/controllers/storageaccounts/storageaccounts_test.go b/pkg/operator/controllers/storageaccounts/storageaccounts_test.go
@@ -5,8 +5,8 @@ package storageaccounts
 
 import (
 	"context"
-	"testing"
 	"strconv"
+	"testing"
 
 	mgmtstorage "github.com/Azure/azure-sdk-for-go/services/storage/mgmt/2019-06-01/storage"
 	"github.com/Azure/go-autorest/autorest/to"
@@ -74,15 +74,15 @@ func TestReconcileManager(t *testing.T) {
 	log := logrus.NewEntry(logrus.StandardLogger())
 
 	for _, tt := range []struct {
-		name                string
-		mocks               func(*mock_storage.MockAccountsClient, *mock_subnet.MockKubeManager)
-		imageregistryclient imageregistryclient.Interface
-		instance func(*arov1alpha1.Cluster)
-		operatorFlag  bool
-		wantErr  error
+		name             string
+		mocks            func(*mock_storage.MockAccountsClient, *mock_subnet.MockKubeManager)
+		imageregistrycli imageregistryclient.Interface
+		instance         func(*arov1alpha1.Cluster)
+		operatorFlag     bool
+		wantErr          error
 	}{
 		{
-			name: "Operator Flag enabled - nothing to do",
+			name:         "Operator Flag enabled - nothing to do",
 			operatorFlag: true,
 			mocks: func(storage *mock_storage.MockAccountsClient, kubeSubnet *mock_subnet.MockKubeManager) {
 				// cluster subnets
@@ -103,7 +103,7 @@ func TestReconcileManager(t *testing.T) {
 				storage.EXPECT().GetProperties(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, gomock.Any()).Return(*result, nil)
 
 			},
-			imageregistryclient: imageregistryfake.NewSimpleClientset(
+			imageregistrycli: imageregistryfake.NewSimpleClientset(
 				&imageregistryv1.Config{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "cluster",
@@ -119,7 +119,7 @@ func TestReconcileManager(t *testing.T) {
 			),
 		},
 		{
-			name: "Operator Flag disabled - nothing to do",
+			name:         "Operator Flag disabled - nothing to do",
 			operatorFlag: false,
 			mocks: func(storage *mock_storage.MockAccountsClient, kubeSubnet *mock_subnet.MockKubeManager) {
 				// cluster subnets
@@ -140,7 +140,7 @@ func TestReconcileManager(t *testing.T) {
 				storage.EXPECT().GetProperties(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, gomock.Any()).Return(*result, nil)
 
 			},
-			imageregistryclient: imageregistryfake.NewSimpleClientset(
+			imageregistrycli: imageregistryfake.NewSimpleClientset(
 				&imageregistryv1.Config{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "cluster",
@@ -156,7 +156,7 @@ func TestReconcileManager(t *testing.T) {
 			),
 		},
 		{
-			name: "Operator Flag enabled - all rules to all accounts",
+			name:         "Operator Flag enabled - all rules to all accounts",
 			operatorFlag: true,
 			mocks: func(storage *mock_storage.MockAccountsClient, kubeSubnet *mock_subnet.MockKubeManager) {
 
@@ -195,7 +195,7 @@ func TestReconcileManager(t *testing.T) {
 				storage.EXPECT().GetProperties(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, gomock.Any()).Return(*result, nil)
 				storage.EXPECT().Update(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, gomock.Eq(updated))
 			},
-			imageregistryclient: imageregistryfake.NewSimpleClientset(
+			imageregistrycli: imageregistryfake.NewSimpleClientset(
 				&imageregistryv1.Config{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "cluster",
@@ -228,12 +228,12 @@ func TestReconcileManager(t *testing.T) {
 			}
 
 			r := reconcileManager{
-				log:                 log,
-				instance:            instance,
-				subscriptionID:      subscriptionId,
-				storage:             storage,
-				kubeSubnets:         kubeSubnet,
-				imageregistryclient: tt.imageregistryclient,
+				log:              log,
+				instance:         instance,
+				subscriptionID:   subscriptionId,
+				storage:          storage,
+				kubeSubnets:      kubeSubnet,
+				imageregistrycli: tt.imageregistrycli,
 			}
 
 			err := r.reconcileAccounts(context.Background())