Azure: check existing role assignments before attempting to create a …

…new one

Currently when re-reconciling an already-provisioned CredentialsRequest in Azure, the actuator will just always attempt to create a role assignment even if it already exists. There is error handling to catch the "RoleAssignmentExists" error, and it just moves along to the next task.

This results in the Resource Group where the cluster is installed having periodic entries in the Activity Log recording these (non-critical) errors:

```
{
    "authorization": {
        "action": "Microsoft.Authorization/roleAssignments/write",
        "scope": "/subscriptions/SUBSCRIPTION_ID/resourceGroups/jdiaz-az-gpqgx-rg/providers/Microsoft.Authorization/roleAssignments/94025186-5e7b-4e18-88de-4625cac3ed19"
    },
    "level": "Error",
    "operationName": {
        "value": "Microsoft.Authorization/roleAssignments/write",
        "localizedValue": "Create role assignment"
    },
    "resourceGroupName": "jdiaz-az-gpqgx-rg",
    "subStatus": {
        "value": "Conflict",
        "localizedValue": "Conflict (HTTP Status Code: 409)"
    },
    "properties": {
        "statusCode": "Conflict",
        "serviceRequestId": "931445cc-fbc6-469a-a6f4-2f7750788255",
        "statusMessage": "{\"error\":{\"code\":\"RoleAssignmentExists\",\"message\":\"The role assignment already exists.\"}}"
    },
}
```

Change the logic to pull the current list of Role Assignments so that we can avoid making unnecessary CreateRoleAssignment calls.

pkg/azure/actuator_test.go

@@ -47,13 +47,14 @@ import (
 )
 
 const (
-	testNamespace       = "default"
-	testAppRegName      = "Test App Reg"
-	testAppRegID        = "some-unique-app-id"
-	testAppRegObjID     = "some-unique-app-obj-id"
-	testCredRequestName = "testCredRequest"
-	testRandomSuffix    = "rando"
-	testRoleName        = "Contributor"
+	testNamespace        = "default"
+	testAppRegName       = "Test App Reg"
+	testAppRegID         = "some-unique-app-id"
+	testAppRegObjID      = "some-unique-app-obj-id"
+	testCredRequestName  = "testCredRequest"
+	testRandomSuffix     = "rando"
+	testRoleName         = "Contributor"
+	testRoleDefinitionID = "some-role-def-id"
 )
 
 var (
@@ -303,6 +304,40 @@ func TestActuator(t *testing.T) {
 				// validation done in gomock.EXEPCT()
 			},
 		},
+		{
+			name:               "Skip role assignment creation when they already exist",
+			existing:           defaultExistingObjects(),
+			credentialsRequest: testCredentialsRequest(t),
+			mockRoleAssignmentsClient: func(mockCtrl *gomock.Controller) *azuremock.MockRoleAssignmentsClient {
+				client := azuremock.NewMockRoleAssignmentsClient(mockCtrl)
+				expectedFilter := fmt.Sprintf("principalId eq '%s'", *testServicePrincipal().ObjectID)
+				// ensure that when listing the role assignments, everything is already assigned
+				client.EXPECT().List(gomock.Any(), gomock.Eq(expectedFilter)).Return(
+					[]authorization.RoleAssignment{
+						{
+							Properties: &authorization.RoleAssignmentPropertiesWithScope{
+								RoleDefinitionID: to.StringPtr(testRoleDefinitionID),
+								Scope:            to.StringPtr("subscriptions//resourceGroups/" + testResourceGroupName),
+							},
+						},
+						{
+							Properties: &authorization.RoleAssignmentPropertiesWithScope{
+								RoleDefinitionID: to.StringPtr(testRoleDefinitionID),
+								Scope:            to.StringPtr("subscriptions//resourceGroups/" + testDNSResourceGroupName),
+							},
+						},
+					}, nil,
+				).Times(2) // once for gathering current role assignments, and once for checking for extra role assignments
+
+				return client
+			},
+			op: func(actuator *azure.Actuator, cr *minterv1.CredentialsRequest) error {
+				return actuator.Update(context.TODO(), cr)
+			},
+			validate: func(t *testing.T, c client.Client) {
+				// validation done in gomock.EXPECT()
+			},
+		},
 	}
 
 	for _, test := range tests {
@@ -460,7 +495,7 @@ func mockServicePrincipalClientNoCalls(mockCtrl *gomock.Controller) *azuremock.M
 func testRoleDefinition() authorization.RoleDefinition {
 	rd := authorization.RoleDefinition{
 		Name: to.StringPtr(testRoleName),
-		ID:   to.StringPtr("some-role-def-id"),
+		ID:   to.StringPtr(testRoleDefinitionID),
 	}
 	return rd
 }
@@ -504,6 +539,12 @@ func defaultMockRoleAssignmentsClient(mockCtrl *gomock.Controller) *azuremock.Mo
 		testRoleAssignment(),
 		nil,
 	)
+	// One list when gathering current role assignments
+	client.EXPECT().List(gomock.Any(), gomock.Any()).Return(
+		[]authorization.RoleAssignment{}, nil,
+	)
+
+	// One more list when checking whether any extra roles are assigned
 	client.EXPECT().List(gomock.Any(), gomock.Any()).Return(
 		[]authorization.RoleAssignment{testRoleAssignment()},
 		nil,

pkg/azure/minter.go

@@ -236,11 +236,34 @@ func (credMinter *AzureCredentialsMinter) AssignResourceScopedRole(ctx context.C
 		return fmt.Errorf("more than one role %q found", targetRole)
 	}
 
+	credMinter.logger.Debugf("getting current role assignments for service principal %s", principalName)
+	filter := fmt.Sprintf("principalId eq '%s'", principalID)
+	currentRoleAssignments, err := credMinter.roleAssignmentsClient.List(ctx, filter)
+	if err != nil {
+		credMinter.logger.WithError(err).Error("failed to list role assignments")
+		return err
+	}
+
 	for _, resourceGroup := range resourceGroups {
 		scope := "subscriptions/" + credMinter.subscriptionID + "/resourceGroups/" + resourceGroup
+
+		// check whether assignment already exists
+		alreadyExists := false
+		for _, r := range currentRoleAssignments {
+			if strings.Contains(*r.Properties.Scope, scope) {
+				credMinter.logger.Debugf("role %s already assigned in resource group %s for service principal %s", targetRole, resourceGroup, principalName)
+				alreadyExists = true
+				break
+			}
+		}
+		if alreadyExists {
+			continue
+		}
+
 		raName := uuid.NewV4().String()
 
 		err = wait.PollImmediate(5*time.Second, 60*time.Second, func() (bool, error) {
+			credMinter.logger.Debugf("assigning role %s for resource group %s", *roleDefinition.Name, resourceGroup)
 			_, err = credMinter.roleAssignmentsClient.Create(ctx, scope, raName, authorization.RoleAssignmentCreateParameters{
 				Properties: &authorization.RoleAssignmentProperties{
 					RoleDefinitionID: roleDefinition.ID,

pkg/azure/passthrough_test.go

@@ -132,13 +132,14 @@ var (
 		},
 	}
 
-	clusterDNS = openshiftapiv1.DNS{
+	testDNSResourceGroupName = "os4-common"
+	clusterDNS               = openshiftapiv1.DNS{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "cluster",
 		},
 		Spec: openshiftapiv1.DNSSpec{
 			PublicZone: &openshiftapiv1.DNSZone{
-				ID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/os4-common/providers/Microsoft.Network/dnszones/devcluster.openshift.com",
+				ID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/" + testDNSResourceGroupName + "/providers/Microsoft.Network/dnszones/devcluster.openshift.com",
 			},
 		},
 	}