🐛 Client: Ensure no stale data remains in target object (#1640)

Fixes #1639

The json deserializer of the stdlib and the one from Kube which aims to
be compatible won't zero out all field types in the object it
deserializes into, for example it lets slices be if the json does not
contain that field. This means that if a non-empty variable is used for
any api call with the client, the resulting content might be a mixture
of previous content and what is on the server.

This PR adds a wrapper around the deserializer that will first zero the
target object.

Author: alvaroaleman

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