Merge pull request #1371 from jim-minter/rbac-fixes

RBAC fixes
2021-03-09 12:12:32 -06:00 · 2021-03-09 12:12:32 -06:00 · 2d0e9811c8
--- a/pkg/cluster/denyassignment.go
+++ b/pkg/cluster/denyassignment.go
@ -19,32 +19,19 @@ func (m *manager) createOrUpdateDenyAssignment(ctx context.Context) error {

 	// needed for AdminUpdate so it would not block other steps
 	if m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile.SPObjectID == "" {
-		m.log.Print("skipping deploySnapshotUpgradeTemplate: SPObjectID is empty")
+		m.log.Print("skipping createOrUpdateDenyAssignment: SPObjectID is empty")
 		return nil
 	}

 	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()},
+		Resources: []*arm.Resource{
+			m.denyAssignment(),
+			m.clusterServicePrincipalRoleDefinition(),
+		},
 	}

 	return m.deployARMTemplate(ctx, resourceGroup, "storage", t, nil)
--- a/pkg/cluster/denyassignment_test.go
+++ b/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,47 +36,14 @@ func TestCreateOrUpdateDenyAssignment(t *testing.T) {
 				},
 			},
 		},
-		subscriptionDoc: &api.SubscriptionDocument{
-			Subscription: &api.Subscription{
-				Properties: &api.SubscriptionProperties{},
-			},
-		},
 	}

 	for _, tt := range []struct {
-		name            string
-		denyAssignments []mgmtauthorization.DenyAssignment
-		mocks           func(*mock_features.MockDeploymentsClient)
+		name  string
+		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{
@ -87,7 +51,10 @@ func TestCreateOrUpdateDenyAssignment(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.denyAssignment()},
+							Resources: []*arm.Resource{
+								m.denyAssignment(),
+								m.clusterServicePrincipalRoleDefinition(),
+							},
 						},
 						Parameters: parameters,
 						Mode:       mgmtfeatures.Incremental,
@ -101,18 +68,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)
--- a/pkg/cluster/deploystorage_resources.go
+++ b/pkg/cluster/deploystorage_resources.go
@ -15,36 +15,10 @@ import (
 	"github.com/Azure/ARO-RP/pkg/api"
 	"github.com/Azure/ARO-RP/pkg/util/arm"
 	"github.com/Azure/ARO-RP/pkg/util/azureclient"
-	"github.com/Azure/ARO-RP/pkg/util/feature"
 	"github.com/Azure/ARO-RP/pkg/util/rbac"
 )

-var extraDenyAssignmentExclusions = map[string][]string{
-	"Microsoft.RedHatOpenShift/RedHatEngineering": {
-		"Microsoft.Network/networkInterfaces/effectiveRouteTable/action",
-	},
-}
-
 func (m *manager) denyAssignment() *arm.Resource {
-	notActions := []string{
-		"Microsoft.Network/networkSecurityGroups/join/action",
-		"Microsoft.Compute/disks/beginGetAccess/action",
-		"Microsoft.Compute/disks/endGetAccess/action",
-		"Microsoft.Compute/disks/write",
-		"Microsoft.Compute/snapshots/beginGetAccess/action",
-		"Microsoft.Compute/snapshots/endGetAccess/action",
-		"Microsoft.Compute/snapshots/write",
-		"Microsoft.Compute/snapshots/delete",
-	}
-
-	var props = m.subscriptionDoc.Subscription.Properties
-
-	for flag, exclusions := range extraDenyAssignmentExclusions {
-		if feature.IsRegisteredForFeature(props, flag) {
-			notActions = append(notActions, exclusions...)
-		}
-	}
-
 	return &arm.Resource{
 		Resource: &mgmtauthorization.DenyAssignment{
 			Name: to.StringPtr("[guid(resourceGroup().id, 'ARO cluster resource group deny assignment')]"),
@ -58,7 +32,17 @@ func (m *manager) denyAssignment() *arm.Resource {
 							"*/delete",
 							"*/write",
 						},
-						NotActions: &notActions,
+						NotActions: &[]string{
+							"Microsoft.Compute/disks/beginGetAccess/action",
+							"Microsoft.Compute/disks/endGetAccess/action",
+							"Microsoft.Compute/disks/write",
+							"Microsoft.Compute/snapshots/beginGetAccess/action",
+							"Microsoft.Compute/snapshots/delete",
+							"Microsoft.Compute/snapshots/endGetAccess/action",
+							"Microsoft.Compute/snapshots/write",
+							"Microsoft.Network/networkInterfaces/effectiveRouteTable/action",
+							"Microsoft.Network/networkSecurityGroups/join/action",
+						},
 					},
 				},
 				Scope: &m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID,
@ -81,55 +65,64 @@ func (m *manager) denyAssignment() *arm.Resource {
 	}
 }

