Remove the provisioning state check around ensure managed rg (Azure#2936

) * Remove the provisioning state check around managed resource group as the FPSP role now has read RG permissions across entire tenant * Refactor to not rely on Azure error codes
jhoreman · Aug 1, 2023 · 5b94eb4 · 5b94eb4
1 parent ede320d
commit 5b94eb4
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 31 deletions.
diff --git a/pkg/cluster/deploybaseresources.go b/pkg/cluster/deploybaseresources.go
@@ -46,20 +46,35 @@ func (m *manager) ensureResourceGroup(ctx context.Context) (err error) {
 	resourceGroup := stringutils.LastTokenByte(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID, '/')
 	group := mgmtfeatures.ResourceGroup{}
 
-	// The FPSP's role definition does not have read on a resource group
-	// if the resource group does not exist.
 	// Retain the existing resource group configuration (such as tags) if it exists
-	if m.doc.OpenShiftCluster.Properties.ProvisioningState != api.ProvisioningStateCreating {
-		group, err = m.resourceGroups.Get(ctx, resourceGroup)
-		if err != nil {
-			if detailedErr, ok := err.(autorest.DetailedError); !ok || detailedErr.StatusCode != http.StatusNotFound {
-				return err
-			}
+	group, err = m.resourceGroups.Get(ctx, resourceGroup)
+	if err != nil {
+		if detailedErr, ok := err.(autorest.DetailedError); !ok || detailedErr.StatusCode != http.StatusNotFound {
+			return err
 		}
+
+		// set field values if the RG doesn't exist
+		group.Location = &m.doc.OpenShiftCluster.Location
+		group.ManagedBy = &m.doc.OpenShiftCluster.ID
+	}
+
+	resourceGroupAlreadyExistsError := &api.CloudError{
+		StatusCode: http.StatusBadRequest,
+		CloudErrorBody: &api.CloudErrorBody{
+			Code: api.CloudErrorCodeClusterResourceGroupAlreadyExists,
+			Message: "Resource group " + m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID +
+				" must not already exist.",
+		},
+	}
+
+	// If managedBy or location don't match, return an error that RG must not already exist
+	if group.Location == nil || !strings.EqualFold(*group.Location, m.doc.OpenShiftCluster.Location) {
+		return resourceGroupAlreadyExistsError
 	}
 
-	group.Location = &m.doc.OpenShiftCluster.Location
-	group.ManagedBy = &m.doc.OpenShiftCluster.ID
+	if group.ManagedBy == nil || !strings.EqualFold(*group.ManagedBy, m.doc.OpenShiftCluster.ID) {
+		return resourceGroupAlreadyExistsError
+	}
 
 	// 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() {
@@ -87,16 +102,6 @@ func (m *manager) ensureResourceGroup(ctx context.Context) (err error) {
 		serviceError = requestErr.ServiceError
 	}
 
-	if serviceError != nil && serviceError.Code == "ResourceGroupManagedByMismatch" {
-		return &api.CloudError{
-			StatusCode: http.StatusBadRequest,
-			CloudErrorBody: &api.CloudErrorBody{
-				Code: api.CloudErrorCodeClusterResourceGroupAlreadyExists,
-				Message: "Resource group " + m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID +
-					" must not already exist.",
-			},
-		}
-	}
 	if serviceError != nil && serviceError.Code == "RequestDisallowedByPolicy" {
 		// if request was disallowed by policy, inform user so they can take appropriate action
 		b, _ := json.Marshal(serviceError)

diff --git a/pkg/cluster/deploybaseresources_test.go b/pkg/cluster/deploybaseresources_test.go
@@ -7,6 +7,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/http"
 	"strings"
 	"testing"
 
@@ -44,14 +45,14 @@ func TestEnsureResourceGroup(t *testing.T) {
 		"yeet": to.StringPtr("yote"),
 	}
 
-	resourceGroupManagedByMismatch := autorest.NewErrorWithError(&azure.RequestError{
-		ServiceError: &azure.ServiceError{Code: "ResourceGroupManagedByMismatch"},
-	}, "", "", nil, "")
-
 	disallowedByPolicy := autorest.NewErrorWithError(&azure.RequestError{
 		ServiceError: &azure.ServiceError{Code: "RequestDisallowedByPolicy"},
 	}, "", "", nil, "")
 
+	resourceGroupNotFound := autorest.NewErrorWithError(&azure.RequestError{
+		ServiceError: &azure.ServiceError{Code: "ResourceGroupNotFound"},
+	}, "", "", &http.Response{StatusCode: http.StatusNotFound}, "")
+
 	for _, tt := range []struct {
 		name              string
 		provisioningState api.ProvisioningState
@@ -62,6 +63,10 @@ func TestEnsureResourceGroup(t *testing.T) {
 			name:              "success - rg doesn't exist",
 			provisioningState: api.ProvisioningStateCreating,
 			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(mgmtfeatures.ResourceGroup{}, resourceGroupNotFound)
+
 				rg.EXPECT().
 					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
 					Return(group, nil)
@@ -83,6 +88,10 @@ func TestEnsureResourceGroup(t *testing.T) {
 				groupWithLocalDevTags.Tags = map[string]*string{
 					"purge": to.StringPtr("true"),
 				}
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(mgmtfeatures.ResourceGroup{}, resourceGroupNotFound)
+
 				rg.EXPECT().
 					CreateOrUpdate(gomock.Any(), resourceGroupName, groupWithLocalDevTags).
 					Return(groupWithLocalDevTags, nil)
@@ -128,23 +137,37 @@ func TestEnsureResourceGroup(t *testing.T) {
 			wantErr: "generic error",
 		},
 		{
-			name:              "fail - CreateOrUpdate returns resourcegroupmanagedbymismatch",
+			name:              "fail - managedBy doesn't match",
 			provisioningState: api.ProvisioningStateCreating,
 			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				badManagedBy := group
+				badManagedBy.ManagedBy = to.StringPtr("does-not-match")
 				rg.EXPECT().
-					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
-					Return(group, resourceGroupManagedByMismatch)
-
-				env.EXPECT().
-					IsLocalDevelopmentMode().
-					Return(false)
+					Get(gomock.Any(), resourceGroupName).
+					Return(badManagedBy, nil)
+			},
+			wantErr: "400: ClusterResourceGroupAlreadyExists: : Resource group " + resourceGroup + " must not already exist.",
+		},
+		{
+			name:              "fail - location doesn't match",
+			provisioningState: api.ProvisioningStateCreating,
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				badLocation := group
+				badLocation.Location = to.StringPtr("bad-location")
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(badLocation, nil)
 			},
 			wantErr: "400: ClusterResourceGroupAlreadyExists: : Resource group " + resourceGroup + " must not already exist.",
 		},
 		{
 			name:              "fail - CreateOrUpdate returns requestdisallowedbypolicy",
 			provisioningState: api.ProvisioningStateCreating,
 			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, nil)
+
 				rg.EXPECT().
 					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
 					Return(group, disallowedByPolicy)
@@ -159,6 +182,10 @@ func TestEnsureResourceGroup(t *testing.T) {
 			name:              "fail - CreateOrUpdate returns generic error",
 			provisioningState: api.ProvisioningStateCreating,
 			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, nil)
+
 				rg.EXPECT().
 					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
 					Return(group, errors.New("generic error"))