Update "no projects" warning in `oc status` #12328

juanvallejo · 2016-12-22T18:56:27Z

Related bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1405636#c3
Related PR: #12304

This patch only re-uses the oc login "no projects" warning in
oc status when a user cannot access the current default namespace
and has not yet created any projects and has not passed a namespace
value with the --namespace flag.

Before

$ oc login -u newuser -p any
$ oc status
You don't have any projects. You can try to create a new project, by
running

    oc new-project <projectname>

$ oc status -n default
You don't have any projects. You can try to create a new project, by
running

    oc new-project <projectname>

After

$ oc login -u newuser -p any
$ oc status
You don't have any projects. You can try to create a new project, by
running

    oc new-project <projectname>

$ oc status -n default
Error from server: User "newuser" cannot get projects in project "default"

$ oc new-project proj1
$ oc new-project proj2

$ oc status
In project proj2 on server https://10.13.137.149:8443

You have no services, deployment configs, or build configs.
Run 'oc new-app' to create an application.

$ oc delete project proj2
$ oc status
Error from server: User "newuser" cannot get projects in project "proj2"

$ oc logout
$ oc login -u newuser -p any
$ oc status
In project proj1 on server https://10.13.137.149:8443

You have no services, deployment configs, or build configs.
Run 'oc new-app' to create an application.

cc @openshift/cli-review

juanvallejo · 2016-12-22T19:12:01Z

[test]

smarterclayton · 2016-12-22T19:53:22Z

$ oc status -n default
Error from server: User "newuser" cannot get projects in project "default"

Does not make sense. Projects aren't nested in projects.

juanvallejo · 2016-12-22T20:12:12Z

@smarterclayton I believe this is an error defined server-side in the authorizer package.

Would it make more sense to change User ... cannot {{verb}} {{resourceName}} in project ... to User ... cannot {{verb}} {{resourceName}} in namespace ... for each of the calls to messageResolver.addNamespacedForbiddenMessageMaker?

juanvallejo · 2016-12-22T20:14:48Z

@smarterclayton

Would it make more sense to change User ... cannot {{verb}} {{resourceName}} in project ... to User ... cannot {{verb}} {{resourceName}} in namespace ... for each of the calls to messageResolver.addNamespacedForbiddenMessageMaker?

Alternatively we could just handle the forbidden error client side and return a more user-friendly message here

smarterclayton · 2016-12-22T22:12:13Z

The error you are seeing cannot be correct - projects are not nested in projects, so whatever is putting that message together is doing so incorrectly. If that's the server, the server should be fixed.

juanvallejo · 2017-01-11T16:38:36Z

@smarterclayton

The error you are seeing cannot be correct - projects are not nested in projects, so whatever is putting that message together is doing so incorrectly. If that's the server, the server should be fixed.

Updated a conditional to authorizer messages to fix this: https://github.com/openshift/origin/pull/12328/files#diff-14287a8c63a62966ec50b251f590618d

EDIT: updated output

$ oc status -n default
Error from server: User "newuser" cannot get project "default"

smarterclayton · 2017-01-11T18:26:07Z

pkg/authorization/authorizer/messages.go

@@ -30,17 +30,17 @@ func NewForbiddenMessageResolver(projectRequestForbiddenTemplate string) *Forbid
 	}

 	// general messages
