Skip to content

Comments

Bug 1388415, added clarification around cluster roles#3178

Merged
ahardin-rh merged 1 commit intoopenshift:masterfrom
ahardin-rh:cluster-role
Jan 5, 2017
Merged

Bug 1388415, added clarification around cluster roles#3178
ahardin-rh merged 1 commit intoopenshift:masterfrom
ahardin-rh:cluster-role

Conversation

@ahardin-rh
Copy link
Contributor

@ahardin-rh ahardin-rh added this to the Next Release milestone Nov 8, 2016
@ahardin-rh ahardin-rh self-assigned this Nov 8, 2016
@ahardin-rh ahardin-rh changed the title [WIP]Bug 1388415, added calrification around cluster roles [WIP]Bug 1388415, added clarification around cluster roles Nov 10, 2016
@ahardin-rh ahardin-rh force-pushed the cluster-role branch 3 times, most recently from b88124e to 0b870cb Compare November 29, 2016 21:22
@ahardin-rh
Copy link
Contributor Author

@benjaminapetersen PTAL at my first draft

@benjaminapetersen
Copy link

Checking, pinging @enj for a look as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe xref to authorization.roles (did this get moved somewhere else)?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the wrong approach. Copying an existing role is fine when you are testing things locally, but does not make sense when you actually intend to use that role. The user needs to build it from scratch so that they actually understand the permissions they are about to give out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@enj Okay, thanks. What are the steps to do that?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, lots of work...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@enj do you think its necessary to do a start from scratch example here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want documentation on how to create a custom role, then it really should start from scratch. Whether you want such documentation is up to you. Specifying all the valid options for each field would be a pain, but that is the only way the doc would actually be useful.

An example role with its important fields filled out:

apiVersion: v1
kind: Role
metadata:
  name: role_name
rules:
- apiGroups: null
  attributeRestrictions:
    apiVersion: v1
    kind: IsPersonalSubjectAccessReview
  resources:
  - localsubjectaccessreviews
  - subjectaccessreviews
  verbs:
  - create

The full Go specification:

// Role is a logical grouping of PolicyRules that can be referenced as a unit by RoleBindings.
type Role struct {
	unversioned.TypeMeta `json:",inline"`
	// Standard object's metadata.
	kapi.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`

	// Rules holds all the PolicyRules for this Role
	Rules []PolicyRule `json:"rules" protobuf:"bytes,2,rep,name=rules"`
}

// PolicyRule holds information that describes a policy rule, but does not contain information
// about who the rule applies to or which namespace the rule applies to.
type PolicyRule struct {
	// Verbs is a list of Verbs that apply to ALL the ResourceKinds and AttributeRestrictions contained in this rule.  VerbAll represents all kinds.
	Verbs []string `json:"verbs" protobuf:"bytes,1,rep,name=verbs"`
	// AttributeRestrictions will vary depending on what the Authorizer/AuthorizationAttributeBuilder pair supports.
	// If the Authorizer does not recognize how to handle the AttributeRestrictions, the Authorizer should report an error.
	AttributeRestrictions kruntime.RawExtension `json:"attributeRestrictions,omitempty" protobuf:"bytes,2,opt,name=attributeRestrictions"`
	// APIGroups is the name of the APIGroup that contains the resources.  If this field is empty, then both kubernetes and origin API groups are assumed.
	// That means that if an action is requested against one of the enumerated resources in either the kubernetes or the origin API group, the request
	// will be allowed
	APIGroups []string `json:"apiGroups" protobuf:"bytes,3,rep,name=apiGroups"`
	// Resources is a list of resources this rule applies to.  ResourceAll represents all resources.
	Resources []string `json:"resources" protobuf:"bytes,4,rep,name=resources"`
	// ResourceNames is an optional white list of names that the rule applies to.  An empty set means that everything is allowed.
	ResourceNames []string `json:"resourceNames,omitempty" protobuf:"bytes,5,rep,name=resourceNames"`
	// NonResourceURLsSlice is a set of partial urls that a user should have access to.  *s are allowed, but only as the full, final step in the path
	// This name is intentionally different than the internal type so that the DefaultConvert works nicely and because the ordering may be different.
	NonResourceURLsSlice []string `json:"nonResourceURLs,omitempty" protobuf:"bytes,6,rep,name=nonResourceURLs"`
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@enj this was sourced from discussion in the BZ. I can remove this.

@ahardin-rh
Copy link
Contributor Author

@enj @benjaminapetersen Updated. PTAL and let me know if we're getting closer. Thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps "We recommend you build it from scratch rather than copying an existing role".

@enj?

@ahardin-rh ahardin-rh changed the title [WIP]Bug 1388415, added clarification around cluster roles Bug 1388415, added clarification around cluster roles Dec 9, 2016
@ahardin-rh
Copy link
Contributor Author

@enj @benjaminapetersen Updated. PTAL.

@benjaminapetersen
Copy link

@ahardin-rh will do, sorry for the delay, I've been OOO. Catching up soon.

@ahardin-rh
Copy link
Contributor Author

@benjaminapetersen @enj PTAL. Thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just built the docs so I could read them in context.

How often does the original Go source code end up in the docs? I'm thinking @enj posted that for our reference/discussion, but we should prob not include here. Reading it over and thinking a bit.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently OOO. But yes, I never intended for the source code to be included. It was mostly there as a reference so that whatever YAML we included was thorough and correct.

@ahardin-rh
Copy link
Contributor Author

@benjaminapetersen @enj Thank you! This is updated.

@benjaminapetersen
Copy link

Building again to check it out

@benjaminapetersen
Copy link

@ahardin-rh Had a few thoughts, made a PR to your branch a minute ago.

@ahardin-rh ahardin-rh force-pushed the cluster-role branch 2 times, most recently from f127f14 to 3b91236 Compare December 21, 2016 23:07
@ahardin-rh
Copy link
Contributor Author

@benjaminapetersen Your updates and my edits are applied. PTAL. Thanks!

Copy link

@xiaocwan xiaocwan Dec 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is a typo "clusterole", should be "clusterrole"

Copy link

@xiaocwan xiaocwan Dec 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be more or less confused to user, could add one comment here like "# To use current project:", or could combine this comand together with next line oc create -f path/to/localrole_exampleview.yaml -n <project_you_want_to_add_the_local_role_exampleview_to> .

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also prefer a comment to explain this command here as above.

Copy link

@xiaocwan xiaocwan Dec 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be "clusterrole_view.yaml" as above "> clusterrole_view.yaml"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thumbs up, not thumbs down 👍 😄

@benjaminapetersen
Copy link

@jwforres fyi

@ahardin-rh
Copy link
Contributor Author

@xiaocwan Many thanks. This is now updated. PTAL and let me know if I missed anything.

@xiaocwan
Copy link

xiaocwan commented Jan 5, 2017

@ahardin-rh LGTM now, thanks.

@ahardin-rh ahardin-rh merged commit 4b4c644 into openshift:master Jan 5, 2017
@ahardin-rh
Copy link
Contributor Author

[rev_history]
|xref:../admin_guide/manage_authorization_policy.adoc#admin-guide-manage-authorization-policy[Managing Authorization Policies]
|Added clarifying details about cluster roles.
%
|xref:../architecture/additional_concepts/authorization.adoc#architecture-additional-concepts-authorization[Additional Concepts -> Authorization]
|Added clarifying details about cluster roles.
%

@gaurav-nelson gaurav-nelson modified the milestones: Next Release, Staging, Published - 09/01/17 Jan 8, 2017
@ahardin-rh ahardin-rh deleted the cluster-role branch November 9, 2017 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants