harden handler validation logic, add tests for it to ensure it works appropriately

Signed-off-by: everettraven <[email protected]>

Author: everettraven

pkg/reconciler/internal/hook/hook_test.go

@@ -18,6 +18,7 @@ package hook_test
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"reflect"
 	"strings"
@@ -130,8 +131,8 @@ var _ = Describe("Hook", func() {
 				drw = internalhook.NewDependentResourceWatcher(c, rm, cache, sch)
 				Expect(drw.Exec(owner, *rel, log)).To(Succeed())
 				Expect(c.WatchCalls).To(HaveLen(2))
-				Expect(validateSourceHandlerType(c.WatchCalls[0].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(BeNil())
-				Expect(validateSourceHandlerType(c.WatchCalls[1].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(BeNil())
+				Expect(validateSourceHandlerType(c.WatchCalls[0].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(Succeed())
+				Expect(validateSourceHandlerType(c.WatchCalls[1].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(Succeed())
 			})
 
 			Context("when the owner is cluster-scoped", func() {
@@ -153,8 +154,8 @@ var _ = Describe("Hook", func() {
 					drw = internalhook.NewDependentResourceWatcher(c, rm, cache, sch)
 					Expect(drw.Exec(owner, *rel, log)).To(Succeed())
 					Expect(c.WatchCalls).To(HaveLen(2))
-					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(BeNil())
-					Expect(validateSourceHandlerType(c.WatchCalls[1].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(BeNil())
+					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(Succeed())
+					Expect(validateSourceHandlerType(c.WatchCalls[1].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(Succeed())
 				})
 				It("should watch cluster-scoped resources with ownerRef handler", func() {
 					rel = &release.Release{
@@ -163,8 +164,8 @@ var _ = Describe("Hook", func() {
 					drw = internalhook.NewDependentResourceWatcher(c, rm, cache, sch)
 					Expect(drw.Exec(owner, *rel, log)).To(Succeed())
 					Expect(c.WatchCalls).To(HaveLen(2))
-					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(BeNil())
-					Expect(validateSourceHandlerType(c.WatchCalls[1].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(BeNil())
+					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(Succeed())
+					Expect(validateSourceHandlerType(c.WatchCalls[1].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(Succeed())
 				})
 				It("should watch resource policy keep resources with annotation handler", func() {
 					rel = &release.Release{
@@ -173,10 +174,10 @@ var _ = Describe("Hook", func() {
 					drw = internalhook.NewDependentResourceWatcher(c, rm, cache, sch)
 					Expect(drw.Exec(owner, *rel, log)).To(Succeed())
 					Expect(c.WatchCalls).To(HaveLen(4))
-					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(BeNil())
-					Expect(validateSourceHandlerType(c.WatchCalls[1].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(BeNil())
-					Expect(validateSourceHandlerType(c.WatchCalls[2].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(BeNil())
-					Expect(validateSourceHandlerType(c.WatchCalls[3].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(BeNil())
+					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(Succeed())
+					Expect(validateSourceHandlerType(c.WatchCalls[1].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(Succeed())
+					Expect(validateSourceHandlerType(c.WatchCalls[2].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(Succeed())
+					Expect(validateSourceHandlerType(c.WatchCalls[3].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(Succeed())
 				})
 			})
 
@@ -200,7 +201,7 @@ var _ = Describe("Hook", func() {
 					drw = internalhook.NewDependentResourceWatcher(c, rm, cache, sch)
 					Expect(drw.Exec(owner, *rel, log)).To(Succeed())
 					Expect(c.WatchCalls).To(HaveLen(1))
-					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(BeNil())
+					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(Succeed())
 				})
 				It("should watch cluster-scoped resources with annotation handler", func() {
 					rel = &release.Release{
@@ -209,7 +210,7 @@ var _ = Describe("Hook", func() {
 					drw = internalhook.NewDependentResourceWatcher(c, rm, cache, sch)
 					Expect(drw.Exec(owner, *rel, log)).To(Succeed())
 					Expect(c.WatchCalls).To(HaveLen(1))
-					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(BeNil())
+					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(Succeed())
 				})
 				It("should watch namespace-scoped resources in a different namespace with annotation handler", func() {
 					rel = &release.Release{
@@ -218,7 +219,7 @@ var _ = Describe("Hook", func() {
 					drw = internalhook.NewDependentResourceWatcher(c, rm, cache, sch)
 					Expect(drw.Exec(owner, *rel, log)).To(Succeed())
 					Expect(c.WatchCalls).To(HaveLen(1))
-					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(BeNil())
+					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(Succeed())
 				})
 				It("should watch resource policy keep resources with annotation handler", func() {
 					rel = &release.Release{
@@ -227,9 +228,9 @@ var _ = Describe("Hook", func() {
 					drw = internalhook.NewDependentResourceWatcher(c, rm, cache, sch)
 					Expect(drw.Exec(owner, *rel, log)).To(Succeed())
 					Expect(c.WatchCalls).To(HaveLen(3))
-					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(BeNil())
-					Expect(validateSourceHandlerType(c.WatchCalls[1].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(BeNil())
-					Expect(validateSourceHandlerType(c.WatchCalls[2].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(BeNil())
+					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(Succeed())
+					Expect(validateSourceHandlerType(c.WatchCalls[1].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(Succeed())
+					Expect(validateSourceHandlerType(c.WatchCalls[2].Source, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(Succeed())
 				})
 				It("should iterate the kind list and be able to set watches on each item", func() {
 					rel = &release.Release{
@@ -238,8 +239,8 @@ var _ = Describe("Hook", func() {
 					drw = internalhook.NewDependentResourceWatcher(c, rm, cache, sch)
 					Expect(drw.Exec(owner, *rel, log)).To(Succeed())
 					Expect(c.WatchCalls).To(HaveLen(2))
-					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(BeNil())
-					Expect(validateSourceHandlerType(c.WatchCalls[1].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(BeNil())
+					Expect(validateSourceHandlerType(c.WatchCalls[0].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(Succeed())
+					Expect(validateSourceHandlerType(c.WatchCalls[1].Source, handler.TypedEnqueueRequestForOwner[*unstructured.Unstructured](sch, rm, owner, handler.OnlyControllerOwner()))).To(Succeed())
 				})
 				It("should error when unable to list objects", func() {
 					rel = &release.Release{
@@ -254,17 +255,49 @@ var _ = Describe("Hook", func() {
 	})
 })
 
+var _ = Describe("validateSourceHandlerType", func() {
+	It("should return an error when source.Source is nil", func() {
+		Expect(validateSourceHandlerType(nil, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(HaveOccurred())
+	})
+	It("should return an error when source.Kind.Handler is nil", func() {
+		Expect(validateSourceHandlerType(source.Kind(nil, &unstructured.Unstructured{}, nil, nil), &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(HaveOccurred())
+	})
+	It("should return an error when expected is nil", func() {
+		Expect(validateSourceHandlerType(source.Kind(nil, &unstructured.Unstructured{}, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{}, nil), nil)).To(HaveOccurred())
+	})
+	It("should return an error when source.Kind.Handler does not match expected type", func() {
+		Expect(validateSourceHandlerType(source.Kind(nil, &unstructured.Unstructured{}, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{}, nil), "string")).To(HaveOccurred())
+	})
+	It("should not return an error when source.Kind.Handler matches expectedType", func() {
+		Expect(validateSourceHandlerType(source.Kind(nil, &unstructured.Unstructured{}, &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{}, nil), &sdkhandler.EnqueueRequestForAnnotation[*unstructured.Unstructured]{})).To(Succeed())
+	})
+})
+
 // validateSourceHandlerType takes in a source.Source and uses reflection to determine
 // if the handler used by the source matches the expected type.
 // It is assumed that the source.Source was created via the source.Kind() function.
 func validateSourceHandlerType(s source.Source, expected interface{}) error {
+	if s == nil {
+		return errors.New("nil source.Source provided")
+	}
 	sourceVal := reflect.Indirect(reflect.ValueOf(s))
+	if !sourceVal.IsValid() {
+		return errors.New("provided source.Source value is invalid")
+	}
 	handlerFieldVal := sourceVal.FieldByName("Handler")
-
+	if !handlerFieldVal.IsValid() {
+		return errors.New("provided source.Source's Handler field is invalid")
+	}
 	handlerField := reflect.Indirect(handlerFieldVal.Elem())
+	if !handlerField.IsValid() {
+		return errors.New("provided source.Source's Handler field value is invalid")
+	}
 	handlerType := handlerField.Type()
 
 	expectedValue := reflect.Indirect(reflect.ValueOf(expected))
+	if !expectedValue.IsValid() {
+		return errors.New("provided expected value is invalid")
+	}
 
 	expectedType := expectedValue.Type()
 	if handlerType != expectedType {