kubectl icon indicating copy to clipboard operation
kubectl copied to clipboard

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

Open dsyer opened this issue 2 years ago • 16 comments

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 avatar Aug 23 '23 13:08 dsyer

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()

dsyer avatar Aug 23 '23 16:08 dsyer

/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.

brianpursley avatar Sep 13 '23 16:09 brianpursley

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.

dsyer avatar Sep 13 '23 16:09 dsyer

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.

Ritikaa96 avatar Sep 20 '23 06:09 Ritikaa96

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

dsyer avatar Sep 20 '23 06:09 dsyer

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.

Ritikaa96 avatar Sep 20 '23 07:09 Ritikaa96

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.

dsyer avatar Sep 20 '23 08:09 dsyer

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

Ritikaa96 avatar Sep 21 '23 03:09 Ritikaa96