🐛 Metadata objects should always preserve GroupVersionKind

Signed-off-by: Vince Prignano <vincepri@vmware.com>

Author: vincepri

pkg/builder/controller_test.go

@@ -408,8 +408,17 @@ var _ = Describe("application", func() {
 				Owns(&appsv1.ReplicaSet{}, OnlyMetadata).
 				Watches(&source.Kind{Type: &appsv1.StatefulSet{}},
 					handler.EnqueueRequestsFromMapFunc(func(o client.Object) []reconcile.Request {
+						defer GinkgoRecover()
+
 						ometa := o.(*metav1.PartialObjectMetadata)
 						statefulSetMaps <- ometa
+
+						// Validate that the GVK is not empty when dealing with PartialObjectMetadata objects.
+						Expect(o.GetObjectKind().GroupVersionKind()).To(Equal(schema.GroupVersionKind{
+							Group:   "apps",
+							Version: "v1",
+							Kind:    "StatefulSet",
+						}))
 						return nil
 					}),
 					OnlyMetadata)

pkg/cache/cache_test.go

@@ -1018,11 +1018,6 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
 					defer deletePod(pod)
 					// re-copy the result in so that we can match on it properly
 					pod.ObjectMeta.DeepCopyInto(&podMeta.ObjectMeta)
-					// NB(directxman12): proto doesn't care typemeta, and
-					// partialobjectmetadata is proto, so no typemeta
-					// TODO(directxman12): we should paper over this in controller-runtime
-					podMeta.APIVersion = ""
-					podMeta.Kind = ""
 
 					By("verifying the object's metadata is received on the channel")
 					Eventually(out).Should(Receive(Equal(podMeta)))
@@ -1056,20 +1051,31 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
 
 					By("listing Pods with restartPolicyOnFailure")
 					listObj := &kmetav1.PartialObjectMetadataList{}
-					listObj.SetGroupVersionKind(schema.GroupVersionKind{
+					gvk := schema.GroupVersionKind{
 						Group:   "",
 						Version: "v1",
 						Kind:    "PodList",
-					})
+					}
+					listObj.SetGroupVersionKind(gvk)
 					err = informer.List(context.Background(), listObj,
 						client.MatchingFields{"metadata.labels.test-label": "test-pod-3"})
 					Expect(err).To(Succeed())
 
+					By("verifying that the GVK has been preserved for the list object")
+					Expect(listObj.GroupVersionKind()).To(Equal(gvk))
+
 					By("verifying that the returned pods have correct restart policy")
 					Expect(listObj.Items).NotTo(BeEmpty())
 					Expect(listObj.Items).Should(HaveLen(1))
 					actual := listObj.Items[0]
 					Expect(actual.GetName()).To(Equal("test-pod-3"))
+
+					By("verifying that the GVK has been preserved for the item in the list")
+					Expect(actual.GroupVersionKind()).To(Equal(schema.GroupVersionKind{
+						Group:   "",
+						Version: "v1",
+						Kind:    "Pod",
+					}))
 				}, 3)
 
 				It("should allow for get informer to be cancelled", func() {

pkg/cache/internal/informers_map.go

@@ -34,7 +34,6 @@ import (
 	"k8s.io/client-go/metadata"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/cache"
-
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 )
 
@@ -216,6 +215,13 @@ func (ip *specificInformersMap) addInformerToMap(gvk schema.GroupVersionKind, ob
 	if err != nil {
 		return nil, false, err
 	}
+
+	switch obj.(type) {
+	case *metav1.PartialObjectMetadata, *metav1.PartialObjectMetadataList:
+		ni = metadataSharedIndexInformerPreserveGVK(gvk, ni)
+	default:
+	}
+
 	i := &MapEntry{
 		Informer: ni,
 		Reader:   CacheReader{indexer: ni.GetIndexer(), groupVersionKind: gvk, scopeName: rm.Scope.Name()},
@@ -278,7 +284,12 @@ func createUnstructuredListWatch(gvk schema.GroupVersionKind, ip *specificInform
 	if err != nil {
 		return nil, err
 	}
-	dynamicClient, err := dynamic.NewForConfig(ip.config)
+
+	// If the rest configuration has a negotiated serializer passed in,
+	// we should remove it and use the one that the dynamic client sets for us.
+	cfg := rest.CopyConfig(ip.config)
+	cfg.NegotiatedSerializer = nil
+	dynamicClient, err := dynamic.NewForConfig(cfg)
 	if err != nil {
 		return nil, err
 	}
@@ -314,8 +325,13 @@ func createMetadataListWatch(gvk schema.GroupVersionKind, ip *specificInformersM
 		return nil, err
 	}
 
+	// Always clear the negotiated serializer and use the one
+	// set from the metadata client.
+	cfg := rest.CopyConfig(ip.config)
+	cfg.NegotiatedSerializer = nil
+
 	// grab the metadata client
-	client, err := metadata.NewForConfig(ip.config)
+	client, err := metadata.NewForConfig(cfg)
 	if err != nil {
 		return nil, err
 	}

pkg/cache/internal/metadata_infomer_wrapper.go

@@ -0,0 +1,71 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package internal
+
+import (
+	"time"
+
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/tools/cache"
+)
+
+func metadataSharedIndexInformerPreserveGVK(gvk schema.GroupVersionKind, si cache.SharedIndexInformer) cache.SharedIndexInformer {
+	return &sharedInformerWrapper{
+		gvk:                 gvk,
+		SharedIndexInformer: si,
+	}
+}
+
+type sharedInformerWrapper struct {
+	gvk schema.GroupVersionKind
+	cache.SharedIndexInformer
+}
+
+func (s *sharedInformerWrapper) AddEventHandler(handler cache.ResourceEventHandler) {
+	s.SharedIndexInformer.AddEventHandler(&handlerPreserveGVK{s.gvk, handler})
+}
+
+func (s *sharedInformerWrapper) AddEventHandlerWithResyncPeriod(handler cache.ResourceEventHandler, resyncPeriod time.Duration) {
+	s.SharedIndexInformer.AddEventHandlerWithResyncPeriod(&handlerPreserveGVK{s.gvk, handler}, resyncPeriod)
+}
+
+type handlerPreserveGVK struct {
+	gvk schema.GroupVersionKind
+	cache.ResourceEventHandler
+}
+
+func (h *handlerPreserveGVK) resetGroupVersionKind(obj interface{}) {
+	if v, ok := obj.(schema.ObjectKind); ok {
+		v.SetGroupVersionKind(h.gvk)
+	}
+}
+
+func (h *handlerPreserveGVK) OnAdd(obj interface{}) {
+	h.resetGroupVersionKind(obj)
+	h.ResourceEventHandler.OnAdd(obj)
+}
+
+func (h *handlerPreserveGVK) OnUpdate(oldObj, newObj interface{}) {
+	h.resetGroupVersionKind(oldObj)
+	h.resetGroupVersionKind(newObj)
+	h.ResourceEventHandler.OnUpdate(oldObj, newObj)
+}
+
+func (h *handlerPreserveGVK) OnDelete(obj interface{}) {
+	h.resetGroupVersionKind(obj)
+	h.ResourceEventHandler.OnDelete(obj)
+}

pkg/client/apiutil/apimachinery.go

@@ -118,15 +118,24 @@ func GVKForObject(obj runtime.Object, scheme *runtime.Scheme) (schema.GroupVersi
 // with the given GroupVersionKind. The REST client will be configured to use the negotiated serializer from
 // baseConfig, if set, otherwise a default serializer will be set.
 func RESTClientForGVK(gvk schema.GroupVersionKind, isUnstructured bool, baseConfig *rest.Config, codecs serializer.CodecFactory) (rest.Interface, error) {
-	cfg := createRestConfig(gvk, isUnstructured, baseConfig)
-	if cfg.NegotiatedSerializer == nil {
-		cfg.NegotiatedSerializer = serializer.WithoutConversionCodecFactory{CodecFactory: codecs}
-	}
-	return rest.RESTClientFor(cfg)
+	return rest.RESTClientFor(createRestConfig(gvk, isUnstructured, baseConfig, codecs))
+}
+
+// serializerWithDecodedGVK is a CodecFactory that overrides the DecoderToVersion of a WithoutConversionCodecFactory
+// in order to avoid clearing the GVK from the decoded object.
+//
+// See https://github.com/kubernetes/kubernetes/issues/80609.
+type serializerWithDecodedGVK struct {
+	serializer.WithoutConversionCodecFactory
+}
+
+// DecoderToVersion returns an decoder that does not do conversion.
+func (f serializerWithDecodedGVK) DecoderToVersion(serializer runtime.Decoder, _ runtime.GroupVersioner) runtime.Decoder {
+	return serializer
 }
 
 //createRestConfig copies the base config and updates needed fields for a new rest config
-func createRestConfig(gvk schema.GroupVersionKind, isUnstructured bool, baseConfig *rest.Config) *rest.Config {
+func createRestConfig(gvk schema.GroupVersionKind, isUnstructured bool, baseConfig *rest.Config, codecs serializer.CodecFactory) *rest.Config {
 	gv := gvk.GroupVersion()
 
 	cfg := rest.CopyConfig(baseConfig)
@@ -147,5 +156,16 @@ func createRestConfig(gvk schema.GroupVersionKind, isUnstructured bool, baseConf
 		}
 		protobufSchemeLock.RUnlock()
 	}
+
+	if cfg.NegotiatedSerializer == nil {
+		if isUnstructured {
+			// If the object is unstructured, we need to preserve the GVK information.
+			// Use our own custom serializer.
+			cfg.NegotiatedSerializer = serializerWithDecodedGVK{serializer.WithoutConversionCodecFactory{CodecFactory: codecs}}
+		} else {
+			cfg.NegotiatedSerializer = serializer.WithoutConversionCodecFactory{CodecFactory: codecs}
+		}
+	}
+
 	return cfg
 }

pkg/client/client.go

@@ -19,6 +19,7 @@ package client
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -159,7 +160,6 @@ type client struct {
 }
 
 // resetGroupVersionKind is a helper function to restore and preserve GroupVersionKind on an object.
-// TODO(vincepri): Remove this function and its calls once controller-runtime dependencies are upgraded to 1.16?
 func (c *client) resetGroupVersionKind(obj runtime.Object, gvk schema.GroupVersionKind) {
 	if gvk != schema.EmptyObjectKind.GroupVersionKind() {
 		if v, ok := obj.(schema.ObjectKind); ok {
@@ -246,6 +246,8 @@ func (c *client) Get(ctx context.Context, key ObjectKey, obj Object) error {
 	case *unstructured.Unstructured:
 		return c.unstructuredClient.Get(ctx, key, obj)
 	case *metav1.PartialObjectMetadata:
+		// Metadata only object should always preserve the GVK coming in from the caller.
+		defer c.resetGroupVersionKind(obj, obj.GetObjectKind().GroupVersionKind())
 		return c.metadataClient.Get(ctx, key, obj)
 	default:
 		return c.typedClient.Get(ctx, key, obj)
@@ -254,11 +256,33 @@ func (c *client) Get(ctx context.Context, key ObjectKey, obj Object) error {
 
 // List implements client.Client
 func (c *client) List(ctx context.Context, obj ObjectList, opts ...ListOption) error {
-	switch obj.(type) {
+	switch x := obj.(type) {
 	case *unstructured.UnstructuredList:
 		return c.unstructuredClient.List(ctx, obj, opts...)
 	case *metav1.PartialObjectMetadataList:
-		return c.metadataClient.List(ctx, obj, opts...)
+		// Metadata only object should always preserve the GVK.
+		gvk := obj.GetObjectKind().GroupVersionKind()
+		defer c.resetGroupVersionKind(obj, gvk)
+
+		// Call the list client.
+		if err := c.metadataClient.List(ctx, obj, opts...); err != nil {
+			return err
+		}
+
+		// Restore the GVK for each item in the list.
+		itemGVK := schema.GroupVersionKind{
+			Group:   gvk.Group,
+			Version: gvk.Version,
+			// TODO: this is producing unsafe guesses that don't actually work,
+			// but it matches ~99% of the cases out there.
+			Kind: strings.TrimSuffix(gvk.Kind, "List"),
+		}
+		for i := range x.Items {
+			item := &x.Items[i]
+			item.SetGroupVersionKind(itemGVK)
+		}
+
+		return nil
 	default:
 		return c.typedClient.List(ctx, obj, opts...)
 	}

pkg/client/client_cache.go

@@ -133,7 +133,6 @@ type resourceMeta struct {
 // isNamespaced returns true if the type is namespaced
 func (r *resourceMeta) isNamespaced() bool {
 	return r.mapping.Scope.Name() != meta.RESTScopeNameRoot
-
 }
 
 // resource returns the resource name of the type

pkg/client/client_test.go

@@ -1558,16 +1558,20 @@ var _ = Describe("Client", func() {
 
 				By("fetching the created Deployment")
 				var actual metav1.PartialObjectMetadata
-				actual.SetGroupVersionKind(schema.GroupVersionKind{
+				gvk := schema.GroupVersionKind{
 					Group:   "apps",
 					Version: "v1",
 					Kind:    "Deployment",
-				})
+				}
+				actual.SetGroupVersionKind(gvk)
 				key := client.ObjectKey{Namespace: ns, Name: dep.Name}
 				err = cl.Get(context.TODO(), key, &actual)
 				Expect(err).NotTo(HaveOccurred())
 				Expect(actual).NotTo(BeNil())
 
+				By("validating that the GVK has been preserved")
+				Expect(actual.GroupVersionKind()).To(Equal(gvk))
+
 				By("validating the fetched deployment equals the created one")
 				Expect(metaOnlyFromObj(dep, scheme)).To(Equal(&actual))
 
@@ -1676,6 +1680,11 @@ var _ = Describe("Client", func() {
 				Expect(deps.Items).NotTo(BeEmpty())
 				hasDep := false
 				for _, item := range deps.Items {
+					Expect(item.GroupVersionKind()).To(Equal(schema.GroupVersionKind{
+						Group:   "apps",
+						Kind:    "Deployment",
+						Version: "v1",
+					}))
 					if item.GetName() == dep.Name && item.GetNamespace() == dep.Namespace {
 						hasDep = true
 						break
@@ -2389,17 +2398,28 @@ var _ = Describe("Client", func() {
 				Expect(err).NotTo(HaveOccurred())
 
 				By("listing all objects of that type in the cluster")
-				metaList := &metav1.PartialObjectMetadataList{}
-				metaList.SetGroupVersionKind(schema.GroupVersionKind{
+				gvk := schema.GroupVersionKind{
 					Group:   "apps",
 					Version: "v1",
 					Kind:    "DeploymentList",
-				})
+				}
+				metaList := &metav1.PartialObjectMetadataList{}
+				metaList.SetGroupVersionKind(gvk)
 				Expect(cl.List(context.Background(), metaList)).NotTo(HaveOccurred())
 
+				By("validating that the list GVK has been preserved")
+				Expect(metaList.GroupVersionKind()).To(Equal(gvk))
+
+				By("validating that the list has the expected deployment")
 				Expect(metaList.Items).NotTo(BeEmpty())
 				hasDep := false
 				for _, item := range metaList.Items {
+					Expect(item.GroupVersionKind()).To(Equal(schema.GroupVersionKind{
+						Group:   "apps",
+						Version: "v1",
+						Kind:    "Deployment",
+					}))
+
 					if item.Name == dep.Name && item.Namespace == dep.Namespace {
 						hasDep = true
 						break