-	messageResolver.addNamespacedForbiddenMessageMaker("create", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot create `+apiGroupIfNotEmpty+`{{.Attributes.GetResource}} in project "{{.Namespace}}"`))
+	messageResolver.addNamespacedForbiddenMessageMaker("create", authorizationapi.ResourceAll, newTemplateForbiddenMessageMaker(`User "{{.User.GetName}}" cannot create `+apiGroupIfNotEmpty+`{{if not .Attributes.IsProjectResource}}{{.Attributes.GetResource}} in {{end}}project "{{.Namespace}}"`))


This doesn't even make sense - projects aren't namspaced, why is GetResource a project resource?

.IsProjectResource needs better phrasing (I will update this), but I did not mean for it to imply that GetResource was a project resource, rather IsProjectResource returns true if the string value of Attributes.GetResource == "projects"

But this is messageResolver.addNamespacedForbiddenMessageMaker - why is create projects getting passed to this? Since projects is never invoked in a namespaced context, I wouldn't expect this message to get used at all (it should be a different message without referencing projects or namespaces at all).

juanvallejo · 2017-01-11T19:11:44Z

@smarterclayton
Updated the conditional statement in authorizer messages to make more sense: https://github.com/openshift/origin/pull/12328/files#diff-14287a8c63a62966ec50b251f590618d

juanvallejo · 2017-01-12T17:17:15Z

@smarterclayton

But this is messageResolver.addNamespacedForbiddenMessageMaker - why is create projects getting passed to this? Since projects is never invoked in a namespaced context, I wouldn't expect this message to get used at all (it should be a different message without referencing projects or namespaces at all).

Updated authorizer messages so that only "get" requests for the "projects" resource have this custom message. "Projects" are both namespaced and non-namespaced in the authorizer depending on where they are on the stack to allow for better handling of "get" requests. cc @deads2k

smarterclayton · 2017-01-13T16:37:26Z

pkg/cmd/cli/describe/projectstatus.go

@@ -146,6 +149,11 @@ func (d *ProjectStatusDescriber) MakeGraph(namespace string) (osgraph.Graph, set
 // Describe returns the description of a project
 func (d *ProjectStatusDescriber) Describe(namespace, name string) (string, error) {
 	var f formatter = namespacedFormatter{}
+	var currentProject string
+
+	if currentContext, exists := d.Config.Contexts[d.Config.CurrentContext]; exists {


Just looking at all this code, it is increasingly complex and not really the right point to be doing the check. Describe() knowing about ClientConfig in this way is leaking the abstraction. You already get "default namespace or not" from the bool returned the .Namespace() call. Line 114 above.

juanvallejo · 2017-01-13T21:34:03Z

@smarterclayton Thanks for the feedback, review comments addressed

juanvallejo · 2017-01-16T18:42:10Z

@smarterclayton or @fabianofranz are there any more comments on this patch? Thanks!

juanvallejo · 2017-01-16T21:02:12Z

conformance flaked on #12362 re[test]

juanvallejo · 2017-01-18T22:09:49Z

integration e2e check flaked on #12540 re[test]

juanvallejo · 2017-01-19T21:26:53Z

@fabianofranz or @smarterclayton are there any more comments for this PR? Thanks

fabianofranz · 2017-01-20T15:15:19Z

LGTM, squash?

juanvallejo · 2017-01-20T15:44:03Z

@fabianofranz Done!

fabianofranz · 2017-01-20T17:14:13Z

[merge]

juanvallejo · 2017-01-23T23:27:00Z

integration check flaked on #12558 re[test]

fabianofranz · 2017-01-24T04:24:21Z

flaked on #12558 re[test]

fabianofranz · 2017-01-24T13:42:25Z

[merge]

fabianofranz · 2017-01-24T15:27:20Z

@juanvallejo Test failure seems legit, ptal. You may need to delete the project by name, instead of --all.

This patch only re-uses the `oc login` "no projects" warning in `oc status` when a user cannot access the current default namespace and has not yet created any projects and has not passed a namespace value with the `--namespace` flag. **Before** ``` $ oc login -u newuser -p any $ oc status You don't have any projects. You can try to create a new project, by running oc new-project <projectname> $ oc status -n default You don't have any projects. You can try to create a new project, by running oc new-project <projectname> ``` **After** ``` $ oc login -u newuser -p any $ oc status You don't have any projects. You can try to create a new project, by running oc new-project <projectname> $ oc status -n default Error from server: User "newuser" cannot get projects in project "default" $ oc new-project proj1 $ oc new-project proj2 $ oc status In project proj2 on server https://10.13.137.149:8443 You have no services, deployment configs, or build configs. Run 'oc new-app' to create an application. $ oc delete project proj2 $ oc status Error from server: User "newuser" cannot get projects in project "proj2" $ oc logout $ oc login -u newuser -p any $ oc status In project proj1 on server https://10.13.137.149:8443 You have no services, deployment configs, or build configs. Run 'oc new-app' to create an application. ```

juanvallejo · 2017-01-24T16:19:44Z

@fabianofranz thanks, turns out I was not waiting for projects to be deleted, so when I tried deleting a new project right after deleting a previous one, I would get a forbidden error.

openshift-bot · 2017-01-24T16:21:33Z

Evaluated for origin test up to 9cfc359

openshift-bot · 2017-01-24T17:41:44Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13249/) (Base Commit: c5868ac)

