Add annotations to roles by enj · Pull Request #11328 · openshift/origin

enj · 2016-10-11T19:26:15Z

Added just the basic bits; now someone tell me what they want to call the annotation... (along with display names and descriptions)

Signed-off-by: Monis Khan mkhan@redhat.com

cc @benjaminapetersen @jwforres @openshift/api-review

deads2k · 2016-10-11T19:27:56Z

pkg/cmd/server/bootstrappolicy/policy.go

 				Name: AdminRoleName,
+				Annotations: map[string]string{
+					RoleDescription: "A project manager. If used in a local binding, an admin user will have rights to view any resource in the project and modify any resource in the project except for role creation and quota. If the cluster-admin wants to allow an admin to modify roles, the cluster-admin must create a project-scoped Policy object using JSON.",
+					RoleSomething:   RoleSomethingVal,


This seems backwards. Since most people will be adding roles that they want users to use, why wouldn't we have an explicit opt-in for exclusion.

deads2k · 2016-10-11T19:28:28Z

pkg/cmd/server/bootstrappolicy/policy.go

+	RoleDisplayName = "openshift.io/display-name"
+
+	// RoleDescription is an annotation that holds the description of the role
+	RoleDescription = "openshift.io/description"


promote the description and display name to a shared package. These cross all api groups.

deads2k · 2016-10-11T19:29:13Z

pkg/cmd/server/bootstrappolicy/policy.go

 				Name: DeployerRoleName,
+				Annotations: map[string]string{
+					RoleDisplayName: "deployer",
+					RoleSomething:   RoleSomethingVal,


Why did you expose this?

deads2k · 2016-10-11T19:29:38Z

pkg/cmd/server/bootstrappolicy/policy.go

 				Name: ImageBuilderRoleName,
+				Annotations: map[string]string{
+					RoleDisplayName: "image-builder",
+					RoleSomething:   RoleSomethingVal,


Why did you expose this?

enj · 2016-10-11T22:59:28Z

@deads2k I exposed those SA-ish roles because there was some talk about making SAs (which may then need these roles to do something). Based on info from @benjaminapetersen and some pruning I have narrowed the data to 8 roles:

$ oc get clusterrole -o jsonpath="{range .items[*]}{.metadata.name}{\" => \"}{.metadata.annotations}{\"\\n\"}{end}" | grep -v 'system-only:true'

system:image-puller => map[openshift.io/display-name:image-puller]
edit => map[openshift.io/description:A user that can modify most objects in a project, but does not have the power to view or modify roles or bindings.]
system:image-builder => map[openshift.io/display-name:image-builder]
basic-user => map[openshift.io/description:A user that can get basic information about projects and users.]
system:image-pusher => map[openshift.io/display-name:image-pusher]
view => map[openshift.io/description:A user who cannot make any modifications, but can see most objects in a project. They cannot view or modify roles or bindings.]
system:deployer => map[openshift.io/display-name:deployer]
admin => map[openshift.io/description:A project manager. If used in a local binding, an admin user will have rights to view any resource in the project and modify any resource in the project except for role creation and quota. If the cluster-admin wants to allow an admin to modify roles, the cluster-admin must create a project-scoped Policy object using JSON.]

display-name usage on 4 SA-ish roles:

$ oc get clusterrole -o jsonpath="{range .items[*]}{.metadata.name}{\" => \"}{.metadata.annotations}{\"\\n\"}{end}" | grep 'display-name'

system:deployer => map[openshift.io/display-name:deployer]
system:image-pusher => map[openshift.io/display-name:image-pusher]
system:image-puller => map[openshift.io/display-name:image-puller]
system:image-builder => map[openshift.io/display-name:image-builder]

The 7 roles with description:

$ oc get clusterrole -o jsonpath="{range .items[*]}{.metadata.name}{\" => \"}{.metadata.annotations}{\"\\n\"}{end}" | grep 'description'

view => map[openshift.io/description:A user who cannot make any modifications, but can see most objects in a project. They cannot view or modify roles or bindings.]
cluster-status => map[authorization.openshift.io/system-only:true openshift.io/description:A user that can get basic cluster status information.]
edit => map[openshift.io/description:A user that can modify most objects in a project, but does not have the power to view or modify roles or bindings.]
cluster-admin => map[authorization.openshift.io/system-only:true openshift.io/description:A super-user that can perform any action in any project. When granted to a user within a local policy, they have full control over quota and roles and every action on every resource in the project.]
basic-user => map[openshift.io/description:A user that can get basic information about projects and users.]
admin => map[openshift.io/description:A project manager. If used in a local binding, an admin user will have rights to view any resource in the project and modify any resource in the project except for role creation and quota. If the cluster-admin wants to allow an admin to modify roles, the cluster-admin must create a project-scoped Policy object using JSON.]
self-provisioner => map[authorization.openshift.io/system-only:true openshift.io/description:A user that can create their own projects.]

enj · 2016-10-11T23:02:10Z

[test]

smarterclayton · 2016-10-12T03:38:53Z

We already have annotations for these and they are in the k8s.io namespace.

On Oct 11, 2016, at 3:28 PM, David Eads notifications@github.com wrote:

@deads2k commented on this pull request.

In pkg/cmd/server/bootstrappolicy/policy.go
#11328 (review):

@@ -28,6 +28,19 @@ import (
userapi "github.com/openshift/origin/pkg/user/api"
)

+const (

// RoleSomething is an annotation that ???
RoleSomething = "authorization.openshift.io/user-friendly"
// Do something ???
RoleSomethingVal = "true"

// RoleDisplayName is an annotation that stores the name displayed
when querying for roles
RoleDisplayName = "openshift.io/display-name"

// RoleDescription is an annotation that holds the description of the role
RoleDescription = "openshift.io/description"

promote the description and display name to a shared package. These cross
all api groups.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#11328 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_pzHU9EOd5UobN-wk_3MbUg8f56zkks5qy-NegaJpZM4KUAkv
.

enj · 2016-10-12T14:31:38Z

We want to use k8s.io/display-name and k8s.io/description instead of the openshift equivalents (which we already use in various places)? What about authorization.openshift.io/system-only?

deads2k · 2016-10-12T14:35:05Z

We want to use k8s.io/display-name and k8s.io/description instead of the openshift equivalents (which we already use in various places)? What about authorization.openshift.io/system-only?

I haven't actually seen these annotations upstream and my grep-fu can't find them. @smarterclayton can I get a pointer to them?

liggitt · 2016-10-19T15:40:38Z

why do we we want display names on roles? I don't think it's worth the confusion (and possible accidental granting of unintentional powers when a UI is involved) around whether you are dealing with a display name or the actual role name

benjaminapetersen · 2016-10-19T15:48:38Z

I was messing around with supporting these edge cases via:

ClusterRole: view
Role: view // copied view, but slightly different

Both can exist, but it is confusing. Hopefully the descriptions are clear, but if no descriptions we have a problem. My personal preference would be that roles cannot have the same name as another role, whatever the scope, and don't both with display names. Is there a good reason to allow?

liggitt · 2016-10-19T16:01:37Z

it's impossible to enforce unique names in different scopes

liggitt · 2016-10-19T16:03:06Z

I would probably represent local roles as <projectname>/<rolename>. Roles cannot contain / characters, and <projectname>/<rolename> is how the CLI describer prints rolerefs in rolebindings

benjaminapetersen · 2016-10-19T18:42:39Z

Ok great, I'll run with that.

benjaminapetersen · 2017-01-06T17:05:19Z

@openshift/api-review @enj

So it's been requested we aim for getting these in for 3.5. It looks like we have a few outstanding questions that need answering at this point:

We need to confirm the name for this annotation. Is authorization.openshift.io/system-only : true sufficient? @smarterclayton mentioned something may exist in k8s.io, and it isn't exactly clear (may not truly be system-only).
Confirm the direction of the above annotation. @deads2k suggested adding the annotation to all built-in roles as an exclude, so that any user defined role would be "shown" by default (even if the user does not add the annotation).
Which annotation to use for descriptions openshift.io/description vs k8s.io/description. We probably need descriptions all roles a user will see by default (not just the 7-8).
We should verify the default visible roles.
Get rid of display name.

benjaminapetersen · 2017-01-06T17:05:49Z

@jwforres fyi above comment

liggitt · 2017-01-10T20:47:50Z

@deads2k suggested adding the annotation to all built-in roles as an exclude, so that any user defined role would be "shown" by default (even if the user does not add the annotation).

seems like a good idea. if we add annotations to the default roles (for descriptions or a "hide-by-default" flag), we need to make role reconciliation annotation-aware. currently, it preserves all annotations from an existing role object... merging them will require thought.

Which annotation to use for descriptions openshift.io/description vs k8s.io/description

For openshift objects, I would use the openshift annotation

Get rid of display name.

agree

benjaminapetersen · 2017-01-11T20:42:11Z

I don't think I have a dog in the fight re: role reconciliation, care to comment @enj @deads2k ?

deads2k · 2017-01-11T20:56:15Z

@enj is this waiting on me for something?

benjaminapetersen · 2017-01-16T18:40:43Z

@enj / any who are interested, how about openshift.io/internal for the show/hide flag? It seems "system-only" is not exactly truthful. "Internal" is a rather ambiguous word, but that might be good here. The point is simply not to show it, not to describe the reason not to show it (which there could be many).

enj · 2017-01-17T14:29:24Z

@liggitt so oadm policy reconcile-cluster-roles does not handle roles in pkg/cmd/server/bootstrappolicy/infra_sa_policy.go. Not sure how to deal with that.

Edit: NVM those get force updated.

liggitt · 2017-01-17T16:36:30Z

pkg/api/constants/constants.go

@@ -0,0 +1,10 @@
+package constants


not sure we want a package for constants... annotation keys typically go in the API package directly

I believe I did that to resolve a circular dependency (but that was a while ago).

liggitt · 2017-01-17T16:41:09Z

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

@@ -131,6 +131,9 @@ func init() {
 		authorizationapi.ClusterRole{
 			ObjectMeta: kapi.ObjectMeta{
 				Name: BuildControllerRoleName,


add these annotations in addServiceAccount? aren't all service account roles infra only?

liggitt · 2017-01-17T16:42:13Z

pkg/cmd/server/bootstrappolicy/policy.go

 			ObjectMeta: kapi.ObjectMeta{
 				Name: ClusterAdminRoleName,
+				Annotations: map[string]string{
+					constants.OpenShiftDescription: "A super-user that can perform any action in any project. When granted to a user within a local policy, they have full control over quota and roles and every action on every resource in the project.",


"...can perform any action in the cluster."?

liggitt · 2017-01-17T16:43:17Z

pkg/cmd/server/bootstrappolicy/policy.go

 			ObjectMeta: kapi.ObjectMeta{
 				Name: AdminRoleName,
+				Annotations: map[string]string{
+					constants.OpenShiftDescription: "A project manager. If used in a local binding, an admin user will have rights to view any resource in the project and modify any resource in the project except for role creation and quota. If the cluster-admin wants to allow an admin to modify roles, the cluster-admin must create a project-scoped Policy object using JSON."},


remove "using JSON"

liggitt · 2017-01-17T16:43:51Z

pkg/cmd/server/bootstrappolicy/policy.go

 			ObjectMeta: kapi.ObjectMeta{
 				Name: ViewRoleName,
+				Annotations: map[string]string{
+					constants.OpenShiftDescription: "A user who cannot make any modifications, but can see most objects in a project. They cannot view or modify roles or bindings."},


call out that they can't see secrets

liggitt · 2017-01-17T16:44:22Z

pkg/cmd/server/bootstrappolicy/policy.go

 			ObjectMeta: kapi.ObjectMeta{
 				Name: BasicUserRoleName,
+				Annotations: map[string]string{
+					constants.OpenShiftDescription: "A user that can get basic information about projects and users."},


this allows listing projects and getting information about yourself, not users in general

liggitt · 2017-01-17T16:44:50Z

pkg/cmd/server/bootstrappolicy/policy.go

 			ObjectMeta: kapi.ObjectMeta{
 				Name: SelfProvisionerRoleName,
+				Annotations: map[string]string{
+					constants.OpenShiftDescription: "A user that can create their own projects.",


they can't create their own, they can request projects

enj · 2017-01-17T16:46:31Z

@liggitt FYI I copied the descriptions verbatim from here so those probably need fixing as well.

enj · 2017-01-17T20:33:56Z

@liggitt All requested changes have been addressed. @jwforres / @benjaminapetersen Let me know if you guys have any comments on the system-only / description annotations since we do not get to change those later.

jwforres

I'm not seeing the roles like system:deployer system:image-puller, system:image-pusher, and system:image-builder. Is there a reason we don't have descriptions on those?

jwforres · 2017-01-17T20:47:56Z

pkg/cmd/server/bootstrappolicy/policy.go

 			ObjectMeta: kapi.ObjectMeta{
 				Name: AdminRoleName,
+				Annotations: map[string]string{
+					oapi.OpenShiftDescription: "A project manager. If used in a local binding, an admin user will have rights to view any resource in the project and modify any resource in the project except for role creation and quota. If the cluster-admin wants to allow an admin to modify roles, the cluster-admin must create a project-scoped Policy object."},


I don't think we want these getting too technical, these descriptions are for end users deciding why they want to choose one role vs another. What if we simplify this whole thing down to:

"A user that has edit rights within the project and can change the project's membership."

Maybe contentious since membership is a console term?

Eh I have heard of membership but I assume it means roles + bindings => policy?

jwforres · 2017-01-17T20:55:10Z

pkg/cmd/server/bootstrappolicy/policy.go

 			ObjectMeta: kapi.ObjectMeta{
 				Name: EditRoleName,
+				Annotations: map[string]string{
+					oapi.OpenShiftDescription: "A user that can modify most objects in a project, but does not have the power to view or modify roles or bindings."},


A user that can create and edit most objects in a project, but can not update the project's membership.

jwforres · 2017-01-17T20:57:23Z

pkg/cmd/server/bootstrappolicy/policy.go

 			ObjectMeta: kapi.ObjectMeta{
 				Name: ViewRoleName,
+				Annotations: map[string]string{
+					oapi.OpenShiftDescription: "A user who cannot make any modifications, but can see most objects in a project. They cannot view or modify roles or bindings or secrets."},


A user who can view but not edit any resources within the project. They can not view secrets.

Isn't the role part important?

jwforres · 2017-01-17T20:58:11Z

pkg/cmd/server/bootstrappolicy/policy.go

 			ObjectMeta: kapi.ObjectMeta{
 				Name: BasicUserRoleName,
+				Annotations: map[string]string{
+					oapi.OpenShiftDescription: "A user that can get basic information about projects."},


what does this even mean? they can Get/List the project but not get any objects within the project?

So we have to be vague enough that the description doesn't get too far out of sync since the specifics of rules will almost certainly change over time. Right now it means:

Get info about yourself

List projectrequests

Get/List clusterroles

List storageclasses

List/Watch projects

jwforres · 2017-01-17T21:00:40Z

pkg/cmd/server/bootstrappolicy/policy.go

 		{
 			ObjectMeta: kapi.ObjectMeta{
 				Name: ClusterAdminRoleName,
+				Annotations: map[string]string{


instead of saying "granted to a user within a local policy" can you say "granted to a user on a project"

Maybe "within a project"?

enj · 2017-01-17T22:32:49Z

I'm not seeing the roles like system:deployer system:image-puller, system:image-pusher, and system:image-builder. Is there a reason we don't have descriptions on those?

@jwforres I copied the descriptions verbatim from here and those were not listed (nor did I have any idea what to describe them as). Maybe @liggitt can provide some?

@liggitt I need your input on @jwforres' suggestions so I can finalize the descriptions.

Signed-off-by: Monis Khan <mkhan@redhat.com>

enj · 2017-01-18T21:02:16Z

@liggitt This is ready for merge.

liggitt · 2017-01-18T21:13:22Z

[merge]

openshift-bot · 2017-01-18T21:21:24Z

Evaluated for origin merge up to d89c5cd

openshift-bot · 2017-01-18T21:53:22Z

Evaluated for origin test up to d89c5cd

enj · 2017-01-18T22:24:47Z

@liggitt sanity check confirmed - reconciliation works as expected.

openshift-bot · 2017-01-18T23:21:29Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13002/) (Base Commit: cf4e84e)

enj · 2017-01-18T23:25:14Z

Flake on #12540 #8571

openshift-bot · 2017-01-19T04:45:31Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13013/) (Base Commit: e4b43ee) (Image: devenv-rhel7_5718)

@jwforres

…le-whitelist Automatic merge from submit-queue. Update membership filter to use MEMBERSHIP_WHITELIST in Constants.js Moving back to using a simple whitelist via [origin issue 16862](openshift/origin#16862) fixes [origin issue 16862](openshift/origin#16862) History: - issue #14411 - PR #14510, - PR #15241 - [PR 11328](openshift/origin#11328) (original) At this point ignoring the annotation `systemOnly` entirely. @jwforres @spadgett @enj

deads2k reviewed Oct 11, 2016

View reviewed changes

This was referenced Oct 24, 2016

Project Membership: adjust initial role filter openshift/origin-web-console#709

Closed

Update filter to immitate future annotation behavior openshift/origin-web-console#712

Merged

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 30, 2016

benjaminapetersen mentioned this pull request Nov 3, 2016

Membership: temporarily eliminate default description tooltip for roles openshift/origin-web-console#813

Merged

enj force-pushed the enj/f/role_annotations/11268 branch from f65c297 to 1cecfd4 Compare January 16, 2017 16:35

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2017

enj requested a review from liggitt January 17, 2017 03:18

benjaminapetersen mentioned this pull request Jan 17, 2017

OpenShift 1.5/3.5 Release Notes Tracker openshift/openshift-docs#3404

Closed

liggitt reviewed Jan 17, 2017

View reviewed changes

jwforres reviewed Jan 17, 2017

View reviewed changes

Add annotations to roles.

d89c5cd

Signed-off-by: Monis Khan <mkhan@redhat.com>

enj force-pushed the enj/f/role_annotations/11268 branch from 6583b22 to d89c5cd Compare January 18, 2017 20:45

enj changed the title ~~WIP: Add annotations to roles~~ Add annotations to roles Jan 18, 2017

openshift-bot merged commit ba792d2 into openshift:master Jan 19, 2017

benjaminapetersen mentioned this pull request May 30, 2017

Influx of roles w/o systemOnly annotation #14411

Closed

benjaminapetersen mentioned this pull request Oct 30, 2017

Update membership filter to use MEMBERSHIP_WHITELIST in Constants.js openshift/origin-web-console#2402

Merged

Conversation

enj commented Oct 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enj commented Oct 11, 2016

Uh oh!

enj commented Oct 11, 2016

Uh oh!

smarterclayton commented Oct 12, 2016

@deads2k commented on this pull request.

Uh oh!

enj commented Oct 12, 2016

Uh oh!

deads2k commented Oct 12, 2016

Uh oh!

liggitt commented Oct 19, 2016

Uh oh!

benjaminapetersen commented Oct 19, 2016

Uh oh!

liggitt commented Oct 19, 2016

Uh oh!

liggitt commented Oct 19, 2016

Uh oh!

benjaminapetersen commented Oct 19, 2016

Uh oh!

benjaminapetersen commented Jan 6, 2017

Uh oh!

benjaminapetersen commented Jan 6, 2017

Uh oh!

liggitt commented Jan 10, 2017

Uh oh!

benjaminapetersen commented Jan 11, 2017

Uh oh!

deads2k commented Jan 11, 2017

Uh oh!

benjaminapetersen commented Jan 16, 2017

Uh oh!

enj commented Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enj commented Jan 17, 2017

Uh oh!

enj commented Jan 17, 2017

Uh oh!

jwforres left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enj commented Jan 17, 2017 •

edited

Loading

liggitt Jan 17, 2017 •

edited

Loading

openshift-bot commented Jan 19, 2017 •

edited

Loading