Split storage account creation and reconciliation steps

tsatam · Nov 2, 2023 · 5e7e1e3 · 5e7e1e3
1 parent 7a36c37
commit 5e7e1e3
Show file tree

Hide file tree

Showing 7 changed files with 350 additions and 68 deletions.
diff --git a/pkg/cluster/deploybaseresources.go b/pkg/cluster/deploybaseresources.go
@@ -140,10 +140,10 @@ func (m *manager) deployBaseResourceTemplate(ctx context.Context) error {
 	}
 
 	resources := []*arm.Resource{
-		m.storageAccount(clusterStorageAccountName, azureRegion, ocpSubnets, true),
+		m.storageAccount(clusterStorageAccountName, azureRegion, ocpSubnets),
 		m.storageAccountBlobContainer(clusterStorageAccountName, "ignition"),
 		m.storageAccountBlobContainer(clusterStorageAccountName, "aro"),
-		m.storageAccount(m.doc.OpenShiftCluster.Properties.ImageRegistryStorageAccountName, azureRegion, ocpSubnets, true),
+		m.storageAccount(m.doc.OpenShiftCluster.Properties.ImageRegistryStorageAccountName, azureRegion, ocpSubnets),
 		m.storageAccountBlobContainer(m.doc.OpenShiftCluster.Properties.ImageRegistryStorageAccountName, "image-registry"),
 		m.clusterNSG(infraID, azureRegion),
 		m.clusterServicePrincipalRBAC(),

diff --git a/pkg/cluster/deploybaseresources_additional.go b/pkg/cluster/deploybaseresources_additional.go
@@ -76,9 +76,57 @@ func (m *manager) clusterServicePrincipalRBAC() *arm.Resource {
 }
 
 // storageAccount will return storage account resource.
-// Legacy storage accounts (public) are not encrypted and cannot be retrofitted.
-// The flag controls this behavior in update/create.
-func (m *manager) storageAccount(name, region string, ocpSubnets []string, encrypted bool) *arm.Resource {
+func (m *manager) storageAccount(name, region string, ocpSubnets []string) *arm.Resource {
+	// shared properties across creation and update
+	properties := m.storageAccountProperties(name, ocpSubnets)
+
+	sa := &mgmtstorage.Account{
+		Kind: mgmtstorage.StorageV2,
+		Sku: &mgmtstorage.Sku{
+			Name: "Standard_LRS",
+		},
+		AccountProperties: &mgmtstorage.AccountProperties{
+			AllowBlobPublicAccess:  properties.AllowBlobPublicAccess,
+			EnableHTTPSTrafficOnly: properties.EnableHTTPSTrafficOnly,
+			MinimumTLSVersion:      properties.MinimumTLSVersion,
+			NetworkRuleSet:         properties.NetworkRuleSet,
+			Encryption: &mgmtstorage.Encryption{
+				RequireInfrastructureEncryption: to.BoolPtr(true),
+				Services: &mgmtstorage.EncryptionServices{
+					Blob: &mgmtstorage.EncryptionService{
+						KeyType: mgmtstorage.KeyTypeAccount,
+						Enabled: to.BoolPtr(true),
+					},
+					File: &mgmtstorage.EncryptionService{
+						KeyType: mgmtstorage.KeyTypeAccount,
+						Enabled: to.BoolPtr(true),
+					},
+					Table: &mgmtstorage.EncryptionService{
+						KeyType: mgmtstorage.KeyTypeAccount,
+						Enabled: to.BoolPtr(true),
+					},
+					Queue: &mgmtstorage.EncryptionService{
+						KeyType: mgmtstorage.KeyTypeAccount,
+						Enabled: to.BoolPtr(true),
+					},
+				},
+				KeySource: mgmtstorage.KeySourceMicrosoftStorage,
+			},
+		},
+		Name:     &name,
+		Location: &region,
+		Type:     to.StringPtr("Microsoft.Storage/storageAccounts"),
+	}
+
+	return &arm.Resource{
+		Resource:   sa,
+		APIVersion: azureclient.APIVersion("Microsoft.Storage"),
+	}
+}
+
+// reconcileStorageAccount will provide all properties we want to enforce on the storage account
+// It is used for both resource creation as well as reconciliation during adminUpdate.
+func (m *manager) storageAccountProperties(storageAccountName string, ocpSubnets []string) *mgmtstorage.AccountPropertiesUpdateParameters {
 	virtualNetworkRules := []mgmtstorage.VirtualNetworkRule{
 		{
 			VirtualNetworkResourceID: to.StringPtr("/subscriptions/" + m.env.SubscriptionID() + "/resourceGroups/" + m.env.ResourceGroup() + "/providers/Microsoft.Network/virtualNetworks/rp-pe-vnet-001/subnets/rp-pe-subnet"),
@@ -101,7 +149,7 @@ func (m *manager) storageAccount(name, region string, ocpSubnets []string, encry
 	// when installing via Hive we need to allow Hive to persist the installConfig graph in the cluster's storage account
 	// TODO: add AKS shard support
 	hiveShard := 1
-	if m.installViaHive && strings.Index(name, "cluster") == 0 {
+	if m.installViaHive && strings.HasPrefix(storageAccountName, "cluster") {
 		virtualNetworkRules = append(virtualNetworkRules, mgmtstorage.VirtualNetworkRule{
 			VirtualNetworkResourceID: to.StringPtr(fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/aks-net/subnets/PodSubnet-%03d", m.env.SubscriptionID(), m.env.ResourceGroup(), hiveShard)),
 			Action:                   mgmtstorage.Allow,
@@ -118,65 +166,24 @@ func (m *manager) storageAccount(name, region string, ocpSubnets []string, encry
 		})
 	}
 
-	sa := &mgmtstorage.Account{
-		Kind: mgmtstorage.StorageV2,
-		Sku: &mgmtstorage.Sku{
-			Name: "Standard_LRS",
-		},
-		AccountProperties: &mgmtstorage.AccountProperties{
-			AllowBlobPublicAccess:  to.BoolPtr(false),
-			EnableHTTPSTrafficOnly: to.BoolPtr(true),
-			MinimumTLSVersion:      mgmtstorage.TLS12,
-			NetworkRuleSet: &mgmtstorage.NetworkRuleSet{
-				Bypass:              mgmtstorage.AzureServices,
-				VirtualNetworkRules: &virtualNetworkRules,
-				DefaultAction:       "Deny",
-			},
-		},
-		Name:     &name,
-		Location: &region,
-		Type:     to.StringPtr("Microsoft.Storage/storageAccounts"),
+	properties := &mgmtstorage.AccountPropertiesUpdateParameters{}
+
+	properties.AllowBlobPublicAccess = to.BoolPtr(false)
+	properties.EnableHTTPSTrafficOnly = to.BoolPtr(true)
+	properties.MinimumTLSVersion = mgmtstorage.TLS12
+	properties.NetworkRuleSet = &mgmtstorage.NetworkRuleSet{
+		Bypass:              mgmtstorage.AzureServices,
+		VirtualNetworkRules: &virtualNetworkRules,
+		DefaultAction:       mgmtstorage.DefaultActionDeny,
 	}
 
 	// In development API calls originates from user laptop so we allow all.
 	// 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 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),
-			Services: &mgmtstorage.EncryptionServices{
-				Blob: &mgmtstorage.EncryptionService{
-					KeyType: mgmtstorage.KeyTypeAccount,
-					Enabled: to.BoolPtr(true),
-				},
-				File: &mgmtstorage.EncryptionService{
-					KeyType: mgmtstorage.KeyTypeAccount,
-					Enabled: to.BoolPtr(true),
-				},
-				Table: &mgmtstorage.EncryptionService{
-					KeyType: mgmtstorage.KeyTypeAccount,
-					Enabled: to.BoolPtr(true),
-				},
-				Queue: &mgmtstorage.EncryptionService{
-					KeyType: mgmtstorage.KeyTypeAccount,
-					Enabled: to.BoolPtr(true),
-				},
-			},
-			KeySource: mgmtstorage.KeySourceMicrosoftStorage,
-		}
+		properties.NetworkRuleSet.DefaultAction = mgmtstorage.DefaultActionAllow
 	}
 
-	return &arm.Resource{
-		Resource:   sa,
-		APIVersion: azureclient.APIVersion("Microsoft.Storage"),
-	}
+	return properties
 }
 
 func (m *manager) storageAccountBlobContainer(storageAccountName, name string) *arm.Resource {

diff --git a/pkg/cluster/deploybaseresources_additional_test.go b/pkg/cluster/deploybaseresources_additional_test.go
@@ -261,7 +261,7 @@ func TestStorageAccount(t *testing.T) {
 				env:            env,
 			}
 
-			got := m.storageAccount(tt.storageAccountName, location, tt.ocpSubnets, true)
+			got := m.storageAccount(tt.storageAccountName, location, tt.ocpSubnets)
 
 			if !reflect.DeepEqual(got, tt.want) {
 				t.Error(cmp.Diff(got, tt.want))

diff --git a/pkg/cluster/storageaccounts.go b/pkg/cluster/storageaccounts.go
@@ -7,10 +7,10 @@ import (
 	"context"
 	"fmt"
 
+	mgmtstorage "github.com/Azure/azure-sdk-for-go/services/storage/mgmt/2019-06-01/storage"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/Azure/ARO-RP/pkg/api"
-	"github.com/Azure/ARO-RP/pkg/util/arm"
 	"github.com/Azure/ARO-RP/pkg/util/stringutils"
 )
 
@@ -26,16 +26,16 @@ func (m *manager) migrateStorageAccounts(ctx context.Context) error {
 	clusterStorageAccountName := "cluster" + m.doc.OpenShiftCluster.Properties.StorageSuffix
 	registryStorageAccountName := m.doc.OpenShiftCluster.Properties.ImageRegistryStorageAccountName
 
-	t := &arm.Template{
-		Schema:         "https://schema.management.azure.com/schemas/2015-01-01/deploymentTemplate.json#",
-		ContentVersion: "1.0.0.0",
-		Resources: []*arm.Resource{
-			m.storageAccount(clusterStorageAccountName, m.doc.OpenShiftCluster.Location, ocpSubnets, false),
-			m.storageAccount(registryStorageAccountName, m.doc.OpenShiftCluster.Location, ocpSubnets, false),
-		},
+	for _, storageAccountName := range []string{clusterStorageAccountName, registryStorageAccountName} {
+		parameters := mgmtstorage.AccountUpdateParameters{
+			AccountPropertiesUpdateParameters: m.storageAccountProperties(storageAccountName, ocpSubnets),
+		}
+		if _, err := m.storage.UpdateAccount(ctx, resourceGroup, storageAccountName, parameters); err != nil {
+			return err
+		}
 	}
 
-	return arm.DeployTemplate(ctx, m.log, m.deployments, resourceGroup, "storage", t, nil)
+	return nil
 }
 
 func (m *manager) populateRegistryStorageAccountName(ctx context.Context) error {