fabianofranz · 2017-01-24T17:42:35Z

[merge]

openshift-bot · 2017-01-24T17:45:41Z

Evaluated for origin merge up to 9cfc359

openshift-bot · 2017-01-25T09:05:33Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13249/) (Base Commit: dced7f7) (Image: devenv-rhel7_5774)

smarterclayton · 2017-01-26T00:53:59Z

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_check/10575/testReport/junit/github/com_openshift_origin_test_cmd_templates_different-namespaces/test_cmd_templates_sh_123__executing__oc_new_project_test_template_project__expecting_success/

Is this related?

smarterclayton · 2017-01-26T00:54:32Z

Looks like it is, I'm reverting.

juanvallejo force-pushed the jvallejo/update-oc-status-warning branch from 2505358 to 94cb181 Compare December 22, 2016 19:19

juanvallejo force-pushed the jvallejo/update-oc-status-warning branch 2 times, most recently from e2af29a to f3283ca Compare January 11, 2017 16:36

juanvallejo force-pushed the jvallejo/update-oc-status-warning branch from f3283ca to 31eae66 Compare January 11, 2017 18:17

smarterclayton reviewed Jan 11, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/update-oc-status-warning branch 2 times, most recently from a300229 to e571ad8 Compare January 12, 2017 17:11

juanvallejo force-pushed the jvallejo/update-oc-status-warning branch 2 times, most recently from 7a43510 to bb1a3b8 Compare January 12, 2017 21:19

smarterclayton reviewed Jan 13, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/update-oc-status-warning branch 2 times, most recently from 9ba087b to 7811df8 Compare January 16, 2017 15:23

juanvallejo force-pushed the jvallejo/update-oc-status-warning branch 5 times, most recently from c129637 to 03de650 Compare January 18, 2017 20:21

juanvallejo force-pushed the jvallejo/update-oc-status-warning branch from 03de650 to 62299d5 Compare January 20, 2017 15:43

juanvallejo force-pushed the jvallejo/update-oc-status-warning branch from 62299d5 to 870d269 Compare January 23, 2017 19:00

juanvallejo force-pushed the jvallejo/update-oc-status-warning branch from 870d269 to b1b51fa Compare January 24, 2017 16:15

juanvallejo force-pushed the jvallejo/update-oc-status-warning branch from b1b51fa to 9cfc359 Compare January 24, 2017 16:18

openshift-bot merged commit 7708f39 into openshift:master Jan 25, 2017

juanvallejo deleted the jvallejo/update-oc-status-warning branch January 25, 2017 15:07

danwinship mentioned this pull request Jan 25, 2017

'oc status' expecting success and text 'You don't have any projects... #12667

Closed

smarterclayton mentioned this pull request Jan 26, 2017

Revert "Update "no projects" warning in oc status" #12671

Merged

juanvallejo mentioned this pull request Jan 26, 2017

Revert #3320b79, make sure all projects are deleted on test start #12668

Merged

juanvallejo added a commit to juanvallejo/origin that referenced this pull request Jan 26, 2017

restore changes from openshift#12328

65a57f0

juanvallejo added a commit to juanvallejo/origin that referenced this pull request Jan 26, 2017

restore changes from openshift#12328

527618e

