Skip to content

add oc create policybinding #8428

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

Merged
merged 1 commit into from
Apr 27, 2016

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Apr 8, 2016

Adds

Usage:
  oc create policybinding TARGET_POLICY_NAMESPACE [options]

Examples:
  # Create a policy binding in namespace "foo" that references the policy in namespace "bar"
  $ oc create policybinding bar -n foo

This will allow a cluster-admin to easily create a policybinding object to allow a project admin to bind roles that are not clusterroles.

@pweil- @miheer @nhr Each of you asked about this in the past 24 hours. This makes it easier to follow the directions here https://docs.openshift.com/enterprise/3.1/architecture/additional_concepts/authorization.html#roles that describe the policybinding interplay, "If you find that these roles do not suit you, a cluster-admin user can create a policyBinding object named <projectname>:default with the CLI using a JSON file. This allows the project admin to bind users to roles that are defined only in the <projectname> local policy."

@deads2k
Copy link
Contributor Author

deads2k commented Apr 8, 2016

For those not wanting to run with --loglevel=8, the json looks like:

{  
    "kind":"PolicyBinding",
    "apiVersion":"v1",
    "metadata":{  
        "name":"foo:default",
        "creationTimestamp":null
    },
    "lastModified":null,
    "policyRef":{  
        "namespace":"foo",
        "name":"default"
    },
    "roleBindings":[  

    ]
}

@deads2k
Copy link
Contributor Author

deads2k commented Apr 8, 2016

[test]

@deads2k deads2k force-pushed the create-policy-binding branch from 89e3c74 to 38f5b39 Compare April 14, 2016 19:18
@deads2k
Copy link
Contributor Author

deads2k commented Apr 14, 2016

@pweil- @miheer do you think this is worth having?

@pweil-
Copy link

pweil- commented Apr 14, 2016

yes, I think it would help bypass some of the confusion of creating a binding with pure json and trying to figure out the naming quirks.

@deads2k deads2k added this to the 1.2.x milestone Apr 14, 2016
@deads2k
Copy link
Contributor Author

deads2k commented Apr 14, 2016

@stevekuznetsov ptal.

return err
}

if useShortOutput := o.OutputFormat == "name"; useShortOutput || len(o.OutputFormat) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this is idiomatic to Kubernetes, but man does it take a while to parse.

@deads2k deads2k force-pushed the create-policy-binding branch 2 times, most recently from 1b11ad3 to 233f43b Compare April 15, 2016 19:37
@deads2k
Copy link
Contributor Author

deads2k commented Apr 19, 2016

@stevekuznetsov anything else?

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2016
@stevekuznetsov
Copy link
Contributor

@deads2k, no, looks pretty straightforward. Seems like a lot of boilerplate, but I'm not sure we want to invent something to do a generic oc create

Example: fmt.Sprintf(policyBindingExample, fullName),
Run: func(cmd *cobra.Command, args []string) {
cmdutil.CheckErr(o.Complete(cmd, f, args))
cmdutil.CheckErr(o.CreatePolicyBinding())
Copy link
Contributor

Choose a reason for hiding this comment

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

We should follow k8s guidelines in all oc commands, especially the new ones, so please Complete, Validate and Run :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should follow k8s guidelines in all oc commands, especially the new ones, so please Complete, Validate and Run :)

I should've known that pattern would bite me some day.

Copy link
Contributor

Choose a reason for hiding this comment

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

We all ❤️ patterns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should follow k8s guidelines in all oc commands, especially the new ones, so please Complete, Validate and Run :)

Done.

@deads2k deads2k force-pushed the create-policy-binding branch from 233f43b to aa6a7b4 Compare April 20, 2016 11:57
@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2016
@deads2k
Copy link
Contributor Author

deads2k commented Apr 20, 2016

marking based on @stevekuznetsov's ok.

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2016
Run: func(cmd *cobra.Command, args []string) {
cmdutil.CheckErr(o.Complete(cmd, f, args))
cmdutil.CheckErr(o.Validate())
cmdutil.CheckErr(o.CreatePolicyBinding())
Copy link
Contributor

Choose a reason for hiding this comment

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

That's still not quite what should be here: Complete, Validate are ok, but the 3rd is Run. 😍

Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, I almost like the non-Run function names, at least Run<thing> as when they're all named Run and I want to tell where it's called, I have no way of doing that (go-oracle nonwithstanding). At least with Run<thing> it's grep-able

Copy link
Contributor

Choose a reason for hiding this comment

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

@stevekuznetsov Run<thing> is the suggested approach in the guidelines ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

@soltysh ah. lol.

I'm amazed they do:

      if err := opts.Complete(f, cmd, args, out); err != nil {
        cmdutil.CheckErr(err)
      }

since cmdutil.CheckErr handles the nil case itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stevekuznetsov yeah, they have still a lot learn from us 😉

@deads2k deads2k force-pushed the create-policy-binding branch from aa6a7b4 to a759435 Compare April 20, 2016 12:44
@soltysh
Copy link
Contributor

soltysh commented Apr 20, 2016

@deads2k thank you!

@deads2k
Copy link
Contributor Author

deads2k commented Apr 20, 2016

@soltysh I like the pattern. First instance of it in the kube or origin repo in a 2014 commit: https://github.com/kubernetes/kubernetes/pull/3275/files#diff-ac0b208737cc45e7d98468252e3097d5R103 . (note the author) :)

I have come around to unique names.

@stevekuznetsov
Copy link
Contributor

@deads2k did we ever come to some consensus on "foo-bar" naming in examples? The rest of the oc create stubs use my-<thing>:

  oc create secret generic my-secret
  oc create namespace my-namespace
  oc create serviceaccount my-service-account

@deads2k
Copy link
Contributor Author

deads2k commented Apr 20, 2016

@deads2k did we ever come to some consensus on "foo-bar" naming in examples? The rest of the oc create stubs use my-:

this one is special. You aren't allow to choose a name of a policybinding.

@stevekuznetsov
Copy link
Contributor

@deads2k right, but you can chose the name of a namespace, as in your example:

  # Create a policy binding in namespace "foo" that references the policy in namespace "bar"
  $ oc create policybinding bar -n foo

Could be more read-able with something like:

  # Create a policy binding in namespace "my-namespace" that references the policy in namespace "donor-namespace"
  $ oc create policybinding donor-namespace -n my-namespace

@deads2k
Copy link
Contributor Author

deads2k commented Apr 20, 2016

re[test]

1 similar comment
@stevekuznetsov
Copy link
Contributor

re[test]

@deads2k deads2k force-pushed the create-policy-binding branch from a759435 to 3dd1f51 Compare April 26, 2016 13:05
@deads2k deads2k force-pushed the create-policy-binding branch from 3dd1f51 to 222bff8 Compare April 26, 2016 14:47
@deads2k
Copy link
Contributor Author

deads2k commented Apr 26, 2016

[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 26, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3306/) (Image: devenv-rhel7_4047)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 222bff8

@smarterclayton
Copy link
Contributor

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 222bff8

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3306/)

@openshift-bot openshift-bot merged commit b016f07 into openshift:master Apr 27, 2016
@deads2k deads2k deleted the create-policy-binding branch September 6, 2016 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants