Skip to content

Commit

Permalink
harden handler validation logic, add tests for it to ensure it works …
Browse files Browse the repository at this point in the history 
…appropriately

Signed-off-by: everettraven <everettraven@gmail.com>
  • Loading branch information
@everettraven
everettraven committed May 20, 2024
1 parent daf9fb4 commit 8752ba7
Showing 1 changed file with 52 additions and 19 deletions.
71 changes: 52 additions & 19 deletions pkg/reconciler/internal/hook/hook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package hook_test

import (
"context"
"errors"
"fmt"
"reflect"
"strings"
Expand Down Expand Up @@ -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() {
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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())
})
})

Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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 {
Expand Down

0 comments on commit 8752ba7

Please sign in to comment.