From dbad3b391c4b90dd1591a2b5fdf2bf6246cd0d88 Mon Sep 17 00:00:00 2001 From: Andrew Denton Date: Wed, 8 Nov 2023 15:32:24 -0800 Subject: [PATCH] Change Action rotateACRTokenPassword to Condition rotateAndValidateACRTokenPassword --- pkg/cluster/acrtoken.go | 24 +++++++++++-------- pkg/cluster/adminupdate_test.go | 18 ++++---------- pkg/cluster/install.go | 8 ++----- .../pullsecret/pullsecret_controller.go | 14 +++++------ .../pullsecret/pullsecret_controller_test.go | 8 ++++++- pkg/util/pullsecret/pullsecret.go | 14 +++++------ pkg/util/pullsecret/pullsecret_test.go | 10 ++++---- 7 files changed, 47 insertions(+), 49 deletions(-) diff --git a/pkg/cluster/acrtoken.go b/pkg/cluster/acrtoken.go index e430e01a838..2b48e30d701 100644 --- a/pkg/cluster/acrtoken.go +++ b/pkg/cluster/acrtoken.go @@ -70,21 +70,21 @@ 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 { @@ -92,14 +92,14 @@ func (m *manager) rotateACRTokenPassword(ctx context.Context) error { 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 { diff --git a/pkg/cluster/adminupdate_test.go b/pkg/cluster/adminupdate_test.go index fd29aef99f1..54e29b499c9 100644 --- a/pkg/cluster/adminupdate_test.go +++ b/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]", diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go index 7cf57cb152a..a2f7f9202b6 100644 --- a/pkg/cluster/install.go +++ b/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), diff --git a/pkg/operator/controllers/pullsecret/pullsecret_controller.go b/pkg/operator/controllers/pullsecret/pullsecret_controller.go index 88e458240bb..d04cf0f8b7b 100644 --- a/pkg/operator/controllers/pullsecret/pullsecret_controller.go +++ b/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 } diff --git a/pkg/operator/controllers/pullsecret/pullsecret_controller_test.go b/pkg/operator/controllers/pullsecret/pullsecret_controller_test.go index f99fd074877..dfe8d2735dc 100644 --- a/pkg/operator/controllers/pullsecret/pullsecret_controller_test.go +++ b/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) diff --git a/pkg/util/pullsecret/pullsecret.go b/pkg/util/pullsecret/pullsecret.go index 836b973eea2..4eeee8bc2ba 100644 --- a/pkg/util/pullsecret/pullsecret.go +++ b/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) } diff --git a/pkg/util/pullsecret/pullsecret_test.go b/pkg/util/pullsecret/pullsecret_test.go index 44af51652f3..0e34f4b3d04 100644 --- a/pkg/util/pullsecret/pullsecret_test.go +++ b/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",