-func (m *manager) clusterServicePrincipalRBAC() []*arm.Resource {
+func (m *manager) clusterServicePrincipalRoleDefinitionName() string {
 	infraSuffix := m.doc.OpenShiftCluster.Properties.InfraID
 	if len(infraSuffix) > 5 {
 		infraSuffix = infraSuffix[len(infraSuffix)-5:]
 	}
-	name := fmt.Sprintf("Azure Red Hat OpenShift cluster (%s)", infraSuffix)
+	return fmt.Sprintf("Azure Red Hat OpenShift cluster (%s)", infraSuffix)
+}

-	return []*arm.Resource{
-		rbac.CustomRoleDefinition(name,
-			[]mgmtauthorization.Permission{
-				{
-					Actions: &[]string{
-						//based on openshift/cluster-api-provider-azure /pkg/cloud/azure/services/disks
-						"Microsoft.Compute/disks/*",
+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/*",

-						//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/*",
+					//needed for user-initiated backup
+					"Microsoft.Compute/snapshots/*",

-						//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/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/publicips
-						"Microsoft.Network/publicIPAddresses/*",
+					//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/securitygroups
-						"Microsoft.Network/networkSecurityGroups/*",
+					//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/virtualmachines
-						"Microsoft.Compute/virtualMachines/*",
+					//based on openshift/cluster-api-provider-azure /pkg/cloud/azure/services/securitygroups
+					"Microsoft.Network/networkSecurityGroups/*",

-						//based on openshift/cluster-insgress-operator /pkg/dns/azure/client
-						"Microsoft.Network/privateDnsZones/A/*",
+					//based on openshift/cluster-api-provider-azure /pkg/cloud/azure/services/virtualmachines
+					"Microsoft.Compute/virtualMachines/*",

-						//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",
-					},
+					//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(name),
+			rbac.CustomRoleDefinitionName(m.clusterServicePrincipalRoleDefinitionName()),
 			"'"+m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile.SPObjectID+"'"),
 	}
 }
--- a/pkg/cluster/deploystorage_test.go
+++ b/pkg/cluster/deploystorage_test.go
@ -6,10 +6,8 @@ package cluster
 import (
 	"context"
 	"fmt"
-	"reflect"
 	"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"
 	"github.com/Azure/go-autorest/autorest/azure"
@ -22,76 +20,6 @@ import (
 	mock_env "github.com/Azure/ARO-RP/pkg/util/mocks/env"
 )

-func TestDenyAssignments(t *testing.T) {
-	for _, tt := range []struct {
-		name         string
-		featureFlags []string
-		want         []string
-	}{
-		{
-			name: "Not registered for snapshots feature",
-			want: []string{
-				"Microsoft.Network/networkSecurityGroups/join/action",
-				"Microsoft.Compute/disks/beginGetAccess/action",
-				"Microsoft.Compute/disks/endGetAccess/action",
-				"Microsoft.Compute/disks/write",
-				"Microsoft.Compute/snapshots/beginGetAccess/action",
-				"Microsoft.Compute/snapshots/endGetAccess/action",
-				"Microsoft.Compute/snapshots/write",
-				"Microsoft.Compute/snapshots/delete",
-			},
-		},
-		{
-			name:         "Registered for engineering feature flag",
-			featureFlags: []string{"Microsoft.RedHatOpenShift/RedHatEngineering"},
-			want: []string{
-				"Microsoft.Network/networkSecurityGroups/join/action",
-				"Microsoft.Compute/disks/beginGetAccess/action",
-				"Microsoft.Compute/disks/endGetAccess/action",
-				"Microsoft.Compute/disks/write",
-				"Microsoft.Compute/snapshots/beginGetAccess/action",
-				"Microsoft.Compute/snapshots/endGetAccess/action",
-				"Microsoft.Compute/snapshots/write",
-				"Microsoft.Compute/snapshots/delete",
-				"Microsoft.Network/networkInterfaces/effectiveRouteTable/action",
-			},
-		},
-	} {
-		t.Run(tt.name, func(t *testing.T) {
-			var features = []api.RegisteredFeatureProfile{}
-			for i := range tt.featureFlags {
-				features = append(features, api.RegisteredFeatureProfile{
-					Name:  tt.featureFlags[i],
-					State: "Registered",
-				})
-			}
-			m := &manager{
-				doc: &api.OpenShiftClusterDocument{
-					OpenShiftCluster: &api.OpenShiftCluster{
-						Properties: api.OpenShiftClusterProperties{
-							ClusterProfile: api.ClusterProfile{
-								ResourceGroupID: "testing",
-							},
-						},
-					},
-				},
-				subscriptionDoc: &api.SubscriptionDocument{
-					Subscription: &api.Subscription{
-						Properties: &api.SubscriptionProperties{
-							RegisteredFeatures: features,
-						},
-					},
-				},
-			}
-			exceptionsToDeniedActions := *(*((m.denyAssignment().Resource).(*mgmtauthorization.DenyAssignment).
-				DenyAssignmentProperties.Permissions))[0].NotActions
-			if !reflect.DeepEqual(exceptionsToDeniedActions, tt.want) {
-				t.Error(exceptionsToDeniedActions)
-			}
-		})
-	}
-}
-
 func TestCreateAndUpdateErrors(t *testing.T) {
 	ctx := context.Background()
 	clusterID := "test-cluster"