From de4f99ad990b3da80e69b430fdd2a3b74c27cbeb Mon Sep 17 00:00:00 2001
From: Varsha <varshaprasad96@gmail.com>
Date: Mon, 6 Dec 2021 11:46:54 +0530
Subject: [PATCH] Add option to watch List object types (#138)

This commit modifies `dependentResourceWatcher` to
accept `List` kinds while watching dependent resources.
It also adds test to verify if `setWatchOnResource`
works for lists.

Reference: https://github.com/operator-framework/operator-sdk/pull/4682

Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>
---
 pkg/reconciler/internal/hook/hook.go      | 66 +++++++++++++++++------
 pkg/reconciler/internal/hook/hook_test.go | 38 +++++++++++++
 2 files changed, 88 insertions(+), 16 deletions(-)

diff --git a/pkg/reconciler/internal/hook/hook.go b/pkg/reconciler/internal/hook/hook.go
index 59456545..e44f904c 100644
--- a/pkg/reconciler/internal/hook/hook.go
+++ b/pkg/reconciler/internal/hook/hook.go
@@ -25,6 +25,7 @@ import (
 	"helm.sh/helm/v3/pkg/releaseutil"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
@@ -69,32 +70,65 @@ func (d *dependentResourceWatcher) Exec(owner *unstructured.Unstructured, rel re
 		}
 
 		depGVK := obj.GroupVersionKind()
-		if _, ok := d.watches[depGVK]; ok || depGVK.Empty() {
+		if depGVK.Empty() {
 			continue
 		}
 
-		useOwnerRef, err := controllerutil.SupportsOwnerReference(d.restMapper, owner, &obj)
-		if err != nil {
-			return err
-		}
+		var setWatchOnResource = func(dependent runtime.Object) error {
+			unstructuredObj := dependent.(*unstructured.Unstructured)
+			gvkDependent := unstructuredObj.GroupVersionKind()
+
+			if gvkDependent.Empty() {
+				return nil
+			}
+
+			_, ok := d.watches[gvkDependent]
+			if ok {
+				return nil
+			}
 
-		if useOwnerRef && !manifestutil.HasResourcePolicyKeep(obj.GetAnnotations()) {
-			if err := d.controller.Watch(&source.Kind{Type: &obj}, &handler.EnqueueRequestForOwner{
-				OwnerType:    owner,
-				IsController: true,
-			}, dependentPredicate); err != nil {
+			useOwnerRef, err := controllerutil.SupportsOwnerReference(d.restMapper, owner, unstructuredObj)
+			if err != nil {
 				return err
 			}
+
+			if useOwnerRef && !manifestutil.HasResourcePolicyKeep(unstructuredObj.GetAnnotations()) { // Setup watch using owner references.
+				if err := d.controller.Watch(&source.Kind{Type: unstructuredObj}, &handler.EnqueueRequestForOwner{
+					OwnerType:    owner,
+					IsController: true,
+				}, dependentPredicate); err != nil {
+					return err
+				}
+			} else { // Setup watch using annotations.
+				if err := d.controller.Watch(&source.Kind{Type: unstructuredObj}, &sdkhandler.EnqueueRequestForAnnotation{
+					Type: owner.GetObjectKind().GroupVersionKind().GroupKind(),
+				}, dependentPredicate); err != nil {
+					return err
+				}
+			}
+
+			d.watches[depGVK] = struct{}{}
+			log.V(1).Info("Watching dependent resource", "dependentAPIVersion", depGVK.GroupVersion(), "dependentKind", depGVK.Kind)
+			return nil
+		}
+
+		// List is not actually a resource and therefore cannot have a
+		// watch on it. The watch will be on the kinds listed in the list
+		// and will therefore need to be handled individually.
+		listGVK := schema.GroupVersionKind{Group: "", Version: "v1", Kind: "List"}
+		if depGVK == listGVK {
+			errListItem := obj.EachListItem(func(o runtime.Object) error {
+				return setWatchOnResource(o)
+			})
+			if errListItem != nil {
+				return errListItem
+			}
 		} else {
-			if err := d.controller.Watch(&source.Kind{Type: &obj}, &sdkhandler.EnqueueRequestForAnnotation{
-				Type: owner.GetObjectKind().GroupVersionKind().GroupKind(),
-			}, dependentPredicate); err != nil {
+			err := setWatchOnResource(&obj)
+			if err != nil {
 				return err
 			}
 		}
-
-		d.watches[depGVK] = struct{}{}
-		log.V(1).Info("Watching dependent resource", "dependentAPIVersion", depGVK.GroupVersion(), "dependentKind", depGVK.Kind)
 	}
 	return nil
 }
diff --git a/pkg/reconciler/internal/hook/hook_test.go b/pkg/reconciler/internal/hook/hook_test.go
index 22583bc8..5d683e03 100644
--- a/pkg/reconciler/internal/hook/hook_test.go
+++ b/pkg/reconciler/internal/hook/hook_test.go
@@ -215,6 +215,24 @@ var _ = Describe("Hook", func() {
 					Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))
 					Expect(c.WatchCalls[2].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))
 				})
+				It("should iterate the kind list and be able to set watches on each item", func() {
+					rel = &release.Release{
+						Manifest: strings.Join([]string{replicaSetList}, "---\n"),
+					}
+					drw = internalhook.NewDependentResourceWatcher(c, rm)
+					Expect(drw.Exec(owner, *rel, log)).To(Succeed())
+					Expect(c.WatchCalls).To(HaveLen(2))
+					Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{}))
+					Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{}))
+				})
+				It("should error when unable to list objects", func() {
+					rel = &release.Release{
+						Manifest: strings.Join([]string{errReplicaSetList}, "---\n"),
+					}
+					drw = internalhook.NewDependentResourceWatcher(c, rm)
+					err := drw.Exec(owner, *rel, log)
+					Expect(err).To(HaveOccurred())
+				})
 			})
 		})
 	})
@@ -280,5 +298,25 @@ metadata:
   name: testClusterRoleBinding
   annotations:
     helm.sh/resource-policy: keep
+`
+	replicaSetList = `
+apiVersion: v1
+kind: List
+items:
+  - apiVersion: apps/v1
+    kind: ReplicaSet
+    metadata: 
+      name: testReplicaSet1
+      namespace: ownerNamespace
+  - apiVersion: apps/v1
+    kind: ReplicaSet
+    metadata: 
+      name: testReplicaSet2
+      namespace: ownerNamespace
+`
+	errReplicaSetList = `
+apiVersion: v1
+kind: List
+items:
 `
 )