Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fakeclient: Patching an object that has allowUnconditionalUpdate=false where the patched version has no RV should succeed #2678

Closed
alvaroaleman opened this issue Feb 9, 2024 · 3 comments
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@alvaroaleman
Copy link
Member

Easiest to show with a testcase, the following test should pass but does not:

--- a/pkg/client/fake/client_test.go
+++ b/pkg/client/fake/client_test.go
@@ -359,6 +359,33 @@ var _ = Describe("Fake client", func() {
                        Expect(list.Items).To(ConsistOf(*dep2))
                })

+               FIt("should not return a conflict error from a patch operation with an empty RV and allowsUnconditionalUpdate=false", 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 be able to Create", func() {
                        By("Creating a new configmap")
                        newcm := &corev1.ConfigMap{

We never noticed this, because all the tests use built-in resoruces and they all allowUnconditionalUpdate

@alvaroaleman alvaroaleman added the kind/bug Categorizes issue or PR as related to a bug. label Feb 9, 2024
@alvaroaleman
Copy link
Member Author

Actually, at least that precise testcase is wrong as it will end up generating a patch that explicitly sets ResourceVersion to null which is indeed getting rejected by the server.

alexandremahdhaoui added a commit to alexandremahdhaoui/controller-runtime that referenced this issue Mar 24, 2024
\# 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
alexandremahdhaoui added a commit to alexandremahdhaoui/controller-runtime that referenced this issue Mar 24, 2024
\# 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 empty string, 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
@sbueringer
Copy link
Member

sbueringer commented Apr 15, 2024

Actually, at least that precise testcase is wrong as it will end up generating a patch that explicitly sets ResourceVersion to null which is indeed getting rejected by the server.

Talked to Alvaro.

Just to clarify. The goal is that the fake client allows a patch which sets resourceVersion to nil.

Verified that the real apiserver behaves the same via envtest:

It("should update an existing object non-namespace object from a go struct", func() {
    cl, err := client.New(cfg, client.Options{})
    Expect(err).NotTo(HaveOccurred())
    Expect(cl).NotTo(BeNil())

    node, err := clientset.CoreV1().Nodes().Create(ctx, node, metav1.CreateOptions{})
    Expect(err).NotTo(HaveOccurred())
    original := node.DeepCopy()

    By("updating the object")
    node.Annotations = map[string]string{"foo": "bar"}
    node.ObjectMeta.ResourceVersion = ""
    err = cl.Patch(context.TODO(), node, client.MergeFrom(original))
    Expect(err).NotTo(HaveOccurred())

    By("validate updated Node had new annotation")
    actual, err := clientset.CoreV1().Nodes().Get(ctx, node.Name, metav1.GetOptions{})
    Expect(err).NotTo(HaveOccurred())
    Expect(actual).NotTo(BeNil())
    Expect(actual.Annotations["foo"]).To(Equal("bar"))
})

@alvaroaleman
Copy link
Member Author

This got fixed in #2725, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

2 participants