Fix improper use of Table request/response to k8s API

Fixes #11712

A change was made that when validation was turned off the Kubernetes
packages were building objects as a Table type. This was done for
display purposes. When details about the objects was going to be
printed as part of #10912.

This broke rollback, and possibly other functionality, as a Table
type was returned in some cases that needed the regular object.
This caused things to break silently.

The fix involved adding in a new Function (and interface) to
query for tables instead of the objects themselves. There was not
a clean way to add it to the existing function that covered all
cases.

A second problem was noticed along the way. When data was output
via status as YAML or JSON it was in the form of a table rather
than the objects themselves. This did not reflect expectations
and did not match the functionality in kubectl. The code was
updated to return a table when that was presented and the objects
when they are being output for YAML or JSON. The API also supports
this handling to SDK users can replicate this functionality.

API changes made here were never released. The functions were
developed for this release of Helm and only ever appeared in an
RC. In this case, they can be changed.

Signed-off-by: Matt Farina <matt.farina@suse.com>

cmd/helm/status.go

@@ -65,6 +65,13 @@ func newStatusCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 			return compListReleases(toComplete, args, cfg)
 		},
 		RunE: func(cmd *cobra.Command, args []string) error {
+
+			// When the output format is a table the resources should be fetched
+			// and displayed as a table. When YAML or JSON the resources will be
+			// returned. This mirrors the handling in kubectl.
+			if outfmt == output.Table {
+				client.ShowResourcesTable = true
+			}
 			rel, err := client.Run(args[0])
 			if err != nil {
 				return err

pkg/action/status.go

@@ -18,6 +18,7 @@ package action
 
 import (
 	"bytes"
+	"errors"
 
 	"helm.sh/helm/v3/pkg/kube"
 	"helm.sh/helm/v3/pkg/release"
@@ -36,9 +37,13 @@ type Status struct {
 	// TODO Helm 4: Remove this flag and output the description by default.
 	ShowDescription bool
 
-	// If true, display resources of release to output format
+	// ShowResources sets if the resources should be retrieved with the status.
 	// TODO Helm 4: Remove this flag and output the resources by default.
 	ShowResources bool
+
+	// ShowResourcesTable is used with ShowResources. When true this will cause
+	// the resulting objects to be retrieved as a kind=table.
+	ShowResourcesTable bool
 }
 
 // NewStatus creates a new Status object with the given configuration.
@@ -63,10 +68,21 @@ func (s *Status) Run(name string) (*release.Release, error) {
 		return nil, err
 	}
 
-	resources, _ := s.cfg.KubeClient.Build(bytes.NewBufferString(rel.Manifest), false)
-
 	if kubeClient, ok := s.cfg.KubeClient.(kube.InterfaceResources); ok {
-		resp, err := kubeClient.Get(resources, bytes.NewBufferString(rel.Manifest))
+		var resources kube.ResourceList
+		if s.ShowResourcesTable {
+			resources, err = kubeClient.BuildTable(bytes.NewBufferString(rel.Manifest), false)
+			if err != nil {
+				return nil, err
+			}
+		} else {
+			resources, err = s.cfg.KubeClient.Build(bytes.NewBufferString(rel.Manifest), false)
+			if err != nil {
+				return nil, err
+			}
+		}
+
+		resp, err := kubeClient.Get(resources, true)
 		if err != nil {
 			return nil, err
 		}
@@ -75,5 +91,5 @@ func (s *Status) Run(name string) (*release.Release, error) {
 
 		return rel, nil
 	}
-	return nil, err
+	return nil, errors.New("unable to get kubeClient with interface InterfaceResources")
 }

pkg/kube/client.go

@@ -149,7 +149,10 @@ func transformRequests(req *rest.Request) {
 	req.Param("includeObject", "Object")
 }
 
-func (c *Client) Get(resources ResourceList, reader io.Reader) (map[string][]runtime.Object, error) {
+// Get retrieves the resource objects supplied. If related is set to true the
+// related pods are fetched as well. If the passed in resources are a table kind
+// the related resources will also be fetched as kind=table.
+func (c *Client) Get(resources ResourceList, related bool) (map[string][]runtime.Object, error) {
 	buf := new(bytes.Buffer)
 	objs := make(map[string][]runtime.Object)
 
@@ -167,9 +170,20 @@ func (c *Client) Get(resources ResourceList, reader io.Reader) (map[string][]run
 		} else {
 			objs[vk] = append(objs[vk], obj)
 
-			objs, err = c.getSelectRelationPod(info, objs, &podSelectors)
-			if err != nil {
-				c.Log("Warning: get the relation pod is failed, err:%s", err.Error())
+			// Only fetch related pods if they are requested
+			if related {
+				// Discover if the existing object is a table. If it is, request
+				// the pods as Tables. Otherwise request them normally.
+				objGVK := obj.GetObjectKind().GroupVersionKind()
+				var isTable bool
+				if objGVK.Kind == "Table" {
+					isTable = true
+				}
+
+				objs, err = c.getSelectRelationPod(info, objs, isTable, &podSelectors)
+				if err != nil {
+					c.Log("Warning: get the relation pod is failed, err:%s", err.Error())
+				}
 			}
 		}
 
@@ -182,7 +196,7 @@ func (c *Client) Get(resources ResourceList, reader io.Reader) (map[string][]run
 	return objs, nil
 }
 
-func (c *Client) getSelectRelationPod(info *resource.Info, objs map[string][]runtime.Object, podSelectors *[]map[string]string) (map[string][]runtime.Object, error) {
+func (c *Client) getSelectRelationPod(info *resource.Info, objs map[string][]runtime.Object, table bool, podSelectors *[]map[string]string) (map[string][]runtime.Object, error) {
 	if info == nil {
 		return objs, nil
 	}
@@ -201,17 +215,33 @@ func (c *Client) getSelectRelationPod(info *resource.Info, objs map[string][]run
 
 	*podSelectors = append(*podSelectors, selector)
 
-	infos, err := c.Factory.NewBuilder().
-		Unstructured().
-		ContinueOnError().
-		NamespaceParam(info.Namespace).
-		DefaultNamespace().
-		ResourceTypes("pods").
-		LabelSelector(labels.Set(selector).AsSelector().String()).
-		TransformRequests(transformRequests).
-		Do().Infos()
-	if err != nil {
-		return objs, err
+	var infos []*resource.Info
+	var err error
+	if table {
+		infos, err = c.Factory.NewBuilder().
+			Unstructured().
+			ContinueOnError().
+			NamespaceParam(info.Namespace).
+			DefaultNamespace().
+			ResourceTypes("pods").
+			LabelSelector(labels.Set(selector).AsSelector().String()).
+			TransformRequests(transformRequests).
+			Do().Infos()
+		if err != nil {
+			return objs, err
+		}
+	} else {
+		infos, err = c.Factory.NewBuilder().
+			Unstructured().
+			ContinueOnError().
+			NamespaceParam(info.Namespace).
+			DefaultNamespace().
+			ResourceTypes("pods").
+			LabelSelector(labels.Set(selector).AsSelector().String()).
+			Do().Infos()
+		if err != nil {
+			return objs, err
+		}
 	}
 	vk := "v1/Pod(related)"
 
@@ -317,21 +347,38 @@ func (c *Client) Build(reader io.Reader, validate bool) (ResourceList, error) {
 	if err != nil {
 		return nil, err
 	}
-	var result ResourceList
+	result, err := c.newBuilder().
+		Unstructured().
+		Schema(schema).
+		Stream(reader, "").
+		Do().Infos()
+	return result, scrubValidationError(err)
+}
+
+// BuildTable validates for Kubernetes objects and returns unstructured infos.
+// The returned kind is a Table.
+func (c *Client) BuildTable(reader io.Reader, validate bool) (ResourceList, error) {
+	validationDirective := metav1.FieldValidationIgnore
 	if validate {
-		result, err = c.newBuilder().
-			Unstructured().
-			Schema(schema).
-			Stream(reader, "").
-			Do().Infos()
-	} else {
-		result, err = c.newBuilder().
-			Unstructured().
-			Schema(schema).
-			Stream(reader, "").
-			TransformRequests(transformRequests).
-			Do().Infos()
+		validationDirective = metav1.FieldValidationStrict
+	}
+
+	dynamicClient, err := c.Factory.DynamicClient()
+	if err != nil {
+		return nil, err
 	}
+
+	verifier := resource.NewQueryParamVerifier(dynamicClient, c.Factory.OpenAPIGetter(), resource.QueryParamFieldValidation)
+	schema, err := c.Factory.Validator(validationDirective, verifier)
+	if err != nil {
+		return nil, err
+	}
+	result, err := c.newBuilder().
+		Unstructured().
+		Schema(schema).
+		Stream(reader, "").
+		TransformRequests(transformRequests).
+		Do().Infos()
 	return result, scrubValidationError(err)
 }
 
@@ -472,10 +519,10 @@ func (c *Client) watchTimeout(t time.Duration) func(*resource.Info) error {
 // For most kinds, it checks to see if the resource is marked as Added or Modified
 // by the Kubernetes event stream. For some kinds, it does more:
 //
-// - Jobs: A job is marked "Ready" when it has successfully completed. This is
-//   ascertained by watching the Status fields in a job's output.
-// - Pods: A pod is marked "Ready" when it has successfully completed. This is
-//   ascertained by watching the status.phase field in a pod's output.
+//   - Jobs: A job is marked "Ready" when it has successfully completed. This is
+//     ascertained by watching the Status fields in a job's output.
+//   - Pods: A pod is marked "Ready" when it has successfully completed. This is
+//     ascertained by watching the status.phase field in a pod's output.
 //
 // Handling for other kinds will be added as necessary.
 func (c *Client) WatchUntilReady(resources ResourceList, timeout time.Duration) error {

pkg/kube/client_test.go

@@ -253,6 +253,45 @@ func TestBuild(t *testing.T) {
 	}
 }
 
+func TestBuildTable(t *testing.T) {
+	tests := []struct {
+		name      string
+		namespace string
+		reader    io.Reader
+		count     int
+		err       bool
+	}{
+		{
+			name:      "Valid input",
+			namespace: "test",
+			reader:    strings.NewReader(guestbookManifest),
+			count:     6,
+		}, {
+			name:      "Valid input, deploying resources into different namespaces",
+			namespace: "test",
+			reader:    strings.NewReader(namespacedGuestbookManifest),
+			count:     1,
+		},
+	}
+
+	c := newTestClient(t)
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Test for an invalid manifest
+			infos, err := c.BuildTable(tt.reader, false)
+			if err != nil && !tt.err {
+				t.Errorf("Got error message when no error should have occurred: %v", err)
+			} else if err != nil && strings.Contains(err.Error(), "--validate=false") {
+				t.Error("error message was not scrubbed")
+			}
+
+			if len(infos) != tt.count {
+				t.Errorf("expected %d result objects, got %d", tt.count, len(infos))
+			}
+		})
+	}
+}
+
 func TestPerform(t *testing.T) {
 	tests := []struct {
 		name       string

pkg/kube/fake/fake.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/cli-runtime/pkg/resource"
 
 	"helm.sh/helm/v3/pkg/kube"
@@ -33,11 +34,13 @@ import (
 type FailingKubeClient struct {
 	PrintingKubeClient
 	CreateError                      error
+	GetError                         error
 	WaitError                        error
 	DeleteError                      error
 	WatchUntilReadyError             error
 	UpdateError                      error
 	BuildError                       error
+	BuildTableError                  error
 	BuildUnstructuredError           error
 	WaitAndGetCompletedPodPhaseError error
 	WaitDuration                     time.Duration
@@ -51,6 +54,14 @@ func (f *FailingKubeClient) Create(resources kube.ResourceList) (*kube.Result, e
 	return f.PrintingKubeClient.Create(resources)
 }
 
+// Get returns the configured error if set or prints
+func (f *FailingKubeClient) Get(resources kube.ResourceList, related bool) (map[string][]runtime.Object, error) {
+	if f.GetError != nil {
+		return nil, f.GetError
+	}
+	return f.PrintingKubeClient.Get(resources, related)
+}
+
 // Waits the amount of time defined on f.WaitDuration, then returns the configured error if set or prints.
 func (f *FailingKubeClient) Wait(resources kube.ResourceList, d time.Duration) error {
 	time.Sleep(f.WaitDuration)
@@ -108,6 +119,14 @@ func (f *FailingKubeClient) Build(r io.Reader, _ bool) (kube.ResourceList, error
 	return f.PrintingKubeClient.Build(r, false)
 }
 
+// BuildTable returns the configured error if set or prints
+func (f *FailingKubeClient) BuildTable(r io.Reader, _ bool) (kube.ResourceList, error) {
+	if f.BuildTableError != nil {
+		return []*resource.Info{}, f.BuildTableError
+	}
+	return f.PrintingKubeClient.BuildTable(r, false)
+}
+
 // WaitAndGetCompletedPodPhase returns the configured error if set or prints
 func (f *FailingKubeClient) WaitAndGetCompletedPodPhase(s string, d time.Duration) (v1.PodPhase, error) {
 	if f.WaitAndGetCompletedPodPhaseError != nil {

pkg/kube/fake/printer.go

@@ -48,7 +48,7 @@ func (p *PrintingKubeClient) Create(resources kube.ResourceList) (*kube.Result,
 	return &kube.Result{Created: resources}, nil
 }
 
-func (p *PrintingKubeClient) Get(resources kube.ResourceList, reader io.Reader) (map[string][]runtime.Object, error) {
+func (p *PrintingKubeClient) Get(resources kube.ResourceList, related bool) (map[string][]runtime.Object, error) {
 	_, err := io.Copy(p.Out, bufferize(resources))
 	if err != nil {
 		return nil, err
@@ -105,6 +105,11 @@ func (p *PrintingKubeClient) Build(_ io.Reader, _ bool) (kube.ResourceList, erro
 	return []*resource.Info{}, nil
 }
 
+// BuildTable implements KubeClient BuildTable.
+func (p *PrintingKubeClient) BuildTable(_ io.Reader, _ bool) (kube.ResourceList, error) {
+	return []*resource.Info{}, nil
+}
+
 // WaitAndGetCompletedPodPhase implements KubeClient WaitAndGetCompletedPodPhase.
 func (p *PrintingKubeClient) WaitAndGetCompletedPodPhase(_ string, _ time.Duration) (v1.PodPhase, error) {
 	return v1.PodSucceeded, nil

pkg/kube/interface.go

@@ -83,8 +83,22 @@ type InterfaceExt interface {
 //
 // TODO Helm 4: Remove InterfaceResources and integrate its method(s) into the Interface.
 type InterfaceResources interface {
-	// Get details of deployed resources in ResourceList to be printed.
-	Get(resources ResourceList, reader io.Reader) (map[string][]runtime.Object, error)
+	// Get details of deployed resources.
+	// The first argument is a list of resources to get. The second argument
+	// specifies if related pods should be fetched. For example, the pods being
+	// managed by a deployment.
+	Get(resources ResourceList, related bool) (map[string][]runtime.Object, error)
+
+	// BuildTable creates a resource list from a Reader. This differs from
+	// Interface.Build() in that a table kind is returned. A table is useful
+	// if you want to use a printer to display the information.
+	//
+	// Reader must contain a YAML stream (one or more YAML documents separated
+	// by "\n---\n")
+	//
+	// Validates against OpenAPI schema if validate is true.
+	// TODO Helm 4: Integrate into Build with an argument
+	BuildTable(reader io.Reader, validate bool) (ResourceList, error)
 }
 
 var _ Interface = (*Client)(nil)