More fixups from the custom role definition fallout

* use createOrUpdateClusterServicePrincipalRBAC to migrate any clusters
  back from the custom role definition to Contributor.  Currently this
  will only take place on customer PUT but later the plan is that this
  will go in admin PUT too.
* re-add cluster deletion handling code to clean up custom role
   definitions if they still exist at cluster deletion time.
* ensure createOrUpdateDenyAssignment always PUTs the deny assignment so
  that we keep it up-to-date.

pkg/cluster/clusterserviceprincipal.go

@@ -29,18 +29,20 @@ 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) ||
-		if strings.EqualFold(*assignment.Scope, resourceGroupID) && strings.HasSuffix(strings.ToLower(*assignment.RoleDefinitionID), rbac.RoleContributor) {
+			strings.HasSuffix(strings.ToLower(*assignment.RoleDefinitionID), strings.ToLower(rbac.RoleOwner)) /* should only matter in development */ {
-			if strings.EqualFold(*assignment.PrincipalID, clusterSPObjectID) {
+			continue
-				found = true
+		}
-			} else {
+
-				toDelete = append(toDelete, assignment)
+		if strings.EqualFold(*assignment.PrincipalID, clusterSPObjectID) &&
-			}
+			strings.HasSuffix(strings.ToLower(*assignment.RoleDefinitionID), strings.ToLower(rbac.RoleContributor)) {
+			found = true
+		} else {
+			toDelete = append(toDelete, assignment)
 		}
 	}
 
@@ -52,6 +54,11 @@ func (m *manager) createOrUpdateClusterServicePrincipalRBAC(ctx context.Context)
 		}
 	}
 
+	err = m.deleteRoleDefinition(ctx)
+	if err != nil {
+		return err
+	}
+
 	if !found {
 		m.log.Info("creating cluster service principal role assignment")
 		t := &arm.Template{

pkg/cluster/delete.go

@@ -22,6 +22,7 @@ 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"
 )
 
@@ -207,6 +208,56 @@ func (m *manager) deleteResources(ctx context.Context) error {
 	return nil
 }
 
+func (m *manager) deleteRoleAssignments(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
+		}
+	}
+
+	return nil
+}
+
+func (m *manager) deleteRoleDefinition(ctx context.Context) error {
+	resourceGroupID := m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID
+
+	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, '/')
 
@@ -222,6 +273,18 @@ func (m *manager) Delete(ctx context.Context) error {
 		return err
 	}
 
+	m.log.Printf("deleting role assignments")
+	err = m.deleteRoleAssignments(ctx)
+	if err != nil {
+		return err
+	}
+
+	m.log.Printf("deleting role definition")
+	err = m.deleteRoleDefinition(ctx)
+	if err != nil {
+		return err
+	}
+
 	m.log.Printf("deleting resources")
 	err = m.deleteResources(ctx)
 	if err != nil {

pkg/cluster/denyassignment.go

@@ -24,22 +24,6 @@ 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#",

pkg/cluster/denyassignment_test.go

@@ -8,16 +8,13 @@ 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"
 )
@@ -39,46 +36,14 @@ func TestCreateOrUpdateDenyAssignment(t *testing.T) {
 				},
 			},
 		},
-		subscriptionDoc: &api.SubscriptionDocument{
-			Subscription: &api.Subscription{
-				Properties: &api.SubscriptionProperties{},
-			},
-		},
 	}
 
 	for _, tt := range []struct {
-		name            string
+		name  string
-		denyAssignments []mgmtauthorization.DenyAssignment
+		mocks func(*mock_features.MockDeploymentsClient)
-		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{
@@ -102,18 +67,15 @@ 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)