⚠️ Client: Ensure no stale data remains in target object

pkg/client/apiutil/apimachinery.go

@@ -21,6 +21,7 @@ package apiutil
 
 import (
 	"fmt"
+	"reflect"
 	"sync"
 
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -163,9 +164,35 @@ func createRestConfig(gvk schema.GroupVersionKind, isUnstructured bool, baseConf
 			// Use our own custom serializer.
 			cfg.NegotiatedSerializer = serializerWithDecodedGVK{serializer.WithoutConversionCodecFactory{CodecFactory: codecs}}
 		} else {
-			cfg.NegotiatedSerializer = serializer.WithoutConversionCodecFactory{CodecFactory: codecs}
+			cfg.NegotiatedSerializer = serializerWithTargetZeroingDecode{NegotiatedSerializer: serializer.WithoutConversionCodecFactory{CodecFactory: codecs}}
 		}
 	}
 
 	return cfg
 }
+
+type serializerWithTargetZeroingDecode struct {
+	runtime.NegotiatedSerializer
+}
+
+func (s serializerWithTargetZeroingDecode) DecoderToVersion(serializer runtime.Decoder, r runtime.GroupVersioner) runtime.Decoder {
+	return targetZeroingDecoder{upstream: s.NegotiatedSerializer.DecoderToVersion(serializer, r)}
+}
+
+type targetZeroingDecoder struct {
+	upstream runtime.Decoder
+}
+
+func (t targetZeroingDecoder) Decode(data []byte, defaults *schema.GroupVersionKind, into runtime.Object) (runtime.Object, *schema.GroupVersionKind, error) {
+	zero(into)
+	return t.upstream.Decode(data, defaults, into)
+}
+
+// zero zeros the value of a pointer.
+func zero(x interface{}) {
+	if x == nil {
+		return
+	}
+	res := reflect.ValueOf(x).Elem()
+	res.Set(reflect.Zero(res.Type()))
+}

pkg/client/client_suite_test.go

@@ -22,7 +22,9 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/examples/crd/pkg"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
 
@@ -43,14 +45,16 @@ var clientset *kubernetes.Clientset
 var _ = BeforeSuite(func() {
 	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
 
-	testenv = &envtest.Environment{}
+	testenv = &envtest.Environment{CRDDirectoryPaths: []string{"./testdata"}}
 
 	var err error
 	cfg, err = testenv.Start()
 	Expect(err).NotTo(HaveOccurred())
 
 	clientset, err = kubernetes.NewForConfig(cfg)
 	Expect(err).NotTo(HaveOccurred())
+
+	Expect(pkg.AddToScheme(scheme.Scheme)).NotTo(HaveOccurred())
 }, 60)
 
 var _ = AfterSuite(func() {

pkg/client/client_test.go

@@ -34,6 +34,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	kscheme "k8s.io/client-go/kubernetes/scheme"
 
+	"sigs.k8s.io/controller-runtime/examples/crd/pkg"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -1397,6 +1398,33 @@ var _ = Describe("Client", func() {
 			PIt("should fail if the GVK cannot be mapped to a Resource", func() {
 
 			})
+
+			// Test this with an integrated type and a CRD to make sure it covers both proto
+			// and json deserialization.
+			for idx, object := range []client.Object{&corev1.ConfigMap{}, &pkg.ChaosPod{}} {
+				idx, object := idx, object
+				It(fmt.Sprintf("should not retain any data in the obj variable that is not on the server for %T", object), func() {
+					cl, err := client.New(cfg, client.Options{})
+					Expect(err).NotTo(HaveOccurred())
+					Expect(cl).NotTo(BeNil())
+
+					object.SetName(fmt.Sprintf("retain-test-%d", idx))
+					object.SetNamespace(ns)
+
+					By("First creating the object")
+					toCreate := object.DeepCopyObject().(client.Object)
+					Expect(cl.Create(ctx, toCreate)).NotTo(HaveOccurred())
+
+					By("Fetching it into a variable that has finalizers set")
+					toGetInto := object.DeepCopyObject().(client.Object)
+					toGetInto.SetFinalizers([]string{"some-finalizer"})
+					Expect(cl.Get(ctx, client.ObjectKeyFromObject(object), toGetInto)).NotTo(HaveOccurred())
+
+					By("Ensuring the created and the received object are equal")
+					Expect(toCreate).Should(Equal(toGetInto))
+				})
+			}
+
 		})
 
 		Context("with unstructured objects", func() {
@@ -1469,6 +1497,30 @@ var _ = Describe("Client", func() {
 				err = cl.Get(context.TODO(), key, u)
 				Expect(err).To(HaveOccurred())
 			})
+
+			It("should not retain any data in the obj variable that is not on the server", func() {
+				object := &unstructured.Unstructured{}
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(cl).NotTo(BeNil())
+
+				object.SetName("retain-unstructured")
+				object.SetNamespace(ns)
+				object.SetAPIVersion("chaosapps.metamagical.io/v1")
+				object.SetKind("ChaosPod")
+
+				By("First creating the object")
+				toCreate := object.DeepCopyObject().(client.Object)
+				Expect(cl.Create(ctx, toCreate)).NotTo(HaveOccurred())
+
+				By("Fetching it into a variable that has finalizers set")
+				toGetInto := object.DeepCopyObject().(client.Object)
+				toGetInto.SetFinalizers([]string{"some-finalizer"})
+				Expect(cl.Get(ctx, client.ObjectKeyFromObject(object), toGetInto)).NotTo(HaveOccurred())
+
+				By("Ensuring the created and the received object are equal")
+				Expect(toCreate).Should(Equal(toGetInto))
+			})
 		})
 		Context("with metadata objects", func() {
 			It("should fetch an existing object for a go struct", func() {
@@ -1547,6 +1599,27 @@ var _ = Describe("Client", func() {
 			PIt("should fail if the GVK cannot be mapped to a Resource", func() {
 
 			})
+
+			It("should not retain any data in the obj variable that is not on the server", func() {
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(cl).NotTo(BeNil())
+
+				By("First creating the object")
+				toCreate := &pkg.ChaosPod{ObjectMeta: metav1.ObjectMeta{Name: "retain-metadata", Namespace: ns}}
+				Expect(cl.Create(ctx, toCreate)).NotTo(HaveOccurred())
+
+				By("Fetching it into a variable that has finalizers set")
+				toGetInto := &metav1.PartialObjectMetadata{
+					TypeMeta:   metav1.TypeMeta{APIVersion: "chaosapps.metamagical.io/v1", Kind: "ChaosPod"},
+					ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: "retain-metadata"},
+				}
+				toGetInto.SetFinalizers([]string{"some-finalizer"})
+				Expect(cl.Get(ctx, client.ObjectKeyFromObject(toGetInto), toGetInto)).NotTo(HaveOccurred())
+
+				By("Ensuring the created and the received objects metadata are equal")
+				Expect(toCreate.ObjectMeta).Should(Equal(toGetInto.ObjectMeta))
+			})
 		})
 	})
 

pkg/client/testdata/examplecrd.yaml

@@ -0,0 +1,17 @@
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  name: chaospods.chaosapps.metamagical.io
+spec:
+  group: chaosapps.metamagical.io
+  names:
+    kind: ChaosPod
+    plural: chaospods
+  scope: Namespaced
+  versions:
+    - name: "v1"
+      storage: true
+      served: true
+      schema:
+        openAPIV3Schema:
+          type: object