Skip to content

Commit

Permalink
bug: Client: Ensure no stale data remains in target object
Browse files Browse the repository at this point in the history
Fixes kubernetes-sigs#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.
  • Loading branch information
alvaroaleman authored and k8s-infra-cherrypick-robot committed Sep 10, 2021
1 parent 2d2f85f commit 99ab43a
Show file tree
Hide file tree
Showing 4 changed files with 123 additions and 2 deletions.
29 changes: 28 additions & 1 deletion pkg/client/apiutil/apimachinery.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package apiutil

import (
"fmt"
"reflect"
"sync"

"k8s.io/apimachinery/pkg/api/meta"
Expand Down Expand Up @@ -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()))
}
6 changes: 5 additions & 1 deletion pkg/client/client_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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() {
Expand Down
73 changes: 73 additions & 0 deletions pkg/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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))
})
})
})

Expand Down
17 changes: 17 additions & 0 deletions pkg/client/testdata/examplecrd.yaml
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 99ab43a

Please sign in to comment.