pkg/storage/azure: Add UserTags while creating Azure Storage Account

Author: chiragkyal

pkg/storage/azure/azure.go

@@ -154,7 +154,7 @@ func (d *driver) accountExists(storageAccountsClient storage.AccountsClient, acc
 	)
 }
 
-func (d *driver) createStorageAccount(storageAccountsClient storage.AccountsClient, resourceGroupName, accountName, location, cloudName string) error {
+func (d *driver) createStorageAccount(storageAccountsClient storage.AccountsClient, resourceGroupName, accountName, location, cloudName string, tagset map[string]*string) error {
 	klog.Infof("attempt to create azure storage account %s (resourceGroup=%q, location=%q)...", accountName, resourceGroupName, location)
 
 	kind := storage.StorageV2
@@ -181,6 +181,7 @@ func (d *driver) createStorageAccount(storageAccountsClient storage.AccountsClie
 				Name: storage.StandardLRS,
 			},
 			AccountPropertiesCreateParameters: params,
+			Tags:                              tagset,
 		},
 	)
 	if err != nil {
@@ -485,9 +486,9 @@ func (d *driver) StorageChanged(cr *imageregistryv1.Config) bool {
 	return !reflect.DeepEqual(cr.Status.Storage.Azure, cr.Spec.Storage.Azure)
 }
 
-// assureStorageAccount makes sure there is a storage account in place. If no storage account name
-// is provided it attempts to generate one. Returns the account name (either the one provided or
-// the one generated), if the account was created or was already there and an error.
+// assureStorageAccount makes sure there is a storage account in place and apply any provided tags.
+// If no storage account name is provided it attempts to generate one. Returns the account name
+// (either the one provided or the one generated), if the account was created or was already there and an error.
 func (d *driver) assureStorageAccount(cfg *Azure, infra *configv1.Infrastructure) (string, bool, error) {
 	environment, err := getEnvironmentByName(d.Config.CloudName)
 	if err != nil {
@@ -516,13 +517,33 @@ func (d *driver) assureStorageAccount(cfg *Azure, infra *configv1.Infrastructure
 		return "", false, fmt.Errorf("create storage account failed, name not available")
 	}
 
+	// Tag the storage account with the openshiftClusterID
+	// along with any user defined tags from the cluster configuration
+	klog.V(2).Info("setting azure storage account tags")
+
+	tagset := map[string]*string{
+		fmt.Sprintf("kubernetes.io_cluster.%s", infra.Status.InfrastructureName): to.StringPtr("owned"),
+	}
+
+	// at this stage we are not keeping user tags in sync. as per enhancement proposal
+	// we only set user provided tags when we created the bucket.
+	hasAzureStatus := infra.Status.PlatformStatus != nil && infra.Status.PlatformStatus.Azure != nil && infra.Status.PlatformStatus.Azure.ResourceTags != nil
+	if hasAzureStatus {
+		klog.V(5).Infof("user has provided %d tags", len(infra.Status.PlatformStatus.Azure.ResourceTags))
+		for _, tag := range infra.Status.PlatformStatus.Azure.ResourceTags {
+			klog.V(5).Infof("user has provided storage account tag: %s: %s", tag.Key, tag.Value)
+			tagset[tag.Key] = to.StringPtr(tag.Value)
+		}
+	}
+	klog.V(5).Infof("tagging storage account with tags: %+v", tagset)
+
 	// regardless if the storage account name was provided by the user or we generated it,
 	// if it is available, we do attempt to create it.
 	var storageAccountCreated bool
 	if *result.NameAvailable {
 		storageAccountCreated = true
 		if err := d.createStorageAccount(
-			storageAccountsClient, cfg.ResourceGroup, accountName, cfg.Region, d.Config.CloudName,
+			storageAccountsClient, cfg.ResourceGroup, accountName, cfg.Region, d.Config.CloudName, tagset,
 		); err != nil {
 			return "", false, err
 		}

pkg/storage/azure/azure_test.go

@@ -1,8 +1,12 @@
 package azure
 
 import (
+	"bytes"
 	"context"
 	"encoding/base64"
+	"encoding/json"
+	"fmt"
+	"io"
 	"net/http"
 	"reflect"
 	"regexp"
@@ -12,6 +16,7 @@ import (
 	"github.com/Azure/azure-pipeline-go/pipeline"
 	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/mocks"
+	"github.com/google/go-cmp/cmp"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -329,6 +334,138 @@ func TestConfigEnvWithUserKey(t *testing.T) {
 	}
 }
 
+// custom sender for mocking
+type sender struct {
+	response []*http.Response
+	body     string
+}
+
+// Do implements the Sender interface for mocking
+// Do accepts the passed request and body, then appends the response and emits it.
+func (s *sender) Do(r *http.Request) (*http.Response, error) {
+	resp := &http.Response{
+		StatusCode: http.StatusOK,
+		Request:    r,
+		Body:       io.NopCloser(bytes.NewBufferString(s.body)),
+	}
+	s.response = append(s.response, resp)
+	return resp, nil
+}
+
+func TestUserProvidedTags(t *testing.T) {
+	for _, tt := range []struct {
+		name         string
+		userTags     []configv1.AzureResourceTag
+		expectedTags map[string]string
+		infraName    string
+		responseBody string
+	}{
+		{
+			name:      "no-user-tags",
+			infraName: "some-infra",
+			// only default tags
+			expectedTags: map[string]string{
+				"kubernetes.io_cluster.some-infra": "owned",
+			},
+			responseBody: `{"nameAvailable":true}`,
+		},
+		{
+			name:      "with-user-tags",
+			infraName: "test-infra",
+			userTags: []configv1.AzureResourceTag{
+				{
+					Key:   "tag1",
+					Value: "value1",
+				},
+				{
+					Key:   "tag2",
+					Value: "value2",
+				},
+			},
+			// default tags and user tags
+			expectedTags: map[string]string{
+				"kubernetes.io_cluster.test-infra": "owned",
+				"tag1":                             "value1",
+				"tag2":                             "value2",
+			},
+			responseBody: `{"nameAvailable":true}`,
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			sender := &sender{
+				body: tt.responseBody,
+			}
+
+			storageConfig := &imageregistryv1.ImageRegistryConfigStorageAzure{}
+
+			drv := NewDriver(context.Background(), storageConfig, nil)
+			drv.authorizer = autorest.NullAuthorizer{}
+			drv.sender = sender
+
+			_, _, err := drv.assureStorageAccount(
+				&Azure{
+					SubscriptionID: "subscription-id",
+					ResourceGroup:  "resource-group",
+				},
+				&configv1.Infrastructure{
+					Status: configv1.InfrastructureStatus{
+						InfrastructureName: tt.infraName,
+						Platform:           configv1.AzurePlatformType,
+						PlatformStatus: &configv1.PlatformStatus{
+							Type: configv1.AzurePlatformType,
+							Azure: &configv1.AzurePlatformStatus{
+								ResourceTags: tt.userTags,
+							},
+						},
+					},
+				},
+			)
+			if err != nil {
+				t.Errorf("unexpected error %q", err)
+			}
+
+			// flag to confirm presence of tags
+			foundTags := false
+
+			for _, resp := range sender.response {
+				if resp != nil && resp.Request != nil && resp.Request.Body != nil {
+
+					reqBody := make(map[string]interface{})
+					if err := json.NewDecoder(resp.Request.Body).Decode(&reqBody); err != nil {
+						t.Fatalf("error decoding request: %q", err)
+					}
+
+					// ignore request without tags
+					if _, ok := reqBody["tags"]; ok {
+						foundTags = true
+
+						tags, ok := reqBody["tags"].(map[string]interface{})
+						if !ok {
+							t.Fatal("unable to type assert tags field")
+						}
+						// convert into correct type
+						receivedTags := make(map[string]string)
+						for k, v := range tags {
+							receivedTags[k] = fmt.Sprintf("%+v", v)
+						}
+
+						// compare the tags
+						if !reflect.DeepEqual(tt.expectedTags, receivedTags) {
+							t.Fatalf(
+								"unexpected tags: %s",
+								cmp.Diff(tt.expectedTags, receivedTags),
+							)
+						}
+					}
+				}
+			}
+			if !foundTags {
+				t.Fatal("no tags present in the request")
+			}
+		})
+	}
+}
+
 func Test_assureStorageAccount(t *testing.T) {
 	for _, tt := range []struct {
 		name          string