Fix MIWI admin update (#4046)

* Fix admin API - add readOnly tag to two fields that need it * Fix admin update - if cluster is MIWI, skip step specific to service principal clusters * Fix admin update - ensure that MIWI cluster doc fields are not overwritten * Remove extra whitespace to appease linter
Azure · Jan 30, 2025 · 433681f · 433681f
1 parent 1ef5ca7
commit 433681f
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 14 deletions.
diff --git 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
@@ -32,14 +32,26 @@ func TestAdminUpdateSteps(t *testing.T) {
 		}
 	}
 
-	zerothSteps := []string{
+	zerothStepsServicePrincipal := []string{
 		"[Action initializeKubernetesClients]",
 		"[Action ensureBillingRecord]",
 		"[Action ensureDefaults]",
 		"[Action fixupClusterSPObjectID]",
 		"[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
@@ -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