ARO-13916 populate client/object IDs after dynamic validation

pkg/cluster/install.go

@@ -220,23 +220,25 @@ func (m *manager) Update(ctx context.Context) error {
 			steps.Action(m.fixupClusterMsiTenantID),
 			steps.Action(m.ensureClusterMsiCertificate),
 			steps.Action(m.initializeClusterMsiClients),
+		)
+	}
+
+	s = append(s, steps.AuthorizationRetryingAction(m.fpAuthorizer, m.validateResources))
+
+	if m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
+		s = append(s,
 			steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs),
 			steps.AuthorizationRetryingAction(m.fpAuthorizer, m.platformWorkloadIdentityIDs),
+			steps.Action(m.federateIdentityCredentials),
 		)
 	} else {
 		s = append(s,
 			// Since ServicePrincipalProfile is now a pointer and our converters re-build the struct,
 			// our update path needs to enrich the doc with SPObjectID since it was overwritten by our API on put/patch.
 			steps.AuthorizationRetryingAction(m.fpAuthorizer, m.fixupClusterSPObjectID),
-		)
-	}
-
-	s = append(s, steps.AuthorizationRetryingAction(m.fpAuthorizer, m.validateResources))
 
-	if m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
-		s = append(s, steps.Action(m.federateIdentityCredentials))
-	} else {
-		s = append(s, steps.Action(m.createOrUpdateClusterServicePrincipalRBAC)) // CSP credentials rotation flow steps
+			// CSP credentials rotation flow steps
+			steps.Action(m.createOrUpdateClusterServicePrincipalRBAC))
 	}
 
 	s = append(s,
@@ -344,6 +346,13 @@ func (m *manager) bootstrap() []steps.Step {
 		s = append(s,
 			steps.Action(m.ensureClusterMsiCertificate),
 			steps.Action(m.initializeClusterMsiClients),
+		)
+	}
+
+	s = append(s, steps.AuthorizationRetryingAction(m.fpAuthorizer, m.validateResources))
+
+	if m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
+		s = append(s,
 			steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs),
 			steps.AuthorizationRetryingAction(m.fpAuthorizer, m.platformWorkloadIdentityIDs),
 		)
@@ -353,9 +362,7 @@ func (m *manager) bootstrap() []steps.Step {
 			steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterSPObjectID),
 		)
 	}
