Repository: Azure/ARO-RP
Pull request #1385 (2021-03-15 12:36:38)

revert CustomRoleDefintion (most of #1264, part of #1361 and #1271)
### Which issue this PR addresses:

Revert #1264  #1361 and #1371

### What this PR does / why we need it:

Revert #1264 due to issues on RoleLimitExceeded on tenant

### Test plan for issue:

CI and E2E

### Is there any documentation that needs to be updated for this PR?

N/A

Makdaam: /lgtm
changes in deploystorage should take care of new cluster creation, and the revert in clusterserviceprincipal.go should revert the put or patch update steps. Let's see if E2E passes.

Patch:
diff --git a/docs/updating-dependencies.md b/docs/updating-dependencies.md
deleted file mode 100644
index d9da4d498..000000000
--- a/docs/updating-dependencies.md
+++ /dev/null
@@ -1,62 +0,0 @@
-# Updating dependencies
-
-To update golang dependencies one has to choose between two options:
-
-1) update the whole dependency tree
-2) update or add what is currently required
-
-
-## Update the whole dependency tree
-
-Done when the need to catch upstream dependencies arises, it can be done by
-calling
-
-```bash
-make vendor
-```
-
-in root folder, which calls `hack/update-go-module-dependencies.sh`.
-
-The reason for calling script instead of directly calling:
-
-```bash
-go get -u ./...
-go mod tidy
-go mod vendor
-```
-
-is that packages modified in this script do not fully support modules and
-semantic versioning via tags. Therefore the proper version is parsed from the version
-branch and fixed using replace directive. Otherwise it will upgrade every time
-the command is started.
-
-When upgrading to a never version of OpenShift, this script have to be updated to
-reflect the proper release.
-
-
-## Update only required dependencies
-
-When adding new dependencies or updating old ones, based on the requirement of
-the PR, one can simply call
-
-```bash
-go get <module>@<release> OR
-go get -u <module>@<release>
-
-go mod tidy
-go mod vendor
-```
-
----
-
-**NOTE** vendoring is required as ARO mirrors all dependencies locally for the CI reliability
-and reproducibility.
-
-**NOTE** that when running `go mod vendor` only modules that are used in the
-source code will be vendored in.
-
-**NOTE** when updating a package modified in `hack/update-go-module-dependencies.sh`
-changes have to be made there also. Otherwise next run of `make vendor` will
-overwrite new changes.
-
-More information on working with modules: https://blog.golang.org/using-go-modules
\ No newline at end of file
diff --git a/pkg/cluster/clusterserviceprincipal.go b/pkg/cluster/clusterserviceprincipal.go
index 0fb9b3021..96e197218 100644
--- a/pkg/cluster/clusterserviceprincipal.go
+++ b/pkg/cluster/clusterserviceprincipal.go
@@ -29,12 +29,13 @@ func (m *manager) createOrUpdateClusterServicePrincipalRBAC(ctx context.Context)
 		return err
 	}
 
