Skip to content

Add kubectl auth can-i --list option which could help users know what actions they can do in specific namespace #64820

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

Conversation

WanLinghao
Copy link
Contributor

@WanLinghao WanLinghao commented Jun 6, 2018

What this PR does / why we need it:
Since API SelfSubjectRulesReview has implemented, I think we need ctl support to make it
easier for users to know what actions they can do in specific namespace.
kubectl auth can-i --list --namespace=test-namespace
The output looks like:

Resources                                       Non-Resource URLs        Resource Names   Verbs
---------                                       -----------------        --------------   -----
*.*                                             []                       []               [*]
                                                [*]                      []               [*]
selfsubjectaccessreviews.authorization.k8s.io   []                       []               [create]
selfsubjectrulesreviews.authorization.k8s.io    []                       []               [create]
                                                [/api/*]                 []               [get]
                                                [/api]                   []               [get]
                                                [/apis/*]                []               [get]
                                                [/apis]                  []               [get]
                                                [/healthz]               []               [get]
                                                [/openapi/*]             []               [get]
                                                [/openapi]               []               [get]
                                                [/swagger-2.0.0.pb-v1]   []               [get]
                                                [/swagger.json]          []               [get]
                                                [/swaggerapi/*]          []               [get]
                                                [/swaggerapi]            []               [get]
                                                [/version/]              []               [get]
                                                [/version]               []               [get]

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #64821

Special notes for your reviewer:

Release note:

Add `kubectl auth can-i --list` option which could help users know what actions they can do in specific namespace.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 6, 2018
@k8s-ci-robot k8s-ci-robot requested review from adohe-zz and ghodss June 6, 2018 09:21
@liggitt
Copy link
Member

liggitt commented Jun 6, 2018

@kubernetes/sig-cli-pr-reviews @kubernetes/sig-auth-pr-reviews

@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Jun 6, 2018
@CaoShuFeng
Copy link
Contributor

/assign
/cc

@php-coder
Copy link
Contributor

I'd mention it in the release notes.

@WanLinghao WanLinghao force-pushed the ctl_selfsubjectrulesreview_support branch from b2f904b to fe25b71 Compare June 7, 2018 10:29
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 7, 2018
@WanLinghao
Copy link
Contributor Author

@php-coder Done

if _, err := fmt.Fprintf(out, "Resources\tNon-Resource URLs\tResource Names\tVerbs\n"); err != nil {
return err
}
if _, err := fmt.Fprintf(out, "---------\t-----------------\t--------------\t-----\n"); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

kubectl api-resources doesn't have such line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CaoShuFeng Since the output here is similar with RBAC clusterrole/role, maybe we can just keep same with them.
And the output of kubectl describe role xxxx is like:

  Resources                                       Non-Resource URLs  Resource Names  Verbs
  ---------                                       -----------------  --------------  -----
  serviceaccounts                                 []                 []              [create delete deletecollection get list patch update watch impersonate]
  configmaps                                      []                 []              [create delete deletecollection get list patch update watch]
  endpoints                                       []                 []              [create delete deletecollection get list patch update watch]
  persistentvolumeclaims                          []                 []              [create delete deletecollection get list patch update watch]
  pods/attach                                     []                 []              [create delete deletecollection get list patch update watch]
  pods/exec                                       []                 []              [create delete deletecollection get list patch update watch]
  pods/portforward                                []                 []              [create delete deletecollection get list patch update watch]
  pods/proxy                                      []                 []              [create delete deletecollection get list patch update watch]
  pods                                            []                 []              [create delete deletecollection get list patch update watch]
  replicationcontrollers/scale                    []                 []              [create delete deletecollection get list patch update watch]

return err
}

err = o.printStatus(response.Status)
Copy link
Contributor

Choose a reason for hiding this comment

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

return o.printStatus(response.Status)

defer w.Flush()

if o.NonResourceOnly {
if err := printNonResourceURLs(w, compactRules, o.NoHeader); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

return nil
}
if o.ResourceOnly {
if err := printResources(w, compactRules, o.NoHeader); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

}
return nil
}
if err := printAll(w, compactRules, o.NoHeader); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

rbacv1 "k8s.io/api/rbac/v1"
authorizationapi "k8s.io/kubernetes/pkg/apis/authorization"
rbacv1helpers "k8s.io/kubernetes/pkg/apis/rbac/v1"
internalauthorizationclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/authorization/internalversion"
Copy link
Contributor

Choose a reason for hiding this comment

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

May need to use v1clientset

return ret
}

func convertNonResourceToPolicyRule(status authorizationapi.SubjectRulesReviewStatus) []rbacv1.PolicyRule {
Copy link
Contributor

Choose a reason for hiding this comment

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

can use v1 api directly? so you can need not add version conversion func

@WanLinghao
Copy link
Contributor Author

@zjj2wry comments addressed, PTAL

@WanLinghao
Copy link
Contributor Author

/retest

Copy link
Contributor

@zjj2wry zjj2wry left a comment

Choose a reason for hiding this comment

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

lgtm

}

cmd := &cobra.Command{
Use: "i-can [--only-resources] [--only-non-resource-urls] [--quiet]",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit --no-headers


func (o *ICanOptions) printStatus(status authorizationapi.SubjectRulesReviewStatus) error {
if status.Incomplete && !o.Quiet {
fmt.Fprintln(o.Out, "warning: the list may be incomplete")
Copy link
Contributor

Choose a reason for hiding this comment

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

o.ErrOut

fmt.Fprintln(o.Out, "warning: the list may be incomplete")
}
if len(status.EvaluationError) != 0 && !o.Quiet {
fmt.Fprintln(o.Out, status.EvaluationError)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@CaoShuFeng
Copy link
Contributor

test ok in my environment.

@WanLinghao
Copy link
Contributor Author

@CaoShuFeng comments addressed

@WanLinghao WanLinghao force-pushed the ctl_selfsubjectrulesreview_support branch from fd8f8d7 to f390e26 Compare November 19, 2018 10:41
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2018
@WanLinghao WanLinghao force-pushed the ctl_selfsubjectrulesreview_support branch from f390e26 to fe6c3a4 Compare November 19, 2018 10:55
@WanLinghao
Copy link
Contributor Author

@soltysh Now this patch would not import any unexpected packages any more. PTAL

@WanLinghao
Copy link
Contributor Author

/retest

@WanLinghao
Copy link
Contributor Author

/test pull-kubernetes-integration

@WanLinghao
Copy link
Contributor Author

@soltysh friendly ping

@WanLinghao
Copy link
Contributor Author

@soltysh PTAL thanks

@WanLinghao WanLinghao force-pushed the ctl_selfsubjectrulesreview_support branch 2 times, most recently from 77e07ad to 48d0d54 Compare December 18, 2018 08:14
@WanLinghao
Copy link
Contributor Author

WanLinghao commented Dec 18, 2018

@soltysh Rebase done, PTAL

@WanLinghao
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

@WanLinghao
Copy link
Contributor Author

@soltysh friendly ping

@WanLinghao
Copy link
Contributor Author

@soltysh

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

Final nits, when you're done please ping me on slack so that I can approve that right away.

@@ -145,6 +158,7 @@ func (o *CanIOptions) Complete(f cmdutil.Factory, args []string) error {
return err
}
o.SelfSARClient = client.AuthorizationV1()
o.SelfSRRClient = client.AuthorizationV1()
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need twice the same client.

@@ -167,11 +188,19 @@ func (o *CanIOptions) Validate() error {
return fmt.Errorf("NonResourceURL and ResourceName can not specified together")
}
}

if o.NoHeaders {
return fmt.Errorf("--no-headers can not set without --list specified")
Copy link
Contributor

Choose a reason for hiding this comment

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

cannot be set...

@@ -214,9 +242,12 @@ func (o *CanIOptions) RunAccessCheck() (bool, error) {
fmt.Fprintf(o.Out, " - %v", response.Status.EvaluationError)
}
fmt.Fprintln(o.Out)
if o.Quiet {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, this is changing previous behavior. We used to os.Exit(1) always when !allowed now you're doing that only when --quiet was specified.

@WanLinghao
Copy link
Contributor Author

WanLinghao commented Jan 19, 2019

@soltysh Thanks for your review, all comments addressed.

…at actions they can do in specific namespace.
@WanLinghao WanLinghao force-pushed the ctl_selfsubjectrulesreview_support branch from 4f22ab2 to d4f5228 Compare January 19, 2019 08:00
@@ -167,9 +197,28 @@ func (o *CanIOptions) Validate() error {
return fmt.Errorf("NonResourceURL and ResourceName can not specified together")
}
}

if o.NoHeaders {
Copy link
Contributor

Choose a reason for hiding this comment

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

this can't be set w/o list flag, yet you're not checking its value so how do you know it wasn't set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soltysh I checked List flag in Line 185

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, sorry I missed the return nil after the internal condition.

@WanLinghao
Copy link
Contributor Author

@soltysh PTAL

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold cancel

@@ -167,9 +197,28 @@ func (o *CanIOptions) Validate() error {
return fmt.Errorf("NonResourceURL and ResourceName can not specified together")
}
}

if o.NoHeaders {
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, sorry I missed the return nil after the internal condition.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 12, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, WanLinghao

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2019
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@WanLinghao
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce

1 similar comment
@WanLinghao
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce

@k8s-ci-robot k8s-ci-robot merged commit a92729a into kubernetes:master Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubectl command to let user know what thay can do in specific namespace
10 participants