MIWI cluster Dynamic Validation update for strict 1:1 matching for provided Platform Workload Identity to expected OCP Operators

pkg/api/v20240812preview/openshiftcluster_validatestatic.go

@@ -520,9 +520,13 @@ func (sv openShiftClusterStaticValidator) validatePlatformIdentities(oc *OpenShi
 		return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "identity", "Cluster identity and platform workload identities require each other.")
 	}
 
-	if operatorRolePresent && len(oc.Identity.UserAssignedIdentities) != 1 {
+	if clusterIdentityPresent && len(oc.Identity.UserAssignedIdentities) != 1 {
 		return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "identity", "The provided cluster identity is invalid; there should be exactly one.")
 	}
 
+	if operatorRolePresent && len(pwip.PlatformWorkloadIdentities) == 0 {
+		return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "properties.platformWorkloadIdentityProfile", "Platform workload identity profile cannot be empty.")
+	}
+
 	return nil
 }

pkg/api/v20240812preview/openshiftcluster_validatestatic_test.go

@@ -1427,6 +1427,9 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin
 				}
 				oc.Properties.ServicePrincipalProfile = nil
 				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					PlatformWorkloadIdentities: map[string]PlatformWorkloadIdentity{
+						"FAKE-OPERATOR": platformIdentity1,
+					},
 					UpgradeableTo: &validUpgradeableToValue,
 				}
 			},
@@ -1441,11 +1444,45 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin
 				}
 				oc.Properties.ServicePrincipalProfile = nil
 				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					PlatformWorkloadIdentities: map[string]PlatformWorkloadIdentity{
+						"FAKE-OPERATOR": platformIdentity1,
+					},
 					UpgradeableTo: &invalidUpgradeableToValue,
 				}
 			},
 			wantErr: `400: InvalidParameter: properties.platformWorkloadIdentityProfile.UpgradeableTo[16.107.invalid]: UpgradeableTo must be a valid OpenShift version in the format 'x.y.z'.`,
 		},
+		{
+			name: "No platform identities provided in PlatformWorkloadIdentityProfile - nil",
+			modify: func(oc *OpenShiftCluster) {
+				oc.Identity = &ManagedServiceIdentity{
+					UserAssignedIdentities: map[string]UserAssignedIdentity{
+						"Dummy": {},
+					},
+				}
+				oc.Properties.ServicePrincipalProfile = nil
+				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					UpgradeableTo: &invalidUpgradeableToValue,
+				}
+			},
+			wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile: Platform workload identity profile cannot be empty.",
+		},
+		{
+			name: "No platform identities provided in PlatformWorkloadIdentityProfile - empty map",
+			modify: func(oc *OpenShiftCluster) {
+				oc.Identity = &ManagedServiceIdentity{
+					UserAssignedIdentities: map[string]UserAssignedIdentity{
+						"Dummy": {},
+					},
+				}
+				oc.Properties.ServicePrincipalProfile = nil
+				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					PlatformWorkloadIdentities: map[string]PlatformWorkloadIdentity{},
+					UpgradeableTo:              &invalidUpgradeableToValue,
+				}
+			},
+			wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile: Platform workload identity profile cannot be empty.",
+		},
 	}
 
 	updateTests := []*validateTest{
@@ -1580,6 +1617,46 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin
 			},
 			wantErr: "400: PropertyChangeNotAllowed: properties.platformWorkloadIdentityProfile.platformWorkloadIdentities: Operator identity cannot be removed or have its name changed.",
 		},
+		{
+			name: "No platform identities provided in PlatformWorkloadIdentityProfile - empty map",
+			current: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					PlatformWorkloadIdentities: map[string]PlatformWorkloadIdentity{
+						"operator1": platformIdentity1,
+					},
+				}
+				oc.Identity = &ManagedServiceIdentity{
+					UserAssignedIdentities: map[string]UserAssignedIdentity{
+						"first": clusterIdentity1,
+					},
+				}
+				oc.Properties.ServicePrincipalProfile = nil
+			},
+			modify: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities = map[string]PlatformWorkloadIdentity{}
+			},
+			wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile: Platform workload identity profile cannot be empty.",
+		},
+		{
+			name: "No platform identities provided in PlatformWorkloadIdentityProfile - nil",
+			current: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					PlatformWorkloadIdentities: map[string]PlatformWorkloadIdentity{
+						"operator1": platformIdentity1,
+					},
+				}
+				oc.Identity = &ManagedServiceIdentity{
+					UserAssignedIdentities: map[string]UserAssignedIdentity{
+						"first": clusterIdentity1,
+					},
+				}
+				oc.Properties.ServicePrincipalProfile = nil
+			},
+			modify: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities = nil
+			},
+			wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile: Platform workload identity profile cannot be empty.",
+		},
 	}
 
 	runTests(t, testModeCreate, createTests)

pkg/cluster/cluster.go

@@ -305,9 +305,11 @@ func New(ctx context.Context, log *logrus.Entry, _env env.Interface, db database
 	}
 
 	if doc.OpenShiftCluster.UsesWorkloadIdentity() {
-		err = m.platformWorkloadIdentityRolesByVersion.PopulatePlatformWorkloadIdentityRolesByVersion(ctx, doc.OpenShiftCluster, dbPlatformWorkloadIdentityRoleSets)
-		if err != nil {
-			return nil, err
+		if m.doc.OpenShiftCluster.Properties.ProvisioningState != api.ProvisioningStateDeleting {
+			err = m.platformWorkloadIdentityRolesByVersion.PopulatePlatformWorkloadIdentityRolesByVersion(ctx, doc.OpenShiftCluster, dbPlatformWorkloadIdentityRoleSets)
+			if err != nil {
+				return nil, err
+			}
 		}
 
 		msiResourceId, err := m.doc.OpenShiftCluster.ClusterMsiResourceId()

pkg/cluster/delete.go

@@ -391,15 +391,7 @@ func (m *manager) deleteFederatedCredentials(ctx context.Context) error {
 		}
 	}
 
-	platformWIRolesByRoleName := m.platformWorkloadIdentityRolesByVersion.GetPlatformWorkloadIdentityRolesByRoleName()
-	platformWorkloadIdentities := m.doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities
-
-	for name, identity := range platformWorkloadIdentities {
-		_, exists := platformWIRolesByRoleName[name]
-		if !exists {
-			continue
-		}
-
+	for _, identity := range m.doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities {
 		identityResourceId, err := azure.ParseResourceID(identity.ResourceID)
 		if err != nil {
 			return err

pkg/cluster/delete_test.go

@@ -34,8 +34,6 @@ import (
 	mock_subnet "github.com/Azure/ARO-RP/pkg/util/mocks/subnet"
 	"github.com/Azure/ARO-RP/pkg/util/platformworkloadidentity"
 	"github.com/Azure/ARO-RP/pkg/util/pointerutils"
-	testdatabase "github.com/Azure/ARO-RP/test/database"
-	"github.com/Azure/ARO-RP/test/util/deterministicuuid"
 	utilerror "github.com/Azure/ARO-RP/test/util/error"
 )
 
@@ -813,57 +811,6 @@ func TestDeleteFederatedCredentials(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		uuidGen := deterministicuuid.NewTestUUIDGenerator(deterministicuuid.OPENSHIFT_VERSIONS)
-		dbPlatformWorkloadIdentityRoleSets, _ := testdatabase.NewFakePlatformWorkloadIdentityRoleSets(uuidGen)
-		f := testdatabase.NewFixture().WithPlatformWorkloadIdentityRoleSets(dbPlatformWorkloadIdentityRoleSets, uuidGen)
-		pir := platformworkloadidentity.NewPlatformWorkloadIdentityRolesByVersionService()
-		f.AddPlatformWorkloadIdentityRoleSetDocuments(&api.PlatformWorkloadIdentityRoleSetDocument{
-			PlatformWorkloadIdentityRoleSet: &api.PlatformWorkloadIdentityRoleSet{
-				Name: "testRoleSet",
-				Properties: api.PlatformWorkloadIdentityRoleSetProperties{
-					OpenShiftVersion: "4.14",
-					PlatformWorkloadIdentityRoles: []api.PlatformWorkloadIdentityRole{
-						{
-							OperatorName:    "CloudControllerManager",
-							ServiceAccounts: []string{ccmServiceAccountName},
-						},
-						{
-							OperatorName:    "ClusterIngressOperator",
-							ServiceAccounts: []string{ingressServiceAccountName},
-						},
-					},
-				},
-			},
-		},
-			&api.PlatformWorkloadIdentityRoleSetDocument{
-				PlatformWorkloadIdentityRoleSet: &api.PlatformWorkloadIdentityRoleSet{
-					Name: "testRoleSet",
-					Properties: api.PlatformWorkloadIdentityRoleSetProperties{
-						OpenShiftVersion: "4.15",
-						PlatformWorkloadIdentityRoles: []api.PlatformWorkloadIdentityRole{
-							{
-								OperatorName:    "CloudControllerManager",
-								ServiceAccounts: []string{"openshift-cloud-controller-manager:cloud-controller-manager"},
-							},
-							{
-								OperatorName:    "ClusterIngressOperator",
-								ServiceAccounts: []string{"openshift-ingress-operator:ingress-operator"},
-							},
-						},
-					},
-				},
-			},
-		)
-		err := f.Create()
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		err = pir.PopulatePlatformWorkloadIdentityRolesByVersion(ctx, tt.doc.OpenShiftCluster, dbPlatformWorkloadIdentityRoleSets)
-		if err != nil {
-			t.Fatal(err)
-		}
-
 		t.Run(tt.name, func(t *testing.T) {
 			controller := gomock.NewController(t)
 			defer controller.Finish()
@@ -876,7 +823,6 @@ func TestDeleteFederatedCredentials(t *testing.T) {
 			m := manager{
 				log:                                    logrus.NewEntry(logrus.StandardLogger()),
 				doc:                                    tt.doc,
-				platformWorkloadIdentityRolesByVersion: pir,
 				clusterMsiFederatedIdentityCredentials: federatedIdentityCredentials,
 			}
 

pkg/cluster/deploybaseresources.go

@@ -28,6 +28,7 @@ import (
 	"github.com/Azure/ARO-RP/pkg/env"
 	"github.com/Azure/ARO-RP/pkg/util/arm"
 	"github.com/Azure/ARO-RP/pkg/util/oidcbuilder"
+	"github.com/Azure/ARO-RP/pkg/util/platformworkloadidentity"
 	"github.com/Azure/ARO-RP/pkg/util/pointerutils"
 	"github.com/Azure/ARO-RP/pkg/util/stringutils"
 )
@@ -485,7 +486,7 @@ func (m *manager) federateIdentityCredentials(ctx context.Context) error {
 
 		platformWIRole, exists := platformWIRolesByRoleName[name]
 		if !exists {
-			continue
+			return platformworkloadidentity.GetPlatformWorkloadIdentityMismatchError(m.doc.OpenShiftCluster, platformWIRolesByRoleName)
 		}
 
 		for _, sa := range platformWIRole.ServiceAccounts {

pkg/cluster/deploybaseresources_additional.go

@@ -17,6 +17,7 @@ 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/platformworkloadidentity"
 	"github.com/Azure/ARO-RP/pkg/util/rbac"
 	"github.com/Azure/ARO-RP/pkg/util/stringutils"
 )
@@ -106,7 +107,7 @@ func (m *manager) platformWorkloadIdentityRBAC() ([]*arm.Resource, error) {
 	for name, identity := range platformWorkloadIdentities {
 		role, exists := platformWIRolesByRoleName[name]
 		if !exists {
-			continue
+			return nil, platformworkloadidentity.GetPlatformWorkloadIdentityMismatchError(m.doc.OpenShiftCluster, platformWIRolesByRoleName)
 		}
 
 		if strings.TrimSpace(identity.ObjectID) == "" {

pkg/cluster/deploybaseresources_test.go

@@ -1811,7 +1811,7 @@ func TestGenerateFederatedIdentityCredentials(t *testing.T) {
 			wantErr: "OIDCIssuer is nil",
 		},
 		{
-			name: "Success - Operator name do not exists in PlatformWorkloadIdentityProfile",
+			name: "Fail - Operator name do not exists in PlatformWorkloadIdentityProfile",
 			oc: &api.OpenShiftClusterDocument{
 				ID: docID,
 				OpenShiftCluster: &api.OpenShiftCluster{
@@ -1857,7 +1857,7 @@ func TestGenerateFederatedIdentityCredentials(t *testing.T) {
 					},
 				)
 			},
-			wantErr: "",
+			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 or %s'. The required platform workload identities are '[CloudControllerManager]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14", "4.15"),
 		},
 	} {
 		uuidGen := deterministicuuid.NewTestUUIDGenerator(deterministicuuid.OPENSHIFT_VERSIONS)

pkg/cluster/workloadidentityresources.go

@@ -81,33 +81,35 @@ func (m *manager) generatePlatformWorkloadIdentitySecrets() ([]*corev1.Secret, e
 
 	secrets := []*corev1.Secret{}
 	for name, identity := range m.doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities {
-		if role, ok := roles[name]; ok {
-			// Skip creating a secret for the ARO Operator. This will be
-			// generated by the ARO Operator deployer instead
-			// (see pkg/operator/deploy/deploy.go#generateOperatorIdentitySecret())
-			if role.OperatorName == pkgoperator.OperatorIdentityName {
-				continue
-			}
-
-			secrets = append(secrets, &corev1.Secret{
-				TypeMeta: metav1.TypeMeta{
-					APIVersion: corev1.SchemeGroupVersion.Identifier(),
-					Kind:       "Secret",
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: role.SecretLocation.Namespace,
-					Name:      role.SecretLocation.Name,
-				},
-				Type: corev1.SecretTypeOpaque,
-				StringData: map[string]string{
-					"azure_client_id":            identity.ClientID,
-					"azure_subscription_id":      subscriptionId,
-					"azure_tenant_id":            tenantId,
-					"azure_region":               region,
-					"azure_federated_token_file": azureFederatedTokenFileLocation,
-				},
-			})
+		role, exists := roles[name]
+		if !exists {
+			return 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
+		// (see pkg/operator/deploy/deploy.go#generateOperatorIdentitySecret())
+		if role.OperatorName == pkgoperator.OperatorIdentityName {
+			continue
+		}
+
+		secrets = append(secrets, &corev1.Secret{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: corev1.SchemeGroupVersion.Identifier(),
+				Kind:       "Secret",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: role.SecretLocation.Namespace,
+				Name:      role.SecretLocation.Name,
+			},
+			Type: corev1.SecretTypeOpaque,
+			StringData: map[string]string{
+				"azure_client_id":            identity.ClientID,
+				"azure_subscription_id":      subscriptionId,
+				"azure_tenant_id":            tenantId,
+				"azure_region":               region,
+				"azure_federated_token_file": azureFederatedTokenFileLocation,
+			},
+		})
 	}
 
 	return secrets, nil

pkg/cluster/workloadidentityresources_test.go

@@ -321,6 +321,7 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
 		identities map[string]api.PlatformWorkloadIdentity
 		roles      []api.PlatformWorkloadIdentityRole
 		want       []*corev1.Secret
+		wantErr    string
 	}{
 		{
 			name:       "no identities, no secrets",
@@ -394,17 +395,15 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
 			},
 		},
 		{
-			name: "ignores identities with no role present",
+			name: "error - identities with unexpected operator name present",
 			identities: map[string]api.PlatformWorkloadIdentity{
 				"foo": {
 					ClientID: "00f00f00-0f00-0f00-0f00-f00f00f00f00",
 				},
-				"bar": {
-					ClientID: "00ba4ba4-0ba4-0ba4-0ba4-ba4ba4ba4ba4",
-				},
 			},
-			roles: []api.PlatformWorkloadIdentityRole{},
-			want:  []*corev1.Secret{},
+			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"),
 		},
 		{
 			name: "skips ARO operator identity",
@@ -466,6 +465,9 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
 							PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{
 								PlatformWorkloadIdentities: tt.identities,
 							},
+							ClusterProfile: api.ClusterProfile{
+								Version: "4.14.40",
+							},
 						},
 					},
 				},
@@ -484,7 +486,7 @@ func TestGeneratePlatformWorkloadIdentitySecrets(t *testing.T) {
 			}
 			got, err := m.generatePlatformWorkloadIdentitySecrets()
 
-			utilerror.AssertErrorMessage(t, err, "")
+			utilerror.AssertErrorMessage(t, err, tt.wantErr)
 			assert.ElementsMatch(t, got, tt.want)
 		})
 	}