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. diff --git a/pkg/cluster/adminupdate_test.go b/pkg/cluster/adminupdate_test.go index 08b78c6e5f4..84e60477db4 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,18 @@ func TestAdminUpdateSteps(t *testing.T) { "[Action fixInfraID]", } + zerothStepsManagedIdentity := []string{ + "[Action initializeKubernetesClients]", + "[Action ensureBillingRecord]", + "[Action ensureDefaults]", + "[Action fixupClusterMsiTenantID]", + "[Action ensureClusterMsiCertificate]", + "[Action initializeClusterMsiClients]", + "[AuthorizationRetryingAction clusterIdentityIDs]", + "[AuthorizationRetryingAction platformWorkloadIdentityIDs]", + "[Action fixInfraID]", + } + generalFixesSteps := []string{ "[Action ensureResourceGroup]", "[Action createOrUpdateDenyAssignment]", @@ -111,7 +123,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 +134,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 +145,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 +156,7 @@ func TestAdminUpdateSteps(t *testing.T) { return doc, true }, shouldRunSteps: utilgenerics.ConcatMultipleSlices( - zerothSteps, generalFixesSteps, certificateRenewalSteps, + zerothStepsServicePrincipal, generalFixesSteps, certificateRenewalSteps, operatorUpdateSteps, hiveSteps, updateProvisionedBySteps, ), }, @@ -158,7 +170,7 @@ func TestAdminUpdateSteps(t *testing.T) { return doc, true }, shouldRunSteps: utilgenerics.ConcatMultipleSlices( - zerothSteps, generalFixesSteps, certificateRenewalSteps, + zerothStepsServicePrincipal, generalFixesSteps, certificateRenewalSteps, hiveSteps, updateProvisionedBySteps, ), }, @@ -171,7 +183,7 @@ func TestAdminUpdateSteps(t *testing.T) { return doc, false }, shouldRunSteps: utilgenerics.ConcatMultipleSlices( - zerothSteps, generalFixesSteps, certificateRenewalSteps, + zerothStepsServicePrincipal, generalFixesSteps, certificateRenewalSteps, operatorUpdateSteps, updateProvisionedBySteps, ), }, @@ -184,7 +196,7 @@ func TestAdminUpdateSteps(t *testing.T) { return doc, true }, shouldRunSteps: utilgenerics.ConcatMultipleSlices( - zerothSteps, generalFixesSteps, certificateRenewalSteps, + zerothStepsServicePrincipal, generalFixesSteps, certificateRenewalSteps, operatorUpdateSteps, hiveSteps, updateProvisionedBySteps, ), }, @@ -196,7 +208,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 +218,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 +231,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 and includes steps that are specific to managed identity", + 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 010f1fe1080..32dc0d132ef 100644 --- a/pkg/cluster/install.go +++ b/pkg/cluster/install.go @@ -88,7 +88,22 @@ 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() { + // 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