Use refreshing actions where required during Update()/AdminUpdate() (#…

…2944) * Use refreshing actions on validateResources during Update() Otherwise, if a customer supplies a new service principal credential, `az aro update` may error out since we don't wait long enough for the credential to propagate through ARM layer * (hack) Add refreshing action on fp authorizer against the cluster sp graph
Azure · Jun 8, 2023 · e856b4c · e856b4c
1 parent 1764bc9
commit e856b4c
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 10 deletions.
diff --git a/pkg/cluster/adminupdate_test.go b/pkg/cluster/adminupdate_test.go
@@ -43,7 +43,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action initializeKubernetesClients-fm]",
 				"[Action ensureBillingRecord-fm]",
 				"[Action ensureDefaults-fm]",
-				"[Action fixupClusterSPObjectID-fm]",
+				"[AuthorizationRetryingAction fixupClusterSPObjectID-fm]",
 				"[Action fixInfraID-fm]",
 				"[Action startVMs-fm]",
 				"[Condition apiServersReady-fm, timeout 30m0s]",
@@ -65,7 +65,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action initializeKubernetesClients-fm]",
 				"[Action ensureBillingRecord-fm]",
 				"[Action ensureDefaults-fm]",
-				"[Action fixupClusterSPObjectID-fm]",
+				"[AuthorizationRetryingAction fixupClusterSPObjectID-fm]",
 				"[Action fixInfraID-fm]",
 				"[Action ensureResourceGroup-fm]",
 				"[Action createOrUpdateDenyAssignment-fm]",
@@ -110,7 +110,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action initializeKubernetesClients-fm]",
 				"[Action ensureBillingRecord-fm]",
 				"[Action ensureDefaults-fm]",
-				"[Action fixupClusterSPObjectID-fm]",
+				"[AuthorizationRetryingAction fixupClusterSPObjectID-fm]",
 				"[Action fixInfraID-fm]",
 				"[Action ensureResourceGroup-fm]",
 				"[Action createOrUpdateDenyAssignment-fm]",
@@ -151,7 +151,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action initializeKubernetesClients-fm]",
 				"[Action ensureBillingRecord-fm]",
 				"[Action ensureDefaults-fm]",
-				"[Action fixupClusterSPObjectID-fm]",
+				"[AuthorizationRetryingAction fixupClusterSPObjectID-fm]",
 				"[Action fixInfraID-fm]",
 				"[Action ensureResourceGroup-fm]",
 				"[Action createOrUpdateDenyAssignment-fm]",
@@ -196,7 +196,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action initializeKubernetesClients-fm]",
 				"[Action ensureBillingRecord-fm]",
 				"[Action ensureDefaults-fm]",
-				"[Action fixupClusterSPObjectID-fm]",
+				"[AuthorizationRetryingAction fixupClusterSPObjectID-fm]",
 				"[Action fixInfraID-fm]",
 				"[Action populateDatabaseIntIP-fm]",
 				"[Action startVMs-fm]",
@@ -223,7 +223,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action initializeKubernetesClients-fm]",
 				"[Action ensureBillingRecord-fm]",
 				"[Action ensureDefaults-fm]",
-				"[Action fixupClusterSPObjectID-fm]",
+				"[AuthorizationRetryingAction fixupClusterSPObjectID-fm]",
 				"[Action fixInfraID-fm]",
 				"[Action ensureResourceGroup-fm]",
 				"[Action createOrUpdateDenyAssignment-fm]",

diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go
@@ -46,7 +46,11 @@ func (m *manager) adminUpdate() []steps.Step {
 		steps.Action(m.initializeKubernetesClients), // must be first
 		steps.Action(m.ensureBillingRecord),         // belt and braces
 		steps.Action(m.ensureDefaults),
-		steps.Action(m.fixupClusterSPObjectID),
+
+		// TODO: this relies on an authorizer that isn't exposed in the manager
+		// struct, so we'll rebuild the fpAuthorizer and use the error catching
+		// to advance
+		steps.AuthorizationRetryingAction(m.fpAuthorizer, m.fixupClusterSPObjectID),
 		steps.Action(m.fixInfraID), // Old clusters lacks infraID in the database. Which makes code prone to errors.
 	}
 
@@ -162,11 +166,15 @@ func (m *manager) clusterWasCreatedByHive() bool {
 
 func (m *manager) Update(ctx context.Context) error {
 	s := []steps.Step{
-		steps.Action(m.validateResources),
+		steps.AuthorizationRetryingAction(m.fpAuthorizer, m.validateResources),
 		steps.Action(m.initializeKubernetesClients), // All init steps are first
 		steps.Action(m.initializeOperatorDeployer),  // depends on kube clients
 		steps.Action(m.initializeClusterSPClients),
-		steps.Action(m.clusterSPObjectID),
+
+		// TODO: this relies on an authorizer that isn't exposed in the manager
+		// struct, so we'll rebuild the fpAuthorizer and use the error catching
+		// to advance
+		steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterSPObjectID),
 		// credentials rotation flow steps
 		steps.Action(m.createOrUpdateClusterServicePrincipalRBAC),
 		steps.Action(m.createOrUpdateDenyAssignment),
@@ -241,7 +249,11 @@ func (m *manager) bootstrap() []steps.Step {
 
 		steps.Action(m.createDNS),
 		steps.Action(m.initializeClusterSPClients), // must run before clusterSPObjectID
-		steps.Action(m.clusterSPObjectID),
+
+		// TODO: this relies on an authorizer that isn't exposed in the manager
+		// struct, so we'll rebuild the fpAuthorizer and use the error catching
+		// to advance
+		steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterSPObjectID),
 		steps.Action(m.ensureResourceGroup),
 		steps.Action(m.ensureServiceEndpoints),
 		steps.Action(m.setMasterSubnetPolicies),

diff --git a/pkg/cluster/serviceprincipal.go b/pkg/cluster/serviceprincipal.go
@@ -51,6 +51,8 @@ func (m *manager) clusterSPObjectID(ctx context.Context) error {
 		var res azgraphrbac.ServicePrincipalObjectResult
 		res, err = m.spApplications.GetServicePrincipalsIDByAppID(ctx, spp.ClientID)
 		if err != nil {
+			// TODO - with migration to MSGraph, this error is different now.
+			// it's fixed with a refreshing action, but we should fix it here too
 			if strings.Contains(err.Error(), "Authorization_IdentityNotFound") {
 				m.log.Info(err)
 				return false, nil