Skip to content

Commit

Permalink
Propagate errors of ARO ImageConfig controller to ARO Operator (Azure…
Browse files Browse the repository at this point in the history 
…#2939)

* Propagate errors of ARO ImageConfig controller to Operator

* Modified test cases
  • Loading branch information
@SrinivasAtmakuri @ellis-johnson
SrinivasAtmakuri authored and ellis-johnson committed Jun 19, 2023
1 parent 02d0ae0 commit f822201
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 16 deletions.
36 changes: 26 additions & 10 deletions pkg/operator/controllers/imageconfig/image_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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,
},
}
}

Expand All @@ -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
Expand All @@ -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
}

Expand All @@ -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
Expand Down
55 changes: 49 additions & 6 deletions pkg/operator/controllers/imageconfig/image_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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{
Expand All @@ -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},
Expand All @@ -62,13 +76,17 @@ func TestImageConfigReconciler(t *testing.T) {
"quay.io",
},
},
startConditions: defaultConditions,
wantConditions: defaultConditions,
},
{
name: "Image config registry source is empty, no action",
image: &configv1.Image{
ObjectMeta: metav1.ObjectMeta{Name: arov1alpha1.SingletonClusterName},
},
wantRegistrySources: configv1.RegistrySources{},
startConditions: defaultConditions,
wantConditions: defaultConditions,
},
{
name: "allowedRegistries exists with duplicates, function should appropriately add registries",
Expand All @@ -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",
Expand All @@ -111,6 +131,8 @@ func TestImageConfigReconciler(t *testing.T) {
"quay.io",
},
},
startConditions: defaultConditions,
wantConditions: defaultConditions,
},
{
name: "AZEnvironment is unset, no action",
Expand All @@ -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},
Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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) {
Expand All @@ -237,25 +279,26 @@ func TestImageConfigReconciler(t *testing.T) {
},
Location: "eastus",
},
Status: arov1alpha1.ClusterStatus{
Conditions: tt.startConditions,
},
}
if tt.instance != nil {
instance = tt.instance
}

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)
}
Expand Down

0 comments on commit f822201

Please sign in to comment.