diff --git a/pkg/operator/controllers/pullsecret/pullsecret_controller.go b/pkg/operator/controllers/pullsecret/pullsecret_controller.go index 397291dcefc..9f5957a5625 100644 --- a/pkg/operator/controllers/pullsecret/pullsecret_controller.go +++ b/pkg/operator/controllers/pullsecret/pullsecret_controller.go @@ -30,6 +30,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/operator/controllers/base" "github.com/Azure/ARO-RP/pkg/operator/predicates" "github.com/Azure/ARO-RP/pkg/util/pullsecret" ) @@ -43,15 +44,16 @@ var rhKeys = []string{"registry.redhat.io", "cloud.openshift.com", "registry.con // Reconciler reconciles a Cluster object type Reconciler struct { - log *logrus.Entry - - client client.Client + base.AROController } func NewReconciler(log *logrus.Entry, client client.Client) *Reconciler { return &Reconciler{ - log: log, - client: client, + AROController: base.AROController{ + Log: log, + Client: client, + Name: ControllerName, + }, } } @@ -65,21 +67,22 @@ func NewReconciler(log *logrus.Entry, client client.Client) *Reconciler { // - If the pull Secret object (which is not owned by the Cluster object) // changes, we'll see the pull Secret object requested. func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { - instance := &arov1alpha1.Cluster{} - err := r.client.Get(ctx, types.NamespacedName{Name: arov1alpha1.SingletonClusterName}, instance) + instance, err := r.GetCluster(ctx) if err != nil { + r.Log.Error(err) return reconcile.Result{}, err } if !instance.Spec.OperatorFlags.GetSimpleBoolean(operator.PullSecretEnabled) { - r.log.Debug("controller is disabled") + r.Log.Debug("controller is disabled") return reconcile.Result{}, nil } - r.log.Debug("running") + r.Log.Debug("running") userSecret := &corev1.Secret{} - err = r.client.Get(ctx, pullSecretName, userSecret) + err = r.Client.Get(ctx, pullSecretName, userSecret) if err != nil && !kerrors.IsNotFound(err) { + r.Log.Error(err) return reconcile.Result{}, err } @@ -87,14 +90,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. // detects if the global pull secret is broken and fixes it by using backup managed by ARO operator if instance.Spec.OperatorFlags.GetSimpleBoolean(operator.PullSecretManaged) { operatorSecret := &corev1.Secret{} - err = r.client.Get(ctx, types.NamespacedName{Namespace: operator.Namespace, Name: operator.SecretName}, operatorSecret) + err = r.Client.Get(ctx, types.NamespacedName{Namespace: operator.Namespace, Name: operator.SecretName}, operatorSecret) if err != nil { + r.Log.Error(err) return reconcile.Result{}, err } // fix pull secret if its broken to have at least the ARO pull secret userSecret, err = r.ensureGlobalPullSecret(ctx, operatorSecret, userSecret) if err != nil { + r.Log.Error(err) return reconcile.Result{}, err } } @@ -104,10 +109,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. // - list of Red Hat pull-secret keys in status. instance.Status.RedHatKeysPresent, err = r.parseRedHatKeys(userSecret) if err != nil { + r.Log.Error(err) return reconcile.Result{}, err } - err = r.client.Update(ctx, instance) + err = r.Client.Update(ctx, instance) + if err == nil { + r.ClearConditions(ctx) + } else { + r.SetDegraded(ctx, err) + } return reconcile.Result{}, err } @@ -175,22 +186,23 @@ func (r *Reconciler) ensureGlobalPullSecret(ctx context.Context, operatorSecret, // delete possible existing userSecret, calling deletion every time and ignoring when secret not found // allows for simpler logic flow, when delete and create are not handled separately // this call happens only when there is a need to change, it has no significant impact on performance - err := r.client.Delete(ctx, secret) - r.log.Info("Global Pull secret Not Found, Creating Again") + err := r.Client.Delete(ctx, secret) + r.Log.Info("Global Pull secret Not Found, Creating Again") if err != nil && !kerrors.IsNotFound(err) { + r.Log.Error(err) return nil, err } - err = r.client.Create(ctx, secret) + err = r.Client.Create(ctx, secret) if err == nil { - r.log.Info("Global Pull secret Created") + r.Log.Info("Global Pull secret Created") } return secret, err } - err = r.client.Update(ctx, secret) + err = r.Client.Update(ctx, secret) if err == nil { - r.log.Info("Updated Existing Global Pull secret") + r.Log.Info("Updated Existing Global Pull secret") } return secret, err } @@ -205,7 +217,7 @@ func (r *Reconciler) parseRedHatKeys(secret *corev1.Secret) (foundKeys []string, // parse keys and validate JSON parsedKeys, err := pullsecret.UnmarshalSecretData(secret) if err != nil { - r.log.Info("pull secret is not valid json - recreating") + r.Log.Info("pull secret is not valid json - recreating") return foundKeys, err } diff --git a/pkg/operator/controllers/pullsecret/pullsecret_controller_test.go b/pkg/operator/controllers/pullsecret/pullsecret_controller_test.go index 8be8b80bde3..eaca4b83603 100644 --- a/pkg/operator/controllers/pullsecret/pullsecret_controller_test.go +++ b/pkg/operator/controllers/pullsecret/pullsecret_controller_test.go @@ -9,7 +9,9 @@ import ( "reflect" "testing" + operatorv1 "github.com/openshift/api/operator/v1" "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -21,6 +23,7 @@ import ( 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/scheme" + utilconditions "github.com/Azure/ARO-RP/test/util/conditions" utilerror "github.com/Azure/ARO-RP/test/util/error" ) @@ -36,14 +39,21 @@ func TestPullSecretReconciler(t *testing.T) { }, } + defaultAvailable := utilconditions.ControllerDefaultAvailable(ControllerName) + defaultProgressing := utilconditions.ControllerDefaultProgressing(ControllerName) + defaultDegraded := utilconditions.ControllerDefaultDegraded(ControllerName) + defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} + tests := []struct { - name string - request ctrl.Request - secrets []client.Object - instance *arov1alpha1.Cluster - wantKeys []string - wantErr bool - want string + name string + request ctrl.Request + secrets []client.Object + instance *arov1alpha1.Cluster + wantKeys []string + wantErr bool + want string + wantErrMsg string + wantConditions []operatorv1.OperatorCondition }{ { name: "deleted pull secret", @@ -57,9 +67,11 @@ func TestPullSecretReconciler(t *testing.T) { Data: map[string][]byte{corev1.DockerConfigJsonKey: []byte(`{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`)}, }, }, - instance: baseCluster, - want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, - wantKeys: nil, + instance: baseCluster, + want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, + wantKeys: nil, + wantErrMsg: "", + wantConditions: defaultConditions, }, { name: "missing arosvc pull secret", @@ -80,9 +92,11 @@ func TestPullSecretReconciler(t *testing.T) { Data: map[string][]byte{corev1.DockerConfigJsonKey: []byte(`{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`)}, }, }, - instance: baseCluster, - want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, - wantKeys: nil, + instance: baseCluster, + want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, + wantKeys: nil, + wantErrMsg: "", + wantConditions: defaultConditions, }, { name: "modified arosvc pull secret", @@ -106,9 +120,11 @@ func TestPullSecretReconciler(t *testing.T) { Data: map[string][]byte{corev1.DockerConfigJsonKey: []byte(`{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`)}, }, }, - instance: baseCluster, - want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, - wantKeys: nil, + instance: baseCluster, + want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, + wantKeys: nil, + wantErrMsg: "", + wantConditions: defaultConditions, }, { name: "unparseable secret", @@ -130,9 +146,11 @@ func TestPullSecretReconciler(t *testing.T) { Data: map[string][]byte{corev1.DockerConfigJsonKey: []byte(`{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`)}, }, }, - instance: baseCluster, - want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, - wantKeys: nil, + instance: baseCluster, + want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, + wantKeys: nil, + wantErrMsg: "", + wantConditions: defaultConditions, }, { name: "wrong secret type", @@ -153,9 +171,11 @@ func TestPullSecretReconciler(t *testing.T) { Data: map[string][]byte{corev1.DockerConfigJsonKey: []byte(`{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`)}, }, }, - instance: baseCluster, - want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, - wantKeys: nil, + instance: baseCluster, + want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, + wantKeys: nil, + wantErrMsg: "", + wantConditions: defaultConditions, }, { name: "no change", @@ -177,9 +197,11 @@ func TestPullSecretReconciler(t *testing.T) { Data: map[string][]byte{corev1.DockerConfigJsonKey: []byte(`{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`)}, }, }, - instance: baseCluster, - want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, - wantKeys: nil, + instance: baseCluster, + want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, + wantKeys: nil, + wantErrMsg: "", + wantConditions: defaultConditions, }, { name: "valid RH keys present", @@ -205,9 +227,11 @@ func TestPullSecretReconciler(t *testing.T) { }, }, }, - instance: baseCluster, - want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="},"registry.redhat.io":{"auth":"ZnJlZDplbnRlcg=="},"cloud.openshift.com":{"auth":"ZnJlZDplbnRlcg=="}}}`, - wantKeys: []string{"registry.redhat.io", "cloud.openshift.com"}, + instance: baseCluster, + want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="},"registry.redhat.io":{"auth":"ZnJlZDplbnRlcg=="},"cloud.openshift.com":{"auth":"ZnJlZDplbnRlcg=="}}}`, + wantKeys: []string{"registry.redhat.io", "cloud.openshift.com"}, + wantErrMsg: "", + wantConditions: defaultConditions, }, { name: "management disabled, valid RH key present", @@ -243,8 +267,10 @@ func TestPullSecretReconciler(t *testing.T) { }, }, }, - want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="},"registry.redhat.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, - wantKeys: []string{"registry.redhat.io"}, + want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="},"registry.redhat.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, + wantKeys: []string{"registry.redhat.io"}, + wantErrMsg: "", + wantConditions: defaultConditions, }, { name: "management disabled, valid RH key missing", @@ -276,8 +302,10 @@ func TestPullSecretReconciler(t *testing.T) { }, }, }, - want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, - wantKeys: nil, + want: `{"auths":{"arosvc.azurecr.io":{"auth":"ZnJlZDplbnRlcg=="}}}`, + wantKeys: nil, + wantErrMsg: "", + wantConditions: defaultConditions, }, } for _, tt := range tests { @@ -285,11 +313,11 @@ func TestPullSecretReconciler(t *testing.T) { ctx := context.Background() clientFake := ctrlfake.NewClientBuilder().WithObjects(tt.instance).WithObjects(tt.secrets...).Build() + assert.NotNil(t, clientFake) + + r := NewReconciler(logrus.NewEntry(logrus.StandardLogger()), clientFake) + assert.NotNil(t, r) - r := &Reconciler{ - log: logrus.NewEntry(logrus.StandardLogger()), - client: clientFake, - } if tt.request.Name == "" { tt.request.NamespacedName = pullSecretName } @@ -300,8 +328,12 @@ func TestPullSecretReconciler(t *testing.T) { return } + utilerror.AssertErrorMessage(t, err, tt.wantErrMsg) + utilconditions.AssertControllerConditions(t, ctx, clientFake, tt.wantConditions) + s := &corev1.Secret{} - err = r.client.Get(ctx, types.NamespacedName{Namespace: "openshift-config", Name: "pull-secret"}, s) + assert.NotNil(t, s) + err = r.Client.Get(ctx, types.NamespacedName{Namespace: "openshift-config", Name: "pull-secret"}, s) if err != nil { t.Error(err) } @@ -315,6 +347,7 @@ func TestPullSecretReconciler(t *testing.T) { } cluster := &arov1alpha1.Cluster{} + assert.NotNil(t, cluster) err = clientFake.Get(ctx, types.NamespacedName{Name: arov1alpha1.SingletonClusterName}, cluster) if err != nil { t.Fatal("Error found") @@ -363,9 +396,8 @@ func TestParseRedHatKeys(t *testing.T) { for _, tt := range test { t.Run(tt.name, func(t *testing.T) { - r := &Reconciler{ - log: logrus.NewEntry(logrus.StandardLogger()), - } + r := NewReconciler(logrus.NewEntry(logrus.StandardLogger()), nil) + assert.NotNil(t, r) out, err := r.parseRedHatKeys(tt.ps) utilerror.AssertErrorMessage(t, err, tt.wantErr) @@ -760,16 +792,15 @@ func TestEnsureGlobalPullSecret(t *testing.T) { for _, tt := range test { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() + assert.NotNil(t, ctx) clientBuilder := ctrlfake.NewClientBuilder() if tt.initialSecret != nil { clientBuilder = clientBuilder.WithObjects(tt.initialSecret) } - r := &Reconciler{ - client: clientBuilder.Build(), - log: logrus.NewEntry(logrus.StandardLogger()), - } + r := NewReconciler(logrus.NewEntry(logrus.StandardLogger()), clientBuilder.Build()) + assert.NotNil(t, r) s, err := r.ensureGlobalPullSecret(ctx, tt.operatorPullSecret, tt.pullSecret) utilerror.AssertErrorMessage(t, err, tt.wantError)