+	// If we have Contributor RBAC role for Cluster SP on the resource group in question
 	// We are interested in Resource group scope only (inherited are returned too).
 	var toDelete []mgmtauthorization.RoleAssignment
 	var found bool
 	for _, assignment := range roleAssignments {
-		if strings.EqualFold(*assignment.Scope, resourceGroupID) &&
-			!strings.HasSuffix(strings.ToLower(*assignment.RoleDefinitionID), strings.ToLower(rbac.RoleOwner)) /* should only matter in development */ {
+		// Contributor assignments only!
+		if strings.EqualFold(*assignment.Scope, resourceGroupID) && strings.HasSuffix(strings.ToLower(*assignment.RoleDefinitionID), rbac.RoleContributor) {
 			if strings.EqualFold(*assignment.PrincipalID, clusterSPObjectID) {
 				found = true
 			} else {
@@ -56,7 +57,7 @@ func (m *manager) createOrUpdateClusterServicePrincipalRBAC(ctx context.Context)
 		t := &arm.Template{
 			Schema:         "https://schema.management.azure.com/schemas/2015-01-01/deploymentTemplate.json#",
 			ContentVersion: "1.0.0.0",
-			Resources:      m.clusterServicePrincipalRBAC(),
+			Resources:      []*arm.Resource{m.clusterServicePrincipalRBAC()},
 		}
 		err = m.deployARMTemplate(ctx, resourceGroup, "storage", t, nil)
 		if err != nil {
diff --git a/pkg/cluster/delete.go b/pkg/cluster/delete.go
index 60a330087..cc1b2d9bb 100644
--- a/pkg/cluster/delete.go
+++ b/pkg/cluster/delete.go
@@ -22,7 +22,6 @@ import (
 	"github.com/Azure/ARO-RP/pkg/util/azureerrors"
 	"github.com/Azure/ARO-RP/pkg/util/deployment"
 	"github.com/Azure/ARO-RP/pkg/util/dns"
-	"github.com/Azure/ARO-RP/pkg/util/rbac"
 	"github.com/Azure/ARO-RP/pkg/util/stringutils"
 )
 
@@ -208,50 +207,6 @@ func (m *manager) deleteResources(ctx context.Context) error {
 	return nil
 }
 
-func (m *manager) deleteRoleAssignmentsAndDefinition(ctx context.Context) error {
-	resourceGroupID := m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID
-	resourceGroup := stringutils.LastTokenByte(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID, '/')
-
-	roleAssignments, err := m.roleAssignments.ListForResourceGroup(ctx, resourceGroup, "")
-	if err != nil {
-		return err
-	}
-
-	for _, assignment := range roleAssignments {
-		if !strings.EqualFold(*assignment.Scope, resourceGroupID) ||
-			strings.HasSuffix(strings.ToLower(*assignment.RoleDefinitionID), strings.ToLower(rbac.RoleOwner)) /* should only matter in development */ {
-			continue
-		}
-
-		m.log.Infof("deleting role assignment %s", *assignment.Name)
-		_, err := m.roleAssignments.Delete(ctx, *assignment.Scope, *assignment.Name)
-		if err != nil {
-			return err
-		}
-	}
-
-	roleDefinitions, err := m.roleDefinitions.List(ctx, resourceGroupID, "")
-	if err != nil {
-		return err
-	}
-
-	for _, definition := range roleDefinitions {
-		if len(*definition.AssignableScopes) != 1 ||
-			!strings.EqualFold((*definition.AssignableScopes)[0], resourceGroupID) ||
-			!strings.HasPrefix(*definition.RoleName, "Azure Red Hat OpenShift cluster") {
-			continue
-		}
-
-		m.log.Infof("deleting role definition %s", *definition.Name)
-		_, err := m.roleDefinitions.Delete(ctx, (*definition.AssignableScopes)[0], *definition.Name)
-		if err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
 func (m *manager) Delete(ctx context.Context) error {
 	resourceGroup := stringutils.LastTokenByte(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID, '/')
 
@@ -267,12 +222,6 @@ func (m *manager) Delete(ctx context.Context) error {
 		return err
 	}
 
-	m.log.Printf("deleting role assignments and role definition")
-	err = m.deleteRoleAssignmentsAndDefinition(ctx)
-	if err != nil {
-		return err
-	}
-
 	m.log.Printf("deleting resources")
 	err = m.deleteResources(ctx)
 	if err != nil {
diff --git a/pkg/cluster/denyassignment.go b/pkg/cluster/denyassignment.go
index 54626d821..c534413b5 100644
--- a/pkg/cluster/denyassignment.go
+++ b/pkg/cluster/denyassignment.go
@@ -24,13 +24,28 @@ func (m *manager) createOrUpdateDenyAssignment(ctx context.Context) error {
 	}
 
 	resourceGroup := stringutils.LastTokenByte(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID, '/')
+	clusterSPObjectID := m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile.SPObjectID
+
+	denyAssignments, err := m.denyAssignments.ListForResourceGroup(ctx, resourceGroup, "")
+	if err != nil {
+		return err
+	}
+
+	for _, assignment := range denyAssignments {
+		if assignment.DenyAssignmentProperties.ExcludePrincipals != nil {
+			for _, ps := range *assignment.DenyAssignmentProperties.ExcludePrincipals {
+				if *ps.ID == clusterSPObjectID {
+					return nil
+				}
+			}
+		}
+	}
 
 	t := &arm.Template{
 		Schema:         "https://schema.management.azure.com/schemas/2015-01-01/deploymentTemplate.json#",
 		ContentVersion: "1.0.0.0",
 		Resources: []*arm.Resource{
 			m.denyAssignment(),
-			m.clusterServicePrincipalRoleDefinition(),
 		},
 	}
 
diff --git a/pkg/cluster/deploystorage.go b/pkg/cluster/deploystorage.go
index 9d7513262..584d9fef9 100644
--- a/pkg/cluster/deploystorage.go
+++ b/pkg/cluster/deploystorage.go
@@ -122,6 +122,7 @@ func (m *manager) deployStorageTemplate(ctx context.Context, installConfig *inst
 		m.clusterStorageAccountBlob("ignition"),
 		m.clusterStorageAccountBlob("aro"),
 		m.clusterNSG(infraID, installConfig.Config.Azure.Region),
+		m.clusterServicePrincipalRBAC(),
 		m.networkPrivateLinkService(installConfig),
 		m.networkPublicIPAddress(installConfig, m.doc.OpenShiftCluster.Properties.InfraID+"-pip-v4"),
 		m.networkInternalLoadBalancer(installConfig),
@@ -140,8 +141,6 @@ func (m *manager) deployStorageTemplate(ctx context.Context, installConfig *inst
 		Resources:      resources,
 	}
 
-	t.Resources = append(t.Resources, m.clusterServicePrincipalRBAC()...)
-
 	if m.env.DeploymentMode() == deployment.Production {
 		t.Resources = append(t.Resources, m.denyAssignment())
 	}
diff --git a/pkg/cluster/deploystorage_resources.go b/pkg/cluster/deploystorage_resources.go
index 92c8ab109..eaf1eca3c 100644
--- a/pkg/cluster/deploystorage_resources.go
+++ b/pkg/cluster/deploystorage_resources.go
@@ -65,66 +65,12 @@ func (m *manager) denyAssignment() *arm.Resource {
 	}
 }
 
-func (m *manager) clusterServicePrincipalRoleDefinitionName() string {
-	infraSuffix := m.doc.OpenShiftCluster.Properties.InfraID
-	if len(infraSuffix) > 5 {
-		infraSuffix = infraSuffix[len(infraSuffix)-5:]
-	}
-	return fmt.Sprintf("Azure Red Hat OpenShift cluster (%s)", infraSuffix)
-}
-
-func (m *manager) clusterServicePrincipalRoleDefinition() *arm.Resource {
-	return rbac.CustomRoleDefinition(m.clusterServicePrincipalRoleDefinitionName(),
-		[]mgmtauthorization.Permission{
-			{
-				Actions: &[]string{
-					//based on openshift/cluster-api-provider-azure /pkg/cloud/azure/services/disks
-					"Microsoft.Compute/disks/*",
-
-					//needed for user-initiated backup
-					"Microsoft.Compute/snapshots/*",
-
-					//based on openshift/cluster-api-provider-azure /pkg/cloud/azure/services/internalloadbalancers
-					//based on openshift/cluster-api-provider-azure /pkg/cloud/azure/services/publicloadbalancers
-					"Microsoft.Network/loadBalancers/*",
-
-					//based on openshift/cluster-api-provider-azure /pkg/cloud/azure/services/networkinterfaces
-					"Microsoft.Network/networkInterfaces/*",
-
-					//based on openshift/cluster-api-provider-azure /pkg/cloud/azure/services/publicips
-					"Microsoft.Network/publicIPAddresses/*",
-
-					//based on openshift/cluster-api-provider-azure /pkg/cloud/azure/services/securitygroups
-					"Microsoft.Network/networkSecurityGroups/*",
-
-					//based on openshift/cluster-api-provider-azure /pkg/cloud/azure/services/virtualmachines
-					"Microsoft.Compute/virtualMachines/*",
-
-					//based on openshift/cluster-insgress-operator /pkg/dns/azure/client
-					"Microsoft.Network/privateDnsZones/A/*",
-
-					//based on openshift/cluster-image-registry-operator /pkg/storage/azure
-					"Microsoft.Storage/storageAccounts/*",
-				},
-				NotActions: &[]string{
-					"Microsoft.Compute/virtualMachines/powerOff/action",
-					"Microsoft.Compute/virtualMachines/deallocate/action",
-					"Microsoft.Compute/virtualMachines/generalize/action",
-					"Microsoft.Compute/virtualMachines/capture/action",
-					"Microsoft.Compute/virtualMachines/performMaintenance/action",
-					"Microsoft.Network/networkSecurityGroups/delete",
-				},
-			},
-		})
-}
-
-func (m *manager) clusterServicePrincipalRBAC() []*arm.Resource {
-	return []*arm.Resource{
-		m.clusterServicePrincipalRoleDefinition(),
-		rbac.ResourceGroupCustomRoleAssignment(
-			rbac.CustomRoleDefinitionName(m.clusterServicePrincipalRoleDefinitionName()),
-			"'"+m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile.SPObjectID+"'"),
-	}
+func (m *manager) clusterServicePrincipalRBAC() *arm.Resource {
+	return rbac.ResourceGroupRoleAssignmentWithName(
+		rbac.RoleContributor,
+		"'"+m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile.SPObjectID+"'",
+		"guid(resourceGroup().id, 'SP / Contributor')",
+	)
 }
 
 func (m *manager) clusterStorageAccount(region string) *arm.Resource {
diff --git a/pkg/util/rbac/rbac.go b/pkg/util/rbac/rbac.go
index 281b9e2ca..20e0a67c8 100644
--- a/pkg/util/rbac/rbac.go
+++ b/pkg/util/rbac/rbac.go
@@ -105,44 +105,3 @@ func resourceGroupRoleAssignmentWithDetails(roleID, spID string, name string, de
 func ResourceGroupRoleAssignmentWithName(roleID, spID string, name string, condition ...interface{}) *arm.Resource {
 	return resourceGroupRoleAssignmentWithDetails("'"+roleID+"'", spID, name, nil, true, condition...)
 }
-
-func CustomRoleDefinitionName(name string) string {
-	return "guid(resourceGroup().id, '" + name + "')"
-}
-
-// ResourceGroupCustomRoleAssignment returns a Resource granting roleID on the
-// current resource group to spID.  Arguments roleID, spID and name must be
-// valid ARM expressions, e.g. "'foo'" or "concat('foo')".  Additionally a
-// dependency is added on the role definition; use this function when the role
-// definition is declared in the same ARM template at non-subscription scope.
-func ResourceGroupCustomRoleAssignment(roleID, spID string, condition ...interface{}) *arm.Resource {
-	dependsOn := []string{
-		"[resourceId('Microsoft.Authorization/roleDefinitions', " + roleID + ")]",
-	}
-
-	return resourceGroupRoleAssignmentWithDetails(roleID, spID, "guid(resourceGroup().id, "+spID+", "+roleID+")", dependsOn, false, condition...)
-}
-
-// CustomRoleDefinition returns a Resource defining a role granting the required
-// actions and dataActions, forbidding notActions and notDataActions.
-func CustomRoleDefinition(name string, permissions []mgmtauthorization.Permission) *arm.Resource {
-	r := &arm.Resource{
-		Resource: mgmtauthorization.RoleDefinition{
-			Name: to.StringPtr("[" + CustomRoleDefinitionName(name) + "]"),
-			Type: to.StringPtr("Microsoft.Authorization/roleDefinitions"),
-			RoleDefinitionProperties: &mgmtauthorization.RoleDefinitionProperties{
-				RoleName:    &name,
-				Description: &name,
-				RoleType:    to.StringPtr("CustomRole"),
-				Permissions: &permissions,
-				//the roles defined here can only be assigned inside the cluster resourceGroup
-				AssignableScopes: &[]string{
-					"[resourceGroup().id]",
-				},
-			},
-		},
-		APIVersion: azureclient.APIVersion("Microsoft.Authorization/roleDefinitions"),
-	}
-
-	return r
-}


Test patch:
diff --git a/pkg/cluster/clusterserviceprincipal_test.go b/pkg/cluster/clusterserviceprincipal_test.go
index fa3679e55..5e21512b7 100644
--- a/pkg/cluster/clusterserviceprincipal_test.go
+++ b/pkg/cluster/clusterserviceprincipal_test.go
@@ -86,7 +86,7 @@ func TestCreateOrUpdateClusterServicePrincipalRBAC(t *testing.T) {
 						Template: &arm.Template{
 							Schema:         "https://schema.management.azure.com/schemas/2015-01-01/deploymentTemplate.json#",
 							ContentVersion: "1.0.0.0",
-							Resources:      m.clusterServicePrincipalRBAC(),
+							Resources:      []*arm.Resource{m.clusterServicePrincipalRBAC()},
 						},
 						Parameters: parameters,
 						Mode:       mgmtfeatures.Incremental,
@@ -117,7 +117,7 @@ func TestCreateOrUpdateClusterServicePrincipalRBAC(t *testing.T) {
 						Template: &arm.Template{
 							Schema:         "https://schema.management.azure.com/schemas/2015-01-01/deploymentTemplate.json#",
 							ContentVersion: "1.0.0.0",
-							Resources:      m.clusterServicePrincipalRBAC(),
+							Resources:      []*arm.Resource{m.clusterServicePrincipalRBAC()},
 						},
 						Parameters: parameters,
 						Mode:       mgmtfeatures.Incremental,
diff --git a/pkg/cluster/denyassignment_test.go b/pkg/cluster/denyassignment_test.go
index d2abf403d..2626edd43 100644
--- a/pkg/cluster/denyassignment_test.go
+++ b/pkg/cluster/denyassignment_test.go
@@ -8,13 +8,16 @@ import (
 	"fmt"
 	"testing"
 
+	mgmtauthorization "github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-09-01-preview/authorization"
 	mgmtfeatures "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-07-01/features"
+	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/golang/mock/gomock"
 	"github.com/sirupsen/logrus"
 
 	"github.com/Azure/ARO-RP/pkg/api"
 	"github.com/Azure/ARO-RP/pkg/util/arm"
 	"github.com/Azure/ARO-RP/pkg/util/deployment"
+	mock_authorization "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/authorization"
 	mock_features "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/features"
 	mock_env "github.com/Azure/ARO-RP/pkg/util/mocks/env"
 )
@@ -36,14 +39,46 @@ func TestCreateOrUpdateDenyAssignment(t *testing.T) {
 				},
 			},
 		},
+		subscriptionDoc: &api.SubscriptionDocument{
+			Subscription: &api.Subscription{
+				Properties: &api.SubscriptionProperties{},
+			},
+		},
 	}
 
 	for _, tt := range []struct {
-		name  string
-		mocks func(*mock_features.MockDeploymentsClient)
+		name            string
+		denyAssignments []mgmtauthorization.DenyAssignment
+		mocks           func(*mock_features.MockDeploymentsClient)
 	}{
+		{
+
+			name: "noop",
+			denyAssignments: []mgmtauthorization.DenyAssignment{
+				{
+					DenyAssignmentProperties: &mgmtauthorization.DenyAssignmentProperties{
+						ExcludePrincipals: &[]mgmtauthorization.Principal{
+							{
+								ID: to.StringPtr(fakeClusterSPObjectId),
+							},
+						},
+					},
+				},
+			},
+		},
 		{
 			name: "needs create",
+			denyAssignments: []mgmtauthorization.DenyAssignment{
+				{
+					DenyAssignmentProperties: &mgmtauthorization.DenyAssignmentProperties{
+						ExcludePrincipals: &[]mgmtauthorization.Principal{
+							{
+								ID: to.StringPtr("00000000-0000-0000-0000-000000000001"),
+							},
+						},
+					},
+				},
+			},
 			mocks: func(client *mock_features.MockDeploymentsClient) {
 				var parameters map[string]interface{}
 				client.EXPECT().CreateOrUpdateAndWait(gomock.Any(), clusterRGName, gomock.Any(), mgmtfeatures.Deployment{
@@ -53,7 +88,6 @@ func TestCreateOrUpdateDenyAssignment(t *testing.T) {
 							ContentVersion: "1.0.0.0",
 							Resources: []*arm.Resource{
 								m.denyAssignment(),
-								m.clusterServicePrincipalRoleDefinition(),
 							},
 						},
 						Parameters: parameters,
@@ -68,15 +102,18 @@ func TestCreateOrUpdateDenyAssignment(t *testing.T) {
 			defer controller.Finish()
 
 			env := mock_env.NewMockInterface(controller)
+			denyAssignments := mock_authorization.NewMockDenyAssignmentClient(controller)
 			deployments := mock_features.NewMockDeploymentsClient(controller)
 
 			env.EXPECT().DeploymentMode().Return(deployment.Production)
+			denyAssignments.EXPECT().ListForResourceGroup(gomock.Any(), gomock.Any(), gomock.Any()).Return(tt.denyAssignments, nil)
 
 			if tt.mocks != nil {
 				tt.mocks(deployments)
 			}
 
 			m.env = env
+			m.denyAssignments = denyAssignments
 			m.deployments = deployments
 
 			err := m.createOrUpdateDenyAssignment(ctx)