-
 	s = append(s,
-		steps.AuthorizationRetryingAction(m.fpAuthorizer, m.validateResources),
 		steps.Action(m.ensurePreconfiguredNSG),
 		steps.Action(m.ensureACRToken),
 		steps.Action(m.ensureInfraID),

pkg/cluster/platformworkloadidentities.go

@@ -7,10 +7,8 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
-	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi"
-
 	"github.com/Azure/ARO-RP/pkg/api"
+	"github.com/Azure/ARO-RP/pkg/util/platformworkloadidentity"
 )
 
 func (m *manager) platformWorkloadIdentityIDs(ctx context.Context) error {
@@ -20,24 +18,9 @@ func (m *manager) platformWorkloadIdentityIDs(ctx context.Context) error {
 	}
 
 	identities := m.doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities
-	updatedIdentities := make(map[string]api.PlatformWorkloadIdentity, len(identities))
-
-	for operatorName, identity := range identities {
-		resourceId, err := arm.ParseResourceID(identity.ResourceID)
-		if err != nil {
-			return fmt.Errorf("platform workload identity '%s' invalid: %w", operatorName, err)
-		}
-
-		identityDetails, err := m.userAssignedIdentities.Get(ctx, resourceId.ResourceGroupName, resourceId.Name, &armmsi.UserAssignedIdentitiesClientGetOptions{})
-		if err != nil {
-			return fmt.Errorf("error occured when retrieving platform workload identity '%s' details: %w", operatorName, err)
-		}
-
-		updatedIdentities[operatorName] = api.PlatformWorkloadIdentity{
-			ResourceID: identity.ResourceID,
-			ClientID:   *identityDetails.Properties.ClientID,
-			ObjectID:   *identityDetails.Properties.PrincipalID,
-		}
+	updatedIdentities, err := platformworkloadidentity.GetPlatformWorkloadIdentityIDs(ctx, identities, m.userAssignedIdentities)
+	if err != nil {
+		return err
 	}
 
 	m.doc, err = m.db.PatchWithLease(ctx, m.doc.Key, func(doc *api.OpenShiftClusterDocument) error {

pkg/cluster/validate.go

@@ -16,6 +16,6 @@ func (m *manager) validateResources(ctx context.Context) error {
 		clusterMSICredential = m.userAssignedIdentities.GetClusterMSICredential()
 	}
 	return validate.NewOpenShiftClusterDynamicValidator(
-		m.log, m.env, m.doc.OpenShiftCluster, m.subscriptionDoc, m.fpAuthorizer, m.armRoleDefinitions, m.clusterMsiFederatedIdentityCredentials, m.platformWorkloadIdentityRolesByVersion, clusterMSICredential,
+		m.log, m.env, m.doc.OpenShiftCluster, m.subscriptionDoc, m.fpAuthorizer, m.armRoleDefinitions, m.clusterMsiFederatedIdentityCredentials, m.userAssignedIdentities, m.platformWorkloadIdentityRolesByVersion, clusterMSICredential,
 	).Dynamic(ctx)
 }

pkg/util/platformworkloadidentity/platformworkloadidentities.go

@@ -0,0 +1,39 @@
+package platformworkloadidentity
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
+	sdkmsi "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi"
+
+	"github.com/Azure/ARO-RP/pkg/api"
+	"github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/armmsi"
+)
+
+func GetPlatformWorkloadIdentityIDs(ctx context.Context, identities map[string]api.PlatformWorkloadIdentity, userAssignedIdentitiesClient armmsi.UserAssignedIdentitiesClient) (map[string]api.PlatformWorkloadIdentity, error) {
+	updatedIdentities := make(map[string]api.PlatformWorkloadIdentity, len(identities))
+
+	for operatorName, identity := range identities {
+		resourceId, err := arm.ParseResourceID(identity.ResourceID)
+		if err != nil {
+			return nil, fmt.Errorf("platform workload identity '%s' invalid: %w", operatorName, err)
+		}
+
+		identityDetails, err := userAssignedIdentitiesClient.Get(ctx, resourceId.ResourceGroupName, resourceId.Name, &sdkmsi.UserAssignedIdentitiesClientGetOptions{})
+		if err != nil {
+			return nil, fmt.Errorf("error occured when retrieving platform workload identity '%s' details: %w", operatorName, err)
+		}
+
+		updatedIdentities[operatorName] = api.PlatformWorkloadIdentity{
+			ResourceID: identity.ResourceID,
+			ClientID:   *identityDetails.Properties.ClientID,
+			ObjectID:   *identityDetails.Properties.PrincipalID,
+		}
+	}
+
+	return updatedIdentities, nil
+}

pkg/validate/dynamic/dynamic.go

@@ -88,6 +88,7 @@ type Dynamic interface {
 		platformWorkloadIdentityRolesByRoleName map[string]api.PlatformWorkloadIdentityRole,
 		roleDefinitions armauthorization.RoleDefinitionsClient,
 		clusterMsiFederatedIdentityCredentials armmsi.FederatedIdentityCredentialsClient,
+		userAssignedIdentityClient armmsi.UserAssignedIdentitiesClient,
 	) error
 }
 

pkg/validate/dynamic/platformworkloadidentityprofile.go

@@ -30,11 +30,15 @@ func (dv *dynamic) ValidatePlatformWorkloadIdentityProfile(
 	platformWorkloadIdentityRolesByRoleName map[string]api.PlatformWorkloadIdentityRole,
 	roleDefinitions armauthorization.RoleDefinitionsClient,
 	clusterMsiFederatedIdentityCredentials armmsi.FederatedIdentityCredentialsClient,
-) error {
+	userAssignedIdentitiesClient armmsi.UserAssignedIdentitiesClient,
+) (err error) {
 	dv.log.Print("ValidatePlatformWorkloadIdentityProfile")
 
 	dv.platformIdentitiesActionsMap = map[string][]string{}
-	dv.platformIdentities = oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities
+	dv.platformIdentities, err = platformworkloadidentity.GetPlatformWorkloadIdentityIDs(ctx, oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities, userAssignedIdentitiesClient)
+	if err != nil {
+		return err
+	}
 
 	// Check if any required platform identity is missing
 	if len(dv.platformIdentities) != len(platformWorkloadIdentityRolesByRoleName) {

pkg/validate/dynamic/platformworkloadidentityprofile_test.go

@@ -1066,6 +1066,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) {
 			_env := mock_env.NewMockInterface(controller)
 			roleDefinitions := mock_armauthorization.NewMockRoleDefinitionsClient(controller)
 			federatedIdentityCredentials := mock_armmsi.NewMockFederatedIdentityCredentialsClient(controller)
+			userAssignedIdentitiesClient := mock_armmsi.NewMockUserAssignedIdentitiesClient(controller)
 
 			dv := &dynamic{
 				env:            _env,
@@ -1077,7 +1078,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) {
 				tt.mocks(roleDefinitions, federatedIdentityCredentials)
 			}
 
-			err := dv.ValidatePlatformWorkloadIdentityProfile(ctx, tt.oc, tt.platformIdentityRoles, roleDefinitions, federatedIdentityCredentials)
+			err := dv.ValidatePlatformWorkloadIdentityProfile(ctx, tt.oc, tt.platformIdentityRoles, roleDefinitions, federatedIdentityCredentials, userAssignedIdentitiesClient)
 			utilerror.AssertErrorMessage(t, err, tt.wantErr)
 
 			if tt.wantPlatformIdentities != nil && !reflect.DeepEqual(tt.wantPlatformIdentities, dv.platformIdentities) {

pkg/validate/openshiftcluster_validatedynamic.go

@@ -40,6 +40,7 @@ func NewOpenShiftClusterDynamicValidator(
 	fpAuthorizer autorest.Authorizer,
 	roleDefinitions armauthorization.RoleDefinitionsClient,
 	clusterMsiFederatedIdentityCredentials armmsi.FederatedIdentityCredentialsClient,
+	userAssignedIdentitiesClient armmsi.UserAssignedIdentitiesClient,
 	platformWorkloadIdentityRolesByVersion platformworkloadidentity.PlatformWorkloadIdentityRolesByVersion,
 	clusterMSICredential azcore.TokenCredential,
 ) OpenShiftClusterDynamicValidator {
@@ -54,6 +55,7 @@ func NewOpenShiftClusterDynamicValidator(
 		clusterMsiFederatedIdentityCredentials: clusterMsiFederatedIdentityCredentials,
 		platformWorkloadIdentityRolesByVersion: platformWorkloadIdentityRolesByVersion,
 		clusterMSICredential:                   clusterMSICredential,
+		userAssignedIdentitiesClient:           userAssignedIdentitiesClient,
 	}
 }
 
@@ -68,6 +70,7 @@ type openShiftClusterDynamicValidator struct {
 	clusterMsiFederatedIdentityCredentials armmsi.FederatedIdentityCredentialsClient
 	platformWorkloadIdentityRolesByVersion platformworkloadidentity.PlatformWorkloadIdentityRolesByVersion
 	clusterMSICredential                   azcore.TokenCredential
+	userAssignedIdentitiesClient           armmsi.UserAssignedIdentitiesClient
 }
 
 // ensureAccessTokenClaims can detect an error when the service principal (fp, cluster sp) has accidentally deleted from
@@ -220,7 +223,7 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error {
 		if err != nil {
 			return err
 		}
-		err = spDynamic.ValidatePlatformWorkloadIdentityProfile(ctx, dv.oc, dv.platformWorkloadIdentityRolesByVersion.GetPlatformWorkloadIdentityRolesByRoleName(), dv.roleDefinitions, dv.clusterMsiFederatedIdentityCredentials)
+		err = spDynamic.ValidatePlatformWorkloadIdentityProfile(ctx, dv.oc, dv.platformWorkloadIdentityRolesByVersion.GetPlatformWorkloadIdentityRolesByRoleName(), dv.roleDefinitions, dv.clusterMsiFederatedIdentityCredentials, dv.userAssignedIdentitiesClient)
 		if err != nil {
 			return err
 		}