From 57c612afb451232348fc5097db4c4d60b8af3e81 Mon Sep 17 00:00:00 2001
From: Alexandre Mahdhaoui <alexandre.mahdhaoui@gmail.com>
Date: Sun, 24 Mar 2024 15:08:30 +0100
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20allow=20fakeclient=20to=20patch?=
 =?UTF-8?q?=20CR=20with=20no=20RV?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

\# Context

This PR should fix a bug
https://github.com/kubernetes-sigs/controller-runtime/issues/2678 where
patching a resource would throw a conflict error when the new object
doesn't specify any RV.

- Because the new RV is an null, an error is thrown here:
  https://github.com/kubernetes-sigs/controller-runtime/blob/01de80b092778fd35abd9e2371d54ce0953d4613/pkg/client/fake/client.go#L429-L430
- The RV is nil, because the computed patch between the old and new
  accessor doesn't include resourceVersion:
  -
https://github.com/evanphx/json-patch/blob/v5.9.0/v5/merge.go#L253-L273
  - Witch is used by `client.MergeFrom()`:
    https://github.com/kubernetes-sigs/controller-runtime/blob/v0.17.2/pkg/client/patch.go#L150-L152

\# Changes

- Fix patch by setting the new accessors' RV to the old accessors' RV.
- Add test to ensure the correct behavior
---
 pkg/client/fake/client_test.go | 32 ++++++++++++++++++++++++++++++++
 pkg/client/patch.go            |  2 ++
 2 files changed, 34 insertions(+)

diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go
index e5487de21a..b76cc61a5d 100644
--- a/pkg/client/fake/client_test.go
+++ b/pkg/client/fake/client_test.go
@@ -550,6 +550,38 @@ var _ = Describe("Fake client", func() {
 			Expect(obj.ObjectMeta.ResourceVersion).To(Equal("1000"))
 		})
 
+		It("should allow patch with non-set ResourceVersion for a resource that doesn't allow unconditional updates", func() {
+			schemeBuilder := &scheme.Builder{GroupVersion: schema.GroupVersion{Group: "test", Version: "v1"}}
+			schemeBuilder.Register(&WithPointerMeta{}, &WithPointerMetaList{})
+
+			scheme := runtime.NewScheme()
+			Expect(schemeBuilder.AddToScheme(scheme)).NotTo(HaveOccurred())
+
+			cl := NewClientBuilder().WithScheme(scheme).Build()
+			original := &WithPointerMeta{
+				ObjectMeta: &metav1.ObjectMeta{
+					Name:      "obj",
+					Namespace: "ns2",
+				}}
+
+			err := cl.Create(context.Background(), original)
+			Expect(err).ToNot(HaveOccurred())
+
+			newObj := &WithPointerMeta{
+				ObjectMeta: &metav1.ObjectMeta{
+					Name:      original.Name,
+					Namespace: original.Namespace,
+					Annotations: map[string]string{
+						"foo": "bar",
+					},
+				}}
+			Expect(cl.Patch(context.Background(), newObj, client.MergeFrom(original))).To(Succeed())
+
+			patched := &WithPointerMeta{}
+			Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(original), patched)).To(Succeed())
+			Expect(patched.Annotations).To(Equal(map[string]string{"foo": "bar"}))
+		})
+
 		It("should reject updates with non-set ResourceVersion for a resource that doesn't allow unconditional updates", func() {
 			By("Creating a new binding")
 			binding := &corev1.Binding{
diff --git a/pkg/client/patch.go b/pkg/client/patch.go
index 11d6083885..6035d64441 100644
--- a/pkg/client/patch.go
+++ b/pkg/client/patch.go
@@ -112,6 +112,8 @@ func (s *mergeFromPatch) Data(obj Object) ([]byte, error) {
 
 		modified = modified.DeepCopyObject().(Object)
 		modified.SetResourceVersion(version)
+	} else if modified.GetResourceVersion() == "" {
+		modified.SetResourceVersion(original.GetResourceVersion())
 	}
 
 	originalJSON, err := json.Marshal(original)