Merge pull request #528 from vincepri/fixup-gvk

🐛 Preserve GroupVersionKind during Update/Patch

Author: k8s-ci-robot

pkg/client/client.go

@@ -24,6 +24,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -103,6 +104,16 @@ type client struct {
 	unstructuredClient unstructuredClient
 }
 
+// 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.15.
+func (c *client) resetGroupVersionKind(obj runtime.Object, gvk schema.GroupVersionKind) {
+	if gvk != schema.EmptyObjectKind.GroupVersionKind() {
+		if v, ok := obj.(schema.ObjectKind); ok {
+			v.SetGroupVersionKind(gvk)
+		}
+	}
+}
+
 // Create implements client.Client
 func (c *client) Create(ctx context.Context, obj runtime.Object, opts ...CreateOption) error {
 	_, ok := obj.(*unstructured.Unstructured)
@@ -114,6 +125,7 @@ func (c *client) Create(ctx context.Context, obj runtime.Object, opts ...CreateO
 
 // Update implements client.Client
 func (c *client) Update(ctx context.Context, obj runtime.Object, opts ...UpdateOption) error {
+	defer c.resetGroupVersionKind(obj, obj.GetObjectKind().GroupVersionKind())
 	_, ok := obj.(*unstructured.Unstructured)
 	if ok {
 		return c.unstructuredClient.Update(ctx, obj, opts...)
@@ -141,6 +153,7 @@ func (c *client) DeleteAllOf(ctx context.Context, obj runtime.Object, opts ...De
 
 // Patch implements client.Client
 func (c *client) Patch(ctx context.Context, obj runtime.Object, patch Patch, opts ...PatchOption) error {
+	defer c.resetGroupVersionKind(obj, obj.GetObjectKind().GroupVersionKind())
 	_, ok := obj.(*unstructured.Unstructured)
 	if ok {
 		return c.unstructuredClient.Patch(ctx, obj, patch, opts...)
@@ -181,6 +194,7 @@ var _ StatusWriter = &statusWriter{}
 
 // Update implements client.StatusWriter
 func (sw *statusWriter) Update(ctx context.Context, obj runtime.Object, opts ...UpdateOption) error {
+	defer sw.client.resetGroupVersionKind(obj, obj.GetObjectKind().GroupVersionKind())
 	_, ok := obj.(*unstructured.Unstructured)
 	if ok {
 		return sw.client.unstructuredClient.UpdateStatus(ctx, obj, opts...)
@@ -190,6 +204,7 @@ func (sw *statusWriter) Update(ctx context.Context, obj runtime.Object, opts ...
 
 // Patch implements client.Client
 func (sw *statusWriter) Patch(ctx context.Context, obj runtime.Object, patch Patch, opts ...PatchOption) error {
+	defer sw.client.resetGroupVersionKind(obj, obj.GetObjectKind().GroupVersionKind())
 	_, ok := obj.(*unstructured.Unstructured)
 	if ok {
 		return sw.client.unstructuredClient.PatchStatus(ctx, obj, patch, opts...)

pkg/client/client_test.go

@@ -59,6 +59,7 @@ func deleteNamespace(ns *corev1.Namespace) {
 var _ = Describe("Client", func() {
 
 	var scheme *runtime.Scheme
+	var depGvk schema.GroupVersionKind
 	var dep *appsv1.Deployment
 	var pod *corev1.Pod
 	var node *corev1.Node
@@ -82,6 +83,11 @@ var _ = Describe("Client", func() {
 				},
 			},
 		}
+		depGvk = schema.GroupVersionKind{
+			Group:   "apps",
+			Kind:    "Deployment",
+			Version: "v1",
+		}
 		// Pod is invalid without a container field in the PodSpec
 		pod = &corev1.Pod{
 			ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("pod-%v", count), Namespace: ns},
@@ -453,6 +459,26 @@ var _ = Describe("Client", func() {
 				close(done)
 			})
 
+			It("should update and preserve type information", func(done Done) {
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(cl).NotTo(BeNil())
+
+				By("initially creating a Deployment")
+				dep, err := clientset.AppsV1().Deployments(ns).Create(dep)
+				Expect(err).NotTo(HaveOccurred())
+
+				By("updating the Deployment")
+				dep.SetGroupVersionKind(depGvk)
+				err = cl.Update(context.TODO(), dep)
+				Expect(err).NotTo(HaveOccurred())
+
+				By("validating updated Deployment has type information")
+				Expect(dep.GroupVersionKind()).To(Equal(depGvk))
+
+				close(done)
+			})
+
 			It("should update an existing object non-namespace object from a go struct", func(done Done) {
 				cl, err := client.New(cfg, client.Options{})
 				Expect(err).NotTo(HaveOccurred())
@@ -550,6 +576,29 @@ var _ = Describe("Client", func() {
 				close(done)
 			})
 
+			It("should update and preserve type information", func(done Done) {
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(cl).NotTo(BeNil())
+
+				By("initially creating a Deployment")
+				dep, err := clientset.AppsV1().Deployments(ns).Create(dep)
+				Expect(err).NotTo(HaveOccurred())
+
+				By("updating the Deployment")
+				u := &unstructured.Unstructured{}
+				Expect(scheme.Convert(dep, u, nil)).To(Succeed())
+				u.SetGroupVersionKind(depGvk)
+				u.SetAnnotations(map[string]string{"foo": "bar"})
+				err = cl.Update(context.TODO(), u)
+				Expect(err).NotTo(HaveOccurred())
+
+				By("validating updated Deployment has type information")
+				Expect(u.GroupVersionKind()).To(Equal(depGvk))
+
+				close(done)
+			})
+
 			It("should update an existing object non-namespace object from a go struct", func(done Done) {
 				cl, err := client.New(cfg, client.Options{})
 				Expect(err).NotTo(HaveOccurred())
@@ -586,11 +635,7 @@ var _ = Describe("Client", func() {
 				By("updating non-existent object")
 				u := &unstructured.Unstructured{}
 				Expect(scheme.Convert(dep, u, nil)).To(Succeed())
-				u.SetGroupVersionKind(schema.GroupVersionKind{
-					Group:   "apps",
-					Kind:    "Deployment",
-					Version: "v1",
-				})
+				u.SetGroupVersionKind(depGvk)
 				err = cl.Update(context.TODO(), dep)
 				Expect(err).To(HaveOccurred())
 
@@ -624,6 +669,49 @@ var _ = Describe("Client", func() {
 				close(done)
 			})
 
+			It("should update status and preserve type information", func(done Done) {
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(cl).NotTo(BeNil())
+
+				By("initially creating a Deployment")
+				dep, err := clientset.AppsV1().Deployments(ns).Create(dep)
+				Expect(err).NotTo(HaveOccurred())
+
+				By("updating the status of Deployment")
+				dep.SetGroupVersionKind(depGvk)
+				dep.Status.Replicas = 1
+				err = cl.Status().Update(context.TODO(), dep)
+				Expect(err).NotTo(HaveOccurred())
+
+				By("validating updated Deployment has type information")
+				Expect(dep.GroupVersionKind()).To(Equal(depGvk))
+
+				close(done)
+			})
+
+			It("should patch status and preserve type information", func(done Done) {
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(cl).NotTo(BeNil())
+
+				By("initially creating a Deployment")
+				dep, err := clientset.AppsV1().Deployments(ns).Create(dep)
+				Expect(err).NotTo(HaveOccurred())
+
+				By("patching the status of Deployment")
+				dep.SetGroupVersionKind(depGvk)
+				depPatch := client.MergeFrom(dep.DeepCopy())
+				dep.Status.Replicas = 1
+				err = cl.Status().Patch(context.TODO(), dep, depPatch)
+				Expect(err).NotTo(HaveOccurred())
+
+				By("validating updated Deployment has type information")
+				Expect(dep.GroupVersionKind()).To(Equal(depGvk))
+
+				close(done)
+			})
+
 			It("should not update spec of an existing object", func(done Done) {
 				cl, err := client.New(cfg, client.Options{})
 				Expect(err).NotTo(HaveOccurred())
@@ -1070,6 +1158,26 @@ var _ = Describe("Client", func() {
 				close(done)
 			})
 
+			It("should patch and preserve type information", func(done Done) {
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(cl).NotTo(BeNil())
+
+				By("initially creating a Deployment")
+				dep, err := clientset.AppsV1().Deployments(ns).Create(dep)
+				Expect(err).NotTo(HaveOccurred())
+
+				By("patching the Deployment")
+				dep.SetGroupVersionKind(depGvk)
+				err = cl.Patch(context.TODO(), dep, client.ConstantPatch(types.MergePatchType, mergePatch))
+				Expect(err).NotTo(HaveOccurred())
+
+				By("validating updated Deployment has type information")
+				Expect(dep.GroupVersionKind()).To(Equal(depGvk))
+
+				close(done)
+			})
+
 			It("should patch an existing object non-namespace object from a go struct", func(done Done) {
 				cl, err := client.New(cfg, client.Options{})
 				Expect(err).NotTo(HaveOccurred())
@@ -1184,6 +1292,28 @@ var _ = Describe("Client", func() {
 				close(done)
 			})
 
+			It("should patch and preserve type information", func(done Done) {
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(cl).NotTo(BeNil())
+
+				By("initially creating a Deployment")
+				dep, err := clientset.AppsV1().Deployments(ns).Create(dep)
+				Expect(err).NotTo(HaveOccurred())
+
+				By("patching the Deployment")
+				u := &unstructured.Unstructured{}
+				Expect(scheme.Convert(dep, u, nil)).To(Succeed())
+				u.SetGroupVersionKind(depGvk)
+				err = cl.Patch(context.TODO(), u, client.ConstantPatch(types.MergePatchType, mergePatch))
+				Expect(err).NotTo(HaveOccurred())
+
+				By("validating updated Deployment has type information")
+				Expect(u.GroupVersionKind()).To(Equal(depGvk))
+
+				close(done)
+			})
+
 			It("should patch an existing object non-namespace object from a go struct", func(done Done) {
 				cl, err := client.New(cfg, client.Options{})
 				Expect(err).NotTo(HaveOccurred())