Change Action rotateACRTokenPassword to Condition rotateAndValidateAC…

…RTokenPassword

pkg/cluster/acrtoken.go

@@ -70,36 +70,36 @@ func (m *manager) ensureACRToken(ctx context.Context) error {
 	return nil
 }
 
-func (m *manager) rotateACRTokenPassword(ctx context.Context) error {
+func (m *manager) rotateAndValidateACRTokenPassword(ctx context.Context) (bool, error) {
 	// we do not want to rotate tokens in local development
 	if m.env.IsLocalDevelopmentMode() || m.env.IsCI() {
-		return nil
+		return true, nil
 	}
 
 	token, err := acrtoken.NewManager(m.env, m.localFpAuthorizer)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	registryProfile := token.GetRegistryProfile(m.doc.OpenShiftCluster)
 	err = token.RotateTokenPassword(ctx, registryProfile)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	m.doc, err = m.db.PatchWithLease(ctx, m.doc.Key, func(doc *api.OpenShiftClusterDocument) error {
 		token.PutRegistryProfile(doc.OpenShiftCluster, registryProfile)
 		return nil
 	})
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	// update cluster pull secret in openshift-azure-operator namespace
 	// secret is stored as a .dockerconfigjson string in the .dockerconfigjson key
 	encodedDockerConfigJson, _, err := pullsecret.SetRegistryProfiles("", registryProfile)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	// wait for response from operator that reconciliation is completed successfully
@@ -114,16 +114,20 @@ func (m *manager) rotateACRTokenPassword(ctx context.Context) error {
 
 	_, err = m.kubernetescli.CoreV1().Secrets(operator.Namespace).Update(ctx, pullSecret, metav1.UpdateOptions{})
 	if err != nil {
-		return err
+		return false, err
 	}
 	err = retryOperation(func() error {
-		return m.rotateOpenShiftConfigSecret(ctx, pullSecret.Data[corev1.DockerConfigJsonKey])
+		e := m.rotateOpenShiftConfigSecret(ctx, pullSecret.Data[corev1.DockerConfigJsonKey])
+		if e != nil {
+			return e
+		}
+		return m.validateACRToken(ctx)
 	})
 	if err != nil {
-		return err
+		return false, err
 	}
 
-	return nil
+	return true, nil
 }
 
 func (m *manager) rotateOpenShiftConfigSecret(ctx context.Context, encodedDockerConfigJson []byte) error {

pkg/cluster/adminupdate_test.go

@@ -130,9 +130,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action fixMCSCert-fm]",
 				"[Action fixMCSUserData-fm]",
 				"[Action ensureGatewayUpgrade-fm]",
-				"[Condition aroDeploymentReady-fm, timeout 5m0s]",
-				"[Action rotateACRTokenPassword-fm]",
-				"[Condition ensureValidACRToken-fm, timeout 5m0s]",
+				"[Condition rotateAndValidateACRTokenPassword-fm, timeout 5m0s]",
 				"[Action configureAPIServerCertificate-fm]",
 				"[Action configureIngressCertificate-fm]",
 				"[Action populateRegistryStorageAccountName-fm]",
@@ -178,7 +176,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action fixMCSCert-fm]",
 				"[Action fixMCSUserData-fm]",
 				"[Action ensureGatewayUpgrade-fm]",
-				"[Action rotateACRTokenPassword-fm]",
+				"[Condition rotateAndValidateACRTokenPassword-fm, timeout 5m0s]",
 				"[Action configureAPIServerCertificate-fm]",
 				"[Action configureIngressCertificate-fm]",
 				"[Action populateRegistryStorageAccountName-fm]",
@@ -220,9 +218,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action fixMCSCert-fm]",
 				"[Action fixMCSUserData-fm]",
 				"[Action ensureGatewayUpgrade-fm]",
-				"[Condition aroDeploymentReady-fm, timeout 5m0s]",
-				"[Action rotateACRTokenPassword-fm]",
-				"[Condition ensureValidACRToken-fm, timeout 5m0s]",
+				"[Condition rotateAndValidateACRTokenPassword-fm, timeout 5m0s]",
 				"[Action configureAPIServerCertificate-fm]",
 				"[Action configureIngressCertificate-fm]",
 				"[Action populateRegistryStorageAccountName-fm]",
@@ -263,9 +259,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action fixMCSCert-fm]",
 				"[Action fixMCSUserData-fm]",
 				"[Action ensureGatewayUpgrade-fm]",
-				"[Condition aroDeploymentReady-fm, timeout 5m0s]",
-				"[Action rotateACRTokenPassword-fm]",
-				"[Condition ensureValidACRToken-fm, timeout 5m0s]",
+				"[Condition rotateAndValidateACRTokenPassword-fm, timeout 5m0s]",
 				"[Action configureAPIServerCertificate-fm]",
 				"[Action configureIngressCertificate-fm]",
 				"[Action populateRegistryStorageAccountName-fm]",
@@ -337,9 +331,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action fixMCSCert-fm]",
 				"[Action fixMCSUserData-fm]",
 				"[Action ensureGatewayUpgrade-fm]",
-				"[Condition aroDeploymentReady-fm, timeout 5m0s]",
-				"[Action rotateACRTokenPassword-fm]",
-				"[Condition ensureValidACRToken-fm, timeout 5m0s]",
+				"[Condition rotateAndValidateACRTokenPassword-fm, timeout 5m0s]",
 				"[Action configureAPIServerCertificate-fm]",
 				"[Action configureIngressCertificate-fm]",
 				"[Action populateRegistryStorageAccountName-fm]",

pkg/cluster/install.go

