From 8ecdef0b4dd24a2aab5294d7ec9d51eb850d458b Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 22 Aug 2021 15:33:33 -0400 Subject: [PATCH] :bug: Client: Ensure no stale data remains in target object Fixes https://github.com/kubernetes-sigs/controller-runtime/issues/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. --- pkg/client/apiutil/apimachinery.go | 29 +++++++++++- pkg/client/client_suite_test.go | 6 ++- pkg/client/client_test.go | 73 +++++++++++++++++++++++++++++ pkg/client/testdata/examplecrd.yaml | 17 +++++++ 4 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 pkg/client/testdata/examplecrd.yaml diff --git a/pkg/client/apiutil/apimachinery.go b/pkg/client/apiutil/apimachinery.go index 2611a20c64..e21eb22389 100644 --- a/pkg/client/apiutil/apimachinery.go +++ b/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())) +} diff --git a/pkg/client/client_suite_test.go b/pkg/client/client_suite_test.go index 980150c91b..c7ed32e7bc 100644 --- a/pkg/client/client_suite_test.go +++ b/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,7 +45,7 @@ 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() @@ -51,6 +53,8 @@ var _ = BeforeSuite(func() { clientset, err = kubernetes.NewForConfig(cfg) Expect(err).NotTo(HaveOccurred()) + + Expect(pkg.AddToScheme(scheme.Scheme)).NotTo(HaveOccurred()) }, 60) var _ = AfterSuite(func() { diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 45fbf6903c..fac8c97ab8 100644 --- a/pkg/client/client_test.go +++ b/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)) + }) }) }) diff --git a/pkg/client/testdata/examplecrd.yaml b/pkg/client/testdata/examplecrd.yaml new file mode 100644 index 0000000000..5409ee9789 --- /dev/null +++ b/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