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

Improve `kubectl auth can-i` command by warning users when they try access resource out of scope #76014

Merged
merged 1 commit into from Apr 16, 2019

Conversation

@WanLinghao
Copy link
Member

commented Apr 2, 2019

/kind feature

What this PR does / why we need it:
kubectl auth can-i command would not hint user when they try to access some resource out of scope. For example, try get namespace inside default namespace.It would be reject by api-server but kubectl auth can-i get namespace --namespace=default would give a yes. After this patch, a warning info would be given.

Fixes #75970

improve `kubectl auth can-i` command by warning users when they try access resource out of scope
@WanLinghao

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

@liggitt PTAL

@@ -196,6 +200,8 @@ func (o *CanIOptions) Validate() error {
if o.Resource != (schema.GroupVersionResource{}) || o.ResourceName != "" {
return fmt.Errorf("NonResourceURL and ResourceName can not specified together")
}
} else {
o.checkResource()

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 2, 2019

Member

put more of the conditions that would keep us from running this check here, and make the function to determine whether the particular resource is namespaced more constrained in its input, like this:

} else if !o.Resource.Empty() && !o.AllNamespaces && o.DiscoveryClient != nil {
  if namespaced, err := isNamespaced(o.Resource, o.DiscoveryClient); err == nil && !namespaced {
    ... warn
  }
}

@WanLinghao WanLinghao force-pushed the WanLinghao:auth_can-i_improve branch from c1d844a to 6de4bb7 Apr 4, 2019

@WanLinghao

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

/test pull-kubernetes-integration

1 similar comment
@WanLinghao

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

/test pull-kubernetes-integration

@WanLinghao

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

/retest

@WanLinghao WanLinghao force-pushed the WanLinghao:auth_can-i_improve branch from 6de4bb7 to 79cff05 Apr 9, 2019

@WanLinghao

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@liggitt Hi, Comments addressed, PTAL

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

this needs a test to demonstrate behavior in the following scenarios:

  • kubectl auth can-i get '*' does not warn about namespaced scope or print an error
  • kubectl auth can-i get foo does not print a namespaced warning message, and only prints a single lookup error
  • kubectl auth can-i get pods does not print a namespaced warning message or a lookup error
  • kubectl auth can-i get nodes prints a namespaced warning message
  • kubectl auth can-i get nodes --all-namespaces does not print a namespaced warning message
}
}

return true, fmt.Errorf("the server doesn't have a resource type '%s' in group '%s'", gvr.Resource, gvr.Group)

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 9, 2019

Member

in error cases, return zero-value for the boolean (return false, fmt.Errorf(...)

pkg/kubectl/cmd/auth/cani.go Show resolved Hide resolved
if namespaced, err := isNamespaced(o.Resource, o.DiscoveryClient); err == nil && !namespaced {
fmt.Fprintf(o.ErrOut, "Warning: resource %s is not namespace scoped\n", o.Resource.Resource)
} else if err != nil {
fmt.Fprintf(o.ErrOut, "Warning: %v\n", err)

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 9, 2019

Member

don't print lookup errors... missing or unknown resources were already warned about and we don't want double warning messages

@@ -196,6 +199,12 @@ func (o *CanIOptions) Validate() error {
if o.Resource != (schema.GroupVersionResource{}) || o.ResourceName != "" {
return fmt.Errorf("NonResourceURL and ResourceName can not specified together")
}
} else if !o.Resource.Empty() && !o.AllNamespaces && o.DiscoveryClient != nil {
if namespaced, err := isNamespaced(o.Resource, o.DiscoveryClient); err == nil && !namespaced {
fmt.Fprintf(o.ErrOut, "Warning: resource %s is not namespace scoped\n", o.Resource.Resource)

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 9, 2019

Member

handle resources with and without a group (see the missing resource warning logic in resourceFor)

@WanLinghao WanLinghao force-pushed the WanLinghao:auth_can-i_improve branch from 79cff05 to 5793786 Apr 10, 2019

'kubectl auth can-i` command would not hint user when they try to access
some resource out of scope. For example, try get namespace inside defaut namespace.
It would be reject by api-server but `kubectl auth can-i get namespace --namespace=default`
would give a `yes`. After this patch, a warning info would be given.
For more detail, please refer issue #75950

@WanLinghao WanLinghao force-pushed the WanLinghao:auth_can-i_improve branch from 5793786 to 7fbd718 Apr 10, 2019

@WanLinghao

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

@liggitt Comments addressed, PTAL

@WanLinghao

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

@liggitt Friendly ping

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, 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

@WanLinghao

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

/kind bug
/sig auth
/priority important-soon

@k8s-ci-robot k8s-ci-robot merged commit 4c4c378 into kubernetes:master Apr 16, 2019

17 of 18 checks passed

pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation WanLinghao authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Context retired. Status moved to "pull-kubernetes-dependencies".
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.