From 472c5736ab01133de504a826bd9ee12cbe4e7904 Mon Sep 17 00:00:00 2001 From: Matt Farina Date: Thu, 12 Jan 2023 08:38:10 -0500 Subject: [PATCH] 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 (cherry picked from commit 36e18fa6e16049b5e5ec8ca4f9fefd76e6abd212) --- cmd/helm/status.go | 7 +++ pkg/action/status.go | 26 +++++++-- pkg/kube/client.go | 113 +++++++++++++++++++++++++++------------ pkg/kube/client_test.go | 39 ++++++++++++++ pkg/kube/fake/fake.go | 19 +++++++ pkg/kube/fake/printer.go | 7 ++- pkg/kube/interface.go | 18 ++++++- 7 files changed, 188 insertions(+), 41 deletions(-) diff --git a/cmd/helm/status.go b/cmd/helm/status.go index a482bf18d..aa22aa02a 100644 --- a/cmd/helm/status.go +++ b/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 diff --git a/pkg/action/status.go b/pkg/action/status.go index 290610330..ee1c9d613 100644 --- a/pkg/action/status.go +++ b/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") } diff --git a/pkg/kube/client.go b/pkg/kube/client.go index 540b39ee7..d30e5c535 100644 --- a/pkg/kube/client.go +++ b/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 { diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index de5358aee..9a17387a9 100644 --- a/pkg/kube/client_test.go +++ b/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 diff --git a/pkg/kube/fake/fake.go b/pkg/kube/fake/fake.go index 0fc953116..fb38c3654 100644 --- a/pkg/kube/fake/fake.go +++ b/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 { diff --git a/pkg/kube/fake/printer.go b/pkg/kube/fake/printer.go index a57580e02..267aa1ff8 100644 --- a/pkg/kube/fake/printer.go +++ b/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 diff --git a/pkg/kube/interface.go b/pkg/kube/interface.go index 79cf73755..11f948e34 100644 --- a/pkg/kube/interface.go +++ b/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)