From 121a367e6e95c0b500052fdf1bb7c5221aec5336 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Wed, 20 Nov 2024 11:05:11 -0500 Subject: [PATCH 1/7] ARO-10859 static validation to reject empty or nil PlatformWorkloadIdentityProfile --- .../openshiftcluster_validatestatic.go | 6 +- .../openshiftcluster_validatestatic_test.go | 77 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/pkg/api/v20240812preview/openshiftcluster_validatestatic.go b/pkg/api/v20240812preview/openshiftcluster_validatestatic.go index bf21b2ac882..5485be299bb 100644 --- a/pkg/api/v20240812preview/openshiftcluster_validatestatic.go +++ b/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 } diff --git a/pkg/api/v20240812preview/openshiftcluster_validatestatic_test.go b/pkg/api/v20240812preview/openshiftcluster_validatestatic_test.go index 23a63851865..3739fb4ffd8 100644 --- a/pkg/api/v20240812preview/openshiftcluster_validatestatic_test.go +++ b/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) From fae7e3f36ec17aa45f44dd186eac860e5d61ed5f Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Wed, 20 Nov 2024 11:07:32 -0500 Subject: [PATCH 2/7] ARO-10859 avoid using platformWorkloadIdentityRolesByVersion for the cluster deletion flow --- pkg/cluster/cluster.go | 8 +++--- pkg/cluster/delete.go | 10 +------ pkg/cluster/delete_test.go | 54 -------------------------------------- 3 files changed, 6 insertions(+), 66 deletions(-) diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index f42fa74659f..af378b980bd 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -303,9 +303,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() diff --git a/pkg/cluster/delete.go b/pkg/cluster/delete.go index 5112c216e64..0dbe8e11af4 100644 --- a/pkg/cluster/delete.go +++ b/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 diff --git a/pkg/cluster/delete_test.go b/pkg/cluster/delete_test.go index 737484f9689..84c84b467ad 100644 --- a/pkg/cluster/delete_test.go +++ b/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, } From 3fac2a427458b132b8ddb7a6fb1d944551addd37 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Wed, 20 Nov 2024 11:13:38 -0500 Subject: [PATCH 3/7] ARO-10859 throw an error for create/update flow whenever an unexpected platform identity is found --- pkg/cluster/deploybaseresources.go | 3 +- pkg/cluster/deploybaseresources_additional.go | 3 +- pkg/cluster/deploybaseresources_test.go | 4 +- pkg/cluster/workloadidentityresources.go | 54 ++++++++++--------- pkg/cluster/workloadidentityresources_test.go | 16 +++--- .../platformworkloadidentityrolesbyversion.go | 25 +++++++++ 6 files changed, 68 insertions(+), 37 deletions(-) diff --git a/pkg/cluster/deploybaseresources.go b/pkg/cluster/deploybaseresources.go index 97cf4c4cf1d..625524f2618 100644 --- a/pkg/cluster/deploybaseresources.go +++ b/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 { diff --git a/pkg/cluster/deploybaseresources_additional.go b/pkg/cluster/deploybaseresources_additional.go index a15e3111a95..1e9e3d35c25 100644 --- a/pkg/cluster/deploybaseresources_additional.go +++ b/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" ) @@ -110,7 +111,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) == "" { diff --git a/pkg/cluster/deploybaseresources_test.go b/pkg/cluster/deploybaseresources_test.go index 26429789d9e..bce957dd794 100644 --- a/pkg/cluster/deploybaseresources_test.go +++ b/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) diff --git a/pkg/cluster/workloadidentityresources.go b/pkg/cluster/workloadidentityresources.go index d7ca06f8e89..66f5d0941f2 100644 --- a/pkg/cluster/workloadidentityresources.go +++ b/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 diff --git a/pkg/cluster/workloadidentityresources_test.go b/pkg/cluster/workloadidentityresources_test.go index 5e0d9d40849..c019140fdd1 100644 --- a/pkg/cluster/workloadidentityresources_test.go +++ b/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) }) } diff --git a/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion.go b/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion.go index 901804a11b1..71a8cc6232b 100644 --- a/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion.go +++ b/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion.go @@ -85,3 +85,28 @@ func (service *PlatformWorkloadIdentityRolesByVersionService) GetPlatformWorkloa } return platformWorkloadIdentityRolesByRoleName } + +func GetPlatformWorkloadIdentityMismatchError(oc *api.OpenShiftCluster, platformWorkloadIdentityRolesByRoleName map[string]api.PlatformWorkloadIdentityRole) error { + requiredOperatorIdentities := []string{} + for _, role := range platformWorkloadIdentityRolesByRoleName { + requiredOperatorIdentities = append(requiredOperatorIdentities, role.OperatorName) + } + currentOpenShiftVersion, err := version.ParseVersion(oc.Properties.ClusterProfile.Version) + if err != nil { + return err + } + currentMinorVersion := currentOpenShiftVersion.MinorVersion() + v := currentMinorVersion + if oc.Properties.PlatformWorkloadIdentityProfile.UpgradeableTo != nil { + upgradeableVersion, err := version.ParseVersion(string(*oc.Properties.PlatformWorkloadIdentityProfile.UpgradeableTo)) + if err != nil { + return err + } + upgradeableMinorVersion := upgradeableVersion.MinorVersion() + if currentMinorVersion != upgradeableMinorVersion && currentOpenShiftVersion.Lt(upgradeableVersion) { + v = fmt.Sprintf("%s or %s", v, upgradeableMinorVersion) + } + } + return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodePlatformWorkloadIdentityMismatch, + "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 '%v'", v, requiredOperatorIdentities) +} From 380c053dc761de13ae13f2ef121dd7930c9b40cf Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Wed, 20 Nov 2024 11:15:45 -0500 Subject: [PATCH 4/7] ARO-10859 update dynamic validation to reject the create/update flow for unexpected platform workload identity --- .../platformworkloadidentityprofile.go | 125 +++++++----------- .../platformworkloadidentityprofile_test.go | 20 ++- 2 files changed, 63 insertions(+), 82 deletions(-) diff --git a/pkg/validate/dynamic/platformworkloadidentityprofile.go b/pkg/validate/dynamic/platformworkloadidentityprofile.go index 50bf1d36ae8..1cfecf36089 100644 --- a/pkg/validate/dynamic/platformworkloadidentityprofile.go +++ b/pkg/validate/dynamic/platformworkloadidentityprofile.go @@ -12,9 +12,9 @@ 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/azureclient/azuresdk/armmsi" + "github.com/Azure/ARO-RP/pkg/util/platformworkloadidentity" "github.com/Azure/ARO-RP/pkg/util/rbac" "github.com/Azure/ARO-RP/pkg/util/stringutils" - "github.com/Azure/ARO-RP/pkg/util/version" ) // Copyright (c) Microsoft Corporation. @@ -34,93 +34,68 @@ func (dv *dynamic) ValidatePlatformWorkloadIdentityProfile( dv.log.Print("ValidatePlatformWorkloadIdentityProfile") dv.platformIdentitiesActionsMap = map[string][]string{} - dv.platformIdentities = map[string]api.PlatformWorkloadIdentity{} - - for k, pwi := range oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities { - _, ok := platformWorkloadIdentityRolesByRoleName[k] - if ok { - dv.platformIdentitiesActionsMap[k] = nil - dv.platformIdentities[k] = pwi - - identityResourceId, err := azure.ParseResourceID(pwi.ResourceID) - if err != nil { - return err - } - - // validate federated identity credentials - federatedCredentials, err := clusterMsiFederatedIdentityCredentials.List(ctx, identityResourceId.ResourceGroup, identityResourceId.ResourceName, &sdkmsi.FederatedIdentityCredentialsClientListOptions{}) - if err != nil { - return err - } - - for _, federatedCredential := range federatedCredentials { - switch { - case federatedCredential == nil, - federatedCredential.Name == nil, - federatedCredential.Properties == nil: - return fmt.Errorf("received invalid federated credential") - case oc.Properties.ProvisioningState == api.ProvisioningStateCreating: - return api.NewCloudError( - http.StatusBadRequest, - api.CloudErrorCodePlatformWorkloadIdentityContainsInvalidFederatedCredential, - fmt.Sprintf("properties.platformWorkloadIdentityProfile.platformWorkloadIdentities.%s.resourceId", k), - "Unexpected federated credential '%s' found on platform workload identity '%s' used for role '%s'. Please ensure this identity is only used for this cluster and does not have any existing federated identity credentials.", - *federatedCredential.Name, - pwi.ResourceID, - k, - ) - case len(federatedCredential.Properties.Audiences) != 1, - *federatedCredential.Properties.Audiences[0] != expectedAudience, - federatedCredential.Properties.Issuer == nil, - *federatedCredential.Properties.Issuer != string(*oc.Properties.ClusterProfile.OIDCIssuer): - return api.NewCloudError( - http.StatusBadRequest, - api.CloudErrorCodePlatformWorkloadIdentityContainsInvalidFederatedCredential, - fmt.Sprintf("properties.platformWorkloadIdentityProfile.platformWorkloadIdentities.%s.resourceId", k), - "Unexpected federated credential '%s' found on platform workload identity '%s' used for role '%s'. Please ensure only federated credentials provisioned by the ARO service for this cluster are present.", - *federatedCredential.Name, - pwi.ResourceID, - k, - ) - } - } - } - } + dv.platformIdentities = oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities // Check if any required platform identity is missing if len(dv.platformIdentities) != len(platformWorkloadIdentityRolesByRoleName) { - requiredOperatorIdentities := []string{} - for _, role := range platformWorkloadIdentityRolesByRoleName { - requiredOperatorIdentities = append(requiredOperatorIdentities, role.OperatorName) + return platformworkloadidentity.GetPlatformWorkloadIdentityMismatchError(oc, platformWorkloadIdentityRolesByRoleName) + } + + for k, pwi := range dv.platformIdentities { + role, exists := platformWorkloadIdentityRolesByRoleName[k] + if !exists { + return platformworkloadidentity.GetPlatformWorkloadIdentityMismatchError(oc, platformWorkloadIdentityRolesByRoleName) } - currentOpenShiftVersion, err := version.ParseVersion(oc.Properties.ClusterProfile.Version) + + roleDefinitionID := stringutils.LastTokenByte(role.RoleDefinitionID, '/') + actions, err := getActionsForRoleDefinition(ctx, roleDefinitionID, roleDefinitions) if err != nil { return err } - currentMinorVersion := currentOpenShiftVersion.MinorVersion() - v := currentMinorVersion - if oc.Properties.PlatformWorkloadIdentityProfile.UpgradeableTo != nil { - upgradeableVersion, err := version.ParseVersion(string(*oc.Properties.PlatformWorkloadIdentityProfile.UpgradeableTo)) - if err != nil { - return err - } - upgradeableMinorVersion := upgradeableVersion.MinorVersion() - if currentMinorVersion != upgradeableMinorVersion && currentOpenShiftVersion.Lt(upgradeableVersion) { - v = fmt.Sprintf("%s or %s", v, upgradeableMinorVersion) - } + dv.platformIdentitiesActionsMap[role.OperatorName] = actions + + identityResourceId, err := azure.ParseResourceID(pwi.ResourceID) + if err != nil { + return err } - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodePlatformWorkloadIdentityMismatch, - "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 '%v'", v, requiredOperatorIdentities) - } - for _, role := range platformWorkloadIdentityRolesByRoleName { - roleDefinitionID := stringutils.LastTokenByte(role.RoleDefinitionID, '/') - actions, err := getActionsForRoleDefinition(ctx, roleDefinitionID, roleDefinitions) + // validate federated identity credentials + federatedCredentials, err := clusterMsiFederatedIdentityCredentials.List(ctx, identityResourceId.ResourceGroup, identityResourceId.ResourceName, &sdkmsi.FederatedIdentityCredentialsClientListOptions{}) if err != nil { return err } - dv.platformIdentitiesActionsMap[role.OperatorName] = actions + for _, federatedCredential := range federatedCredentials { + switch { + case federatedCredential == nil, + federatedCredential.Name == nil, + federatedCredential.Properties == nil: + return fmt.Errorf("received invalid federated credential") + case oc.Properties.ProvisioningState == api.ProvisioningStateCreating: + return api.NewCloudError( + http.StatusBadRequest, + api.CloudErrorCodePlatformWorkloadIdentityContainsInvalidFederatedCredential, + fmt.Sprintf("properties.platformWorkloadIdentityProfile.platformWorkloadIdentities.%s.resourceId", k), + "Unexpected federated credential '%s' found on platform workload identity '%s' used for role '%s'. Please ensure this identity is only used for this cluster and does not have any existing federated identity credentials.", + *federatedCredential.Name, + pwi.ResourceID, + k, + ) + case len(federatedCredential.Properties.Audiences) != 1, + *federatedCredential.Properties.Audiences[0] != expectedAudience, + federatedCredential.Properties.Issuer == nil, + *federatedCredential.Properties.Issuer != string(*oc.Properties.ClusterProfile.OIDCIssuer): + return api.NewCloudError( + http.StatusBadRequest, + api.CloudErrorCodePlatformWorkloadIdentityContainsInvalidFederatedCredential, + fmt.Sprintf("properties.platformWorkloadIdentityProfile.platformWorkloadIdentities.%s.resourceId", k), + "Unexpected federated credential '%s' found on platform workload identity '%s' used for role '%s'. Please ensure only federated credentials provisioned by the ARO service for this cluster are present.", + *federatedCredential.Name, + pwi.ResourceID, + k, + ) + } + } } return nil diff --git a/pkg/validate/dynamic/platformworkloadidentityprofile_test.go b/pkg/validate/dynamic/platformworkloadidentityprofile_test.go index 4a27e2c093c..6adf434a52c 100644 --- a/pkg/validate/dynamic/platformworkloadidentityprofile_test.go +++ b/pkg/validate/dynamic/platformworkloadidentityprofile_test.go @@ -263,9 +263,6 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { expectedPlatformIdentity1FederatedCredName := platformworkloadidentity.GetPlatformWorkloadIdentityFederatedCredName(clusterResourceId, platformIdentity1ResourceId, platformIdentity1SAName) expectedOIDCIssuer := "https://fakeissuer.fakedomain/fakecluster" platformWorkloadIdentities := map[string]api.PlatformWorkloadIdentity{ - "Dummy2": { - ResourceID: platformIdentity1, - }, "Dummy1": { ResourceID: platformIdentity1, }, @@ -532,6 +529,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) federatedIdentityCredentials.EXPECT().List(gomock.Any(), gomock.Eq(platformIdentity1ResourceId.ResourceGroup), gomock.Eq(platformIdentity1ResourceId.ResourceName), gomock.Any()). Return(nil, fmt.Errorf("something unexpected occurred")) }, @@ -560,6 +558,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) federatedIdentityCredentials.EXPECT().List(gomock.Any(), gomock.Eq(platformIdentity1ResourceId.ResourceGroup), gomock.Eq(platformIdentity1ResourceId.ResourceName), gomock.Any()). Return([]*sdkmsi.FederatedIdentityCredential{ { @@ -604,6 +603,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) federatedIdentityCredentials.EXPECT().List(gomock.Any(), gomock.Eq(platformIdentity1ResourceId.ResourceGroup), gomock.Eq(platformIdentity1ResourceId.ResourceName), gomock.Any()). Return([]*sdkmsi.FederatedIdentityCredential{ { @@ -648,6 +648,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) federatedIdentityCredentials.EXPECT().List(gomock.Any(), gomock.Eq(platformIdentity1ResourceId.ResourceGroup), gomock.Eq(platformIdentity1ResourceId.ResourceName), gomock.Any()). Return([]*sdkmsi.FederatedIdentityCredential{ { @@ -692,6 +693,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) federatedIdentityCredentials.EXPECT().List(gomock.Any(), gomock.Eq(platformIdentity1ResourceId.ResourceGroup), gomock.Eq(platformIdentity1ResourceId.ResourceName), gomock.Any()). Return([]*sdkmsi.FederatedIdentityCredential{ { @@ -935,7 +937,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { 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 '[Dummy3]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14"), }, { - name: "Fail - Mismatch between desired and provided platform Identities - count mismatch 2", + name: "Fail - Mismatch between desired and provided platform Identities - count mismatch and operator missing", platformIdentityRoles: validRolesForVersion, oc: &api.OpenShiftCluster{ ID: clusterID, @@ -1013,19 +1015,23 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { Properties: api.OpenShiftClusterProperties{ PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{ PlatformWorkloadIdentities: map[string]api.PlatformWorkloadIdentity{ - "Dummy2": { - ResourceID: "Invalid UUID", - }, "Dummy1": { ResourceID: "Invalid UUID", }, }, }, + ClusterProfile: api.ClusterProfile{ + Version: openShiftVersion, + OIDCIssuer: pointerutils.ToPtr(api.OIDCIssuer(expectedOIDCIssuer)), + }, }, Identity: &api.ManagedServiceIdentity{ UserAssignedIdentities: clusterMSI, }, }, + mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) + }, wantErr: "parsing failed for Invalid UUID. Invalid resource Id format", }, { From f029b9b3fa5e3160d252330bb05b9ae947d9b0d4 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Mon, 25 Nov 2024 11:13:37 -0500 Subject: [PATCH 5/7] ARO-10859 sort required identities for consistent error messaging --- .../platformworkloadidentityrolesbyversion.go | 5 + ...formworkloadidentityrolesbyversion_test.go | 116 ++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion.go b/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion.go index 71a8cc6232b..302f5263f93 100644 --- a/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion.go +++ b/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "net/http" + "sort" "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/database" @@ -87,10 +88,14 @@ func (service *PlatformWorkloadIdentityRolesByVersionService) GetPlatformWorkloa } func GetPlatformWorkloadIdentityMismatchError(oc *api.OpenShiftCluster, platformWorkloadIdentityRolesByRoleName map[string]api.PlatformWorkloadIdentityRole) error { + if !oc.UsesWorkloadIdentity() { + return fmt.Errorf("GetPlatformWorkloadIdentityMismatchError called for a Cluster Service Principal cluster") + } requiredOperatorIdentities := []string{} for _, role := range platformWorkloadIdentityRolesByRoleName { requiredOperatorIdentities = append(requiredOperatorIdentities, role.OperatorName) } + sort.Strings(requiredOperatorIdentities) currentOpenShiftVersion, err := version.ParseVersion(oc.Properties.ClusterProfile.Version) if err != nil { return err diff --git a/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion_test.go b/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion_test.go index d2a99ae362c..b8abe52db58 100644 --- a/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion_test.go +++ b/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion_test.go @@ -5,6 +5,7 @@ package platformworkloadidentity import ( "context" + "fmt" "testing" "k8s.io/utils/ptr" @@ -291,3 +292,118 @@ func TestNewPlatformWorkloadIdentityRolesByVersion(t *testing.T) { }) } } + +func TestGetPlatformWorkloadIdentityMismatchError(t *testing.T) { + invalidVersion := "4.1450" + for _, tt := range []struct { + name string + oc *api.OpenShiftCluster + wantErr string + checkConsistency bool + }{ + { + name: "Exit the func for non MIWI clusters that has ServicePrincipalProfile", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ServicePrincipalProfile: &api.ServicePrincipalProfile{ + SPObjectID: "00000000-0000-0000-0000-000000000000", + }, + }, + }, + wantErr: "GetPlatformWorkloadIdentityMismatchError called for a Cluster Service Principal cluster", + }, + { + name: "Exit the func for non MIWI clusters that has no PlatformWorkloadIdentityProfile or ServicePrincipalProfile", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{}, + }, + wantErr: "GetPlatformWorkloadIdentityMismatchError called for a Cluster Service Principal cluster", + }, + { + name: "Invalid Cluster version", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + Version: invalidVersion, + }, + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{ + UpgradeableTo: ptr.To(api.UpgradeableTo("4.15.40")), + }, + }, + }, + wantErr: fmt.Sprintf(`could not parse version "%s"`, invalidVersion), + }, + { + name: "Invalid UpgradeableTo version", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + Version: "4.14.40", + }, + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{ + UpgradeableTo: ptr.To(api.UpgradeableTo(invalidVersion)), + }, + }, + }, + wantErr: fmt.Sprintf(`could not parse version "%s"`, invalidVersion), + }, + { + name: "Unexpected Identites in the doc", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + Version: "4.14.40", + }, + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, + }, + }, + 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 '[bar foo]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14"), + checkConsistency: true, + }, + { + name: "Unexpected Identites in the doc with UpgradeableTo", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + Version: "4.14.40", + }, + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{ + UpgradeableTo: ptr.To(api.UpgradeableTo("4.15.40")), + }, + }, + }, + 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 '[bar foo]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14", "4.15"), + }, + { + name: "Unexpected Identites in the doc with UpgradeableTo lower than cluster version", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + Version: "4.14.40", + }, + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{ + UpgradeableTo: ptr.To(api.UpgradeableTo("4.13.40")), + }, + }, + }, + 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 '[bar foo]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14"), + }, + } { + t.Run(tt.name, func(t *testing.T) { + pir := &PlatformWorkloadIdentityRolesByVersionService{ + platformWorkloadIdentityRoles: []api.PlatformWorkloadIdentityRole{ + {OperatorName: "foo"}, + {OperatorName: "bar"}, + }, + } + iteration := 1 + if tt.checkConsistency { + iteration = 5 + } + for i := 0; i < iteration; i++ { + err := GetPlatformWorkloadIdentityMismatchError(tt.oc, pir.GetPlatformWorkloadIdentityRolesByRoleName()) + utilerror.AssertErrorMessage(t, err, tt.wantErr) + } + }) + } +} From 79f70e52bf88ff2db8c0cbf0c880fdecde22163a Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Wed, 4 Dec 2024 09:38:35 -0500 Subject: [PATCH 6/7] ARO-10859 update empty pwi map validation error message --- .../v20240812preview/openshiftcluster_validatestatic.go | 2 +- .../openshiftcluster_validatestatic_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/api/v20240812preview/openshiftcluster_validatestatic.go b/pkg/api/v20240812preview/openshiftcluster_validatestatic.go index 5485be299bb..9f9f935dd2a 100644 --- a/pkg/api/v20240812preview/openshiftcluster_validatestatic.go +++ b/pkg/api/v20240812preview/openshiftcluster_validatestatic.go @@ -525,7 +525,7 @@ func (sv openShiftClusterStaticValidator) validatePlatformIdentities(oc *OpenShi } if operatorRolePresent && len(pwip.PlatformWorkloadIdentities) == 0 { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "properties.platformWorkloadIdentityProfile", "Platform workload identity profile cannot be empty.") + return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "properties.platformWorkloadIdentityProfile.platformWorkloadIdentities", "The set of platform workload identities cannot be empty.") } return nil diff --git a/pkg/api/v20240812preview/openshiftcluster_validatestatic_test.go b/pkg/api/v20240812preview/openshiftcluster_validatestatic_test.go index 3739fb4ffd8..e0e027e05ab 100644 --- a/pkg/api/v20240812preview/openshiftcluster_validatestatic_test.go +++ b/pkg/api/v20240812preview/openshiftcluster_validatestatic_test.go @@ -1465,7 +1465,7 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin UpgradeableTo: &invalidUpgradeableToValue, } }, - wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile: Platform workload identity profile cannot be empty.", + wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile.platformWorkloadIdentities: The set of platform workload identities cannot be empty.", }, { name: "No platform identities provided in PlatformWorkloadIdentityProfile - empty map", @@ -1481,7 +1481,7 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin UpgradeableTo: &invalidUpgradeableToValue, } }, - wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile: Platform workload identity profile cannot be empty.", + wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile.platformWorkloadIdentities: The set of platform workload identities cannot be empty.", }, } @@ -1635,7 +1635,7 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin modify: func(oc *OpenShiftCluster) { oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities = map[string]PlatformWorkloadIdentity{} }, - wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile: Platform workload identity profile cannot be empty.", + wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile.platformWorkloadIdentities: The set of platform workload identities cannot be empty.", }, { name: "No platform identities provided in PlatformWorkloadIdentityProfile - nil", @@ -1655,7 +1655,7 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin modify: func(oc *OpenShiftCluster) { oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities = nil }, - wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile: Platform workload identity profile cannot be empty.", + wantErr: "400: InvalidParameter: properties.platformWorkloadIdentityProfile.platformWorkloadIdentities: The set of platform workload identities cannot be empty.", }, } From a72470ce750719073c0c396a8e664e94f01099a2 Mon Sep 17 00:00:00 2001 From: Rajdeep Chauhan Date: Thu, 5 Dec 2024 11:56:11 -0500 Subject: [PATCH 7/7] ARO-10859 update test case name Co-authored-by: Caden Marchese <56140267+cadenmarchese@users.noreply.github.com> --- pkg/cluster/deploybaseresources_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cluster/deploybaseresources_test.go b/pkg/cluster/deploybaseresources_test.go index bce957dd794..c439c2af40f 100644 --- a/pkg/cluster/deploybaseresources_test.go +++ b/pkg/cluster/deploybaseresources_test.go @@ -1811,7 +1811,7 @@ func TestGenerateFederatedIdentityCredentials(t *testing.T) { wantErr: "OIDCIssuer is nil", }, { - name: "Fail - Operator name do not exists in PlatformWorkloadIdentityProfile", + name: "Fail - Operator name does not exist in PlatformWorkloadIdentityProfile", oc: &api.OpenShiftClusterDocument{ ID: docID, OpenShiftCluster: &api.OpenShiftCluster{