Merge pull request #2328 from bennerv/retain-tags-on-ensureResourceGroup

Persist tags on ensureResourceGroup

pkg/cluster/deploystorage.go

@@ -12,7 +12,6 @@ import (
 	"strings"
 
 	mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
-	mgmtfeatures "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-07-01/features"
 	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/azure"
 	"github.com/Azure/go-autorest/autorest/to"
@@ -45,16 +44,19 @@ func (m *manager) ensureInfraID(ctx context.Context) (err error) {
 func (m *manager) ensureResourceGroup(ctx context.Context) error {
 	resourceGroup := stringutils.LastTokenByte(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID, '/')
 
-	group := mgmtfeatures.ResourceGroup{
-		Location:  &m.doc.OpenShiftCluster.Location,
-		ManagedBy: &m.doc.OpenShiftCluster.ID,
+	// Retain the existing resource group configuration (such as tags) if it exists
+	group, err := m.resourceGroups.Get(ctx, resourceGroup)
+	if err != nil {
+		if detailedErr, ok := err.(autorest.DetailedError); !ok || detailedErr.StatusCode != http.StatusNotFound {
+			return err
 		}
+	}
+
+	group.Location = &m.doc.OpenShiftCluster.Location
+	group.ManagedBy = &m.doc.OpenShiftCluster.ID
+
+	// HACK: set purge=true on dev clusters so our purger wipes them out since there is not deny assignment in place
 	if m.env.IsLocalDevelopmentMode() {
-		// grab tags so we do not accidently remove them on createOrUpdate, set purge tag to true for dev clusters
-		rg, err := m.resourceGroups.Get(ctx, resourceGroup)
-		if err == nil {
-			group.Tags = rg.Tags
-		}
 		if group.Tags == nil {
 			group.Tags = map[string]*string{}
 		}
@@ -64,7 +66,7 @@ func (m *manager) ensureResourceGroup(ctx context.Context) error {
 	// According to https://stackoverflow.microsoft.com/a/245391/62320,
 	// re-PUTting our RG should re-create RP RBAC after a customer subscription
 	// migrates between tenants.
-	_, err := m.resourceGroups.CreateOrUpdate(ctx, resourceGroup, group)
+	_, err = m.resourceGroups.CreateOrUpdate(ctx, resourceGroup, group)
 
 	var serviceError *azure.ServiceError
 	// CreateOrUpdate wraps DetailedError wrapping a *RequestError (if error generated in ResourceGroup CreateOrUpdateResponder at least)

pkg/cluster/deploystorage_test.go

@@ -5,7 +5,9 @@ package cluster
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"net/http"
 	"strings"
 	"testing"
 
@@ -25,7 +27,7 @@ import (
 	testdatabase "github.com/Azure/ARO-RP/test/database"
 )
 
-func TestCreateAndUpdateErrors(t *testing.T) {
+func TestEnsureResourceGroup(t *testing.T) {
 	ctx := context.Background()
 	clusterID := "test-cluster"
 	resourceGroupName := "fakeResourceGroup"
@@ -37,41 +39,126 @@ func TestCreateAndUpdateErrors(t *testing.T) {
 		ManagedBy: &clusterID,
 	}
 
+	groupWithTags := group
+	groupWithTags.Tags = map[string]*string{
+		"yeet": to.StringPtr("yote"),
+	}
+
 	disallowedByPolicy := autorest.NewErrorWithError(&azure.RequestError{
 		ServiceError: &azure.ServiceError{Code: "RequestDisallowedByPolicy"},
 	}, "", "", nil, "")
 
 	for _, tt := range []struct {
 		name    string
-		result  mgmtfeatures.ResourceGroup
-		mocks   func(*mock_features.MockResourceGroupsClient, interface{})
+		mocks   func(*mock_features.MockResourceGroupsClient, *mock_env.MockInterface)
 		wantErr string
 	}{
 		{
-			name: "ResourceGroup creation was fine",
-			mocks: func(rg *mock_features.MockResourceGroupsClient, result interface{}) {
+			name: "success - rg doesn't exist",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
 				rg.EXPECT().
-					CreateOrUpdate(ctx, resourceGroupName, group).
-					Return(result, nil)
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, autorest.DetailedError{StatusCode: http.StatusNotFound})
+
+				rg.EXPECT().
+					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
+					Return(group, nil)
+
+				env.EXPECT().
+					IsLocalDevelopmentMode().
+					Return(false)
+
+				env.EXPECT().
+					EnsureARMResourceGroupRoleAssignment(gomock.Any(), gomock.Any(), resourceGroupName).
+					Return(nil)
 			},
 		},
 		{
-			name: "ResourceGroup creation failed with RequestDisallowedByPolicy",
-			mocks: func(rg *mock_features.MockResourceGroupsClient, result interface{}) {
+			name: "success - rg doesn't exist and localdev mode tags set",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				groupWithLocalDevTags := group
+				groupWithLocalDevTags.Tags = map[string]*string{
+					"purge": to.StringPtr("true"),
+				}
 				rg.EXPECT().
-					CreateOrUpdate(ctx, resourceGroupName, group).
-					Return(result, disallowedByPolicy)
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, autorest.DetailedError{StatusCode: http.StatusNotFound})
+
+				rg.EXPECT().
+					CreateOrUpdate(gomock.Any(), resourceGroupName, groupWithLocalDevTags).
+					Return(groupWithLocalDevTags, nil)
+
+				env.EXPECT().
+					IsLocalDevelopmentMode().
+					Return(true)
+
+				env.EXPECT().
+					EnsureARMResourceGroupRoleAssignment(gomock.Any(), gomock.Any(), resourceGroupName).
+					Return(nil)
+			},
+		},
+		{
+			name: "success - rg exists and maintain tags",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(groupWithTags, nil)
+
+				rg.EXPECT().
+					CreateOrUpdate(gomock.Any(), resourceGroupName, groupWithTags).
+					Return(groupWithTags, nil)
+
+				env.EXPECT().
+					IsLocalDevelopmentMode().
+					Return(false)
+
+				env.EXPECT().
+					EnsureARMResourceGroupRoleAssignment(gomock.Any(), gomock.Any(), resourceGroupName).
+					Return(nil)
+			},
+		},
+		{
+			name: "fail - get rg returns generic error",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, errors.New("generic error"))
+			},
+			wantErr: "generic error",
+		},
+		{
+			name: "fail - CreateOrUpdate returns requestdisallowedbypolicy",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, autorest.DetailedError{StatusCode: http.StatusNotFound})
+
+				rg.EXPECT().
+					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
+					Return(group, disallowedByPolicy)
+
+				env.EXPECT().
+					IsLocalDevelopmentMode().
+					Return(false)
 			},
 			wantErr: `400: DeploymentFailed: : Deployment failed. Details: : : {"code":"RequestDisallowedByPolicy","message":"","target":null,"details":null,"innererror":null,"additionalInfo":null}`,
 		},
 		{
-			name: "ResourceGroup creation failed with other error",
-			mocks: func(rg *mock_features.MockResourceGroupsClient, result interface{}) {
+			name: "fail - CreateOrUpdate returns generic error",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
 				rg.EXPECT().
-					CreateOrUpdate(ctx, resourceGroupName, group).
-					Return(result, fmt.Errorf("Any other error"))
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, autorest.DetailedError{StatusCode: http.StatusNotFound})
+
+				rg.EXPECT().
+					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
+					Return(group, errors.New("generic error"))
+
+				env.EXPECT().
+					IsLocalDevelopmentMode().
+					Return(false)
 			},
-			wantErr: "Any other error",
+			wantErr: "generic error",
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
@@ -79,12 +166,10 @@ func TestCreateAndUpdateErrors(t *testing.T) {
 			defer controller.Finish()
 
 			resourceGroupsClient := mock_features.NewMockResourceGroupsClient(controller)
-			tt.mocks(resourceGroupsClient, tt.result)
-
 			env := mock_env.NewMockInterface(controller)
+			tt.mocks(resourceGroupsClient, env)
+
 			env.EXPECT().Location().AnyTimes().Return(location)
-			env.EXPECT().EnsureARMResourceGroupRoleAssignment(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
-			env.EXPECT().IsLocalDevelopmentMode().Return(false)
 
 			m := &manager{
 				log:            logrus.NewEntry(logrus.StandardLogger()),