From 50eb4cc17d9772b970e1d535306317fc0d9cfab6 Mon Sep 17 00:00:00 2001 From: kimorris27 Date: Mon, 13 Jan 2025 12:20:38 -0600 Subject: [PATCH 1/4] Fix admin API - add readOnly tag to two fields that need it --- pkg/api/admin/openshiftcluster.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/api/admin/openshiftcluster.go b/pkg/api/admin/openshiftcluster.go index 515ec0d2916..13b7864d4f6 100644 --- a/pkg/api/admin/openshiftcluster.go +++ b/pkg/api/admin/openshiftcluster.go @@ -451,10 +451,10 @@ type PlatformWorkloadIdentity struct { // UserAssignedIdentity stores information about a user-assigned managed identity in a predefined format required by Microsoft's Managed Identity team. type UserAssignedIdentity struct { // The ClientID of the ClusterUserAssignedIdentity resource - ClientID string `json:"clientId,omitempty"` + ClientID string `json:"clientId,omitempty" swagger:"readOnly"` // The PrincipalID of the ClusterUserAssignedIdentity resource - PrincipalID string `json:"principalId,omitempty"` + PrincipalID string `json:"principalId,omitempty" swagger:"readOnly"` } // The ManagedServiceIdentity type. From be8564fc86ec09800e849c0f74d22a6c168bb024 Mon Sep 17 00:00:00 2001 From: kimorris27 Date: Mon, 13 Jan 2025 13:08:47 -0600 Subject: [PATCH 2/4] Fix admin update - if cluster is MIWI, skip step specific to service principal clusters --- pkg/cluster/adminupdate_test.go | 45 +++++++++++++++++++++++++-------- pkg/cluster/install.go | 5 +++- 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/pkg/cluster/adminupdate_test.go b/pkg/cluster/adminupdate_test.go index 08b78c6e5f4..fd08fdfd24e 100644 --- a/pkg/cluster/adminupdate_test.go +++ b/pkg/cluster/adminupdate_test.go @@ -32,7 +32,7 @@ func TestAdminUpdateSteps(t *testing.T) { } } - zerothSteps := []string{ + zerothStepsServicePrincipal := []string{ "[Action initializeKubernetesClients]", "[Action ensureBillingRecord]", "[Action ensureDefaults]", @@ -40,6 +40,13 @@ func TestAdminUpdateSteps(t *testing.T) { "[Action fixInfraID]", } + zerothStepsManagedIdentity := []string{ + "[Action initializeKubernetesClients]", + "[Action ensureBillingRecord]", + "[Action ensureDefaults]", + "[Action fixInfraID]", + } + generalFixesSteps := []string{ "[Action ensureResourceGroup]", "[Action createOrUpdateDenyAssignment]", @@ -111,7 +118,7 @@ func TestAdminUpdateSteps(t *testing.T) { doc.OpenShiftCluster.Properties.MaintenanceTask = api.MaintenanceTaskOperator return doc, true }, - shouldRunSteps: utilgenerics.ConcatMultipleSlices(zerothSteps, operatorUpdateSteps), + shouldRunSteps: utilgenerics.ConcatMultipleSlices(zerothStepsServicePrincipal, operatorUpdateSteps), }, { name: "ARO Operator Update on <= 4.6 cluster does not update operator", @@ -122,7 +129,7 @@ func TestAdminUpdateSteps(t *testing.T) { doc.OpenShiftCluster.Properties.ClusterProfile.Version = "4.6.62" return doc, true }, - shouldRunSteps: utilgenerics.ConcatMultipleSlices(zerothSteps), + shouldRunSteps: utilgenerics.ConcatMultipleSlices(zerothStepsServicePrincipal), }, { name: "ARO Operator Update on 4.7.0 cluster does update operator", @@ -133,7 +140,7 @@ func TestAdminUpdateSteps(t *testing.T) { doc.OpenShiftCluster.Properties.ClusterProfile.Version = "4.7.0" return doc, true }, - shouldRunSteps: utilgenerics.ConcatMultipleSlices(zerothSteps, operatorUpdateSteps), + shouldRunSteps: utilgenerics.ConcatMultipleSlices(zerothStepsServicePrincipal, operatorUpdateSteps), }, { name: "Everything update and adopt Hive.", @@ -144,7 +151,7 @@ func TestAdminUpdateSteps(t *testing.T) { return doc, true }, shouldRunSteps: utilgenerics.ConcatMultipleSlices( - zerothSteps, generalFixesSteps, certificateRenewalSteps, + zerothStepsServicePrincipal, generalFixesSteps, certificateRenewalSteps, operatorUpdateSteps, hiveSteps, updateProvisionedBySteps, ), }, @@ -158,7 +165,7 @@ func TestAdminUpdateSteps(t *testing.T) { return doc, true }, shouldRunSteps: utilgenerics.ConcatMultipleSlices( - zerothSteps, generalFixesSteps, certificateRenewalSteps, + zerothStepsServicePrincipal, generalFixesSteps, certificateRenewalSteps, hiveSteps, updateProvisionedBySteps, ), }, @@ -171,7 +178,7 @@ func TestAdminUpdateSteps(t *testing.T) { return doc, false }, shouldRunSteps: utilgenerics.ConcatMultipleSlices( - zerothSteps, generalFixesSteps, certificateRenewalSteps, + zerothStepsServicePrincipal, generalFixesSteps, certificateRenewalSteps, operatorUpdateSteps, updateProvisionedBySteps, ), }, @@ -184,7 +191,7 @@ func TestAdminUpdateSteps(t *testing.T) { return doc, true }, shouldRunSteps: utilgenerics.ConcatMultipleSlices( - zerothSteps, generalFixesSteps, certificateRenewalSteps, + zerothStepsServicePrincipal, generalFixesSteps, certificateRenewalSteps, operatorUpdateSteps, hiveSteps, updateProvisionedBySteps, ), }, @@ -196,7 +203,7 @@ func TestAdminUpdateSteps(t *testing.T) { doc.OpenShiftCluster.Properties.MaintenanceTask = api.MaintenanceTaskRenewCerts return doc, true }, - shouldRunSteps: utilgenerics.ConcatMultipleSlices(zerothSteps, certificateRenewalSteps), + shouldRunSteps: utilgenerics.ConcatMultipleSlices(zerothStepsServicePrincipal, certificateRenewalSteps), }, { name: "SyncClusterObject steps", @@ -206,7 +213,7 @@ func TestAdminUpdateSteps(t *testing.T) { doc.OpenShiftCluster.Properties.MaintenanceTask = api.MaintenanceTaskSyncClusterObject return doc, true }, - shouldRunSteps: utilgenerics.ConcatMultipleSlices(zerothSteps, syncClusterObjectSteps), + shouldRunSteps: utilgenerics.ConcatMultipleSlices(zerothStepsServicePrincipal, syncClusterObjectSteps), }, { name: "adminUpdate() does not adopt Hive-created clusters", @@ -219,7 +226,23 @@ func TestAdminUpdateSteps(t *testing.T) { return doc, true }, shouldRunSteps: utilgenerics.ConcatMultipleSlices( - zerothSteps, generalFixesSteps, certificateRenewalSteps, + zerothStepsServicePrincipal, generalFixesSteps, certificateRenewalSteps, + operatorUpdateSteps, updateProvisionedBySteps, + ), + }, + { + name: "adminUpdate() on managed identity cluster skips steps that are specific to service principal clusters", + fixture: func() (*api.OpenShiftClusterDocument, bool) { + doc := baseClusterDoc() + doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile = &api.PlatformWorkloadIdentityProfile{} + doc.OpenShiftCluster.Properties.ProvisioningState = api.ProvisioningStateAdminUpdating + doc.OpenShiftCluster.Properties.MaintenanceTask = api.MaintenanceTaskEverything + doc.OpenShiftCluster.Properties.HiveProfile.Namespace = "aro-00000000-0000-0000-0000-000000000000" + doc.OpenShiftCluster.Properties.HiveProfile.CreatedByHive = true + return doc, true + }, + shouldRunSteps: utilgenerics.ConcatMultipleSlices( + zerothStepsManagedIdentity, generalFixesSteps, certificateRenewalSteps, operatorUpdateSteps, updateProvisionedBySteps, ), }, diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go index bb42f3c81d9..e8f6b99f0cf 100644 --- a/pkg/cluster/install.go +++ b/pkg/cluster/install.go @@ -88,7 +88,10 @@ func (m *manager) getZerothSteps() []steps.Step { steps.Action(m.initializeKubernetesClients), // must be first steps.Action(m.ensureBillingRecord), // belt and braces steps.Action(m.ensureDefaults), - steps.Action(m.fixupClusterSPObjectID), + } + + if !m.doc.OpenShiftCluster.UsesWorkloadIdentity() { + bootstrap = append(bootstrap, steps.Action(m.fixupClusterSPObjectID)) } // Generic fix-up actions that are fairly safe to always take, and don't require a running cluster From 99e47f9c47436882391b2d02fedaa1e5cbe44bfc Mon Sep 17 00:00:00 2001 From: kimorris27 Date: Mon, 13 Jan 2025 15:52:33 -0600 Subject: [PATCH 3/4] Fix admin update - ensure that MIWI cluster doc fields are not overwritten --- pkg/cluster/adminupdate_test.go | 7 ++++++- pkg/cluster/install.go | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/pkg/cluster/adminupdate_test.go b/pkg/cluster/adminupdate_test.go index fd08fdfd24e..84e60477db4 100644 --- a/pkg/cluster/adminupdate_test.go +++ b/pkg/cluster/adminupdate_test.go @@ -44,6 +44,11 @@ func TestAdminUpdateSteps(t *testing.T) { "[Action initializeKubernetesClients]", "[Action ensureBillingRecord]", "[Action ensureDefaults]", + "[Action fixupClusterMsiTenantID]", + "[Action ensureClusterMsiCertificate]", + "[Action initializeClusterMsiClients]", + "[AuthorizationRetryingAction clusterIdentityIDs]", + "[AuthorizationRetryingAction platformWorkloadIdentityIDs]", "[Action fixInfraID]", } @@ -231,7 +236,7 @@ func TestAdminUpdateSteps(t *testing.T) { ), }, { - name: "adminUpdate() on managed identity cluster skips steps that are specific to service principal clusters", + name: "adminUpdate() on managed identity cluster skips steps that are specific to service principal clusters and includes steps that are specific to managed identity", fixture: func() (*api.OpenShiftClusterDocument, bool) { doc := baseClusterDoc() doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile = &api.PlatformWorkloadIdentityProfile{} diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go index e8f6b99f0cf..c444dc05cf5 100644 --- a/pkg/cluster/install.go +++ b/pkg/cluster/install.go @@ -90,8 +90,21 @@ func (m *manager) getZerothSteps() []steps.Step { steps.Action(m.ensureDefaults), } - if !m.doc.OpenShiftCluster.UsesWorkloadIdentity() { + if m.doc.OpenShiftCluster.UsesWorkloadIdentity() { + // Since API converters rebuild the struct during PUT/PATCH, we need to repopulate the tenant ID + // in the cluster doc for MSI stuff to work. + managedIdentitySteps := []steps.Step{ + steps.Action(m.fixupClusterMsiTenantID), + steps.Action(m.ensureClusterMsiCertificate), + steps.Action(m.initializeClusterMsiClients), + steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs), + steps.AuthorizationRetryingAction(m.fpAuthorizer, m.platformWorkloadIdentityIDs), + } + + bootstrap = append(bootstrap, managedIdentitySteps...) + } else { bootstrap = append(bootstrap, steps.Action(m.fixupClusterSPObjectID)) + } // Generic fix-up actions that are fairly safe to always take, and don't require a running cluster From cb2a392577999362ef5df351f516ce43f133efae Mon Sep 17 00:00:00 2001 From: kimorris27 Date: Tue, 14 Jan 2025 07:08:54 -0600 Subject: [PATCH 4/4] Remove extra whitespace to appease linter --- pkg/cluster/install.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go index c444dc05cf5..b70958e3483 100644 --- a/pkg/cluster/install.go +++ b/pkg/cluster/install.go @@ -104,7 +104,6 @@ func (m *manager) getZerothSteps() []steps.Step { bootstrap = append(bootstrap, managedIdentitySteps...) } else { bootstrap = append(bootstrap, steps.Action(m.fixupClusterSPObjectID)) - } // Generic fix-up actions that are fairly safe to always take, and don't require a running cluster