Bug 1302533 - Besides 'oc replace', it is better to prompt possible cause when `oc edit` with valid editing fails due to restricted permission [NEEDINFO]
Besides 'oc replace', it is better to prompt possible cause when `oc edit` wi...
Status: ASSIGNED
Product: OpenShift Origin
Classification: Red Hat
Component: Command Line Interface (Show other bugs)
3.x
Unspecified Unspecified
medium Severity low
: ---
: ---
Assigned To: Juan Vallejo
Xingxing Xia
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-28 00:40 EST by Xingxing Xia
Modified: 2016-10-11 16:48 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
xxia: needinfo? (ffranz)


Attachments (Terms of Use)

  None (edit)
Description Xingxing Xia 2016-01-28 00:40:25 EST
Description of problem:
With build strategy is only allowed to sti, if `oc edit bc` changes build strategy to docker or custom, then this editing will fail with error messages like: 
Error: the BuildConfig ruby-sample-build could not be updated: BuildConfig "ruby-sample-build" is forbidden: build strategy Docker is not allowed
You can run `oc update -f /tmp/oc-edit-tepf5.yaml` to try this update again.

But if run `oc update -f /tmp/oc-edit-tepf5.yaml`, this command will fail too, with similar message:
Error from server: error when replacing "/tmp/oc-edit-tepf5.yaml": BuildConfig "ruby-sample-build" is forbidden: build strategy Docker is not allowed

So it is better to remove the suggestion after `oc edit bc` fails.

Version-Release number of selected component (if applicable):
openshift/oc v1.1.1-180-gbe6c7f2
kubernetes v1.1.0-origin-1107-g4c8e6f4

How reproducible:
Always

Steps to Reproduce:
1. Disable docker/custom strategy, allow only sti strategy:
$ oc edit clusterrole admin edit --config=openshift.local.config/master/admin.kubeconfig
For each role, remove the builds/docker and builds/custom lines then save

2. Create a sti bc
$ oc new-app -f origin/examples/sample-app/application-template-stibuild.json
3. Edit bc, change as follows and save, quit:
$ oc edit bc ruby-sample-build
<-- snip -->
  strategy:
    sourceStrategy:    # <-- Change to dockerStrategy
      env:
      - name: EXAMPLE
        value: sample-app
      from:
        kind: ImageStreamTag
        name: ruby-22-centos7:latest
    type: Source        # <--  Change to Docker
<-- snip -->
Actual results:
3. It gives messages:
Error: the BuildConfig ruby-sample-build could not be updated: BuildConfig "ruby-sample-build" is forbidden: build strategy Docker is not allowed
You can run `oc update -f /tmp/oc-edit-tepf5.yaml` to try this update again.

Expected results:
3. Should remove the suggestion "You can run `oc update -f /tmp/oc-edit-tepf5.yaml` to try this update again."

Additional info:
Comment 1 Xingxing Xia 2016-02-29 02:55:14 EST
In latest version, actual result of step 3 just includes the suggestion message "You can run `oc ...", but does not include the error message "Error: ... is forbidden: build strategy Docker is not allowed". The error message should be kept.
Comment 2 Juan Vallejo 2016-09-16 18:07:56 EDT
I am having trouble reproducing this bug in version:

oc v1.3.0-rc1+85eb37b-dirty
kubernetes v1.4.0-beta.3+d19513f
features: Basic-Auth

Server https://10.13.137.149:8443
openshift v1.3.0-rc1
kubernetes v1.3.0+52492b4

Could you please verify that it still exists on your end?

Further, when I try step 1 ("Disable docker/custom strategy, allow only sti strategy"), I am unable to find "builds/docker" and "builds/custom"
Comment 3 Xingxing Xia 2016-09-18 02:16:21 EDT
Hmm, now it is 'unable to find "builds/docker" and "builds/custom"' according to step 1.
But the common problem still exists. That is, when users try to update resource and fail due to permission, it will give 'oc update' (now named 'oc replace') suggestion. 
Rewrite the description (here use another resource as an example to represent the common problem):
1. Login to openshift with normal user
2. oc edit clusterrole view # e.g. try to remove one '  - watch' then press ':wq'

It will prompt:
error: clusterroles "view" could not be patched: User ... cannot "patch" "clusterroles" with name "view" in project ""
You can run `oc replace -f /tmp/oc-edit-5cu8u.yaml` to try this update again.

If follow the suggestion, the suggestion still fails:
$ oc replace -f /tmp/oc-edit-5cu8u.yaml
Error from server: error when replacing "/tmp/oc-edit-5cu8u.yaml": User ... cannot update clusterroles at the cluster scope


This bug wants to say: Now that the suggestion will bring failure to user again, it is better to remove the suggestion, and give one suggestion of real use, e.g. tell user to get proper permission.
Comment 4 Xingxing Xia 2016-09-19 04:56:16 EDT
Another example:
$ oc edit namespace xxia-proj
error: namespaces "xxia-proj" could not be patched: User ... cannot "patch" "namespaces" with name "xxia-proj" in project "xxia-proj"
You can run `oc replace -f /tmp/oc-edit-oki0a.yaml` to try this update again.
$ oc replace -f /tmp/oc-edit-oki0a.yaml
Error from server: error when replacing "/tmp/oc-edit-oki0a.yaml": User ... cannot update namespaces in project "xxia-proj"
Comment 5 Juan Vallejo 2016-09-19 15:41:10 EDT
Hm, this would require an upstream change, and kubernetes does not have a concept of "policy", which means the "replace" suggestion would not give the same error to a user using kubectl. We do have plans to port `oc / oadm policy` to upstream, but until then, I do not think this is something we can specifically change for our cli client.

ffranz WDYT?
Comment 6 Xingxing Xia 2016-09-19 22:27:35 EDT
Got your concern.
Hmm, after some thinking, the 'oc replace' suggestion could be kept because user may have done much editing, the suggestion could help
user find the copy of the much editing. Thus, just need additionally prompt possible cause, such as need proper permission
Comment 7 Fabiano Franz 2016-09-21 17:53:42 EDT
In my opinion the most recent version is good enough already. The permissions cause is there ("user cannot patch namespaces in project..."), we don't have much more info than that. Moving to QA but I'd say no longer an issue.
Comment 8 Xingxing Xia 2016-09-21 23:47:42 EDT
(In reply to Fabiano Franz from comment #7)
> In my opinion the most recent version is good enough already. The
> permissions cause is there ("user cannot patch namespaces in project..."),

Thank you! In terms of this opinion, INDEED "permissions cause is there". But my initial thought was:
The wording "user cannot" is a little obscure. User may have various guess/explanation of it. E.g invalid syntax can cause "user cannot", permission can cause "user cannot", and so on.

In actual experience, when I first saw the prompt after oc edit as in comment 3, I didn't know what was up at first glance until I ran `oc replace -f /tmp/oc-edit-5cu8u.yaml` and then knew permission cause.
Frankly speaking, other QEs other than UI/CLI guys also encountered similarly when they `oc edit` resources, e.g. daemonset, and asked me, the guy's question at first glance was whether `oc edit daemonset` has distinction than common resource pod/dc etc, the guy was not knowing policy and after I checked I told the guy the permission cause.
So my intention was: if the prompt can tell specific wording like 'permission cause', it is better and more intuitive from customer perspective.

e.g.:
$ oc patch pod/database-1-tqo7n -p '{"spec":{"serviceAccountName":"builder"}}'
The Pod "database-1-tqo7n" is invalid.
spec: Forbidden: pod updates may not change fields other than `containers[*].image` or `spec.activeDeadlineSeconds`

This prompt is specific wording about immutable fields. If it only tells "user cannot" update pod, it is obscure what is the cause.

WDYT?
Comment 9 Juan Vallejo 2016-10-11 16:48:47 EDT
I am not sure if we should include permission specific wording (e.g. Forbidden) on an upstream change, as it still would not apply to users of `kubectl`.

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