From f8222019a3b90c9ca58c46b3ec5d98a6af45dddb Mon Sep 17 00:00:00 2001 From: Srinivas Atmakuri Date: Thu, 15 Jun 2023 10:51:35 +0530 Subject: [PATCH] Propagate errors of ARO ImageConfig controller to ARO Operator (#2939) * Propagate errors of ARO ImageConfig controller to Operator * Modified test cases --- .../imageconfig/image_controller.go | 36 ++++++++---- .../imageconfig/image_controller_test.go | 55 +++++++++++++++++-- 2 files changed, 75 insertions(+), 16 deletions(-) diff --git a/pkg/operator/controllers/imageconfig/image_controller.go b/pkg/operator/controllers/imageconfig/image_controller.go index 01aaad5007e..c9275053478 100644 --- a/pkg/operator/controllers/imageconfig/image_controller.go +++ b/pkg/operator/controllers/imageconfig/image_controller.go @@ -20,6 +20,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" 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/util/azureclient" ) @@ -33,15 +34,16 @@ const ( ) 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, + }, } } @@ -51,17 +53,17 @@ func NewReconciler(log *logrus.Entry, client client.Client) *Reconciler { // - Fails fast if both are not nil, unsupported 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) + err := r.Client.Get(ctx, types.NamespacedName{Name: arov1alpha1.SingletonClusterName}, instance) if err != nil { return reconcile.Result{}, err } if !instance.Spec.OperatorFlags.GetSimpleBoolean(controllerEnabled) { - r.log.Debug("controller is disabled") + r.Log.Debug("controller is disabled") return reconcile.Result{}, nil } - r.log.Debug("running") + r.Log.Debug("running") requiredRegistries, err := GetCloudAwareRegistries(instance) if err != nil { // Not returning error as it will requeue again @@ -70,14 +72,19 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. // Get image.config yaml imageconfig := &configv1.Image{} - err = r.client.Get(ctx, types.NamespacedName{Name: request.Name}, imageconfig) + err = r.Client.Get(ctx, types.NamespacedName{Name: request.Name}, imageconfig) if err != nil { + r.Log.Error(err) + r.SetDegraded(ctx, err) + return reconcile.Result{}, err } // Fail fast if both are not nil if imageconfig.Spec.RegistrySources.AllowedRegistries != nil && imageconfig.Spec.RegistrySources.BlockedRegistries != nil { err := errors.New("both AllowedRegistries and BlockedRegistries are present") + r.Log.Error(err) + r.SetDegraded(ctx, err) return reconcile.Result{}, err } @@ -102,7 +109,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. } // Update image config registry - return reconcile.Result{}, r.client.Update(ctx, imageconfig) + err = r.Client.Update(ctx, imageconfig) + if err != nil { + r.Log.Error(err) + r.SetDegraded(ctx, err) + + return reconcile.Result{}, err + } + + r.ClearConditions(ctx) + return reconcile.Result{}, nil } // SetupWithManager setup the manager diff --git a/pkg/operator/controllers/imageconfig/image_controller_test.go b/pkg/operator/controllers/imageconfig/image_controller_test.go index d5fa1ef8d76..c621f5c7795 100644 --- a/pkg/operator/controllers/imageconfig/image_controller_test.go +++ b/pkg/operator/controllers/imageconfig/image_controller_test.go @@ -8,8 +8,10 @@ import ( "reflect" "strconv" "testing" + "time" configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -20,17 +22,26 @@ import ( "github.com/Azure/ARO-RP/pkg/util/azureclient" "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" ) // Test reconcile function func TestImageConfigReconciler(t *testing.T) { + transitionTime := metav1.Time{Time: time.Now()} + defaultAvailable := utilconditions.ControllerDefaultAvailable(ControllerName) + defaultProgressing := utilconditions.ControllerDefaultProgressing(ControllerName) + defaultDegraded := utilconditions.ControllerDefaultDegraded(ControllerName) + defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} + type test struct { name string instance *arov1alpha1.Cluster image *configv1.Image wantRegistrySources configv1.RegistrySources wantErr string + startConditions []operatorv1.OperatorCondition + wantConditions []operatorv1.OperatorCondition } for _, tt := range []*test{ @@ -46,6 +57,9 @@ func TestImageConfigReconciler(t *testing.T) { }, Location: "eastus", }, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, }, image: &configv1.Image{ ObjectMeta: metav1.ObjectMeta{Name: arov1alpha1.SingletonClusterName}, @@ -62,6 +76,8 @@ func TestImageConfigReconciler(t *testing.T) { "quay.io", }, }, + startConditions: defaultConditions, + wantConditions: defaultConditions, }, { name: "Image config registry source is empty, no action", @@ -69,6 +85,8 @@ func TestImageConfigReconciler(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: arov1alpha1.SingletonClusterName}, }, wantRegistrySources: configv1.RegistrySources{}, + startConditions: defaultConditions, + wantConditions: defaultConditions, }, { name: "allowedRegistries exists with duplicates, function should appropriately add registries", @@ -91,6 +109,8 @@ func TestImageConfigReconciler(t *testing.T) { "arointsvc.eastus.data.azurecr.io", }, }, + startConditions: defaultConditions, + wantConditions: defaultConditions, }, { name: "blockedRegistries exists, function should delete registries", @@ -111,6 +131,8 @@ func TestImageConfigReconciler(t *testing.T) { "quay.io", }, }, + startConditions: defaultConditions, + wantConditions: defaultConditions, }, { name: "AZEnvironment is unset, no action", @@ -121,6 +143,9 @@ func TestImageConfigReconciler(t *testing.T) { controllerEnabled: strconv.FormatBool(true), }, }, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, }, image: &configv1.Image{ ObjectMeta: metav1.ObjectMeta{Name: arov1alpha1.SingletonClusterName}, @@ -137,6 +162,8 @@ func TestImageConfigReconciler(t *testing.T) { "quay.io", }, }, + startConditions: defaultConditions, + wantConditions: defaultConditions, }, { name: "Both AllowedRegistries and BlockedRegistries are present, function should fail silently and not requeue", @@ -163,7 +190,18 @@ func TestImageConfigReconciler(t *testing.T) { "quay.io", }, }, - wantErr: `both AllowedRegistries and BlockedRegistries are present`, + wantErr: `both AllowedRegistries and BlockedRegistries are present`, + startConditions: defaultConditions, + wantConditions: []operatorv1.OperatorCondition{ + defaultAvailable, + defaultProgressing, + { + Type: ControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, + Status: operatorv1.ConditionTrue, + LastTransitionTime: transitionTime, + Message: `both AllowedRegistries and BlockedRegistries are present`, + }, + }, }, { name: "uses Public Cloud cluster's ACRDomain configuration for both Azure registries", @@ -193,6 +231,8 @@ func TestImageConfigReconciler(t *testing.T) { "fakesvc.anyplace.data.azurecr.io", }, }, + startConditions: defaultConditions, + wantConditions: defaultConditions, }, { name: "uses USGov Cloud cluster's ACRDomain configuration for both Azure registries", @@ -222,6 +262,8 @@ func TestImageConfigReconciler(t *testing.T) { "fakesvc.anyplace.data.azurecr.us", }, }, + startConditions: defaultConditions, + wantConditions: defaultConditions, }, } { t.Run(tt.name, func(t *testing.T) { @@ -237,6 +279,9 @@ func TestImageConfigReconciler(t *testing.T) { }, Location: "eastus", }, + Status: arov1alpha1.ClusterStatus{ + Conditions: tt.startConditions, + }, } if tt.instance != nil { instance = tt.instance @@ -244,18 +289,16 @@ func TestImageConfigReconciler(t *testing.T) { clientFake := ctrlfake.NewClientBuilder().WithObjects(instance, tt.image).Build() - r := &Reconciler{ - log: logrus.NewEntry(logrus.StandardLogger()), - client: clientFake, - } + r := NewReconciler(logrus.NewEntry(logrus.StandardLogger()), clientFake) request := ctrl.Request{} request.Name = "cluster" _, err := r.Reconcile(ctx, request) utilerror.AssertErrorMessage(t, err, tt.wantErr) + utilconditions.AssertControllerConditions(t, ctx, clientFake, tt.wantConditions) imgcfg := &configv1.Image{} - err = r.client.Get(ctx, types.NamespacedName{Name: request.Name}, imgcfg) + err = r.Client.Get(ctx, types.NamespacedName{Name: request.Name}, imgcfg) if err != nil { t.Fatal(err) }