Repository: Azure/ARO-RP
Pull request #1264 (2021-01-22 09:07:10)

use custom role definition for cluster SP in new clusters
### Which issue this PR addresses:
https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/8900346/ - a step towards the goal.

### What this PR does / why we need it:

Adds a role definition and role assignment when the RedHatEngineering flag is set on the subscription.
This should allow us to test if the First Party SP can create and assign the role correctly.

### Test plan for issue:

Create cluster, scale up, scale down, deploy an application (test registry and ingress).

### Is there any documentation that needs to be updated for this PR?

N/A for now.

github-actions[bot]: Please rebase pull request.
github-actions[bot]: Please rebase pull request.
jim-minter: Applied fixes, but this conflicts with #1279.
github-actions[bot]: Please rebase pull request.

Patch:
diff --git a/pkg/cluster/clusterserviceprincipal.go b/pkg/cluster/clusterserviceprincipal.go
index 156d708fe..7f71fb3c3 100644
--- a/pkg/cluster/clusterserviceprincipal.go
+++ b/pkg/cluster/clusterserviceprincipal.go
@@ -29,13 +29,12 @@ 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 {
-		// Contributor assignments only!
-		if strings.EqualFold(*assignment.Scope, resourceGroupID) && strings.HasSuffix(strings.ToLower(*assignment.RoleDefinitionID), rbac.RoleContributor) {
+		if strings.EqualFold(*assignment.Scope, resourceGroupID) &&
+			!strings.HasSuffix(strings.ToLower(*assignment.RoleDefinitionID), strings.ToLower(rbac.RoleOwner)) /* should only matter in development */ {
 			if strings.EqualFold(*assignment.PrincipalID, clusterSPObjectID) {
 				found = true
 			} else {
@@ -45,7 +44,7 @@ func (m *manager) createOrUpdateClusterServicePrincipalRBAC(ctx context.Context)
 	}
 
 	for _, assignment := range toDelete {
-		m.log.Infof("Deleting Contributor roleAssignment %s", *assignment.Name)
+		m.log.Infof("deleting role assignment %s", *assignment.Name)
 		_, err := m.roleAssignments.Delete(ctx, *assignment.Scope, *assignment.Name)
 		if err != nil {
 			return err
@@ -53,11 +52,11 @@ func (m *manager) createOrUpdateClusterServicePrincipalRBAC(ctx context.Context)
 	}
 
 	if !found {
-		m.log.Info("Contributor roleAssignment not found for cluster service principal. Creating")
+		m.log.Info("creating cluster service principal role assignment")
 		t := &arm.Template{
 			Schema:         "https://schema.management.azure.com/schemas/2015-01-01/deploymentTemplate.json#",
 			ContentVersion: "1.0.0.0",
-			Resources:      []*arm.Resource{m.clusterServicePrincipalRBAC()},
+			Resources:      m.clusterServicePrincipalRBAC(),
 		}
 		err = m.deployARMTemplate(ctx, resourceGroup, "storage", t, nil)
 		if err != nil {
diff --git a/pkg/cluster/deploystorage.go b/pkg/cluster/deploystorage.go
index 019a8f413..56525b813 100644
--- a/pkg/cluster/deploystorage.go
+++ b/pkg/cluster/deploystorage.go
@@ -106,7 +106,6 @@ 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),
@@ -125,6 +124,8 @@ 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 5d6522cc1..d8a60d492 100644
--- a/pkg/cluster/deploystorage_resources.go
+++ b/pkg/cluster/deploystorage_resources.go
@@ -81,12 +81,51 @@ func (m *manager) denyAssignment() *arm.Resource {
 	}
 }
 
-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) clusterServicePrincipalRBAC() []*arm.Resource {
+	return []*arm.Resource{
+		rbac.CustomRoleDefinition("Azure Red Hat OpenShift Cluster",
+			[]mgmtauthorization.Permission{
+				{
+					Actions: &[]string{
+						//based on openshift/cluster-api-provider-azure /pkg/cloud/azure/services/disks
+						"Microsoft.Compute/disks/*",
+
+						//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",
+					},
+				},
+			}),
+		rbac.ResourceGroupCustomRoleAssignment(
+			rbac.CustomRoleDefinitionName("Azure Red Hat OpenShift Cluster"),
+			"'"+m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile.SPObjectID+"'"),
+	}
 }
 
 func (m *manager) clusterStorageAccount(region string) *arm.Resource {
diff --git a/pkg/util/rbac/rbac.go b/pkg/util/rbac/rbac.go
index cb57a8341..281b9e2ca 100644
--- a/pkg/util/rbac/rbac.go
+++ b/pkg/util/rbac/rbac.go
@@ -28,7 +28,7 @@ const (
 func ResourceRoleAssignment(roleID, spID, resourceType, resourceName string, condition ...interface{}) *arm.Resource {
 	resourceID := "resourceId('" + resourceType + "', " + resourceName + ")"
 
-	return ResourceRoleAssignmentWithName(roleID, spID, resourceType, resourceName, "concat("+resourceName+", '/Microsoft.Authorization/', guid("+resourceID+", "+spID+", '"+roleID+"'))", condition)
+	return ResourceRoleAssignmentWithName(roleID, spID, resourceType, resourceName, "concat("+resourceName+", '/Microsoft.Authorization/', guid("+resourceID+", "+spID+", '"+roleID+"'))", condition...)
 }
 
 // ResourceRoleAssignmentWithName returns a Resource granting roleID on the
@@ -67,27 +67,27 @@ func ResourceRoleAssignmentWithName(roleID, spID, resourceType, resourceName, na
 // resource group to spID.  Argument spID must be a valid ARM expression, e.g.
 // "'foo'" or "concat('foo')".  Use this function in new ARM templates.
 func ResourceGroupRoleAssignment(roleID, spID string, condition ...interface{}) *arm.Resource {
-	return ResourceGroupRoleAssignmentWithName(roleID, spID, "guid(resourceGroup().id, '"+spID+"', '"+roleID+"')", condition)
+	return ResourceGroupRoleAssignmentWithName(roleID, spID, "guid(resourceGroup().id, '"+spID+"', '"+roleID+"')", condition...)
 }
 
-// ResourceGroupRoleAssignmentWithName returns a Resource granting roleID on the
-// current resource group to spID.  Arguments spID and name must be valid ARM
-// expressions, e.g. "'foo'" or "concat('foo')".  Use this function in ARM
-// templates which have already been deployed, to preserve the name and avoid a
-// RoleAssignmentExists error.
-func ResourceGroupRoleAssignmentWithName(roleID, spID string, name string, condition ...interface{}) *arm.Resource {
+func resourceGroupRoleAssignmentWithDetails(roleID, spID string, name string, dependsOn []string, subscriptionScope bool, condition ...interface{}) *arm.Resource {
+	resourceIDFunction := "resourceId"
+	if subscriptionScope {
+		resourceIDFunction = "subscriptionResourceId"
+	}
 	r := &arm.Resource{
 		Resource: mgmtauthorization.RoleAssignment{
 			Name: to.StringPtr("[" + name + "]"),
 			Type: to.StringPtr("Microsoft.Authorization/roleAssignments"),
 			RoleAssignmentPropertiesWithScope: &mgmtauthorization.RoleAssignmentPropertiesWithScope{
 				Scope:            to.StringPtr("[resourceGroup().id]"),
-				RoleDefinitionID: to.StringPtr("[subscriptionResourceId('Microsoft.Authorization/roleDefinitions', '" + roleID + "')]"),
+				RoleDefinitionID: to.StringPtr("[" + resourceIDFunction + "('Microsoft.Authorization/roleDefinitions', " + roleID + ")]"),
 				PrincipalID:      to.StringPtr("[" + spID + "]"),
 				PrincipalType:    mgmtauthorization.ServicePrincipal,
 			},
 		},
 		APIVersion: azureclient.APIVersion("Microsoft.Authorization"),
+		DependsOn:  dependsOn,
 	}
 
 	if len(condition) > 0 {
@@ -96,3 +96,53 @@ func ResourceGroupRoleAssignmentWithName(roleID, spID string, name string, condi
 
 	return r
 }
+
+// ResourceGroupRoleAssignmentWithName returns a Resource granting roleID on the
+// current resource group to spID.  Arguments spID and name must be valid ARM
+// expressions, e.g. "'foo'" or "concat('foo')".  Use this function in ARM
+// templates which have already been deployed, to preserve the name and avoid a
+// RoleAssignmentExists error.
+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 83f7b2f63..917daa42a 100644
--- a/pkg/cluster/clusterserviceprincipal_test.go
+++ b/pkg/cluster/clusterserviceprincipal_test.go
@@ -87,7 +87,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:      []*arm.Resource{m.clusterServicePrincipalRBAC()},
+							Resources:      m.clusterServicePrincipalRBAC(),
 						},
 						Parameters: parameters,
 						Mode:       mgmtfeatures.Incremental,
@@ -118,7 +118,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:      []*arm.Resource{m.clusterServicePrincipalRBAC()},
+							Resources:      m.clusterServicePrincipalRBAC(),
 						},
 						Parameters: parameters,
 						Mode:       mgmtfeatures.Incremental,