Fix MIWI Cluster update flow to add new openshift operator identity (#…

…4037) * ARO-13916 fix new operator identity addition flow for miwi cluster update * ARO-13916 add the federateIdentityCredentials step back to update * ARO-13916 populate client/object IDs after dynamic validation * ARO-13916 update dynamic validation unit test cases * ARO-13916 persist user assigned identities with client and object id after dynamic validation
Azure · Jan 15, 2025 · bcc519c · bcc519c
1 parent e723a44
commit bcc519c
Show file tree

Hide file tree

Showing 12 changed files with 144 additions and 52 deletions.
diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go
@@ -137,6 +137,7 @@ type manager struct {
 	openShiftClusterDocumentVersioner openShiftClusterDocumentVersioner
 
 	platformWorkloadIdentityRolesByVersion platformworkloadidentity.PlatformWorkloadIdentityRolesByVersion
+	platformWorkloadIdentities             map[string]api.PlatformWorkloadIdentity
 }
 
 // New returns a cluster manager

diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go
@@ -220,6 +220,7 @@ func (m *manager) Update(ctx context.Context) error {
 			steps.Action(m.fixupClusterMsiTenantID),
 			steps.Action(m.ensureClusterMsiCertificate),
 			steps.Action(m.initializeClusterMsiClients),
+			steps.Action(m.platformWorkloadIdentityIDs),
 		)
 	}
 
@@ -228,7 +229,7 @@ func (m *manager) Update(ctx context.Context) error {
 	if m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
 		s = append(s,
 			steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs),
-			steps.AuthorizationRetryingAction(m.fpAuthorizer, m.platformWorkloadIdentityIDs),
+			steps.AuthorizationRetryingAction(m.fpAuthorizer, m.persistPlatformWorkloadIdentityIDs),
 			steps.Action(m.federateIdentityCredentials),
 		)
 	} else {
@@ -238,8 +239,7 @@ func (m *manager) Update(ctx context.Context) error {
 			steps.AuthorizationRetryingAction(m.fpAuthorizer, m.fixupClusterSPObjectID),
 
 			// CSP credentials rotation flow steps
-			steps.Action(m.createOrUpdateClusterServicePrincipalRBAC),
-		)
+			steps.Action(m.createOrUpdateClusterServicePrincipalRBAC))
 	}
 
 	s = append(s,
@@ -347,18 +347,24 @@ func (m *manager) bootstrap() []steps.Step {
 		s = append(s,
 			steps.Action(m.ensureClusterMsiCertificate),
 			steps.Action(m.initializeClusterMsiClients),
+			steps.Action(m.platformWorkloadIdentityIDs),
+		)
+	}
+
+	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.AuthorizationRetryingAction(m.fpAuthorizer, m.persistPlatformWorkloadIdentityIDs),
 		)
 	} else {
 		s = append(s,
 			steps.Action(m.initializeClusterSPClients),
 			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),

diff --git a/pkg/cluster/platformworkloadidentities.go b/pkg/cluster/platformworkloadidentities.go
@@ -13,8 +13,20 @@ import (
 	"github.com/Azure/ARO-RP/pkg/api"
 )
 
+func (m *manager) persistPlatformWorkloadIdentityIDs(ctx context.Context) (err error) {
+	if !m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
+		return fmt.Errorf("persistPlatformWorkloadIdentityIDs called for CSP cluster")
+	}
+
+	m.doc, err = m.db.PatchWithLease(ctx, m.doc.Key, func(doc *api.OpenShiftClusterDocument) error {
+		doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities = m.platformWorkloadIdentities
+		return nil
+	})
+
+	return err
+}
+
 func (m *manager) platformWorkloadIdentityIDs(ctx context.Context) error {
-	var err error
 	if !m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
 		return fmt.Errorf("platformWorkloadIdentityIDs called for CSP cluster")
 	}
@@ -40,10 +52,6 @@ func (m *manager) platformWorkloadIdentityIDs(ctx context.Context) error {
 		}
 	}
 
-	m.doc, err = m.db.PatchWithLease(ctx, m.doc.Key, func(doc *api.OpenShiftClusterDocument) error {
-		doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities = updatedIdentities
-		return nil
-	})
-
-	return err
+	m.platformWorkloadIdentities = updatedIdentities
+	return nil
 }
diff --git a/pkg/cluster/platformworkloadidentities_test.go b/pkg/cluster/platformworkloadidentities_test.go
@@ -186,6 +186,11 @@ func TestPlatformWorkloadIdentityIDs(t *testing.T) {
 			err := m.platformWorkloadIdentityIDs(ctx)
 			utilerror.AssertErrorMessage(t, err, tt.wantErr)
 
+			if err == nil {
+				err = m.persistPlatformWorkloadIdentityIDs(ctx)
+				utilerror.AssertErrorMessage(t, err, tt.wantErr)
+			}
+
 			if tt.wantIdentities != nil {
 				assert.Equal(t, *tt.wantIdentities, m.doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities)
 			}

diff --git a/pkg/cluster/validate.go b/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.platformWorkloadIdentities, m.platformWorkloadIdentityRolesByVersion, clusterMSICredential,
 	).Dynamic(ctx)
 }
