Fix Managed Identity Cluster creation dynamic validation flow (#3891)

* Fix Dynamic Validation Flow for Workload Identity

* fix the mock msi object id export

* Remove unwanted code

* update OIDC_STORAGE_ACCOUNT_NAME local env setup

* ARO-11049 resolve comments

Makefile

@@ -533,6 +533,7 @@ run-rp: ci-rp podman-secrets
 		-e ARO_ADOPT_BY_HIVE="true" \
 		-e MOCK_MSI_TENANT_ID \
 		-e MOCK_MSI_CLIENT_ID \
+		-e MOCK_MSI_OBJECT_ID \
 		-e MOCK_MSI_CERT \
 		--secret aks.kubeconfig,target=/app/secrets/aks.kubeconfig \
 		--secret proxy-client.key,target=/app/secrets/proxy-client.key \

docs/deploy-development-rp.md

@@ -77,6 +77,7 @@ mock a cluster MSI. This script will also create the platform identities, platfo
    - `RP_MODE`: Set to `development` to use a development RP running at
      https://localhost:8443/.
    - `MOCK_MSI_CLIENT_ID`: Client ID for service principal that mocks cluster MSI (see previous step).
+   - `MOCK_MSI_OBJECT_ID`: Object ID for service principal that mocks cluster MSI (see previous step).
    - `MOCK_MSI_CERT`: Base64 encoded certificate for service principal that mocks cluster MSI (see previous step).
    - `MOCK_MSI_TENANT_ID`: Tenant ID for service principal that mocks cluster MSI (see previous step).
    - `PLATFORM_WORKLOAD_IDENTITY_ROLE_SETS`: The platform workload identity role sets (see previous step or value in `local_dev_env.sh`).

docs/prepare-a-shared-rp-development-environment.md

@@ -436,7 +436,7 @@ az ad app credential reset \
    PARENT_DOMAIN_RESOURCEGROUP='$PARENT_DOMAIN_RESOURCEGROUP'
    export DOMAIN_NAME="\$LOCATION.\$PARENT_DOMAIN_NAME"
    export AZURE_ENVIRONMENT='AzurePublicCloud'
-   export OIDC_STORAGE_ACCOUNT_NAME="${RESOURCEGROUP//-}oic"
+   export OIDC_STORAGE_ACCOUNT_NAME="\${RESOURCEGROUP//-}oic"
    EOF
    ```
 

env.example

@@ -13,6 +13,7 @@ export ARO_IMAGE=arointsvc.azurecr.io/aro:latest
 # out or remove them from your env file if you're only going to be creating service principal
 # clusters.
 export MOCK_MSI_CLIENT_ID="replace_with_value_output_by_hack/devtools/msi.sh"
+export MOCK_MSI_OBJECT_ID="replace_with_value_output_by_hack/devtools/msi.sh"
 export MOCK_MSI_CERT="replace_with_value_output_by_hack/devtools/msi.sh"
 export MOCK_MSI_TENANT_ID="replace_with_value_output_by_hack/devtools/msi.sh"
 export PLATFORM_WORKLOAD_IDENTITY_ROLE_SETS="replace_with_value_output_by_hack/devtools/msi.sh"

hack/devtools/local_dev_env.sh

@@ -121,6 +121,10 @@ get_mock_msi_tenantID() {
     echo "$1" | jq -r .tenant
 }
 
+get_mock_msi_objectID() {
+    az ad sp list --all --filter "appId eq '$1'" | jq -r ".[] | .id"
+}
+
 get_mock_msi_cert() {
     certFilePath=$(echo "$1" | jq -r '.fileWithCertAndPrivateKey')
     base64EncodedCert=$(base64 -w 0 "$certFilePath")
@@ -234,6 +238,7 @@ create_miwi_env_file() {
     mockClientID=$(get_mock_msi_clientID "$mockMSI")
     mockTenantID=$(get_mock_msi_tenantID "$mockMSI")
     mockCert=$(get_mock_msi_cert "$mockMSI")
+    mockObjectID=$(get_mock_msi_objectID "$mockClientID")
 
     setup_platform_identity
     cluster_msi_role_assignment "${mockClientID}"
@@ -243,6 +248,7 @@ export LOCATION=eastus
 export ARO_IMAGE=arointsvc.azurecr.io/aro:latest
 export RP_MODE=development # to use a development RP running at https://localhost:8443/
 export MOCK_MSI_CLIENT_ID="$mockClientID"
+export MOCK_MSI_OBJECT_ID="$mockObjectID"
 export MOCK_MSI_TENANT_ID="$mockTenantID"
 export MOCK_MSI_CERT="$mockCert"
 export PLATFORM_WORKLOAD_IDENTITY_ROLE_SETS="$PLATFORM_WORKLOAD_IDENTITY_ROLE_SETS"

hack/devtools/msi.sh

@@ -29,12 +29,14 @@ fi
 mockClientID=$(get_mock_msi_clientID "$sp")
 mockTenantID=$(get_mock_msi_tenantID "$sp")
 base64EncodedCert=$(get_mock_msi_cert "$sp")
+mockObjectID=$(get_mock_msi_objectID "$mockClientID")
 
 setup_platform_identity
 cluster_msi_role_assignment "${mockClientID}"
 
 # Print the extracted values
 echo "Cluster MSI Client ID: $mockClientID"
+echo "Cluster MSI Object ID: $mockObjectID"
 echo "Cluster MSI Tenant ID: $mockTenantID"
 echo "Cluster MSI Base64 Encoded Certificate: $base64EncodedCert"
 echo "Platform workload identity role sets: $PLATFORM_WORKLOAD_IDENTITY_ROLE_SETS"

pkg/cluster/cluster.go

@@ -225,7 +225,7 @@ func New(ctx context.Context, log *logrus.Entry, _env env.Interface, db database
 		return nil, err
 	}
 
-	armRoleDefinitionsClient, err := armauthorization.NewArmRoleDefinitionsClient(fpCredClusterTenant, clientOptions)
+	armRoleDefinitionsClient, err := armauthorization.NewArmRoleDefinitionsClient(fpCredClusterTenant, r.SubscriptionID, clientOptions)
 	if err != nil {
 		return nil, err
 	}

pkg/cluster/deploybaseresources.go

@@ -195,11 +195,16 @@ func (m *manager) deployBaseResourceTemplate(ctx context.Context) error {
 		m.storageAccount(m.doc.OpenShiftCluster.Properties.ImageRegistryStorageAccountName, azureRegion, ocpSubnets, true),
 		m.storageAccountBlobContainer(m.doc.OpenShiftCluster.Properties.ImageRegistryStorageAccountName, "image-registry"),
 		m.clusterNSG(infraID, azureRegion),
-		m.clusterServicePrincipalRBAC(),
 		m.networkPrivateLinkService(azureRegion),
 		m.networkInternalLoadBalancer(azureRegion),
 	}
 
+	if !m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
+		resources = append(resources,
+			m.clusterServicePrincipalRBAC(),
+		)
+	}
+
 	// Create a public load balancer routing if needed
 	if m.doc.OpenShiftCluster.Properties.NetworkProfile.OutboundType == api.OutboundTypeLoadbalancer {
 		m.newPublicLoadBalancer(ctx, &resources)

pkg/env/prod.go

@@ -406,6 +406,7 @@ func (p *prod) MsiDataplaneClientOptions(msiResourceId *arm.ResourceID) (*policy
 	if p.FeatureIsSet(FeatureUseMockMsiRp) {
 		keysToValidate := []string{
 			"MOCK_MSI_CLIENT_ID",
+			"MOCK_MSI_OBJECT_ID",
 			"MOCK_MSI_CERT",
 			"MOCK_MSI_TENANT_ID",
 		}
@@ -425,6 +426,7 @@ func (p *prod) MsiDataplaneClientOptions(msiResourceId *arm.ResourceID) (*policy
 							ClientID:                   ptr.To(os.Getenv("MOCK_MSI_CLIENT_ID")),
 							ClientSecret:               ptr.To(os.Getenv("MOCK_MSI_CERT")),
 							TenantID:                   ptr.To(os.Getenv("MOCK_MSI_TENANT_ID")),
+							ObjectID:                   ptr.To(os.Getenv("MOCK_MSI_OBJECT_ID")),
 							ResourceID:                 ptr.To(msiResourceId.String()),
 							AuthenticationEndpoint:     ptr.To(p.Environment().Cloud.ActiveDirectoryAuthorityHost),
 							CannotRenewAfter:           &placeholder,
@@ -437,7 +439,6 @@ func (p *prod) MsiDataplaneClientOptions(msiResourceId *arm.ResourceID) (*policy
 								XMSAzNwperimid: []*string{&placeholder},
 								XMSAzTm:        &placeholder,
 							},
-							ObjectID: &placeholder,
 						},
 					},
 				},

pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go

@@ -5,6 +5,7 @@ package armauthorization
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
@@ -17,13 +18,20 @@ type RoleDefinitionsClient interface {
 
 type ArmRoleDefinitionsClient struct {
 	*armauthorization.RoleDefinitionsClient
+	subscriptionID string
 }
 
 var _ RoleDefinitionsClient = &ArmRoleDefinitionsClient{}
 
-func NewArmRoleDefinitionsClient(credential azcore.TokenCredential, options *arm.ClientOptions) (*ArmRoleDefinitionsClient, error) {
+func NewArmRoleDefinitionsClient(credential azcore.TokenCredential, subscriptionID string, options *arm.ClientOptions) (*ArmRoleDefinitionsClient, error) {
 	client, err := armauthorization.NewRoleDefinitionsClient(credential, options)
 	return &ArmRoleDefinitionsClient{
 		RoleDefinitionsClient: client,
+		subscriptionID:        subscriptionID,
 	}, err
 }
+
+func (client ArmRoleDefinitionsClient) GetByID(ctx context.Context, roleDefinitionID string, options *armauthorization.RoleDefinitionsClientGetByIDOptions) (armauthorization.RoleDefinitionsClientGetByIDResponse, error) {
+	roleID := fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", client.subscriptionID, roleDefinitionID)
+	return client.RoleDefinitionsClient.GetByID(ctx, roleID, options)
+}

pkg/validate/dynamic/platformworkloadidentityprofile.go

@@ -11,6 +11,7 @@ import (
 	"github.com/Azure/ARO-RP/pkg/api"
 	"github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/armauthorization"
 	"github.com/Azure/ARO-RP/pkg/util/rbac"
+	"github.com/Azure/ARO-RP/pkg/util/stringutils"
 	"github.com/Azure/ARO-RP/pkg/util/version"
 )
 
@@ -63,7 +64,8 @@ func (dv *dynamic) ValidatePlatformWorkloadIdentityProfile(ctx context.Context,
 	}
 
 	for _, role := range platformWorkloadIdentityRolesByRoleName {
-		actions, err := getActionsForRoleDefinition(ctx, role.RoleDefinitionID, roleDefinitions)
+		roleDefinitionID := stringutils.LastTokenByte(role.RoleDefinitionID, '/')
+		actions, err := getActionsForRoleDefinition(ctx, roleDefinitionID, roleDefinitions)
 		if err != nil {
 			return err
 		}