From 3db87fbdcdf92c897678f408d096e5ddabbfca0c Mon Sep 17 00:00:00 2001 From: bennerv <10840174+bennerv@users.noreply.github.com> Date: Tue, 16 Aug 2022 11:37:02 -0400 Subject: [PATCH] Persist tags on ensureResourceGroup --- pkg/cluster/deploystorage.go | 24 +++--- pkg/cluster/deploystorage_test.go | 125 +++++++++++++++++++++++++----- 2 files changed, 118 insertions(+), 31 deletions(-) diff --git a/pkg/cluster/deploystorage.go b/pkg/cluster/deploystorage.go index e6fb43ccc..98634c5e7 100644 --- a/pkg/cluster/deploystorage.go +++ b/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, - } - 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 + // 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() { 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) diff --git a/pkg/cluster/deploystorage_test.go b/pkg/cluster/deploystorage_test.go index 85c3912d8..3c0d895ff 100644 --- a/pkg/cluster/deploystorage_test.go +++ b/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()),