Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] oc create clusterrole: new command #11937

Closed

Conversation

php-coder
Copy link
Contributor

@php-coder php-coder commented Nov 16, 2016

Add a new command oc create clusterrole that takes role name, list of actions, list of resources and creates a new cluster role.

Example:

$ oc create clusterrole dockerbuilder --verbs create --resources builds/docker
clusterrole "dockerbuilder" created
$ oc get clusterrole dockerbuilder -o yaml
apiVersion: v1
kind: ClusterRole
metadata:
  creationTimestamp: 2016-11-16T18:06:28Z
  name: dockerbuilder
  resourceVersion: "42371"
  selfLink: /oapi/v1/clusterroles/dockerbuilder4
  uid: 65470a15-ac27-11e6-8811-080027242396
rules:
- apiGroups:
  - ""
  attributeRestrictions: null
  resources:
  - builds/docker
  verbs:
  - create

Fixes #3804

TODO:

  • rename to oc create clusterrole
  • validate the actions and resources
  • add --force option
  • don't assume "" api group
  • add example
  • PR with the equivalent command upstream for rbac clusterroles
  • add tests

@php-coder
Copy link
Contributor Author

php-coder commented Nov 16, 2016

@csrwng @deads2k Cesar, David, is this what you had in mind?


Name string
Resources []string
Actions []string
Copy link
Contributor

Choose a reason for hiding this comment

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

name the options and flags to match the API object fields? (verbs, resources, api-groups, etc)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I named it "actions" because such name were used in the original issue.

}

cmd.Flags().StringSliceVarP(&options.Resources, "resources", "", options.Resources, "list of resources (separated by comma)")
cmd.Flags().StringSliceVarP(&options.Actions, "actions", "", options.Actions, "list of actions (separated by comma)")
Copy link
Contributor

Choose a reason for hiding this comment

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

are we happy not ever allowing commas in verbs or resources?

Copy link
Contributor

Choose a reason for hiding this comment

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

are we happy not ever allowing commas in verbs or resources?

I'm fine with that.

}

func (o *NewClusterRoleOptions) CreateRole() error {
rule, err := authapi.NewRule(o.Actions...).Resources(o.Resources...).Groups("").Rule()
Copy link
Contributor

Choose a reason for hiding this comment

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

don't assume "" api group

Copy link
Contributor

Choose a reason for hiding this comment

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

we'll likely need to express distinct verb/resource pairings ("create a role that lets me get,list,watch pods and create,update,delete,list,watch replicasets")

Copy link
Contributor

Choose a reason for hiding this comment

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

we'll likely need to express distinct verb/resource pairings ("create a role that lets me get,list,watch pods and create,update,delete,list,watch replicasets")

I'd like to have set commands to handle that sort of thing as well. That would make the need to handle distinct tuples in a single command less important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not understand yet for what this field is used and which values it accepts. Will investigate it..

Commands: []*cobra.Command{
NewCmdCreateClusterRole(CreateClusterRoleRecommendedName, fullName+" "+CreateClusterRoleRecommendedName, f, out),
Copy link
Contributor

Choose a reason for hiding this comment

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

this should go under the create subtree, not the policy subtree

Copy link
Contributor

Choose a reason for hiding this comment

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

this should go under the create subtree, not the policy subtree

I agree. I'd also like to see a pull with the equivalent command upstream for rbac clusterroles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see "create" subtree in oadm command. I see only commands with prefix "create-" in the "Configuration" subtree. Did you mean this section?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's under oc create

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see "create" subtree in oadm command. I see only commands with prefix "create-" in the "Configuration" subtree. Did you mean this section?

oc create clusterrole is appropriate since its an API resource.

@csrwng
Copy link
Contributor

csrwng commented Nov 16, 2016

Based on comments in the original issue, it's also desirable to validate the actions and resources passed in. @deads2k is that possible?

@deads2k
Copy link
Contributor

deads2k commented Nov 16, 2016

Based on comments in the original issue, it's also desirable to validate the actions and resources passed in. @deads2k is that possible?

I'd like to see them linted, possibly with a --force for "I know what I'm doing, just make it!". All lowercase, known verbs, maybe matches discovery for some resource plus a whitelist. We want to avoid dumb typos.

@csrwng
Copy link
Contributor

csrwng commented Nov 16, 2016

Is there a place that we can get a list of known verbs or resources from?

@deads2k
Copy link
Contributor

deads2k commented Nov 16, 2016

Is there a place that we can get a list of known verbs or resources from?

For resources you can see the discovery doc. For verbs, you could start a "well-known" set upstream.

@liggitt
Copy link
Contributor

liggitt commented Nov 16, 2016

resources from discovery, if the server's up (though that won't include virtual resources like builds/jenkinspipeline or imagestreams/layers)

nothing in discovery for verbs yet (and even if there is eventually, it won't include verbs like impersonate and use)

@php-coder php-coder changed the title [WIP] oadm policy create-cluster-role: new command [WIP] oc create clusterrole: new command Nov 23, 2016
@openshift-bot 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 Feb 26, 2017
@openshift-bot
Copy link
Contributor

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2017
@php-coder
Copy link
Contributor Author

I'm closing this in favor of kubernetes/kubernetes#41538 that will be available after next rebase to the next version of Kubernetes.

@php-coder php-coder closed this Mar 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants