ROX-19221: Fix reconciling with label selector for multiple reconcilers (#42)

* Add filter predicate to secret kind watch

* Change fix to filter resource in the reconcile function

* Fix doc string typo

* Update description for setting broken action client

Author: kurlov

pkg/reconciler/reconciler.go

@@ -41,6 +41,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/source"
@@ -271,7 +272,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.
 //
@@ -602,7 +603,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.
@@ -655,6 +656,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		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).

pkg/reconciler/reconciler_test.go

@@ -503,6 +503,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}
 		})
@@ -535,6 +536,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() {
@@ -553,6 +556,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",
 						}),
@@ -568,6 +572,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",
 						}),
@@ -1502,6 +1507,40 @@ var _ = Describe("Reconciler", func() {
 								})
 							})
 						})
+						When("label selector succeeds", func() {
+							It("reconciles only matching label", func() {
+								By("setting an invalid action client getter to assert different reconcile results", func() {
+									r.actionClientGetter = helmclient.ActionClientGetterFunc(func(context.Context, 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": "baz"})
+									Expect(mgr.GetClient().Update(ctx, obj)).To(Succeed())
+								})
+
+								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).ToNot(HaveOccurred())
+								})
+
+								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"))
+								})
+							})
+						})
 					})
 				})
 			})