Repository: Azure/ARO-RP
Pull request #2434 (2022-09-30 15:35:08)

workaround - FPSP Role Assignment Doesn't Have Get
Don't "GET" a resource group unless it already exists.  

### Which issue this PR addresses:

Fixes TODO

### What this PR does / why we need it:

https://github.com/Azure/ARO-RP/pull/2328/files#diff-fc2ecaea9c3fb03887bc89a5290dd452dc919a651168e73a3b3a83a25d8ff525R48

### Test plan for issue:

Test in prod because that's the only place to accurately test this.  We also need to PUCM an existing cluster to ensure we can still "GET" an existing resource group.

### Is there any documentation that needs to be updated for this PR?




Patch:
diff --git a/pkg/cluster/deploystorage.go b/pkg/cluster/deploystorage.go
index ec9d2ff36..707b1bd29 100644
--- a/pkg/cluster/deploystorage.go
+++ b/pkg/cluster/deploystorage.go
@@ -12,6 +12,7 @@ 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"
@@ -41,14 +42,19 @@ func (m *manager) ensureInfraID(ctx context.Context) (err error) {
 	return err
 }
 
-func (m *manager) ensureResourceGroup(ctx context.Context) error {
+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
-	group, err := m.resourceGroups.Get(ctx, resourceGroup)
-	if err != nil {
-		if detailedErr, ok := err.(autorest.DetailedError); !ok || detailedErr.StatusCode != http.StatusNotFound {
-			return err
+	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
+			}
 		}
 	}
 


Test patch:
diff --git a/pkg/cluster/deploystorage_test.go b/pkg/cluster/deploystorage_test.go
index 0a4d00822..b2e843097 100644
--- a/pkg/cluster/deploystorage_test.go
+++ b/pkg/cluster/deploystorage_test.go
@@ -7,7 +7,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"net/http"
 	"strings"
 	"testing"
 
@@ -53,17 +52,15 @@ func TestEnsureResourceGroup(t *testing.T) {
 	}, "", "", nil, "")
 
 	for _, tt := range []struct {
-		name    string
-		mocks   func(*mock_features.MockResourceGroupsClient, *mock_env.MockInterface)
-		wantErr string
+		name              string
+		provisioningState api.ProvisioningState
+		mocks             func(*mock_features.MockResourceGroupsClient, *mock_env.MockInterface)
+		wantErr           string
 	}{
 		{
-			name: "success - rg doesn't exist",
+			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(group, autorest.DetailedError{StatusCode: http.StatusNotFound})
-
 				rg.EXPECT().
 					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
 					Return(group, nil)
@@ -78,16 +75,13 @@ func TestEnsureResourceGroup(t *testing.T) {
 			},
 		},
 		{
-			name: "success - rg doesn't exist and localdev mode tags set",
+			name:              "success - rg doesn't exist and localdev mode tags set",
+			provisioningState: api.ProvisioningStateCreating,
 			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
 				groupWithLocalDevTags := group
 				groupWithLocalDevTags.Tags = map[string]*string{
 					"purge": to.StringPtr("true"),
 				}
-				rg.EXPECT().
-					Get(gomock.Any(), resourceGroupName).
-					Return(group, autorest.DetailedError{StatusCode: http.StatusNotFound})
-
 				rg.EXPECT().
 					CreateOrUpdate(gomock.Any(), resourceGroupName, groupWithLocalDevTags).
 					Return(groupWithLocalDevTags, nil)
@@ -102,7 +96,8 @@ func TestEnsureResourceGroup(t *testing.T) {
 			},
 		},
 		{
-			name: "success - rg exists and maintain tags",
+			name:              "success - rg exists and maintain tags",
+			provisioningState: api.ProvisioningStateAdminUpdating,
 			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
 				rg.EXPECT().
 					Get(gomock.Any(), resourceGroupName).
@@ -122,7 +117,8 @@ func TestEnsureResourceGroup(t *testing.T) {
 			},
 		},
 		{
-			name: "fail - get rg returns generic error",
+			name:              "fail - get rg returns generic error",
+			provisioningState: api.ProvisioningStateAdminUpdating,
 			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
 				rg.EXPECT().
 					Get(gomock.Any(), resourceGroupName).
@@ -131,12 +127,9 @@ func TestEnsureResourceGroup(t *testing.T) {
 			wantErr: "generic error",
 		},
 		{
-			name: "fail - CreateOrUpdate returns resourcegroupmanagedbymismatch",
+			name:              "fail - CreateOrUpdate returns resourcegroupmanagedbymismatch",
+			provisioningState: api.ProvisioningStateCreating,
 			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, resourceGroupManagedByMismatch)
@@ -148,12 +141,9 @@ func TestEnsureResourceGroup(t *testing.T) {
 			wantErr: "400: ClusterResourceGroupAlreadyExists: : Resource group " + resourceGroup + " must not already exist.",
 		},
 		{
-			name: "fail - CreateOrUpdate returns requestdisallowedbypolicy",
+			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, autorest.DetailedError{StatusCode: http.StatusNotFound})
-
 				rg.EXPECT().
 					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
 					Return(group, disallowedByPolicy)
@@ -165,12 +155,9 @@ func TestEnsureResourceGroup(t *testing.T) {
 			wantErr: `400: DeploymentFailed: : Deployment failed. Details: : : {"code":"RequestDisallowedByPolicy","message":"","target":null,"details":null,"innererror":null,"additionalInfo":null}`,
 		},
 		{
-			name: "fail - CreateOrUpdate returns generic error",
+			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, autorest.DetailedError{StatusCode: http.StatusNotFound})
-
 				rg.EXPECT().
 					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
 					Return(group, errors.New("generic error"))
@@ -201,6 +188,7 @@ func TestEnsureResourceGroup(t *testing.T) {
 							ClusterProfile: api.ClusterProfile{
 								ResourceGroupID: resourceGroup,
 							},
+							ProvisioningState: tt.provisioningState,
 						},
 						Location: location,
 						ID:       clusterID,