diff --git a/pkg/cluster/workloadidentityresources.go b/pkg/cluster/workloadidentityresources.go
@@ -35,7 +35,7 @@ func (m *manager) generateWorkloadIdentityResources() (map[string]kruntime.Objec
 	}
 
 	resources := map[string]kruntime.Object{}
-	if platformWorkloadIdentitySecrets, err := m.generatePlatformWorkloadIdentitySecrets(); err != nil {
+	if platformWorkloadIdentitySecrets, _, err := m.generatePlatformWorkloadIdentitySecretsAndNamespaces(); err != nil {
 		return nil, err
 	} else {
 		for _, secret := range platformWorkloadIdentitySecrets {
@@ -60,30 +60,34 @@ func (m *manager) generateWorkloadIdentityResources() (map[string]kruntime.Objec
 }
 
 func (m *manager) deployPlatformWorkloadIdentitySecrets(ctx context.Context) error {
-	secrets, err := m.generatePlatformWorkloadIdentitySecrets()
+	secrets, namespaces, err := m.generatePlatformWorkloadIdentitySecretsAndNamespaces()
 	if err != nil {
 		return err
 	}
-	resources := make([]kruntime.Object, len(secrets))
-	for i, secret := range secrets {
-		resources[i] = secret
+	resources := []kruntime.Object{}
+	for _, namespace := range namespaces {
+		resources = append(resources, namespace)
+	}
+	for _, secret := range secrets {
+		resources = append(resources, secret)
 	}
 
 	return m.ch.Ensure(ctx, resources...)
 }
 
-func (m *manager) generatePlatformWorkloadIdentitySecrets() ([]*corev1.Secret, error) {
+func (m *manager) generatePlatformWorkloadIdentitySecretsAndNamespaces() ([]*corev1.Secret, []*corev1.Namespace, error) {
 	subscriptionId := m.subscriptionDoc.ID
 	tenantId := m.subscriptionDoc.Subscription.Properties.TenantID
 	region := m.doc.OpenShiftCluster.Location
 
 	roles := m.platformWorkloadIdentityRolesByVersion.GetPlatformWorkloadIdentityRolesByRoleName()
 
 	secrets := []*corev1.Secret{}
+	namespaces := []*corev1.Namespace{}
 	for name, identity := range m.doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities {
 		role, exists := roles[name]
 		if !exists {
-			return nil, platformworkloadidentity.GetPlatformWorkloadIdentityMismatchError(m.doc.OpenShiftCluster, roles)
+			return nil, nil, platformworkloadidentity.GetPlatformWorkloadIdentityMismatchError(m.doc.OpenShiftCluster, roles)
 		}
 		// Skip creating a secret for the ARO Operator. This will be
 		// generated by the ARO Operator deployer instead
@@ -110,9 +114,19 @@ func (m *manager) generatePlatformWorkloadIdentitySecrets() ([]*corev1.Secret, e
 				"azure_federated_token_file": azureFederatedTokenFileLocation,
 			},
 		})
+
+		namespaces = append(namespaces, &corev1.Namespace{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: corev1.SchemeGroupVersion.Identifier(),
+				Kind:       "Namespace",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name: role.SecretLocation.Namespace,
+			},
+		})
 	}
 
-	return secrets, nil
+	return secrets, namespaces, nil
 }
 
 func (m *manager) generateCloudCredentialOperatorSecret() (*corev1.Secret, error) {

diff --git a/pkg/cluster/workloadidentityresources_test.go b/pkg/cluster/workloadidentityresources_test.go
@@ -317,17 +317,19 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
 	location := "eastus"
 
 	for _, tt := range []struct {
-		name       string
-		identities map[string]api.PlatformWorkloadIdentity
-		roles      []api.PlatformWorkloadIdentityRole
-		want       []*corev1.Secret
-		wantErr    string
+		name           string
+		identities     map[string]api.PlatformWorkloadIdentity
+		roles          []api.PlatformWorkloadIdentityRole
+		wantSecrets    []*corev1.Secret
+		wantNamespaces []*corev1.Namespace
+		wantErr        string
 	}{
 		{
-			name:       "no identities, no secrets",
-			identities: map[string]api.PlatformWorkloadIdentity{},
-			roles:      []api.PlatformWorkloadIdentityRole{},
-			want:       []*corev1.Secret{},
+			name:           "no identities, no secrets",
+			identities:     map[string]api.PlatformWorkloadIdentity{},
+			roles:          []api.PlatformWorkloadIdentityRole{},
+			wantSecrets:    []*corev1.Secret{},
+			wantNamespaces: []*corev1.Namespace{},
 		},
 		{
 			name: "converts cluster PWIs if a role definition is present",
@@ -355,7 +357,7 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
 					},
 				},
 			},
-			want: []*corev1.Secret{
+			wantSecrets: []*corev1.Secret{
 				{
 					TypeMeta: metav1.TypeMeta{
 						APIVersion: "v1",
@@ -393,6 +395,26 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
 					},
 				},
 			},
+			wantNamespaces: []*corev1.Namespace{
+				{
+					TypeMeta: metav1.TypeMeta{
+						APIVersion: "v1",
+						Kind:       "Namespace",
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "openshift-foo",
+					},
+				},
+				{
+					TypeMeta: metav1.TypeMeta{
+						APIVersion: "v1",
+						Kind:       "Namespace",
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "openshift-bar",
+					},
+				},
+			},
 		},
 		{
 			name: "error - identities with unexpected operator name present",
@@ -401,9 +423,10 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
 					ClientID: "00f00f00-0f00-0f00-0f00-f00f00f00f00",
 				},
 			},
-			roles:   []api.PlatformWorkloadIdentityRole{},
-			want:    []*corev1.Secret{},
-			wantErr: fmt.Sprintf("400: %s: properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities: There's a mismatch between the required and expected set of platform workload identities for the requested OpenShift minor version '%s'. The required platform workload identities are '[]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14"),
+			roles:          []api.PlatformWorkloadIdentityRole{},
+			wantSecrets:    []*corev1.Secret{},
+			wantNamespaces: []*corev1.Namespace{},
+			wantErr:        fmt.Sprintf("400: %s: properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities: There's a mismatch between the required and expected set of platform workload identities for the requested OpenShift minor version '%s'. The required platform workload identities are '[]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14"),
 		},
 		{
 			name: "skips ARO operator identity",
@@ -431,7 +454,7 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
 					},
 				},
 			},
