Merge pull request #796 from alvaroaleman/construct-pointer-for-non-pointers-only-with-test

🐛 Fix informer cache creating pointers to pointers

Author: k8s-ci-robot

pkg/cache/informer_cache.go

@@ -70,47 +70,65 @@ 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()
-		cacheTypeValue := reflect.Zero(reflect.PtrTo(elemType))
+		if elemType.Kind() != reflect.Ptr {
+			elemType = reflect.PtrTo(elemType)
+		}
+
+		cacheTypeValue := reflect.Zero(elemType)
 		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"
@@ -167,6 +168,11 @@ var _ = Describe("controller", func() {
 			Expect(err).NotTo(HaveOccurred())
 			Expect(<-reconciled).To(Equal(expectedReconcileRequest))
 
+			By("Listing a type with a slice of pointers as items field")
+			err = cm.GetClient().
+				List(context.Background(), &controllertest.UnconventionalListTypeList{})
+			Expect(err).NotTo(HaveOccurred())
+
 			close(done)
 		}, 5)
 	})

pkg/controller/controller_suite_test.go

@@ -21,13 +21,18 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"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"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	"sigs.k8s.io/controller-runtime/pkg/metrics"
+	crscheme "sigs.k8s.io/controller-runtime/pkg/scheme"
 )
 
 func TestSource(t *testing.T) {
@@ -42,9 +47,19 @@ var clientset *kubernetes.Clientset
 var _ = BeforeSuite(func(done Done) {
 	logf.SetLogger(zap.LoggerTo(GinkgoWriter, true))
 
-	testenv = &envtest.Environment{}
+	err := (&crscheme.Builder{
+		GroupVersion: schema.GroupVersion{Group: "chaosapps.metamagical.io", Version: "v1"},
+	}).
+		Register(
+			&controllertest.UnconventionalListType{},
+			&controllertest.UnconventionalListTypeList{},
+		).AddToScheme(scheme.Scheme)
+	Expect(err).To(BeNil())
+
+	testenv = &envtest.Environment{
+		CRDDirectoryPaths: []string{"testdata/crds"},
+	}
 
-	var err error
 	cfg, err = testenv.Start()
 	Expect(err).NotTo(HaveOccurred())
 

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
+}

pkg/controller/testdata/crds/unconventionallisttype.yaml

@@ -0,0 +1,11 @@
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  name: unconventionallisttypes.chaosapps.metamagical.io
+spec:
+  group: chaosapps.metamagical.io
+  names:
+    kind: UnconventionalListType
+    plural: unconventionallisttypes
+  scope: Namespaced
+  version: "v1"