Add unittest to the object for objectlist finding logic

Author: alvaroaleman

pkg/cache/informer_cache.go

@@ -70,26 +70,48 @@ func (ip *informerCache) Get(ctx context.Context, key client.ObjectKey, out runt
 
 // List implements Reader
 func (ip *informerCache) List(ctx context.Context, out runtime.Object, opts ...client.ListOption) error {
-	gvk, err := apiutil.GVKForObject(out, ip.Scheme)
+
+	gvk, cacheTypeObj, err := ip.objectTypeForListObject(out)
+	if err != nil {
+		return err
+	}
+
+	started, cache, err := ip.InformersMap.Get(*gvk, cacheTypeObj)
 	if err != nil {
 		return err
 	}
 
+	if !started {
+		return &ErrCacheNotStarted{}
+	}
+
+	return cache.Reader.List(ctx, out, opts...)
+}
+
+// objectTypeForListObject tries to find the runtime.Object and associated GVK
+// for a single object corresponding to the passed-in list type. We need them
+// because they are used as cache map key.
+func (ip *informerCache) objectTypeForListObject(list runtime.Object) (*schema.GroupVersionKind, runtime.Object, error) {
+	gvk, err := apiutil.GVKForObject(list, ip.Scheme)
+	if err != nil {
+		return nil, nil, err
+	}
+
 	if !strings.HasSuffix(gvk.Kind, "List") {
-		return fmt.Errorf("non-list type %T (kind %q) passed as output", out, gvk)
+		return nil, nil, fmt.Errorf("non-list type %T (kind %q) passed as output", list, gvk)
 	}
 	// we need the non-list GVK, so chop off the "List" from the end of the kind
 	gvk.Kind = gvk.Kind[:len(gvk.Kind)-4]
-	_, isUnstructured := out.(*unstructured.UnstructuredList)
+	_, isUnstructured := list.(*unstructured.UnstructuredList)
 	var cacheTypeObj runtime.Object
 	if isUnstructured {
 		u := &unstructured.Unstructured{}
 		u.SetGroupVersionKind(gvk)
 		cacheTypeObj = u
 	} else {
-		itemsPtr, err := apimeta.GetItemsPtr(out)
+		itemsPtr, err := apimeta.GetItemsPtr(list)
 		if err != nil {
-			return nil
+			return nil, nil, err
 		}
 		// http://knowyourmeme.com/memes/this-is-fine
 		elemType := reflect.Indirect(reflect.ValueOf(itemsPtr)).Type().Elem()
@@ -101,20 +123,12 @@ func (ip *informerCache) List(ctx context.Context, out runtime.Object, opts ...c
 		var ok bool
 		cacheTypeObj, ok = cacheTypeValue.Interface().(runtime.Object)
 		if !ok {
-			return fmt.Errorf("cannot get cache for %T, its element %T is not a runtime.Object", out, cacheTypeValue.Interface())
+			return nil, nil, fmt.Errorf("cannot get cache for %T, its element %T is not a runtime.Object", list, cacheTypeValue.Interface())
 		}
 	}
 
-	started, cache, err := ip.InformersMap.Get(gvk, cacheTypeObj)
-	if err != nil {
-		return err
-	}
-
-	if !started {
-		return &ErrCacheNotStarted{}
-	}
+	return &gvk, cacheTypeObj, nil
 
-	return cache.Reader.List(ctx, out, opts...)
 }
 
 // GetInformerForKind returns the informer for the GroupVersionKind

pkg/cache/informer_cache_unit_test.go

@@ -0,0 +1,82 @@
+package cache
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/kubernetes/scheme"
+
+	"sigs.k8s.io/controller-runtime/pkg/cache/internal"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllertest"
+	crscheme "sigs.k8s.io/controller-runtime/pkg/scheme"
+)
+
+const (
+	itemPointerSliceTypeGroupName = "jakob.fabian"
+	itemPointerSliceTypeVersion   = "v1"
+)
+
+var _ = Describe("ip.objectTypeForListObject", func() {
+	ip := &informerCache{
+		InformersMap: &internal.InformersMap{Scheme: scheme.Scheme},
+	}
+
+	It("should error on non-list types", func() {
+		_, _, err := ip.objectTypeForListObject(&corev1.Pod{})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(Equal(`non-list type *v1.Pod (kind "/v1, Kind=Pod") passed as output`))
+	})
+
+	It("should find the object type for unstructured lists", func() {
+		unstructuredList := &unstructured.UnstructuredList{}
+		unstructuredList.SetAPIVersion("v1")
+		unstructuredList.SetKind("PodList")
+
+		gvk, obj, err := ip.objectTypeForListObject(unstructuredList)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(gvk.Group).To(Equal(""))
+		Expect(gvk.Version).To(Equal("v1"))
+		Expect(gvk.Kind).To(Equal("Pod"))
+		referenceUnstructured := &unstructured.Unstructured{}
+		referenceUnstructured.SetGroupVersionKind(*gvk)
+		Expect(obj).To(Equal(referenceUnstructured))
+
+	})
+
+	It("should find the object type of a list with a slice of literals items field", func() {
+		gvk, obj, err := ip.objectTypeForListObject(&corev1.PodList{})
+		Expect(err).ToNot(HaveOccurred())
+		Expect(gvk.Group).To(Equal(""))
+		Expect(gvk.Version).To(Equal("v1"))
+		Expect(gvk.Kind).To(Equal("Pod"))
+		var referencePod *corev1.Pod
+		Expect(obj).To(Equal(referencePod))
+
+	})
+
+	It("should find the object type of a list with a slice of pointers items field", func() {
+		By("registering the type", func() {
+			err := (&crscheme.Builder{
+				GroupVersion: schema.GroupVersion{Group: itemPointerSliceTypeGroupName, Version: itemPointerSliceTypeVersion},
+			}).
+				Register(
+					&controllertest.UnconventionalListType{},
+					&controllertest.UnconventionalListTypeList{},
+				).AddToScheme(scheme.Scheme)
+			Expect(err).To(BeNil())
+		})
+
+		By("calling objectTypeForListObject", func() {
+			gvk, obj, err := ip.objectTypeForListObject(&controllertest.UnconventionalListTypeList{})
+			Expect(err).ToNot(HaveOccurred())
+			Expect(gvk.Group).To(Equal(itemPointerSliceTypeGroupName))
+			Expect(gvk.Version).To(Equal(itemPointerSliceTypeVersion))
+			Expect(gvk.Kind).To(Equal("UnconventionalListType"))
+			var referenceObject *controllertest.UnconventionalListType
+			Expect(obj).To(Equal(referenceObject))
+		})
+	})
+})

pkg/controller/controller_integration_test.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllertest"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
@@ -169,7 +170,7 @@ var _ = Describe("controller", func() {
 
 			By("Listing a type with a slice of pointers as items field")
 			err = cm.GetClient().
-				List(context.Background(), &UnconventionalListTypeList{})
+				List(context.Background(), &controllertest.UnconventionalListTypeList{})
 			Expect(err).NotTo(HaveOccurred())
 
 			close(done)

pkg/controller/controller_suite_test.go

@@ -21,13 +21,12 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllertest"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
@@ -51,8 +50,10 @@ var _ = BeforeSuite(func(done Done) {
 	err := (&crscheme.Builder{
 		GroupVersion: schema.GroupVersion{Group: "chaosapps.metamagical.io", Version: "v1"},
 	}).
-		Register(&UnconventionalListType{}, &UnconventionalListTypeList{}).
-		AddToScheme(scheme.Scheme)
+		Register(
+			&controllertest.UnconventionalListType{},
+			&controllertest.UnconventionalListTypeList{},
+		).AddToScheme(scheme.Scheme)
 	Expect(err).To(BeNil())
 
 	testenv = &envtest.Environment{
@@ -77,53 +78,3 @@ var _ = AfterSuite(func() {
 	// Put the DefaultBindAddress back
 	metrics.DefaultBindAddress = ":8080"
 })
-
-var _ runtime.Object = &UnconventionalListType{}
-var _ runtime.Object = &UnconventionalListTypeList{}
-
-// UnconventionalListType is used to test CRDs with List types that
-// have a slice of pointers rather than a slice of literals.
-type UnconventionalListType struct {
-	metav1.TypeMeta   `json:",inline"`
-	metav1.ObjectMeta `json:"metadata,omitempty"`
-	Spec              string `json:"spec,omitempty"`
-}
-
-// DeepCopyObject implements runtime.Object
-// Handwritten for simplicity.
-func (u *UnconventionalListType) DeepCopyObject() runtime.Object {
-	return u.DeepCopy()
-}
-
-func (u *UnconventionalListType) DeepCopy() *UnconventionalListType {
-	return &UnconventionalListType{
-		TypeMeta:   u.TypeMeta,
-		ObjectMeta: *u.ObjectMeta.DeepCopy(),
-		Spec:       u.Spec,
-	}
-}
-
-// UnconventionalListTypeList is used to test CRDs with List types that
-// have a slice of pointers rather than a slice of literals.
-type UnconventionalListTypeList struct {
-	metav1.TypeMeta `json:",inline"`
-	metav1.ListMeta `json:"metadata,omitempty"`
-	Items           []*UnconventionalListType `json:"items"`
-}
-
-// DeepCopyObject implements runtime.Object
-// Handwritten for simplicity.
-func (u *UnconventionalListTypeList) DeepCopyObject() runtime.Object {
-	return u.DeepCopy()
-}
-
-func (u *UnconventionalListTypeList) DeepCopy() *UnconventionalListTypeList {
-	out := &UnconventionalListTypeList{
-		TypeMeta: u.TypeMeta,
-		ListMeta: *u.ListMeta.DeepCopy(),
-	}
-	for _, item := range u.Items {
-		out.Items = append(out.Items, item.DeepCopy())
-	}
-	return out
-}

pkg/controller/controllertest/unconventionallisttypecrd.go

@@ -0,0 +1,56 @@
+package controllertest
+
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+var _ runtime.Object = &UnconventionalListType{}
+var _ runtime.Object = &UnconventionalListTypeList{}
+
+// UnconventionalListType is used to test CRDs with List types that
+// have a slice of pointers rather than a slice of literals.
+type UnconventionalListType struct {
+	metav1.TypeMeta   `json:",inline"`
+	metav1.ObjectMeta `json:"metadata,omitempty"`
+	Spec              string `json:"spec,omitempty"`
+}
+
+// DeepCopyObject implements runtime.Object
+// Handwritten for simplicity.
+func (u *UnconventionalListType) DeepCopyObject() runtime.Object {
+	return u.DeepCopy()
+}
+
+func (u *UnconventionalListType) DeepCopy() *UnconventionalListType {
+	return &UnconventionalListType{
+		TypeMeta:   u.TypeMeta,
+		ObjectMeta: *u.ObjectMeta.DeepCopy(),
+		Spec:       u.Spec,
+	}
+}
+
+// UnconventionalListTypeList is used to test CRDs with List types that
+// have a slice of pointers rather than a slice of literals.
+type UnconventionalListTypeList struct {
+	metav1.TypeMeta `json:",inline"`
+	metav1.ListMeta `json:"metadata,omitempty"`
+	Items           []*UnconventionalListType `json:"items"`
+}
+
+// DeepCopyObject implements runtime.Object
+// Handwritten for simplicity.
+func (u *UnconventionalListTypeList) DeepCopyObject() runtime.Object {
+	return u.DeepCopy()
+}
+
+func (u *UnconventionalListTypeList) DeepCopy() *UnconventionalListTypeList {
+	out := &UnconventionalListTypeList{
+		TypeMeta: u.TypeMeta,
+		ListMeta: *u.ListMeta.DeepCopy(),
+	}
+	for _, item := range u.Items {
+		out.Items = append(out.Items, item.DeepCopy())
+	}
+	return out
+}