Skip to content

Commit

Permalink
🐛 allow fakeclient to patch CR with no RV
Browse files Browse the repository at this point in the history
\# Context

This PR should fix a bug
kubernetes-sigs#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
  • Loading branch information
alexandremahdhaoui committed Mar 24, 2024
1 parent e08b286 commit 57c612a
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 0 deletions.
32 changes: 32 additions & 0 deletions pkg/client/fake/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
2 changes: 2 additions & 0 deletions pkg/client/patch.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 57c612a

Please sign in to comment.