-			want: []*corev1.Secret{
+			wantSecrets: []*corev1.Secret{
 				{
 					TypeMeta: metav1.TypeMeta{
 						APIVersion: "v1",
@@ -451,6 +474,17 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
 					},
 				},
 			},
+			wantNamespaces: []*corev1.Namespace{
+				{
+					TypeMeta: metav1.TypeMeta{
+						APIVersion: "v1",
+						Kind:       "Namespace",
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "openshift-foo",
+					},
+				},
+			},
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
@@ -484,10 +518,11 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
 					controller, tt.roles,
 				),
 			}
-			got, err := m.generatePlatformWorkloadIdentitySecrets()
+			gotSecrets, gotNamespaces, err := m.generatePlatformWorkloadIdentitySecretsAndNamespaces()
 
 			utilerror.AssertErrorMessage(t, err, tt.wantErr)
-			assert.ElementsMatch(t, got, tt.want)
+			assert.ElementsMatch(t, gotSecrets, tt.wantSecrets)
+			assert.ElementsMatch(t, gotNamespaces, tt.wantNamespaces)
 		})
 	}
 }

diff --git a/pkg/util/mocks/dynamic/dynamic.go b/pkg/util/mocks/dynamic/dynamic.go
diff --git a/pkg/validate/dynamic/dynamic.go b/pkg/validate/dynamic/dynamic.go
@@ -88,6 +88,7 @@ type Dynamic interface {
 		platformWorkloadIdentityRolesByRoleName map[string]api.PlatformWorkloadIdentityRole,
 		roleDefinitions armauthorization.RoleDefinitionsClient,
 		clusterMsiFederatedIdentityCredentials armmsi.FederatedIdentityCredentialsClient,
+		platformWorkloadIdentities map[string]api.PlatformWorkloadIdentity,
 	) error
 }
 

diff --git a/pkg/validate/dynamic/platformworkloadidentityprofile.go b/pkg/validate/dynamic/platformworkloadidentityprofile.go
@@ -30,11 +30,12 @@ func (dv *dynamic) ValidatePlatformWorkloadIdentityProfile(
 	platformWorkloadIdentityRolesByRoleName map[string]api.PlatformWorkloadIdentityRole,
 	roleDefinitions armauthorization.RoleDefinitionsClient,
 	clusterMsiFederatedIdentityCredentials armmsi.FederatedIdentityCredentialsClient,
-) error {
+	platformWorkloadIdentities map[string]api.PlatformWorkloadIdentity, // Platform Workload Identities with object and client IDs
+) (err error) {
 	dv.log.Print("ValidatePlatformWorkloadIdentityProfile")
 
 	dv.platformIdentitiesActionsMap = map[string][]string{}
-	dv.platformIdentities = oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities
+	dv.platformIdentities = platformWorkloadIdentities
 
 	// Check if any required platform identity is missing
 	if len(dv.platformIdentities) != len(platformWorkloadIdentityRolesByRoleName) {