@@ -102,9 +102,7 @@ func (m *manager) adminUpdate() []steps.Step {
 	if isEverything {
 		toRun = append(toRun,
 			steps.Action(m.ensureGatewayUpgrade),
-			steps.Condition(m.aroDeploymentReady, 5*time.Minute, true),
-			steps.Action(m.rotateACRTokenPassword),
-			steps.Action(m.validateACRToken),
+			steps.Condition(m.rotateAndValidateACRTokenPassword, 5*time.Minute, true),
 		)
 	}
 
@@ -197,9 +195,7 @@ func (m *manager) Update(ctx context.Context) error {
 		steps.Action(m.createOrUpdateDenyAssignment),
 		steps.Action(m.startVMs),
 		steps.Condition(m.apiServersReady, 30*time.Minute, true),
-		steps.Condition(m.aroDeploymentReady, 5*time.Minute, true),
-		steps.Action(m.rotateACRTokenPassword),
-		steps.Action(m.validateACRToken),
+		steps.Condition(m.rotateAndValidateACRTokenPassword, 5*time.Minute, true),
 		steps.Action(m.configureAPIServerCertificate),
 		steps.Action(m.configureIngressCertificate),
 		steps.Action(m.renewMDSDCertificate),

pkg/operator/controllers/pullsecret/pullsecret_controller.go

@@ -47,13 +47,15 @@ var rhKeys = []string{"registry.redhat.io", "cloud.openshift.com", "registry.con
 type Reconciler struct {
 	log *logrus.Entry
 
-	client client.Client
+	client         client.Client
+	registryClient pullsecret.RegistryClient
 }
 
 func NewReconciler(log *logrus.Entry, client client.Client) *Reconciler {
 	return &Reconciler{
-		log:    log,
-		client: client,
+		log:            log,
+		client:         client,
+		registryClient: pullsecret.NewRegistryClient(),
 	}
 }
 
@@ -196,8 +198,7 @@ func (r *Reconciler) ensureGlobalPullSecret(ctx context.Context, operatorSecret,
 			return secret, err
 		}
 
-		rc := pullsecret.NewRegistryClient()
-		err = rc.ValidatePullSecret(ctx, secret)
+		err = r.registryClient.ValidatePullSecret(ctx, secret)
 		return secret, err
 	}
 
@@ -206,8 +207,7 @@ func (r *Reconciler) ensureGlobalPullSecret(ctx context.Context, operatorSecret,
 		return secret, err
 	}
 
-	rc := pullsecret.NewRegistryClient()
-	err = rc.ValidatePullSecret(ctx, secret)
+	err = r.registryClient.ValidatePullSecret(ctx, secret)
 	return secret, err
 }
 

pkg/operator/controllers/pullsecret/pullsecret_controller_test.go

@@ -9,6 +9,7 @@ import (
 	"reflect"
 	"testing"
 
+	container_types "github.com/containers/image/v5/types"
 	"github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -20,6 +21,7 @@ import (
 	"github.com/Azure/ARO-RP/pkg/operator"
 	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
 	"github.com/Azure/ARO-RP/pkg/util/cmp"
+	"github.com/Azure/ARO-RP/pkg/util/pullsecret"
 	_ "github.com/Azure/ARO-RP/pkg/util/scheme"
 	utilerror "github.com/Azure/ARO-RP/test/util/error"
 )
@@ -289,6 +291,9 @@ func TestPullSecretReconciler(t *testing.T) {
 			r := &Reconciler{
 				log:    logrus.NewEntry(logrus.StandardLogger()),
 				client: clientFake,
+				registryClient: pullsecret.RegistryClient{
+					CheckAuth: func(ctx context.Context, sc *container_types.SystemContext, s1, s2, s3 string) error { return nil },
+				},
 			}
 			if tt.request.Name == "" {
 				tt.request.NamespacedName = pullSecretName
@@ -767,7 +772,8 @@ func TestEnsureGlobalPullSecret(t *testing.T) {
 			}
 
 			r := &Reconciler{
-				client: clientBuilder.Build(),
+				client:         clientBuilder.Build(),
+				registryClient: pullsecret.RegistryClient{CheckAuth: func(ctx context.Context, sc *container_types.SystemContext, s1, s2, s3 string) error { return nil }},
 			}
 
 			s, err := r.ensureGlobalPullSecret(ctx, tt.operatorPullSecret, tt.pullSecret)

pkg/util/pullsecret/pullsecret.go

@@ -170,18 +170,18 @@ func Build(oc *api.OpenShiftCluster, ps string) (string, error) {
 	return pullSecret, nil
 }
 
-type registryClient struct {
-	checkAuth func(context.Context, *types.SystemContext, string, string, string) error
+type RegistryClient struct {
+	CheckAuth func(context.Context, *types.SystemContext, string, string, string) error
 }
 
-func NewRegistryClient() registryClient {
-	return registryClient{
-		checkAuth: dockerregistry.CheckAuth,
+func NewRegistryClient() RegistryClient {
+	return RegistryClient{
+		CheckAuth: dockerregistry.CheckAuth,
 	}
 }
 
 // ValidatePullSecret validates a passed in pull secret by attempting to log in to the registry
-func (r *registryClient) ValidatePullSecret(ctx context.Context, secret *corev1.Secret) error {
+func (r *RegistryClient) ValidatePullSecret(ctx context.Context, secret *corev1.Secret) error {
 	dockerConfig, err := UnmarshalSecretData(secret)
 	if err != nil {
 		return err
@@ -200,7 +200,7 @@ func (r *registryClient) ValidatePullSecret(ctx context.Context, secret *corev1.
 		if err != nil {
 			return err
 		}
-		err = r.checkAuth(ctx, nil, auth[0], auth[1], registry)
+		err = r.CheckAuth(ctx, nil, auth[0], auth[1], registry)
 		if err != nil {
 			return fmt.Errorf("failed to authenticate to registry %s: %w", registry, err)
 		}

pkg/util/pullsecret/pullsecret_test.go

@@ -379,13 +379,13 @@ func TestUnmarshalSecretData(t *testing.T) {
 
 func TestValidatePullSecret(t *testing.T) {
 	azurecrError := "unable to retrieve auth token: invalid username/password: unauthorized: authentication required, visit https://aka.ms/acr/authorization for more information."
-	erroringRegistry := registryClient{
-		checkAuth: func(ctx context.Context, sc *types.SystemContext, s1, s2, s3 string) error {
+	erroringRegistry := RegistryClient{
+		CheckAuth: func(ctx context.Context, sc *types.SystemContext, s1, s2, s3 string) error {
 			return fmt.Errorf(azurecrError)
 		},
 	}
-	succeedingRegistry := registryClient{
-		checkAuth: func(ctx context.Context, sc *types.SystemContext, s1, s2, s3 string) error {
+	succeedingRegistry := RegistryClient{
+		CheckAuth: func(ctx context.Context, sc *types.SystemContext, s1, s2, s3 string) error {
 			return nil
 		},
 	}
@@ -394,7 +394,7 @@ func TestValidatePullSecret(t *testing.T) {
 		ps       *corev1.Secret
 		wantAuth map[string]string
 		wantErr  string
-		client   registryClient
+		client   RegistryClient
 	}{
 		{
 			name: "ok secret",