From 7449f3abad50ea6a3cab3c2a79fe9688178179a2 Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Mon, 30 Oct 2023 16:04:08 -0400 Subject: [PATCH 1/4] genevalogging: Use AROController as base type --- .../genevalogging/genevalogging.go | 2 +- .../genevalogging/genevalogging_controller.go | 34 ++++++++++--------- .../genevalogging/genevalogging_test.go | 7 ++-- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/pkg/operator/controllers/genevalogging/genevalogging.go b/pkg/operator/controllers/genevalogging/genevalogging.go index 5e6641db1cd..e5a4aef6db8 100644 --- a/pkg/operator/controllers/genevalogging/genevalogging.go +++ b/pkg/operator/controllers/genevalogging/genevalogging.go @@ -24,7 +24,7 @@ import ( func (r *Reconciler) securityContextConstraints(ctx context.Context, name, serviceAccountName string) (*securityv1.SecurityContextConstraints, error) { scc := &securityv1.SecurityContextConstraints{} - err := r.client.Get(ctx, types.NamespacedName{Name: "privileged"}, scc) + err := r.Client.Get(ctx, types.NamespacedName{Name: "privileged"}, scc) if err != nil { return nil, err } diff --git a/pkg/operator/controllers/genevalogging/genevalogging_controller.go b/pkg/operator/controllers/genevalogging/genevalogging_controller.go index 30b478effb5..b1231283079 100644 --- a/pkg/operator/controllers/genevalogging/genevalogging_controller.go +++ b/pkg/operator/controllers/genevalogging/genevalogging_controller.go @@ -19,6 +19,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/util/dynamichelper" ) @@ -34,62 +35,63 @@ const ( // Reconciler reconciles a Cluster object type Reconciler struct { - log *logrus.Entry + base.AROController - dh dynamichelper.Interface - client client.Client + dh dynamichelper.Interface } func NewReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *Reconciler { return &Reconciler{ - log: log, - - dh: dh, - client: client, + AROController: base.AROController{ + Log: log, + Client: client, + Name: ControllerName, + }, + dh: dh, } } // Reconcile the genevalogging deployment. 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(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") 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 { return reconcile.Result{}, err } resources, err := r.resources(ctx, instance, operatorSecret.Data[GenevaCertName], operatorSecret.Data[GenevaKeyName]) if err != nil { - r.log.Error(err) + r.Log.Error(err) return reconcile.Result{}, err } err = dynamichelper.SetControllerReferences(resources, instance) if err != nil { - r.log.Error(err) + r.Log.Error(err) return reconcile.Result{}, err } err = dynamichelper.Prepare(resources) if err != nil { - r.log.Error(err) + r.Log.Error(err) return reconcile.Result{}, err } err = r.dh.Ensure(ctx, resources...) if err != nil { - r.log.Error(err) + r.Log.Error(err) return reconcile.Result{}, err } diff --git a/pkg/operator/controllers/genevalogging/genevalogging_test.go b/pkg/operator/controllers/genevalogging/genevalogging_test.go index bcc98d42d51..9d57415bb9b 100644 --- a/pkg/operator/controllers/genevalogging/genevalogging_test.go +++ b/pkg/operator/controllers/genevalogging/genevalogging_test.go @@ -18,6 +18,7 @@ import ( ctrlfake "sigs.k8s.io/controller-runtime/pkg/client/fake" 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/scheme" "github.com/Azure/ARO-RP/pkg/util/version" testdatabase "github.com/Azure/ARO-RP/test/database" @@ -154,8 +155,10 @@ func TestGenevaLoggingDaemonset(t *testing.T) { } r := &Reconciler{ - log: logrus.NewEntry(logrus.StandardLogger()), - client: ctrlfake.NewClientBuilder().WithObjects(instance).Build(), + AROController: base.AROController{ + Log: logrus.NewEntry(logrus.StandardLogger()), + Client: ctrlfake.NewClientBuilder().WithObjects(instance).Build(), + }, } daemonset, err := r.daemonset(instance) From e07f6c6bb734f54db3e192d8a23c52ec80b9962d Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Tue, 31 Oct 2023 11:02:19 -0400 Subject: [PATCH 2/4] genevalogging: Split off business logic for uniform error handling Errors will be handled in the Reconcile method. --- .../genevalogging/genevalogging_controller.go | 50 +++++++++++-------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/pkg/operator/controllers/genevalogging/genevalogging_controller.go b/pkg/operator/controllers/genevalogging/genevalogging_controller.go index b1231283079..44d9a8f53dd 100644 --- a/pkg/operator/controllers/genevalogging/genevalogging_controller.go +++ b/pkg/operator/controllers/genevalogging/genevalogging_controller.go @@ -51,45 +51,55 @@ func NewReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Int } } -// Reconcile the genevalogging deployment. -func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { - instance, err := r.GetCluster(ctx) +func (r *Reconciler) ensureResources(ctx context.Context, instance *arov1alpha1.Cluster) error { + operatorSecret := &corev1.Secret{} + operatorSecretName := types.NamespacedName{ + Namespace: operator.Namespace, + Name: operator.SecretName, + } + err := r.Client.Get(ctx, operatorSecretName, operatorSecret) if err != nil { - r.Log.Error(err) - return reconcile.Result{}, err + return err } - if !instance.Spec.OperatorFlags.GetSimpleBoolean(controllerEnabled) { - r.Log.Debug("controller is disabled") - return reconcile.Result{}, nil + resources, err := r.resources(ctx, instance, operatorSecret.Data[GenevaCertName], operatorSecret.Data[GenevaKeyName]) + if err != nil { + return err } - r.Log.Debug("running") - operatorSecret := &corev1.Secret{} - err = r.Client.Get(ctx, types.NamespacedName{Namespace: operator.Namespace, Name: operator.SecretName}, operatorSecret) + err = dynamichelper.SetControllerReferences(resources, instance) if err != nil { - return reconcile.Result{}, err + return err } - resources, err := r.resources(ctx, instance, operatorSecret.Data[GenevaCertName], operatorSecret.Data[GenevaKeyName]) + err = dynamichelper.Prepare(resources) if err != nil { - r.Log.Error(err) - return reconcile.Result{}, err + return err } - err = dynamichelper.SetControllerReferences(resources, instance) + err = r.dh.Ensure(ctx, resources...) if err != nil { - r.Log.Error(err) - return reconcile.Result{}, err + return err } - err = dynamichelper.Prepare(resources) + return nil +} + +// Reconcile the genevalogging deployment. +func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { + instance, err := r.GetCluster(ctx) if err != nil { r.Log.Error(err) return reconcile.Result{}, err } - err = r.dh.Ensure(ctx, resources...) + if !instance.Spec.OperatorFlags.GetSimpleBoolean(controllerEnabled) { + r.Log.Debug("controller is disabled") + return reconcile.Result{}, nil + } + + r.Log.Debug("running") + err = r.ensureResources(ctx, instance) if err != nil { r.Log.Error(err) return reconcile.Result{}, err From 44d7535de621d0fabb4bf981d8d139b4d4579dfd Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Wed, 18 Oct 2023 14:00:17 -0400 Subject: [PATCH 3/4] genevalogging: Add condition for controller status --- .../controllers/genevalogging/genevalogging_controller.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/operator/controllers/genevalogging/genevalogging_controller.go b/pkg/operator/controllers/genevalogging/genevalogging_controller.go index 44d9a8f53dd..15ee6626e50 100644 --- a/pkg/operator/controllers/genevalogging/genevalogging_controller.go +++ b/pkg/operator/controllers/genevalogging/genevalogging_controller.go @@ -102,9 +102,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. err = r.ensureResources(ctx, instance) if err != nil { r.Log.Error(err) + r.SetDegraded(ctx, err) return reconcile.Result{}, err } + r.ClearConditions(ctx) return reconcile.Result{}, nil } From 787c71844edf121857e5834d5ede3e568b834385 Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Wed, 1 Nov 2023 11:19:15 -0400 Subject: [PATCH 4/4] genevalogging: Check status conditions in unit tests --- .../genevalogging/genevalogging_test.go | 79 ++++++++++++++++++- 1 file changed, 77 insertions(+), 2 deletions(-) diff --git a/pkg/operator/controllers/genevalogging/genevalogging_test.go b/pkg/operator/controllers/genevalogging/genevalogging_test.go index 9d57415bb9b..c0114b2dc7f 100644 --- a/pkg/operator/controllers/genevalogging/genevalogging_test.go +++ b/pkg/operator/controllers/genevalogging/genevalogging_test.go @@ -4,24 +4,32 @@ package genevalogging // Licensed under the Apache License 2.0. import ( + "context" "errors" "fmt" "testing" "github.com/go-test/deep" + "github.com/golang/mock/gomock" operatorv1 "github.com/openshift/api/operator/v1" + securityv1 "github.com/openshift/api/security/v1" "github.com/sirupsen/logrus" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" ctrlfake "sigs.k8s.io/controller-runtime/pkg/client/fake" + "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" + mock_dynamichelper "github.com/Azure/ARO-RP/pkg/util/mocks/dynamichelper" _ "github.com/Azure/ARO-RP/pkg/util/scheme" "github.com/Azure/ARO-RP/pkg/util/version" testdatabase "github.com/Azure/ARO-RP/test/database" + utilconditions "github.com/Azure/ARO-RP/test/util/conditions" + utilerror "github.com/Azure/ARO-RP/test/util/error" ) func getContainer(d *appsv1.DaemonSet, containerName string) (corev1.Container, error) { @@ -34,11 +42,32 @@ func getContainer(d *appsv1.DaemonSet, containerName string) (corev1.Container, } func TestGenevaLoggingDaemonset(t *testing.T) { + nominalMocks := func(mockDh *mock_dynamichelper.MockInterface) { + mockDh.EXPECT().Ensure( + gomock.Any(), + gomock.AssignableToTypeOf(&securityv1.SecurityContextConstraints{}), + gomock.AssignableToTypeOf(&corev1.Namespace{}), + gomock.AssignableToTypeOf(&corev1.ConfigMap{}), + gomock.AssignableToTypeOf(&corev1.Secret{}), + gomock.AssignableToTypeOf(&corev1.ServiceAccount{}), + gomock.AssignableToTypeOf(&appsv1.DaemonSet{}), + ).Times(1) + } + + defaultConditions := []operatorv1.OperatorCondition{ + utilconditions.ControllerDefaultAvailable(ControllerName), + utilconditions.ControllerDefaultProgressing(ControllerName), + utilconditions.ControllerDefaultDegraded(ControllerName), + } + tests := []struct { name string request ctrl.Request operatorFlags arov1alpha1.OperatorFlags validateDaemonset func(*appsv1.DaemonSet) []error + mocks func(mockDh *mock_dynamichelper.MockInterface) + wantErrMsg string + wantConditions []operatorv1.OperatorCondition }{ { name: "no flags given", @@ -72,6 +101,9 @@ func TestGenevaLoggingDaemonset(t *testing.T) { return }, + mocks: nominalMocks, + wantErrMsg: "", + wantConditions: defaultConditions, }, { name: "fluentbit changed", @@ -106,6 +138,9 @@ func TestGenevaLoggingDaemonset(t *testing.T) { return }, + mocks: nominalMocks, + wantErrMsg: "", + wantConditions: defaultConditions, }, { name: "mdsd changed", @@ -140,13 +175,23 @@ func TestGenevaLoggingDaemonset(t *testing.T) { return }, + mocks: nominalMocks, + wantErrMsg: "", + wantConditions: []operatorv1.OperatorCondition{ + utilconditions.ControllerDefaultAvailable(ControllerName), + utilconditions.ControllerDefaultProgressing(ControllerName), + utilconditions.ControllerDefaultDegraded(ControllerName), + }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + controller := gomock.NewController(t) + defer controller.Finish() + instance := &arov1alpha1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, - Status: arov1alpha1.ClusterStatus{Conditions: []operatorv1.OperatorCondition{}}, + Status: arov1alpha1.ClusterStatus{Conditions: defaultConditions}, Spec: arov1alpha1.ClusterSpec{ ResourceID: testdatabase.GetResourcePath("00000000-0000-0000-0000-000000000000", "testcluster"), OperatorFlags: tt.operatorFlags, @@ -154,11 +199,34 @@ func TestGenevaLoggingDaemonset(t *testing.T) { }, } + resources := []client.Object{ + instance, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: operator.Namespace, + Name: operator.SecretName, + }, + Data: map[string][]byte{ + GenevaCertName: {}, + GenevaKeyName: {}, + }, + }, + &securityv1.SecurityContextConstraints{ + ObjectMeta: metav1.ObjectMeta{ + Name: "privileged", + }, + }, + } + + mockDh := mock_dynamichelper.NewMockInterface(controller) + r := &Reconciler{ AROController: base.AROController{ Log: logrus.NewEntry(logrus.StandardLogger()), - Client: ctrlfake.NewClientBuilder().WithObjects(instance).Build(), + Client: ctrlfake.NewClientBuilder().WithObjects(resources...).Build(), + Name: ControllerName, }, + dh: mockDh, } daemonset, err := r.daemonset(instance) @@ -170,6 +238,13 @@ func TestGenevaLoggingDaemonset(t *testing.T) { for _, err := range errs { t.Error(err) } + + tt.mocks(mockDh) + ctx := context.Background() + _, err = r.Reconcile(ctx, tt.request) + + utilerror.AssertErrorMessage(t, err, tt.wantErrMsg) + utilconditions.AssertControllerConditions(t, ctx, r.Client, tt.wantConditions) }) } }