From 7126477e5b2367fcc5303724989f15b172430716 Mon Sep 17 00:00:00 2001 From: Aleksandr Kurlov Date: Tue, 22 Aug 2023 11:20:01 +0200 Subject: [PATCH 1/4] Add filter predicate to secret kind watch --- pkg/reconciler/reconciler.go | 1 + pkg/reconciler/reconciler_test.go | 80 +++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 3a30b70..cafcbec 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -1161,6 +1161,7 @@ func (r *Reconciler) setupWatches(mgr ctrl.Manager, c controller.Controller) err if err := c.Watch( source.Kind(mgr.GetCache(), secret), handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), obj, handler.OnlyControllerOwner()), + preds..., ); err != nil { return err } diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index ce574e8..4b25ce9 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -517,6 +517,8 @@ var _ = Describe("Reconciler", func() { cancel() }) + selector := metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}} + // After migration to Ginkgo v2 this can be rewritten using e.g. DescribeTable. parameterizedReconcilerTests := func(opts reconcilerTestSuiteOpts) { BeforeEach(func() { @@ -535,6 +537,7 @@ var _ = Describe("Reconciler", func() { WithUpgradeAnnotations(annotation.UpgradeDescription{}), WithUninstallAnnotations(annotation.UninstallDescription{}), WithPauseReconcileAnnotation("my.domain/pause-reconcile"), + WithSelector(selector), WithOverrideValues(map[string]string{ "image.repository": "custom-nginx", }), @@ -550,6 +553,7 @@ var _ = Describe("Reconciler", func() { WithUpgradeAnnotations(annotation.UpgradeDescription{}), WithUninstallAnnotations(annotation.UninstallDescription{}), WithPauseReconcileAnnotation("my.domain/pause-reconcile"), + WithSelector(selector), WithOverrideValues(map[string]string{ "image.repository": "custom-nginx", }), @@ -1392,6 +1396,21 @@ var _ = Describe("Reconciler", func() { }) }) }) + When("label selector is present", func() { + It("reconciles only with label", func() { + By("adding label to the CR", func() { + Expect(mgr.GetClient().Get(ctx, objKey, obj)).To(Succeed()) + obj.SetLabels(map[string]string{"foo": "bar"}) + Expect(mgr.GetClient().Update(ctx, obj)).To(Succeed()) + }) + + By("successfully reconciling a request", func() { + res, err := r.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(err).To(BeNil()) + }) + }) + }) }) }) }) @@ -1444,6 +1463,67 @@ var _ = Describe("Reconciler", func() { Expect(controllerSetupCalled).To(BeTrue()) }) }) + + var _ = Describe("Test label selector for two reconcilers", func() { + var ( + mgr manager.Manager + firstReconciler *Reconciler + secondReconciler *Reconciler + err error + ) + ctx := context.Background() + obj := testutil.BuildTestCR(gvk) + objKey := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} + req := reconcile.Request{NamespacedName: objKey} + + It("Building two reconcilers", func() { + By("preparing first reconciler", func() { + mgr = getManagerOrFail() + firstReconciler, err = New( + WithGroupVersionKind(gvk), + WithChart(chrt), + WithInstallAnnotations(annotation.InstallDescription{}), + WithUpgradeAnnotations(annotation.UpgradeDescription{}), + WithUninstallAnnotations(annotation.UninstallDescription{}), + WithOverrideValues(map[string]string{ + "image.repository": "custom-nginx", + }), + WithSelector(metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}), + ) + Expect(err).To(BeNil()) + Expect(firstReconciler.SetupWithManager(mgr)).To(Succeed()) + }) + + By("preparing first reconciler", func() { + secondReconciler, err = New( + WithGroupVersionKind(gvk), + WithChart(chrt), + WithInstallAnnotations(annotation.InstallDescription{}), + WithUpgradeAnnotations(annotation.UpgradeDescription{}), + WithUninstallAnnotations(annotation.UninstallDescription{}), + WithOverrideValues(map[string]string{ + "image.repository": "custom-nginx", + }), + WithSelector(metav1.LabelSelector{MatchLabels: map[string]string{"foo": "baz"}}), + ) + Expect(err).To(BeNil()) + Expect(secondReconciler.SetupWithManager(mgr)).To(Succeed()) + }) + }) + + It("Successfully reconcile", func() { + obj.SetLabels(map[string]string{"foo": "bar"}) + Expect(mgr.GetClient().Create(ctx, obj)).To(Succeed()) + + res, err := firstReconciler.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(err).To(BeNil()) + + res, err = secondReconciler.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(err).To(BeNil()) + }) + }) }) func getManagerOrFail() manager.Manager { From 3d7adc3c8e44ee9c430cd4b23fd047b5850b535e Mon Sep 17 00:00:00 2001 From: Aleksandr Kurlov Date: Thu, 24 Aug 2023 02:12:12 +0200 Subject: [PATCH 2/4] Change fix to filter resource in the reconcile function --- pkg/reconciler/reconciler.go | 7 ++- pkg/reconciler/reconciler_test.go | 91 +++++++++---------------------- 2 files changed, 31 insertions(+), 67 deletions(-) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index cafcbec..fbe31e0 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" errs "github.com/pkg/errors" + "sigs.k8s.io/controller-runtime/pkg/event" "strings" "sync" "time" @@ -650,6 +651,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. return ctrl.Result{}, err } + if r.selectorPredicate != nil && !r.selectorPredicate.Generic(event.GenericEvent{Object: obj}) { + log.V(1).Info("Label selector does not match, skipping reconcile") + return ctrl.Result{}, nil + } + // The finalizer must be present on the CR before we can do anything. Otherwise, if the reconciliation fails, // there might be resources created by the chart that will not be garbage-collected // (cluster-scoped resources or resources in other namespaces, which are not bound by an owner reference). @@ -1161,7 +1167,6 @@ func (r *Reconciler) setupWatches(mgr ctrl.Manager, c controller.Controller) err if err := c.Watch( source.Kind(mgr.GetCache(), secret), handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), obj, handler.OnlyControllerOwner()), - preds..., ); err != nil { return err } diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 4b25ce9..6a458cc 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -484,6 +484,7 @@ var _ = Describe("Reconciler", func() { Expect(mgr.GetCache().WaitForCacheSync(ctx)).To(BeTrue()) obj = testutil.BuildTestCR(gvk) + obj.SetLabels(map[string]string{"foo": "bar"}) objKey = types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} req = reconcile.Request{NamespacedName: objKey} @@ -1396,19 +1397,38 @@ var _ = Describe("Reconciler", func() { }) }) }) - When("label selector is present", func() { - It("reconciles only with label", func() { - By("adding label to the CR", func() { + When("label selector succeeds", func() { + It("reconciles only matching label", func() { + By("setting a broken action client getter for the reconciler", func() { + r.actionClientGetter = helmclient.ActionClientGetterFunc(func(client.Object) (helmclient.ActionInterface, error) { + fakeClient := helmfake.NewActionClient() + return &fakeClient, nil + }) + }) + + By("setting not matching label to the CR", func() { Expect(mgr.GetClient().Get(ctx, objKey, obj)).To(Succeed()) - obj.SetLabels(map[string]string{"foo": "bar"}) + obj.SetLabels(map[string]string{"foo": "baz"}) Expect(mgr.GetClient().Update(ctx, obj)).To(Succeed()) }) - By("successfully reconciling a request", func() { + By("reconciling is skipped, action client was not called and no error returned", func() { res, err := r.Reconcile(ctx, req) Expect(res).To(Equal(reconcile.Result{})) Expect(err).To(BeNil()) }) + + By("setting matching label to the CR", func() { + Expect(mgr.GetClient().Get(ctx, objKey, obj)).To(Succeed()) + obj.SetLabels(map[string]string{"foo": "bar"}) + Expect(mgr.GetClient().Update(ctx, obj)).To(Succeed()) + }) + + By("reconciling is not skipped and error returned because of broken action client", func() { + res, err := r.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(err).To(MatchError("get not implemented")) + }) }) }) }) @@ -1463,67 +1483,6 @@ var _ = Describe("Reconciler", func() { Expect(controllerSetupCalled).To(BeTrue()) }) }) - - var _ = Describe("Test label selector for two reconcilers", func() { - var ( - mgr manager.Manager - firstReconciler *Reconciler - secondReconciler *Reconciler - err error - ) - ctx := context.Background() - obj := testutil.BuildTestCR(gvk) - objKey := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} - req := reconcile.Request{NamespacedName: objKey} - - It("Building two reconcilers", func() { - By("preparing first reconciler", func() { - mgr = getManagerOrFail() - firstReconciler, err = New( - WithGroupVersionKind(gvk), - WithChart(chrt), - WithInstallAnnotations(annotation.InstallDescription{}), - WithUpgradeAnnotations(annotation.UpgradeDescription{}), - WithUninstallAnnotations(annotation.UninstallDescription{}), - WithOverrideValues(map[string]string{ - "image.repository": "custom-nginx", - }), - WithSelector(metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}), - ) - Expect(err).To(BeNil()) - Expect(firstReconciler.SetupWithManager(mgr)).To(Succeed()) - }) - - By("preparing first reconciler", func() { - secondReconciler, err = New( - WithGroupVersionKind(gvk), - WithChart(chrt), - WithInstallAnnotations(annotation.InstallDescription{}), - WithUpgradeAnnotations(annotation.UpgradeDescription{}), - WithUninstallAnnotations(annotation.UninstallDescription{}), - WithOverrideValues(map[string]string{ - "image.repository": "custom-nginx", - }), - WithSelector(metav1.LabelSelector{MatchLabels: map[string]string{"foo": "baz"}}), - ) - Expect(err).To(BeNil()) - Expect(secondReconciler.SetupWithManager(mgr)).To(Succeed()) - }) - }) - - It("Successfully reconcile", func() { - obj.SetLabels(map[string]string{"foo": "bar"}) - Expect(mgr.GetClient().Create(ctx, obj)).To(Succeed()) - - res, err := firstReconciler.Reconcile(ctx, req) - Expect(res).To(Equal(reconcile.Result{})) - Expect(err).To(BeNil()) - - res, err = secondReconciler.Reconcile(ctx, req) - Expect(res).To(Equal(reconcile.Result{})) - Expect(err).To(BeNil()) - }) - }) }) func getManagerOrFail() manager.Manager { From 373457119736ea3ffaa29acdb828e4f464d3768c Mon Sep 17 00:00:00 2001 From: Aleksandr Kurlov Date: Thu, 24 Aug 2023 02:14:01 +0200 Subject: [PATCH 3/4] Fix doc string typo --- pkg/reconciler/reconciler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index fbe31e0..7e6efff 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -277,7 +277,7 @@ func WithOverrideValues(overrides map[string]string) Option { } } -// WithDependentWatchesEnabled is an Option that configures whether the +// SkipDependentWatches is an Option that configures whether the // Reconciler will register watches for dependent objects in releases and // trigger reconciliations when they change. // @@ -598,7 +598,7 @@ func WithControllerSetupFunc(f ControllerSetupFunc) Option { } // ControllerSetup allows restricted access to the Controller using the WithControllerSetupFunc option. -// Currently the only supposed configuration is adding additional watchers do the controller. +// Currently, the only supposed configuration is adding additional watchers do the controller. type ControllerSetup interface { // Watch takes events provided by a Source and uses the EventHandler to // enqueue reconcile.Requests in response to the events. From 24b3290d50887ef0d36ea89ac87db9e875ae0527 Mon Sep 17 00:00:00 2001 From: Aleksandr Kurlov Date: Fri, 25 Aug 2023 16:31:51 +0200 Subject: [PATCH 4/4] Update description for setting broken action client --- pkg/reconciler/reconciler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 6a458cc..5f22f64 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -1399,7 +1399,7 @@ var _ = Describe("Reconciler", func() { }) When("label selector succeeds", func() { It("reconciles only matching label", func() { - By("setting a broken action client getter for the reconciler", func() { + By("setting an invalid action client getter to assert different reconcile results", func() { r.actionClientGetter = helmclient.ActionClientGetterFunc(func(client.Object) (helmclient.ActionInterface, error) { fakeClient := helmfake.NewActionClient() return &fakeClient, nil