Merge pull request #42216 from smarterclayton/direct_filter

Automatic merge from submit-queue

Don't filter items when resources requested by name

Add tracking on resource.Builder if a "named" item is requested (from
file, stream, url, or resource args) and use that in `get` to accurately
determine whether to filter resources. Add tests.

Fixes #41150, #40492

```release-note
Completed pods should not be hidden when requested by name via `kubectl get`.
```

pkg/kubectl/cmd/get.go

@@ -20,9 +20,9 @@ import (
 	"fmt"
 	"io"
 
+	"github.com/golang/glog"
 	"github.com/spf13/cobra"
 
-	"github.com/golang/glog"
 	kapierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -163,9 +163,6 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 	if err != nil {
 		return err
 	}
-	filterFuncs := f.DefaultResourceFilterFunc()
-	filterOpts := f.DefaultResourceFilterOptions(cmd, allNamespaces)
-
 	cmdNamespace, enforceNamespace, err := f.DefaultNamespace()
 	if err != nil {
 		return err
@@ -187,20 +184,18 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 		return cmdutil.UsageError(cmd, usageString)
 	}
 
-	argsHasNames, err := resource.HasNames(args)
-	if err != nil {
-		return err
-	}
-
 	// always show resources when getting by name or filename, or if the output
 	// is machine-consumable, or if multiple resource kinds were requested.
-	if len(options.Filenames) > 0 || argsHasNames || cmdutil.OutputsRawFormat(cmd) {
+	if cmdutil.OutputsRawFormat(cmd) {
 		if !cmd.Flag("show-all").Changed {
 			cmd.Flag("show-all").Value.Set("true")
 		}
 	}
 	export := cmdutil.GetFlagBool(cmd, "export")
 
+	filterFuncs := f.DefaultResourceFilterFunc()
+	filterOpts := f.DefaultResourceFilterOptions(cmd, allNamespaces)
+
 	// handle watch separately since we cannot watch multiple resource types
 	isWatch, isWatchOnly := cmdutil.GetFlagBool(cmd, "watch"), cmdutil.GetFlagBool(cmd, "watch-only")
 	if isWatch || isWatchOnly {
@@ -224,6 +219,9 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 		if len(infos) != 1 {
 			return i18n.Errorf("watch is only supported on individual resources and resource collections - %d resources were found", len(infos))
 		}
+		if r.TargetsSingleItems() {
+			filterFuncs = nil
+		}
 		info := infos[0]
 		mapping := info.ResourceMapping()
 		printer, err := f.PrinterForMapping(cmd, mapping, allNamespaces)
@@ -308,7 +306,9 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 	if err != nil {
 		return err
 	}
-
+	if r.TargetsSingleItems() {
+		filterFuncs = nil
+	}
 	if options.IgnoreNotFound {
 		r.IgnoreErrors(kapierrors.IsNotFound)
 	}

pkg/kubectl/cmd/get_test.go

@@ -211,6 +211,59 @@ func TestGetObjects(t *testing.T) {
 	}
 }
 
+func TestGetObjectsFiltered(t *testing.T) {
+	initTestErrorHandler(t)
+
+	pods, _, _ := testData()
+	pods.Items[0].Status.Phase = api.PodFailed
+	first := &pods.Items[0]
+	second := &pods.Items[1]
+
+	testCases := []struct {
+		args   []string
+		resp   runtime.Object
+		flags  map[string]string
+		expect []runtime.Object
+	}{
+		{args: []string{"pods", "foo"}, resp: first, expect: []runtime.Object{first}},
+		{args: []string{"pods", "foo"}, flags: map[string]string{"show-all": "false"}, resp: first, expect: []runtime.Object{first}},
+		{args: []string{"pods"}, flags: map[string]string{"show-all": "true"}, resp: pods, expect: []runtime.Object{first, second}},
+		{args: []string{"pods/foo"}, resp: first, expect: []runtime.Object{first}},
+		{args: []string{"pods"}, flags: map[string]string{"output": "yaml"}, resp: pods, expect: []runtime.Object{first, second}},
+		{args: []string{}, flags: map[string]string{"filename": "../../../examples/storage/cassandra/cassandra-controller.yaml"}, resp: pods, expect: []runtime.Object{first, second}},
+
+		{args: []string{"pods"}, resp: pods, expect: []runtime.Object{second}},
+		{args: []string{"pods"}, flags: map[string]string{"show-all": "false"}, resp: pods, expect: []runtime.Object{second}},
+	}
+
+	for i, test := range testCases {
+		t.Logf("%d", i)
+		f, tf, codec, _ := cmdtesting.NewAPIFactory()
+		tf.Printer = &testPrinter{}
+		tf.UnstructuredClient = &fake.RESTClient{
+			APIRegistry:          api.Registry,
+			NegotiatedSerializer: unstructuredSerializer,
+			Resp:                 &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, test.resp)},
+		}
+		tf.Namespace = "test"
+		buf := bytes.NewBuffer([]byte{})
+		errBuf := bytes.NewBuffer([]byte{})
+
+		cmd := NewCmdGet(f, buf, errBuf)
+		cmd.SetOutput(buf)
+		for k, v := range test.flags {
+			cmd.Flags().Lookup(k).Value.Set(v)
+		}
+		cmd.Run(cmd, test.args)
+
+		verifyObjects(t, test.expect, tf.Printer.(*testPrinter).Objects)
+
+		if len(buf.String()) == 0 {
+			t.Errorf("%d: unexpected empty output", i)
+		}
+	}
+}
+
 func TestGetObjectIgnoreNotFound(t *testing.T) {
 	initTestErrorHandler(t)
 
@@ -313,7 +366,7 @@ func verifyObjects(t *testing.T, expected, actual []runtime.Object) {
 	var err error
 
 	if len(actual) != len(expected) {
-		t.Fatal(actual)
+		t.Fatalf("expected %d, got %d", len(expected), len(actual))
 	}
 	for i, obj := range actual {
 		switch obj.(type) {
@@ -330,7 +383,7 @@ func verifyObjects(t *testing.T, expected, actual []runtime.Object) {
 			t.Fatal(err)
 		}
 		if !apiequality.Semantic.DeepEqual(expected[i], actualObj) {
-			t.Errorf("unexpected object: \n%#v\n%#v", expected[i], actualObj)
+			t.Errorf("unexpected object: %d \n%#v\n%#v", i, expected[i], actualObj)
 		}
 	}
 }
@@ -658,7 +711,7 @@ func TestGetMultipleTypeObjectsWithDirectReference(t *testing.T) {
 	}
 }
 
-func TestGetByNameForcesFlag(t *testing.T) {
+func TestGetByFormatForcesFlag(t *testing.T) {
 	pods, _, _ := testData()
 
 	f, tf, codec, _ := cmdtesting.NewAPIFactory()
@@ -674,7 +727,8 @@ func TestGetByNameForcesFlag(t *testing.T) {
 
 	cmd := NewCmdGet(f, buf, errBuf)
 	cmd.SetOutput(buf)
-	cmd.Run(cmd, []string{"pods", "foo"})
+	cmd.Flags().Lookup("output").Value.Set("yaml")
+	cmd.Run(cmd, []string{"pods"})
 
 	showAllFlag, _ := cmd.Flags().GetBool("show-all")
 	if !showAllFlag {

pkg/kubectl/resource/builder.go

@@ -567,25 +567,31 @@ func (b *Builder) visitorResult() *Result {
 }
 
 func (b *Builder) visitBySelector() *Result {
+	result := &Result{
+		targetsSingleItems: false,
+	}
+
 	if len(b.names) != 0 {
-		return &Result{err: fmt.Errorf("name cannot be provided when a selector is specified")}
+		return result.withError(fmt.Errorf("name cannot be provided when a selector is specified"))
 	}
 	if len(b.resourceTuples) != 0 {
-		return &Result{err: fmt.Errorf("selectors and the all flag cannot be used when passing resource/name arguments")}
+		return result.withError(fmt.Errorf("selectors and the all flag cannot be used when passing resource/name arguments"))
 	}
 	if len(b.resources) == 0 {
-		return &Result{err: fmt.Errorf("at least one resource must be specified to use a selector")}
+		return result.withError(fmt.Errorf("at least one resource must be specified to use a selector"))
 	}
 	mappings, err := b.resourceMappings()
 	if err != nil {
-		return &Result{err: err}
+		result.err = err
+		return result
 	}
 
 	visitors := []Visitor{}
 	for _, mapping := range mappings {
 		client, err := b.mapper.ClientForMapping(mapping)
 		if err != nil {
-			return &Result{err: err}
+			result.err = err
+			return result
 		}
 		selectorNamespace := b.namespace
 		if mapping.Scope.Name() != meta.RESTScopeNameNamespace {
@@ -594,9 +600,12 @@ func (b *Builder) visitBySelector() *Result {
 		visitors = append(visitors, NewSelector(client, mapping, selectorNamespace, b.selector, b.export))
 	}
 	if b.continueOnError {
-		return &Result{visitor: EagerVisitorList(visitors), sources: visitors}
+		result.visitor = EagerVisitorList(visitors)
+	} else {
+		result.visitor = VisitorList(visitors)
 	}
-	return &Result{visitor: VisitorList(visitors), sources: visitors}
+	result.sources = visitors
+	return result
 }
 
 func (b *Builder) visitByResource() *Result {
@@ -607,14 +616,20 @@ func (b *Builder) visitByResource() *Result {
 		isSingleItemImplied = len(b.resourceTuples) == 1
 	}
 
+	result := &Result{
+		singleItemImplied:  isSingleItemImplied,
+		targetsSingleItems: true,
+	}
+
 	if len(b.resources) != 0 {
-		return &Result{singleItemImplied: isSingleItemImplied, err: fmt.Errorf("you may not specify individual resources and bulk resources in the same call")}
+		return result.withError(fmt.Errorf("you may not specify individual resources and bulk resources in the same call"))
 	}
 
 	// retrieve one client for each resource
 	mappings, err := b.resourceTupleMappings()
 	if err != nil {
-		return &Result{singleItemImplied: isSingleItemImplied, err: err}
+		result.err = err
+		return result
 	}
 	clients := make(map[string]RESTClient)
 	for _, mapping := range mappings {
@@ -624,7 +639,8 @@ func (b *Builder) visitByResource() *Result {
 		}
 		client, err := b.mapper.ClientForMapping(mapping)
 		if err != nil {
-			return &Result{err: err}
+			result.err = err
+			return result
 		}
 		clients[s] = client
 	}
@@ -633,12 +649,12 @@ func (b *Builder) visitByResource() *Result {
 	for _, tuple := range b.resourceTuples {
 		mapping, ok := mappings[tuple.Resource]
 		if !ok {
-			return &Result{singleItemImplied: isSingleItemImplied, err: fmt.Errorf("resource %q is not recognized: %v", tuple.Resource, mappings)}
+			return result.withError(fmt.Errorf("resource %q is not recognized: %v", tuple.Resource, mappings))
 		}
 		s := fmt.Sprintf("%s/%s", mapping.GroupVersionKind.GroupVersion().String(), mapping.Resource)
 		client, ok := clients[s]
 		if !ok {
-			return &Result{singleItemImplied: isSingleItemImplied, err: fmt.Errorf("could not find a client for resource %q", tuple.Resource)}
+			return result.withError(fmt.Errorf("could not find a client for resource %q", tuple.Resource))
 		}
 
 		selectorNamespace := b.namespace
@@ -650,7 +666,7 @@ func (b *Builder) visitByResource() *Result {
 				if b.allNamespace {
 					errMsg = "a resource cannot be retrieved by name across all namespaces"
 				}
-				return &Result{singleItemImplied: isSingleItemImplied, err: fmt.Errorf(errMsg)}
+				return result.withError(fmt.Errorf(errMsg))
 			}
 		}
 
@@ -664,31 +680,38 @@ func (b *Builder) visitByResource() *Result {
 	} else {
 		visitors = VisitorList(items)
 	}
-	return &Result{singleItemImplied: isSingleItemImplied, visitor: visitors, sources: items}
+	result.visitor = visitors
+	result.sources = items
+	return result
 }
 
 func (b *Builder) visitByName() *Result {
-	isSingleItemImplied := len(b.names) == 1
+	result := &Result{
+		singleItemImplied:  len(b.names) == 1,
+		targetsSingleItems: true,
+	}
 
 	if len(b.paths) != 0 {
-		return &Result{singleItemImplied: isSingleItemImplied, err: fmt.Errorf("when paths, URLs, or stdin is provided as input, you may not specify a resource by arguments as well")}
+		return result.withError(fmt.Errorf("when paths, URLs, or stdin is provided as input, you may not specify a resource by arguments as well"))
 	}
 	if len(b.resources) == 0 {
-		return &Result{singleItemImplied: isSingleItemImplied, err: fmt.Errorf("you must provide a resource and a resource name together")}
+		return result.withError(fmt.Errorf("you must provide a resource and a resource name together"))
 	}
 	if len(b.resources) > 1 {
-		return &Result{singleItemImplied: isSingleItemImplied, err: fmt.Errorf("you must specify only one resource")}
+		return result.withError(fmt.Errorf("you must specify only one resource"))
 	}
 
 	mappings, err := b.resourceMappings()
 	if err != nil {
-		return &Result{singleItemImplied: isSingleItemImplied, err: err}
+		result.err = err
+		return result
 	}
 	mapping := mappings[0]
 
 	client, err := b.mapper.ClientForMapping(mapping)
 	if err != nil {
-		return &Result{err: err}
+		result.err = err
+		return result
 	}
 
 	selectorNamespace := b.namespace
@@ -700,7 +723,7 @@ func (b *Builder) visitByName() *Result {
 			if b.allNamespace {
 				errMsg = "a resource cannot be retrieved by name across all namespaces"
 			}
-			return &Result{singleItemImplied: isSingleItemImplied, err: fmt.Errorf(errMsg)}
+			return result.withError(fmt.Errorf(errMsg))
 		}
 	}
 
@@ -709,19 +732,25 @@ func (b *Builder) visitByName() *Result {
 		info := NewInfo(client, mapping, selectorNamespace, name, b.export)
 		visitors = append(visitors, info)
 	}
-	return &Result{singleItemImplied: isSingleItemImplied, visitor: VisitorList(visitors), sources: visitors}
+	result.visitor = VisitorList(visitors)
+	result.sources = visitors
+	return result
 }
 
 func (b *Builder) visitByPaths() *Result {
-	singleItemImplied := !b.dir && !b.stream && len(b.paths) == 1
+	result := &Result{
+		singleItemImplied:  !b.dir && !b.stream && len(b.paths) == 1,
+		targetsSingleItems: true,
+	}
+
 	if len(b.resources) != 0 {
-		return &Result{singleItemImplied: singleItemImplied, err: fmt.Errorf("when paths, URLs, or stdin is provided as input, you may not specify resource arguments as well")}
+		return result.withError(fmt.Errorf("when paths, URLs, or stdin is provided as input, you may not specify resource arguments as well"))
 	}
 	if len(b.names) != 0 {
-		return &Result{err: fmt.Errorf("name cannot be provided when a path is specified")}
+		return result.withError(fmt.Errorf("name cannot be provided when a path is specified"))
 	}
 	if len(b.resourceTuples) != 0 {
-		return &Result{err: fmt.Errorf("resource/name arguments cannot be provided when a path is specified")}
+		return result.withError(fmt.Errorf("resource/name arguments cannot be provided when a path is specified"))
 	}
 
 	var visitors Visitor
@@ -746,7 +775,9 @@ func (b *Builder) visitByPaths() *Result {
 	if b.selector != nil {
 		visitors = NewFilteredVisitor(visitors, FilterBySelector(b.selector))
 	}
-	return &Result{singleItemImplied: singleItemImplied, visitor: visitors, sources: b.paths}
+	result.visitor = visitors
+	result.sources = b.paths
+	return result
 }
 
 // Do returns a Result object with a Visitor for the resources identified by the Builder.

pkg/kubectl/resource/result.go

@@ -43,6 +43,7 @@ type Result struct {
 
 	sources            []Visitor
 	singleItemImplied  bool
+	targetsSingleItems bool
 
 	ignoreErrors []utilerrors.Matcher
 
@@ -50,6 +51,19 @@ type Result struct {
 	info []*Info
 }
 
+// withError allows a fluent style for internal result code.
+func (r *Result) withError(err error) *Result {
+	r.err = err
+	return r
+}
+
+// TargetsSingleItems returns true if any of the builder arguments pointed
+// to non-list calls (if the user explicitly asked for any object by name).
+// This includes directories, streams, URLs, and resource name tuples.
+func (r *Result) TargetsSingleItems() bool {
+	return r.targetsSingleItems
+}
+
 // IgnoreErrors will filter errors that occur when by visiting the result
 // (but not errors that occur by creating the result in the first place),
 // eliminating any that match fns. This is best used in combination with