juanvallejo added a commit to juanvallejo/origin that referenced this pull request Jan 26, 2017

restore changes from openshift#12328

4442a04

juanvallejo added a commit to juanvallejo/origin that referenced this pull request Jan 26, 2017

restore changes from openshift#12328

7dfb2d6

juanvallejo mentioned this pull request Feb 1, 2017

Running oc status with no projects shouldn't return error #8899

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update "no projects" warning in `oc status` #12328

Update "no projects" warning in `oc status` #12328

juanvallejo commented Dec 22, 2016

juanvallejo commented Dec 22, 2016

smarterclayton commented Dec 22, 2016

juanvallejo commented Dec 22, 2016

juanvallejo commented Dec 22, 2016

smarterclayton commented Dec 22, 2016

juanvallejo commented Jan 11, 2017 •

edited

Loading

smarterclayton Jan 11, 2017

juanvallejo Jan 11, 2017

smarterclayton Jan 11, 2017

juanvallejo commented Jan 11, 2017

juanvallejo commented Jan 12, 2017

smarterclayton Jan 13, 2017

juanvallejo commented Jan 13, 2017

juanvallejo commented Jan 16, 2017

juanvallejo commented Jan 16, 2017

juanvallejo commented Jan 18, 2017

juanvallejo commented Jan 19, 2017

fabianofranz commented Jan 20, 2017

juanvallejo commented Jan 20, 2017

fabianofranz commented Jan 20, 2017

juanvallejo commented Jan 23, 2017

fabianofranz commented Jan 24, 2017

fabianofranz commented Jan 24, 2017

fabianofranz commented Jan 24, 2017

juanvallejo commented Jan 24, 2017

openshift-bot commented Jan 24, 2017

openshift-bot commented Jan 24, 2017

fabianofranz commented Jan 24, 2017

openshift-bot commented Jan 24, 2017

openshift-bot commented Jan 25, 2017 •

edited

Loading

smarterclayton commented Jan 26, 2017

smarterclayton commented Jan 26, 2017

Update "no projects" warning in oc status #12328

Update "no projects" warning in oc status #12328

Conversation

juanvallejo commented Dec 22, 2016

juanvallejo commented Dec 22, 2016

smarterclayton commented Dec 22, 2016

juanvallejo commented Dec 22, 2016

juanvallejo commented Dec 22, 2016

smarterclayton commented Dec 22, 2016

juanvallejo commented Jan 11, 2017 • edited Loading

smarterclayton Jan 11, 2017

Choose a reason for hiding this comment

juanvallejo Jan 11, 2017

Choose a reason for hiding this comment

smarterclayton Jan 11, 2017

Choose a reason for hiding this comment

juanvallejo commented Jan 11, 2017

juanvallejo commented Jan 12, 2017

smarterclayton Jan 13, 2017

Choose a reason for hiding this comment

juanvallejo commented Jan 13, 2017

juanvallejo commented Jan 16, 2017

juanvallejo commented Jan 16, 2017

juanvallejo commented Jan 18, 2017

juanvallejo commented Jan 19, 2017

fabianofranz commented Jan 20, 2017

juanvallejo commented Jan 20, 2017

fabianofranz commented Jan 20, 2017

juanvallejo commented Jan 23, 2017

fabianofranz commented Jan 24, 2017

fabianofranz commented Jan 24, 2017

fabianofranz commented Jan 24, 2017

juanvallejo commented Jan 24, 2017

openshift-bot commented Jan 24, 2017

openshift-bot commented Jan 24, 2017

fabianofranz commented Jan 24, 2017

openshift-bot commented Jan 24, 2017

openshift-bot commented Jan 25, 2017 • edited Loading

smarterclayton commented Jan 26, 2017

smarterclayton commented Jan 26, 2017

Update "no projects" warning in `oc status` #12328

Update "no projects" warning in `oc status` #12328

juanvallejo commented Jan 11, 2017 •

edited

Loading

openshift-bot commented Jan 25, 2017 •

edited

Loading