Bug 1302092 - oc patch confusing behavior with JSONPatch
oc patch confusing behavior with JSONPatch
Status: CLOSED NOTABUG
Product: OpenShift Origin
Classification: Red Hat
Component: Pod (Show other bugs)
3.x
Unspecified Unspecified
medium Severity low
: ---
: ---
Assigned To: Solly Ross
DeShuai Ma
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-26 13:24 EST by Aleksandar Kostadinov
Modified: 2017-03-03 10:13 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2017-03-03 10:13:38 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Aleksandar Kostadinov 2016-01-26 13:24:23 EST
Description of problem:
Behavior of `oc patch` when given a JSONPatch parameter is confusing. It reports that object is patched but in fact no modification to the underlying object is performed. See an example:

> [root@ip-172-18-15-105 ~]# oc get scc privileged -o yaml
> ...
> users:
> - system:serviceaccount:openshift-infra:build-controller
> - system:serviceaccount:default:default
> 
> [root@ip-172-18-15-105 ~]# oc patch scc privileged -p '{"op": "add", "path": "/users", "value": "system:admin"}'
> "privileged" patched
> 
> [root@ip-172-18-15-105 ~]# oc get scc privileged -o yaml
> ...
> users:
> - system:serviceaccount:openshift-infra:build-controller
> - system:serviceaccount:default:default

I think the `oc patch` command should have a parameter to specify that the parameter is a JSONPatch (not a merge JSON), i.e. set rest request content type to `application/json-patch+json`, `application/merge-patch+json` or `application/strategic-merge-patch+json` as user desires. Also it may try to detect JSONPatch by itself as the format is pretty much obvious.

But ultimately an error should be reported by CLI when operation dod not actually succeed. Probably it also depends on server side?

I can confirm that regular merge-patch+json parameter works fine for the above use case. I know about `oadm policy add-scc-to-user` but still the bug holds true.

Version-Release number of selected component (if applicable):
oc v1.1.1-168-g5597bae
kubernetes v1.1.0-origin-1107-g4c8e6f4

How reproducible:
always
Comment 1 Solly Ross 2016-02-02 16:30:14 EST
It looks like there's two parts to this issue/request:

1. `oc patch` (and equivalently `kubectl patch`) should accept an argument to denote the type of patch that is being used.

This seems like a good feature to have, and should not be very difficult.

2. Submitting a patch in one format when using another should throw an error:

This is a bit trickier, since it seems like what's happening at the server level is this:

Suppose we're patching an object `{"a": 1}` with `{"op": "add", "path": "/a", "value": 2}`.  Then, the server receives the patch, and attempts the merge patch (or strategic merge patch).  The patch completes successfully, resulting in the JSON blob `{"a": 1, "op": "add", "path": "/a", "value": 2}` as the "patched" object.  The object is then decoded from JSON back into a Go object, at which point the extraneous parts of the "patched" object are discarded, and then the object is updated in etcd.  Thus, it appears that the patch goes through successfully.

Ergo, it might be a bit trickier to provide this functionality.
Comment 2 Aleksandar Kostadinov 2016-02-02 16:43:30 EST
Excellent explanation, thank you. But isn't it incorrect that decoding JSON back into a Go object causes extraneous parts of the "patched" object to be discarded instead of throwing an error?

Current behavior appears to me as a good way to leave user unaware that the patch did not actually work properly. Failing on wrong JSON on the other hand gives a clear indication something was wrong at the point of the faulty operation so would be easier to debug.
Comment 3 Solly Ross 2016-02-03 11:24:05 EST
> Excellent explanation, thank you. But isn't it incorrect that decoding JSON back into a Go object causes extraneous parts of the "patched" object to be discarded instead of throwing an error?

AFAICT, this behavior is due to the way that Go's JSON decoder works.

I agree that probably we should at least warn if this happens.  I'll see if I can figure out a good way to work around this.
Comment 5 Derek Carr 2017-03-03 10:12:03 EST
This needs to be an RFE for CLI to support more patch strategies.
Comment 6 Derek Carr 2017-03-03 10:13:38 EST
this not a bug, please open an RFE if you want to advocate for this feature.

Note You need to log in before you can comment on or make changes to this bug.