OpenShift incorrectly applies RFC 6902 JSON PATCH

k-wall commented 5 years ago

The OpenShift server side appears to be incorrectly applying valid RFC 6902 JSON PATCH documents when patching existing statefulset resources. This leads to resources in an unexpected state and in some cases, this is leading the server rejecting the request with errors such as:

io.fabric8.kubernetes.client.KubernetesClientException: Failure executing: PATCH at: https://172.30.0.1/apis/apps/v1/namespaces/xxxxxx-infra/statefulsets/broker-mdh1f76xc7-mqv7. Message: Unable to access invalid index: 20.

We have investigated the issue, and believe the issue is the implementation of the RFC 6902 move operation within OpenShift. Instead of inserting the moved value back into the document, OpenShift's implementation is overwriting the value at the target. This leads to an incorrect result document. Subsequent processing of following operations may also end in error, such as the one above.

I believe the OpenShift implementation is not compliant with the RFC.

RFC 6092, 4.4. Move, says:

The "move" operation removes the value at a specified location and adds it to the target location.

it then goes on to say:

This operation is functionally identical to a "remove" operation on the "from" location, followed immediately by an "add" operation at the target location with the value that was just removed.

section 4.1 defines add like this:

If the target location specifies an array index, a new value is inserted into the array at the specified index.

Version

oc v3.11.0+0cbc58b kubernetes v1.11.0+d4cacc0 features: Basic-Auth GSSAPI Kerberos SPNEGO

Server https://127.0.0.1:8443 kubernetes v1.11.0+d4cacc0

Steps To Reproduce

See: https://github.com/k-wall/rfc6902-json-patch/blob/master/README.md

I've provided a simple example. This example provides a statefulstate and a patch to mutate the environment variables of a statefulset using the following PATCH:

[
{
  "op" : "move",
  "from" : "/spec/template/spec/containers/0/env/1",
  "path" : "/spec/template/spec/containers/0/env/2"
}
 ]

Current Result

B has replaced D. This is incorrect.

                        "env": [
                            {
                                "name": "A",
                                "value": "a"
                            },
                            {
                                "name": "C",
                                "value": "c"
                            },
                            {
                                "name": "B",
                                "value": "b"
                            }
                        ],

Expected Result

B is moved to after C.

          "env" : [ {
            "name" : "A",
            "value" : "a"
          }, {
            "name" : "C",
            "value" : "c"
          }, {
            "name" : "B",
            "value" : "b"
          }, {
            "name" : "D",
            "value" : "d"
          } ],

lucab commented 5 years ago

Ack, I think you are hitting this logic bug in the underlying library https://github.com/evanphx/json-patch/issues/69.

k-wall commented 5 years ago

@lucab thanks, indeed, that looks very likely to be our issue. I think https://github.com/openshift/origin/commit/6eeaa8ded2ae8537aa78f002c9916411e4392857 means that this change is already in OpenShift 4.1, is that right? (Unfortunately, I can't easily test on on 4.1 at the moment but should be able to soon).

Do you know if there is a plan to backport to 3.11?

lucab commented 5 years ago

I don't know, but someone across @deads2k @sttts @soltysh may.

sttts commented 5 years ago

@lucab yes, looks like it is fixed in 4.1 with https://github.com/openshift/origin/pull/22267.

soltysh commented 5 years ago

Based on previous comment I'm going to close this issue since it's solved with the current release. /close

openshift-ci-robot commented 5 years ago

@soltysh: Closing this issue.

In response to [this](https://github.com/openshift/origin/issues/23206#issuecomment-504964731): >Based on previous comment I'm going to close this issue since it's solved with the current release. >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

openshift / origin