From 631d171dd55e3deb9117149218c8a484366becbd Mon Sep 17 00:00:00 2001
From: Kipp Morris <117932707+kimorris27@users.noreply.github.com>
Date: Tue, 27 Jun 2023 09:28:28 -0500
Subject: [PATCH] Fix storage account controller (#2989)

* Turn `gatewayEnabled` into a shared helper function

* Fix bug in storage account controller

This fix should have been included together with the service endpoint removal implementation.

If the storage account controller tries to add virtual network rules connecting the storage accounts to the cluster subnets, the call will fail complaining that the cluster subnets are missing service endpoints. So we need to make sure we don't add those rules if egress lockdown is enabled.

* Fix name of new test case
---
 .../storageaccounts/storageaccounts.go        | 22 ++++++++++++-------
 .../storageaccounts/storageaccounts_test.go   | 14 ++++++++++++
 .../subnets/subnet_serviceendpoint.go         |  8 ++-----
 pkg/operator/helpers.go                       | 12 ++++++++++
 4 files changed, 42 insertions(+), 14 deletions(-)
 create mode 100644 pkg/operator/helpers.go

diff --git a/pkg/operator/controllers/storageaccounts/storageaccounts.go b/pkg/operator/controllers/storageaccounts/storageaccounts.go
index c44cc37be02..d38ca7ae55b 100644
--- a/pkg/operator/controllers/storageaccounts/storageaccounts.go
+++ b/pkg/operator/controllers/storageaccounts/storageaccounts.go
@@ -13,24 +13,30 @@ import (
 	imageregistryv1 "github.com/openshift/api/imageregistry/v1"
 	"k8s.io/apimachinery/pkg/types"
 
+	"github.com/Azure/ARO-RP/pkg/operator"
 	"github.com/Azure/ARO-RP/pkg/util/stringutils"
 )
 
 func (r *reconcileManager) reconcileAccounts(ctx context.Context) error {
 	resourceGroup := stringutils.LastTokenByte(r.instance.Spec.ClusterResourceGroupID, '/')
 
-	subnets, err := r.kubeSubnets.List(ctx)
-	if err != nil {
-		return err
-	}
-
 	serviceSubnets := r.instance.Spec.ServiceSubnets
-	for _, subnet := range subnets {
-		serviceSubnets = append(serviceSubnets, subnet.ResourceID)
+
+	// Only include the master and worker subnets in the storage accounts' virtual
+	// network rules if egress lockdown is not enabled.
+	if !operator.GatewayEnabled(r.instance) {
+		subnets, err := r.kubeSubnets.List(ctx)
+		if err != nil {
+			return err
+		}
+
+		for _, subnet := range subnets {
+			serviceSubnets = append(serviceSubnets, subnet.ResourceID)
+		}
 	}
 
 	rc := &imageregistryv1.Config{}
-	err = r.client.Get(ctx, types.NamespacedName{Name: "cluster"}, rc)
+	err := r.client.Get(ctx, types.NamespacedName{Name: "cluster"}, rc)
 	if err != nil {
 		return err
 	}
diff --git a/pkg/operator/controllers/storageaccounts/storageaccounts_test.go b/pkg/operator/controllers/storageaccounts/storageaccounts_test.go
index e83acbd2c29..e7550d017e4 100644
--- a/pkg/operator/controllers/storageaccounts/storageaccounts_test.go
+++ b/pkg/operator/controllers/storageaccounts/storageaccounts_test.go
@@ -164,6 +164,20 @@ func TestReconcileManager(t *testing.T) {
 				storage.EXPECT().Update(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, updated)
 			},
 		},
+		{
+			name:         "Operator Flag enabled - nothing to do because egress lockdown is enabled",
+			operatorFlag: true,
+			instance: func(cluster *arov1alpha1.Cluster) {
+				cluster.Spec.GatewayDomains = []string{"somegatewaydomain.com"}
+			},
+			mocks: func(storage *mock_storage.MockAccountsClient, kubeSubnet *mock_subnet.MockKubeManager) {
+				// storage objects in azure
+				result := getValidAccount([]string{})
+
+				storage.EXPECT().GetProperties(gomock.Any(), clusterResourceGroupName, clusterStorageAccountName, gomock.Any()).Return(*result, nil)
+				storage.EXPECT().GetProperties(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, gomock.Any()).Return(*result, nil)
+			},
+		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			controller := gomock.NewController(t)
diff --git a/pkg/operator/controllers/subnets/subnet_serviceendpoint.go b/pkg/operator/controllers/subnets/subnet_serviceendpoint.go
index 7bad0317abe..64823719121 100644
--- a/pkg/operator/controllers/subnets/subnet_serviceendpoint.go
+++ b/pkg/operator/controllers/subnets/subnet_serviceendpoint.go
@@ -12,12 +12,12 @@ import (
 	"github.com/Azure/go-autorest/autorest/to"
 
 	"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/operator"
 	"github.com/Azure/ARO-RP/pkg/util/subnet"
 )
 
 func (r *reconcileManager) ensureSubnetServiceEndpoints(ctx context.Context, s subnet.Subnet) error {
-	if !gatewayEnabled(r.instance) {
+	if !operator.GatewayEnabled(r.instance) {
 		r.log.Debug("Reconciling service endpoints on subnet ", s.ResourceID)
 
 		subnetObject, err := r.subnets.Get(ctx, s.ResourceID)
@@ -66,7 +66,3 @@ func (r *reconcileManager) ensureSubnetServiceEndpoints(ctx context.Context, s s
 	r.log.Debug("Skipping service endpoint reconciliation since egress lockdown is enabled")
 	return nil
 }
-
-func gatewayEnabled(cluster *arov1alpha1.Cluster) bool {
-	return len(cluster.Spec.GatewayDomains) > 0
-}
diff --git a/pkg/operator/helpers.go b/pkg/operator/helpers.go
new file mode 100644
index 00000000000..f8f89dced42
--- /dev/null
+++ b/pkg/operator/helpers.go
@@ -0,0 +1,12 @@
+package operator
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
+)
+
+func GatewayEnabled(cluster *arov1alpha1.Cluster) bool {
+	return len(cluster.Spec.GatewayDomains) > 0
+}