kubernetes / kubectl

Issue tracker and mirror of kubectl code
Apache License 2.0
2.87k stars 921 forks source link

Only show what I have permission to see in kubectl get all #1475

Open dsyer opened 1 year ago

dsyer commented 1 year ago

Currently kubectl get all is a useful default tool for getting a quick look at what is going on in my context, despite the misleading name I still like it. But if I don't have permission to view some of the resources I see errors, and that's distracting and quite unhelpful. It make an empty namespace look like a train wreck (you only see errors, and many people add CRDs to the "all" list but don't give you permission to see them). Would it be really hard to just skip the error messages for permission denied, by default anyway?

dsyer commented 1 year ago

This patch seems to do what I mean (as shown by the test). I have no idea if it captures all uses of "all".

diff --git a/pkg/cmd/get/get.go b/pkg/cmd/get/get.go
index f1658699..be833c83 100644
--- a/pkg/cmd/get/get.go
+++ b/pkg/cmd/get/get.go
@@ -455,6 +455,11 @@ func (o *GetOptions) Run(f cmdutil.Factory, args []string) error {
        chunkSize = 0
    }

+   var isAll bool = false
+   if len(args) == 1 && args[0] == "all" {
+       isAll = true;
+   }
+
    r := f.NewBuilder().
        Unstructured().
        NamespaceParam(o.Namespace).DefaultNamespace().AllNamespaces(o.AllNamespaces).
@@ -477,6 +482,10 @@ func (o *GetOptions) Run(f cmdutil.Factory, args []string) error {
        return err
    }

+   if isAll {
+       r.IgnoreErrors(apierrors.IsForbidden)
+   }
+
    if !o.IsHumanReadablePrinter {
        return o.printGeneric(r)
    }
diff --git a/pkg/cmd/get/get_test.go b/pkg/cmd/get/get_test.go
index 0776b674..8ed17877 100644
--- a/pkg/cmd/get/get_test.go
+++ b/pkg/cmd/get/get_test.go
@@ -592,6 +592,69 @@ func TestNoBlankLinesForGetAll(t *testing.T) {
    }
 }

+func TestNoErrorLinesForGetAll(t *testing.T) {
+   tf := cmdtesting.NewTestFactory().WithNamespace("test")
+   defer tf.Cleanup()
+
+   codec := scheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...)
+   tf.UnstructuredClient = &fake.RESTClient{
+       NegotiatedSerializer: resource.UnstructuredPlusDefaultContentConfig().NegotiatedSerializer,
+       Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
+           switch p, m := req.URL.Path, req.Method; {
+           case p == "/namespaces/test/pods" && m == "GET":
+               return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: emptyTableObjBody(codec)}, nil
+           case p == "/namespaces/test/replicationcontrollers" && m == "GET":
+               return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: emptyTableObjBody(codec)}, nil
+           case p == "/namespaces/test/services" && m == "GET":
+               return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: emptyTableObjBody(codec)}, nil
+           case p == "/namespaces/test/statefulsets" && m == "GET":
+               return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: emptyTableObjBody(codec)}, nil
+           case p == "/namespaces/test/horizontalpodautoscalers" && m == "GET":
+               return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: emptyTableObjBody(codec)}, nil
+           case p == "/namespaces/test/jobs" && m == "GET":
+               return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: emptyTableObjBody(codec)}, nil
+           case p == "/namespaces/test/cronjobs" && m == "GET":
+               return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: emptyTableObjBody(codec)}, nil
+           case p == "/namespaces/test/daemonsets" && m == "GET":
+               return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: emptyTableObjBody(codec)}, nil
+           case p == "/namespaces/test/deployments" && m == "GET":
+               return &http.Response{StatusCode: http.StatusForbidden, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, &metav1.Status{
+                   TypeMeta: metav1.TypeMeta{
+                       Kind:       "Status",
+                       APIVersion: "v1",
+                   },
+                   Status:  metav1.StatusFailure,
+                   Code:    http.StatusForbidden,
+                   Reason:  metav1.StatusReasonForbidden,
+                   Message: "Not allowed",
+               })}, nil
+           case p == "/namespaces/test/replicasets" && m == "GET":
+               return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: emptyTableObjBody(codec)}, nil
+
+           default:
+               t.Fatalf("request url: %#v,and request: %#v", req.URL, req)
+               return nil, nil
+           }
+       }),
+   }
+
+   streams, _, buf, errbuf := genericiooptions.NewTestIOStreams()
+   cmd := NewCmdGet("kubectl", tf, streams)
+   cmd.SetOut(buf)
+   cmd.SetErr(buf)
+   cmd.Run(cmd, []string{"all"})
+
+   expected := ``
+   if e, a := expected, buf.String(); e != a {
+       t.Errorf("expected\n%v\ngot\n%v", e, a)
+   }
+   expectedErr := `No resources found in test namespace.
+`
+   if e, a := expectedErr, errbuf.String(); e != a {
+       t.Errorf("expectedErr\n%v\ngot\n%v", e, a)
+   }
+}
+
 func TestNotFoundMessageForGetNonNamespacedResources(t *testing.T) {
    tf := cmdtesting.NewTestFactory().WithNamespace("test")
    defer tf.Cleanup()
brianpursley commented 1 year ago

/triage accepted

We often prefer not to add new flags, but there is some value in this functionality and since there is a PR, we can discuss on the PR review.

dsyer commented 1 year ago

If you don’t like new flags can we leave that out (use my patch from the comment, instead of the PR)? I’d happily submit a separate request.

Ritikaa96 commented 1 year ago

Hi @dsyer Is the PR raised? It will be really helpful if the PR mentioned in the comments is linked here. It will be beneficial for people following the feature.

dsyer commented 1 year ago

There is a PR linked above (GitHub does it automatically). Look above Brian’s comment.

Ritikaa96 commented 1 year ago

Oh. so i might have misunderstood that. so this issue is(possibly) a sub-part of that PR. I thought the patch has been added as a separate PR. Thanks for clarifying.

dsyer commented 1 year ago

Oh I see, sorry for the confusion. The PR includes my patch (without attribution). I can send a separate PR if anyone wants to see it.

Ritikaa96 commented 1 year ago

Hi @dsyer , that depends on the sig experts and other people working on this issue. Let's wait for the verdict.

k8s-triage-robot commented 1 month ago

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted