From d69fef0b07a0605f275a0841cc7624c647852994 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Wed, 20 Nov 2024 11:13:38 -0500 Subject: [PATCH] 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 b9dcff04343..b65eb8dbb1f 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" ) @@ -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) == "" { 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) +}