From cde407b7d10a2ba2b2ba72466d90ce25fd953ee5 Mon Sep 17 00:00:00 2001 From: Chris Berry Date: Fri, 7 Jan 2022 13:37:19 +0000 Subject: [PATCH 01/12] Add hook annotations to output pod logs to client on success and fail Signed-off-by: Chris Berry --- pkg/action/action_test.go | 15 ++- pkg/action/hooks.go | 88 +++++++++++- pkg/action/hooks_test.go | 208 +++++++++++++++++++++++++++++ pkg/action/install_test.go | 4 + pkg/kube/client.go | 50 ++++++- pkg/kube/client_test.go | 35 +++++ pkg/kube/fake/printer.go | 20 ++- pkg/kube/interface.go | 7 + pkg/release/hook.go | 16 +++ pkg/releaseutil/manifest_sorter.go | 36 +++-- 10 files changed, 452 insertions(+), 27 deletions(-) create mode 100644 pkg/action/hooks_test.go diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index 71ea83789..47cff6ec1 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -111,6 +111,14 @@ type chartOptions struct { type chartOption func(*chartOptions) func buildChart(opts ...chartOption) *chart.Chart { + defaultTemplates := []*chart.File{ + {Name: "templates/hello", Data: []byte("hello: world")}, + {Name: "templates/hooks", Data: []byte(manifestWithHook)}, + } + return buildChartWithTemplates(defaultTemplates, opts...) +} + +func buildChartWithTemplates(templates []*chart.File, opts ...chartOption) *chart.Chart { c := &chartOptions{ Chart: &chart.Chart{ // TODO: This should be more complete. @@ -119,18 +127,13 @@ func buildChart(opts ...chartOption) *chart.Chart { Name: "hello", Version: "0.1.0", }, - // This adds a basic template and hooks. - Templates: []*chart.File{ - {Name: "templates/hello", Data: []byte("hello: world")}, - {Name: "templates/hooks", Data: []byte(manifestWithHook)}, - }, + Templates: templates, }, } for _, opt := range opts { opt(c) } - return c.Chart } diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index ecca1d997..95d843ce0 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -17,12 +17,19 @@ package action import ( "bytes" + "fmt" + "log" "sort" "time" + "helm.sh/helm/v4/pkg/kube" + + "helm.sh/helm/v4/pkg/chartutil" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/pkg/errors" - "helm.sh/helm/v4/pkg/kube" "helm.sh/helm/v4/pkg/release" helmtime "helm.sh/helm/v4/pkg/time" ) @@ -44,7 +51,7 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, for _, h := range executingHooks { // Set default delete policy to before-hook-creation - if len(h.DeletePolicies) == 0 { + if h.DeletePolicies == nil || len(h.DeletePolicies) == 0 { // TODO(jlegrone): Only apply before-hook-creation delete policy to run to completion // resources. For all other resource types update in place if a // resource with the same name already exists and is owned by the @@ -87,10 +94,18 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, // Mark hook as succeeded or failed if err != nil { h.LastRun.Phase = release.HookPhaseFailed + // If a hook is failed, check the annotation of the hook to determine if we should copy the logs client side + if errOutputting := cfg.outputLogsByPolicy(h, rl.Namespace, release.HookOutputOnFailed); errOutputting != nil { + // We log the error here as we want to propagate the hook failure upwards to the release object. + log.Printf("error outputting logs for hook failure: %v", errOutputting) + } // If a hook is failed, check the annotation of the hook to determine whether the hook should be deleted // under failed condition. If so, then clear the corresponding resource object in the hook - if err := cfg.deleteHookByPolicy(h, release.HookFailed, timeout); err != nil { - return err + if errDeleting := cfg.deleteHookByPolicy(h, release.HookFailed, timeout); err != nil { + // We log the error here as we want to propagate the hook failure upwards to the release object. + // This is a change in behaviour as the edge case previously would lose the hook error and only + // raise the delete hook error. + log.Printf("error the hook resource on hook failure: %v", errDeleting) } return err } @@ -98,9 +113,13 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, } // If all hooks are successful, check the annotation of each hook to determine whether the hook should be deleted - // under succeeded condition. If so, then clear the corresponding resource object in each hook + // or output should be logged under succeeded condition. If so, then clear the corresponding resource object in each hook for i := len(executingHooks) - 1; i >= 0; i-- { h := executingHooks[i] + if err := cfg.outputLogsByPolicy(h, rl.Namespace, release.HookOutputOnSucceeded); err != nil { + // We log here as we still want to attempt hook resource deletion even if output logging fails. + log.Printf("error outputting logs for hook failure: %v", err) + } if err := cfg.deleteHookByPolicy(h, release.HookSucceeded, timeout); err != nil { return err } @@ -158,3 +177,62 @@ func hookHasDeletePolicy(h *release.Hook, policy release.HookDeletePolicy) bool } return false } + +// outputLogsByPolicy outputs a pods logs if the hook policy instructs it to +func (cfg *Configuration) outputLogsByPolicy(h *release.Hook, releaseNamespace string, policy release.HookOutputLogPolicy) error { + if hookHasOutputLogPolicy(h, policy) { + namespace, err := cfg.deriveNamespace(h, releaseNamespace) + if err != nil { + return err + } + switch h.Kind { + case "Job": + return cfg.outputContainerLogsForListOptions(namespace, metav1.ListOptions{LabelSelector: fmt.Sprintf("job-name=%s", h.Name)}) + case "Pod": + return cfg.outputContainerLogsForListOptions(namespace, metav1.ListOptions{FieldSelector: fmt.Sprintf("metadata.name=%s", h.Name)}) + default: + return nil + } + } + return nil +} + +func (cfg *Configuration) outputContainerLogsForListOptions(namespace string, listOptions metav1.ListOptions) error { + //TODO Helm 4: Remove this check when GetPodList and OutputContainerLogsForPodList are moved from InterfaceExt to Interface + if kubeClient, ok := cfg.KubeClient.(kube.InterfaceExt); ok { + podList, err := kubeClient.GetPodList(namespace, listOptions) + if err != nil { + return err + } + err = kubeClient.OutputContainerLogsForPodList(podList, namespace, log.Writer()) + return err + } + return nil +} + +func (cfg *Configuration) deriveNamespace(h *release.Hook, namespace string) (string, error) { + values, err := chartutil.ReadValues([]byte(h.Manifest)) + if err != nil { + return "", errors.Wrapf(err, "unable to parse kubernetes manifest for output logs hook %s", h.Path) + } + value, err := values.PathValue("metadata.namespace") + switch err.(type) { + case nil: + return value.(string), nil + case chartutil.ErrNoValue: + return namespace, nil + default: + return "", errors.Wrapf(err, "unable to parse path of metadata.namespace in yaml for output logs hook %s", h.Path) + } +} + +// hookHasOutputLogPolicy determines whether the defined hook output log policy matches the hook output log policies +// supported by helm. +func hookHasOutputLogPolicy(h *release.Hook, policy release.HookOutputLogPolicy) bool { + for _, v := range h.OutputLogPolicies { + if policy == v { + return true + } + } + return false +} diff --git a/pkg/action/hooks_test.go b/pkg/action/hooks_test.go new file mode 100644 index 000000000..25a28f60f --- /dev/null +++ b/pkg/action/hooks_test.go @@ -0,0 +1,208 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package action + +import ( + "bytes" + "fmt" + "io/ioutil" + "testing" + + "github.com/stretchr/testify/assert" + + "helm.sh/helm/v3/pkg/chart" + kubefake "helm.sh/helm/v3/pkg/kube/fake" + "helm.sh/helm/v3/pkg/release" +) + +func podManifestWithOutputLogs(hookDefinitions []release.HookOutputLogPolicy) string { + hookDefinitionString := convertHooksToCommaSeparated(hookDefinitions) + return fmt.Sprintf(`kind: Pod +metadata: + name: finding-sharky, + annotations: + "helm.sh/hook": pre-install + "helm.sh/hook-output-log-policy": %s +spec: + containers: + - name: sharky-test + image: fake-image + cmd: fake-command`, hookDefinitionString) +} + +func podManifestWithOutputLogWithNamespace(hookDefinitions []release.HookOutputLogPolicy) string { + hookDefinitionString := convertHooksToCommaSeparated(hookDefinitions) + return fmt.Sprintf(`kind: Pod +metadata: + name: finding-george + namespace: sneaky-namespace + annotations: + "helm.sh/hook": pre-install + "helm.sh/hook-output-log-policy": %s +spec: + containers: + - name: george-test + image: fake-image + cmd: fake-command`, hookDefinitionString) +} + +func jobManifestWithOutputLog(hookDefinitions []release.HookOutputLogPolicy) string { + hookDefinitionString := convertHooksToCommaSeparated(hookDefinitions) + return fmt.Sprintf(`kind: Job +apiVersion: batch/v1 +metadata: + name: losing-religion + annotations: + "helm.sh/hook": pre-install + "helm.sh/hook-output-log-policy": %s +spec: + completions: 1 + parallelism: 1 + activeDeadlineSeconds: 30 + template: + spec: + containers: + - name: religion-container + image: religion-image + cmd: religion-command`, hookDefinitionString) +} + +func jobManifestWithOutputLogWithNamespace(hookDefinitions []release.HookOutputLogPolicy) string { + hookDefinitionString := convertHooksToCommaSeparated(hookDefinitions) + return fmt.Sprintf(`kind: Job +apiVersion: batch/v1 +metadata: + name: losing-religion + namespace: rem-namespace + annotations: + "helm.sh/hook": pre-install + "helm.sh/hook-output-log-policy": %s +spec: + completions: 1 + parallelism: 1 + activeDeadlineSeconds: 30 + template: + spec: + containers: + - name: religion-container + image: religion-image + cmd: religion-command`, hookDefinitionString) +} + +func convertHooksToCommaSeparated(hookDefinitions []release.HookOutputLogPolicy) string { + var commaSeparated string + for i, policy := range hookDefinitions { + if i+1 == len(hookDefinitions) { + commaSeparated += policy.String() + } else { + commaSeparated += policy.String() + "," + } + } + return commaSeparated +} + +func TestInstallRelease_HookOutputLogsOnFailure(t *testing.T) { + // Should output on failure with expected namespace if hook-failed is set + runInstallForHooksWithFailure(t, podManifestWithOutputLogs([]release.HookOutputLogPolicy{release.HookOutputOnFailed}), "spaced", true) + runInstallForHooksWithFailure(t, podManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnFailed}), "sneaky-namespace", true) + runInstallForHooksWithFailure(t, jobManifestWithOutputLog([]release.HookOutputLogPolicy{release.HookOutputOnFailed}), "spaced", true) + runInstallForHooksWithFailure(t, jobManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnFailed}), "rem-namespace", true) + + // Should not output on failure with expected namespace if hook-succeed is set + runInstallForHooksWithFailure(t, podManifestWithOutputLogs([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded}), "", false) + runInstallForHooksWithFailure(t, podManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded}), "", false) + runInstallForHooksWithFailure(t, jobManifestWithOutputLog([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded}), "", false) + runInstallForHooksWithFailure(t, jobManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded}), "", false) +} + +func TestInstallRelease_HookOutputLogsOnSuccess(t *testing.T) { + // Should output on success with expected namespace if hook-succeeded is set + runInstallForHooksWithSuccess(t, podManifestWithOutputLogs([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded}), "spaced", true) + runInstallForHooksWithSuccess(t, podManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded}), "sneaky-namespace", true) + runInstallForHooksWithSuccess(t, jobManifestWithOutputLog([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded}), "spaced", true) + runInstallForHooksWithSuccess(t, jobManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded}), "rem-namespace", true) + + // Should not output on success if hook-failed is set + runInstallForHooksWithSuccess(t, podManifestWithOutputLogs([]release.HookOutputLogPolicy{release.HookOutputOnFailed}), "", false) + runInstallForHooksWithSuccess(t, podManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnFailed}), "", false) + runInstallForHooksWithSuccess(t, jobManifestWithOutputLog([]release.HookOutputLogPolicy{release.HookOutputOnFailed}), "", false) + runInstallForHooksWithSuccess(t, jobManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnFailed}), "", false) +} + +func TestInstallRelease_HooksOutputLogsOnSuccessAndFailure(t *testing.T) { + // Should output on success with expected namespace if hook-succeeded and hook-failed is set + runInstallForHooksWithSuccess(t, podManifestWithOutputLogs([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded, release.HookOutputOnFailed}), "spaced", true) + runInstallForHooksWithSuccess(t, podManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded, release.HookOutputOnFailed}), "sneaky-namespace", true) + runInstallForHooksWithSuccess(t, jobManifestWithOutputLog([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded, release.HookOutputOnFailed}), "spaced", true) + runInstallForHooksWithSuccess(t, jobManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded, release.HookOutputOnFailed}), "rem-namespace", true) + + // Should output on failure if hook-succeeded and hook-failed is set + runInstallForHooksWithFailure(t, podManifestWithOutputLogs([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded, release.HookOutputOnFailed}), "spaced", true) + runInstallForHooksWithFailure(t, podManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded, release.HookOutputOnFailed}), "sneaky-namespace", true) + runInstallForHooksWithFailure(t, jobManifestWithOutputLog([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded, release.HookOutputOnFailed}), "spaced", true) + runInstallForHooksWithFailure(t, jobManifestWithOutputLogWithNamespace([]release.HookOutputLogPolicy{release.HookOutputOnSucceeded, release.HookOutputOnFailed}), "rem-namespace", true) +} + +func runInstallForHooksWithSuccess(t *testing.T, manifest, expectedNamespace string, shouldOutput bool) { + var expectedOutput string + if shouldOutput { + expectedOutput = fmt.Sprintf("attempted to output logs for namespace: %s", expectedNamespace) + } + is := assert.New(t) + instAction := installAction(t) + instAction.ReleaseName = "failed-hooks" + outBuffer := &bytes.Buffer{} + instAction.cfg.KubeClient = &kubefake.PrintingKubeClient{Out: ioutil.Discard, LogOutput: outBuffer} + + templates := []*chart.File{ + {Name: "templates/hello", Data: []byte("hello: world")}, + {Name: "templates/hooks", Data: []byte(manifest)}, + } + vals := map[string]interface{}{} + + res, err := instAction.Run(buildChartWithTemplates(templates), vals) + is.NoError(err) + is.Equal(expectedOutput, outBuffer.String()) + is.Equal(release.StatusDeployed, res.Info.Status) +} + +func runInstallForHooksWithFailure(t *testing.T, manifest, expectedNamespace string, shouldOutput bool) { + var expectedOutput string + if shouldOutput { + expectedOutput = fmt.Sprintf("attempted to output logs for namespace: %s", expectedNamespace) + } + is := assert.New(t) + instAction := installAction(t) + instAction.ReleaseName = "failed-hooks" + failingClient := instAction.cfg.KubeClient.(*kubefake.FailingKubeClient) + failingClient.WatchUntilReadyError = fmt.Errorf("failed watch") + instAction.cfg.KubeClient = failingClient + outBuffer := &bytes.Buffer{} + failingClient.PrintingKubeClient = kubefake.PrintingKubeClient{Out: ioutil.Discard, LogOutput: outBuffer} + + templates := []*chart.File{ + {Name: "templates/hello", Data: []byte("hello: world")}, + {Name: "templates/hooks", Data: []byte(manifest)}, + } + vals := map[string]interface{}{} + + res, err := instAction.Run(buildChartWithTemplates(templates), vals) + is.Error(err) + is.Contains(res.Info.Description, "failed pre-install") + is.Equal(expectedOutput, outBuffer.String()) + is.Equal(release.StatusFailed, res.Info.Status) +} diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index 9f738f0bc..a1eadf693 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -17,6 +17,7 @@ limitations under the License. package action import ( + "bytes" "context" "fmt" "io" @@ -354,11 +355,14 @@ func TestInstallRelease_FailedHooks(t *testing.T) { failer := instAction.cfg.KubeClient.(*kubefake.FailingKubeClient) failer.WatchUntilReadyError = fmt.Errorf("Failed watch") instAction.cfg.KubeClient = failer + outBuffer := &bytes.Buffer{} + failer.PrintingKubeClient = kubefake.PrintingKubeClient{Out: ioutil.Discard, LogOutput: outBuffer} vals := map[string]interface{}{} res, err := instAction.Run(buildChart(), vals) is.Error(err) is.Contains(res.Info.Description, "failed post-install") + is.Equal("", outBuffer.String()) is.Equal(release.StatusFailed, res.Info.Status) } diff --git a/pkg/kube/client.go b/pkg/kube/client.go index 0b84f5219..bf7e77c5a 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -29,6 +29,8 @@ import ( "sync" "time" + "k8s.io/client-go/rest" + jsonpatch "github.com/evanphx/json-patch" "github.com/pkg/errors" batch "k8s.io/api/batch/v1" @@ -83,7 +85,7 @@ type Client struct { // Namespace allows to bypass the kubeconfig file for the choice of the namespace Namespace string - kubeClient *kubernetes.Clientset + kubeClient kubernetes.Interface } func init() { @@ -111,7 +113,7 @@ func New(getter genericclioptions.RESTClientGetter) *Client { var nopLogger = func(_ string, _ ...interface{}) {} // getKubeClient get or create a new KubernetesClientSet -func (c *Client) getKubeClient() (*kubernetes.Clientset, error) { +func (c *Client) getKubeClient() (kubernetes.Interface, error) { var err error if c.kubeClient == nil { c.kubeClient, err = c.Factory.KubernetesClientSet() @@ -131,7 +133,7 @@ func (c *Client) IsReachable() error { if err != nil { return errors.Wrap(err, "Kubernetes cluster unreachable") } - if _, err := client.ServerVersion(); err != nil { + if _, err := client.Discovery().ServerVersion(); err != nil { return errors.Wrap(err, "Kubernetes cluster unreachable") } return nil @@ -812,6 +814,48 @@ func (c *Client) waitForPodSuccess(obj runtime.Object, name string) (bool, error return false, nil } +// GetPodList uses the kubernetes interface to get the list of pods filtered by listOptions +func (c *Client) GetPodList(namespace string, listOptions metav1.ListOptions) (*v1.PodList, error) { + podList, err := c.kubeClient.CoreV1().Pods(namespace).List(context.Background(), listOptions) + if err != nil { + return nil, fmt.Errorf("failed to get pod list with options: %+v with error: %v", listOptions, err) + } + return podList, nil +} + +// OutputContainerLogsForPodList is a helper that outputs logs for a list of pods +func (c *Client) OutputContainerLogsForPodList(podList *v1.PodList, namespace string, writer io.Writer) error { + for _, pod := range podList.Items { + for _, container := range pod.Spec.Containers { + options := &v1.PodLogOptions{ + Container: container.Name, + } + request := c.kubeClient.CoreV1().Pods(namespace).GetLogs(pod.Name, options) + err2 := copyRequestStreamToWriter(request, pod.Name, container.Name, writer) + if err2 != nil { + return err2 + } + } + } + return nil +} + +func copyRequestStreamToWriter(request *rest.Request, podName, containerName string, writer io.Writer) error { + readCloser, err := request.Stream(context.Background()) + if err != nil { + return errors.Errorf("Failed to stream pod logs for pod: %s, container: %s", podName, containerName) + } + defer readCloser.Close() + _, err = io.Copy(writer, readCloser) + if err != nil { + return errors.Errorf("Failed to copy IO from logs for pod: %s, container: %s", podName, containerName) + } + if err != nil { + return errors.Errorf("Failed to close reader for pod: %s, container: %s", podName, containerName) + } + return nil +} + // scrubValidationError removes kubectl info from the message. func scrubValidationError(err error) error { if err == nil { diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index f2d6bcb59..d9bd72783 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -24,10 +24,13 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/cli-runtime/pkg/resource" + k8sfake "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest/fake" cmdtesting "k8s.io/kubectl/pkg/cmd/testing" @@ -682,6 +685,38 @@ func TestReal(t *testing.T) { } } +func TestGetPodList(t *testing.T) { + + namespace := "some-namespace" + names := []string{"dave", "jimmy"} + var responsePodList v1.PodList + for _, name := range names { + responsePodList.Items = append(responsePodList.Items, newPodWithStatus(name, v1.PodStatus{}, namespace)) + } + + kubeClient := k8sfake.NewSimpleClientset(&responsePodList) + c := Client{Namespace: namespace, kubeClient: kubeClient} + + podList, err := c.GetPodList(namespace, metav1.ListOptions{}) + clientAssertions := assert.New(t) + clientAssertions.NoError(err) + clientAssertions.Equal(&responsePodList, podList) + +} + +func TestOutputContainerLogsForPodList(t *testing.T) { + namespace := "some-namespace" + somePodList := newPodList("jimmy", "three", "structs") + + kubeClient := k8sfake.NewSimpleClientset(&somePodList) + c := Client{Namespace: namespace, kubeClient: kubeClient} + outBuffer := &bytes.Buffer{} + err := c.OutputContainerLogsForPodList(&somePodList, namespace, outBuffer) + clientAssertions := assert.New(t) + clientAssertions.NoError(err) + clientAssertions.Equal("fake logsfake logsfake logs", outBuffer.String()) +} + const testServiceManifest = ` kind: Service apiVersion: v1 diff --git a/pkg/kube/fake/printer.go b/pkg/kube/fake/printer.go index 0b957d725..4b9a6d523 100644 --- a/pkg/kube/fake/printer.go +++ b/pkg/kube/fake/printer.go @@ -17,6 +17,7 @@ limitations under the License. package fake import ( + "fmt" "io" "strings" "time" @@ -31,7 +32,8 @@ import ( // PrintingKubeClient implements KubeClient, but simply prints the reader to // the given output. type PrintingKubeClient struct { - Out io.Writer + Out io.Writer + LogOutput io.Writer } // IsReachable checks if the cluster is reachable @@ -110,6 +112,22 @@ func (p *PrintingKubeClient) BuildTable(_ io.Reader, _ bool) (kube.ResourceList, return []*resource.Info{}, nil } +// WaitAndGetCompletedPodPhase implements KubeClient WaitAndGetCompletedPodPhase. +func (p *PrintingKubeClient) WaitAndGetCompletedPodPhase(_ string, _ time.Duration) (v1.PodPhase, error) { + return v1.PodSucceeded, nil +} + +// GetPodList implements KubeClient GetPodList. +func (p *PrintingKubeClient) GetPodList(_ string, _ metav1.ListOptions) (*v1.PodList, error) { + return &v1.PodList{}, nil +} + +// OutputContainerLogsForPodList implements KubeClient OutputContainerLogsForPodList. +func (p *PrintingKubeClient) OutputContainerLogsForPodList(_ *v1.PodList, someNamespace string, _ io.Writer) error { + _, err := io.Copy(p.LogOutput, strings.NewReader(fmt.Sprintf("attempted to output logs for namespace: %s", someNamespace))) + return err +} + // DeleteWithPropagationPolicy implements KubeClient delete. // // It only prints out the content to be deleted. diff --git a/pkg/kube/interface.go b/pkg/kube/interface.go index 7dc7ad8bc..4d295f560 100644 --- a/pkg/kube/interface.go +++ b/pkg/kube/interface.go @@ -20,6 +20,7 @@ import ( "io" "time" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -73,6 +74,12 @@ type Interface interface { type InterfaceExt interface { // WaitForDelete wait up to the given timeout for the specified resources to be deleted. WaitForDelete(resources ResourceList, timeout time.Duration) error + + // GetPodList list all pods that match the specified listOptions + GetPodList(namespace string, listOptions metav1.ListOptions) (*v1.PodList, error) + + // OutputContainerLogsForPodList output the logs for a pod list + OutputContainerLogsForPodList(podList *v1.PodList, namespace string, writer io.Writer) error } // InterfaceDeletionPropagation is introduced to avoid breaking backwards compatibility for Interface implementers. diff --git a/pkg/release/hook.go b/pkg/release/hook.go index e2cdc0eb8..5ff61fdaa 100644 --- a/pkg/release/hook.go +++ b/pkg/release/hook.go @@ -50,6 +50,17 @@ const ( func (x HookDeletePolicy) String() string { return string(x) } +// HookOutputLogPolicy specifies the hook output log policy +type HookOutputLogPolicy string + +// Hook output log policy types +const ( + HookOutputOnSucceeded HookOutputLogPolicy = "hook-succeeded" + HookOutputOnFailed HookOutputLogPolicy = "hook-failed" +) + +func (x HookOutputLogPolicy) String() string { return string(x) } + // HookAnnotation is the label name for a hook const HookAnnotation = "helm.sh/hook" @@ -59,6 +70,9 @@ const HookWeightAnnotation = "helm.sh/hook-weight" // HookDeleteAnnotation is the label name for the delete policy for a hook const HookDeleteAnnotation = "helm.sh/hook-delete-policy" +// HookOutputLogAnnotation is the label name for the output log policy for a hook +const HookOutputLogAnnotation = "helm.sh/hook-output-log-policy" + // Hook defines a hook object. type Hook struct { Name string `json:"name,omitempty"` @@ -76,6 +90,8 @@ type Hook struct { Weight int `json:"weight,omitempty"` // DeletePolicies are the policies that indicate when to delete the hook DeletePolicies []HookDeletePolicy `json:"delete_policies,omitempty"` + // OutputLogPolicies defines whether we should copy hook logs back to main process + OutputLogPolicies []HookOutputLogPolicy `json:"output_log_policies,omitempty"` } // A HookExecution records the result for the last execution of a hook for a given release. diff --git a/pkg/releaseutil/manifest_sorter.go b/pkg/releaseutil/manifest_sorter.go index b2db2ff9f..4aaae2d8c 100644 --- a/pkg/releaseutil/manifest_sorter.go +++ b/pkg/releaseutil/manifest_sorter.go @@ -123,11 +123,18 @@ func SortManifests(files map[string]string, _ chartutil.VersionSet, ordering Kin // // To determine the policy to delete the hook, it looks for a YAML structure like this: // -// kind: SomeKind -// apiVersion: v1 -// metadata: -// annotations: -// helm.sh/hook-delete-policy: hook-succeeded +// kind: SomeKind +// apiVersion: v1 +// metadata: +// annotations: +// helm.sh/hook-delete-policy: hook-succeeded +// To determine the policy to output logs of the hook (for Pod and Job only), it looks for a YAML structure like this: +// +// kind: Pod +// apiVersion: v1 +// metadata: +// annotations: +// helm.sh/hook-output-log-policy: hook-succeeded,hook-failed func (file *manifestFile) sort(result *result) error { // Go through manifests in order found in file (function `SplitManifests` creates integer-sortable keys) var sortedEntryKeys []string @@ -166,13 +173,14 @@ func (file *manifestFile) sort(result *result) error { hw := calculateHookWeight(entry) h := &release.Hook{ - Name: entry.Metadata.Name, - Kind: entry.Kind, - Path: file.path, - Manifest: m, - Events: []release.HookEvent{}, - Weight: hw, - DeletePolicies: []release.HookDeletePolicy{}, + Name: entry.Metadata.Name, + Kind: entry.Kind, + Path: file.path, + Manifest: m, + Events: []release.HookEvent{}, + Weight: hw, + DeletePolicies: []release.HookDeletePolicy{}, + OutputLogPolicies: []release.HookOutputLogPolicy{}, } isUnknownHook := false @@ -196,6 +204,10 @@ func (file *manifestFile) sort(result *result) error { operateAnnotationValues(entry, release.HookDeleteAnnotation, func(value string) { h.DeletePolicies = append(h.DeletePolicies, release.HookDeletePolicy(value)) }) + + operateAnnotationValues(entry, release.HookOutputLogAnnotation, func(value string) { + h.OutputLogPolicies = append(h.OutputLogPolicies, release.HookOutputLogPolicy(value)) + }) } return nil From 3964f84ac86a190b8e163554a056f67f09555cfe Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 3 May 2023 10:35:25 +0100 Subject: [PATCH 02/12] Tidy up imports Signed-off-by: Chris --- pkg/action/hooks_test.go | 6 +++--- pkg/action/install_test.go | 2 +- pkg/kube/client.go | 2 -- pkg/kube/fake/printer.go | 1 + 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/action/hooks_test.go b/pkg/action/hooks_test.go index 25a28f60f..76de9e505 100644 --- a/pkg/action/hooks_test.go +++ b/pkg/action/hooks_test.go @@ -19,7 +19,7 @@ package action import ( "bytes" "fmt" - "io/ioutil" + "io" "testing" "github.com/stretchr/testify/assert" @@ -166,7 +166,7 @@ func runInstallForHooksWithSuccess(t *testing.T, manifest, expectedNamespace str instAction := installAction(t) instAction.ReleaseName = "failed-hooks" outBuffer := &bytes.Buffer{} - instAction.cfg.KubeClient = &kubefake.PrintingKubeClient{Out: ioutil.Discard, LogOutput: outBuffer} + instAction.cfg.KubeClient = &kubefake.PrintingKubeClient{Out: io.Discard, LogOutput: outBuffer} templates := []*chart.File{ {Name: "templates/hello", Data: []byte("hello: world")}, @@ -192,7 +192,7 @@ func runInstallForHooksWithFailure(t *testing.T, manifest, expectedNamespace str failingClient.WatchUntilReadyError = fmt.Errorf("failed watch") instAction.cfg.KubeClient = failingClient outBuffer := &bytes.Buffer{} - failingClient.PrintingKubeClient = kubefake.PrintingKubeClient{Out: ioutil.Discard, LogOutput: outBuffer} + failingClient.PrintingKubeClient = kubefake.PrintingKubeClient{Out: io.Discard, LogOutput: outBuffer} templates := []*chart.File{ {Name: "templates/hello", Data: []byte("hello: world")}, diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index a1eadf693..f78fa40d2 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -356,7 +356,7 @@ func TestInstallRelease_FailedHooks(t *testing.T) { failer.WatchUntilReadyError = fmt.Errorf("Failed watch") instAction.cfg.KubeClient = failer outBuffer := &bytes.Buffer{} - failer.PrintingKubeClient = kubefake.PrintingKubeClient{Out: ioutil.Discard, LogOutput: outBuffer} + failer.PrintingKubeClient = kubefake.PrintingKubeClient{Out: io.Discard, LogOutput: outBuffer} vals := map[string]interface{}{} res, err := instAction.Run(buildChart(), vals) diff --git a/pkg/kube/client.go b/pkg/kube/client.go index bf7e77c5a..361111fed 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -29,8 +29,6 @@ import ( "sync" "time" - "k8s.io/client-go/rest" - jsonpatch "github.com/evanphx/json-patch" "github.com/pkg/errors" batch "k8s.io/api/batch/v1" diff --git a/pkg/kube/fake/printer.go b/pkg/kube/fake/printer.go index 4b9a6d523..65515812e 100644 --- a/pkg/kube/fake/printer.go +++ b/pkg/kube/fake/printer.go @@ -22,6 +22,7 @@ import ( "strings" "time" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/cli-runtime/pkg/resource" From a55a4770691fb7d98b2db8c8f66e20159396053d Mon Sep 17 00:00:00 2001 From: Chris Berry Date: Wed, 28 Aug 2024 21:14:17 +0100 Subject: [PATCH 03/12] Fix lint Signed-off-by: Chris Berry --- pkg/action/hooks.go | 6 +++--- pkg/releaseutil/manifest_sorter.go | 21 +++++++++++---------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 95d843ce0..b1e8e1d66 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -51,7 +51,7 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, for _, h := range executingHooks { // Set default delete policy to before-hook-creation - if h.DeletePolicies == nil || len(h.DeletePolicies) == 0 { + if len(h.DeletePolicies) == 0 { // TODO(jlegrone): Only apply before-hook-creation delete policy to run to completion // resources. For all other resource types update in place if a // resource with the same name already exists and is owned by the @@ -157,7 +157,7 @@ func (cfg *Configuration) deleteHookByPolicy(h *release.Hook, policy release.Hoo return errors.New(joinErrors(errs)) } - //wait for resources until they are deleted to avoid conflicts + // wait for resources until they are deleted to avoid conflicts if kubeClient, ok := cfg.KubeClient.(kube.InterfaceExt); ok { if err := kubeClient.WaitForDelete(resources, timeout); err != nil { return err @@ -198,7 +198,7 @@ func (cfg *Configuration) outputLogsByPolicy(h *release.Hook, releaseNamespace s } func (cfg *Configuration) outputContainerLogsForListOptions(namespace string, listOptions metav1.ListOptions) error { - //TODO Helm 4: Remove this check when GetPodList and OutputContainerLogsForPodList are moved from InterfaceExt to Interface + // TODO Helm 4: Remove this check when GetPodList and OutputContainerLogsForPodList are moved from InterfaceExt to Interface if kubeClient, ok := cfg.KubeClient.(kube.InterfaceExt); ok { podList, err := kubeClient.GetPodList(namespace, listOptions) if err != nil { diff --git a/pkg/releaseutil/manifest_sorter.go b/pkg/releaseutil/manifest_sorter.go index 4aaae2d8c..844ce161b 100644 --- a/pkg/releaseutil/manifest_sorter.go +++ b/pkg/releaseutil/manifest_sorter.go @@ -123,18 +123,19 @@ func SortManifests(files map[string]string, _ chartutil.VersionSet, ordering Kin // // To determine the policy to delete the hook, it looks for a YAML structure like this: // -// kind: SomeKind -// apiVersion: v1 -// metadata: -// annotations: -// helm.sh/hook-delete-policy: hook-succeeded +// kind: SomeKind +// apiVersion: v1 +// metadata: +// annotations: +// helm.sh/hook-delete-policy: hook-succeeded +// // To determine the policy to output logs of the hook (for Pod and Job only), it looks for a YAML structure like this: // -// kind: Pod -// apiVersion: v1 -// metadata: -// annotations: -// helm.sh/hook-output-log-policy: hook-succeeded,hook-failed +// kind: Pod +// apiVersion: v1 +// metadata: +// annotations: +// helm.sh/hook-output-log-policy: hook-succeeded,hook-failed func (file *manifestFile) sort(result *result) error { // Go through manifests in order found in file (function `SplitManifests` creates integer-sortable keys) var sortedEntryKeys []string From 3d4e679d9faad8167aeb99f4d02eed2ed99723de Mon Sep 17 00:00:00 2001 From: Chris Berry Date: Tue, 14 Jan 2025 10:49:28 +0000 Subject: [PATCH 04/12] Update based on review comments Signed-off-by: Chris Berry --- pkg/action/hooks.go | 2 +- pkg/action/hooks_test.go | 6 +++--- pkg/kube/interface.go | 10 ++++++++-- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index b1e8e1d66..021a563c3 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -199,7 +199,7 @@ func (cfg *Configuration) outputLogsByPolicy(h *release.Hook, releaseNamespace s func (cfg *Configuration) outputContainerLogsForListOptions(namespace string, listOptions metav1.ListOptions) error { // TODO Helm 4: Remove this check when GetPodList and OutputContainerLogsForPodList are moved from InterfaceExt to Interface - if kubeClient, ok := cfg.KubeClient.(kube.InterfaceExt); ok { + if kubeClient, ok := cfg.KubeClient.(kube.InterfaceLogs); ok { podList, err := kubeClient.GetPodList(namespace, listOptions) if err != nil { return err diff --git a/pkg/action/hooks_test.go b/pkg/action/hooks_test.go index 76de9e505..0f4a9be34 100644 --- a/pkg/action/hooks_test.go +++ b/pkg/action/hooks_test.go @@ -24,9 +24,9 @@ import ( "github.com/stretchr/testify/assert" - "helm.sh/helm/v3/pkg/chart" - kubefake "helm.sh/helm/v3/pkg/kube/fake" - "helm.sh/helm/v3/pkg/release" + "helm.sh/helm/v4/pkg/chart" + kubefake "helm.sh/helm/v4/pkg/kube/fake" + "helm.sh/helm/v4/pkg/release" ) func podManifestWithOutputLogs(hookDefinitions []release.HookOutputLogPolicy) string { diff --git a/pkg/kube/interface.go b/pkg/kube/interface.go index 4d295f560..f701690e3 100644 --- a/pkg/kube/interface.go +++ b/pkg/kube/interface.go @@ -68,13 +68,18 @@ type Interface interface { IsReachable() error } -// InterfaceExt is introduced to avoid breaking backwards compatibility for Interface implementers. +// InterfaceExt was introduced to avoid breaking backwards compatibility for Interface implementers. // // TODO Helm 4: Remove InterfaceExt and integrate its method(s) into the Interface. type InterfaceExt interface { // WaitForDelete wait up to the given timeout for the specified resources to be deleted. WaitForDelete(resources ResourceList, timeout time.Duration) error +} +// InterfaceLogs was introduced to avoid breaking backwards compatibility for Interface implementers. +// +// TODO Helm 4: Remove InterfaceLogs and integrate its method(s) into the Interface. +type InterfaceLogs interface { // GetPodList list all pods that match the specified listOptions GetPodList(namespace string, listOptions metav1.ListOptions) (*v1.PodList, error) @@ -86,7 +91,7 @@ type InterfaceExt interface { // // TODO Helm 4: Remove InterfaceDeletionPropagation and integrate its method(s) into the Interface. type InterfaceDeletionPropagation interface { - // Delete destroys one or more resources. The deletion propagation is handled as per the given deletion propagation value. + // DeleteWithPropagationPolicy destroys one or more resources. The deletion propagation is handled as per the given deletion propagation value. DeleteWithPropagationPolicy(resources ResourceList, policy metav1.DeletionPropagation) (*Result, []error) } @@ -114,5 +119,6 @@ type InterfaceResources interface { var _ Interface = (*Client)(nil) var _ InterfaceExt = (*Client)(nil) +var _ InterfaceLogs = (*Client)(nil) var _ InterfaceDeletionPropagation = (*Client)(nil) var _ InterfaceResources = (*Client)(nil) From 243cb2e21f38767314e2dcd5b827b5aa4adc94fb Mon Sep 17 00:00:00 2001 From: Chris Berry Date: Tue, 14 Jan 2025 11:09:40 +0000 Subject: [PATCH 05/12] Update based on review comments Signed-off-by: Chris Berry --- pkg/action/hooks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 021a563c3..539f8e7c1 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -198,7 +198,7 @@ func (cfg *Configuration) outputLogsByPolicy(h *release.Hook, releaseNamespace s } func (cfg *Configuration) outputContainerLogsForListOptions(namespace string, listOptions metav1.ListOptions) error { - // TODO Helm 4: Remove this check when GetPodList and OutputContainerLogsForPodList are moved from InterfaceExt to Interface + // TODO Helm 4: Remove this check when GetPodList and OutputContainerLogsForPodList are moved from InterfaceLogs to Interface if kubeClient, ok := cfg.KubeClient.(kube.InterfaceLogs); ok { podList, err := kubeClient.GetPodList(namespace, listOptions) if err != nil { From f729b9ade059579ee53182bafcb399a8b7d86f5a Mon Sep 17 00:00:00 2001 From: Scott Rigby Date: Thu, 20 Feb 2025 12:36:11 -0500 Subject: [PATCH 06/12] add short circuit return Co-authored-by: George Jenkins Signed-off-by: Scott Rigby --- pkg/action/hooks.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 539f8e7c1..310e6d372 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -180,7 +180,9 @@ func hookHasDeletePolicy(h *release.Hook, policy release.HookDeletePolicy) bool // outputLogsByPolicy outputs a pods logs if the hook policy instructs it to func (cfg *Configuration) outputLogsByPolicy(h *release.Hook, releaseNamespace string, policy release.HookOutputLogPolicy) error { - if hookHasOutputLogPolicy(h, policy) { + if !hookHasOutputLogPolicy(h, policy) { + return nil + } namespace, err := cfg.deriveNamespace(h, releaseNamespace) if err != nil { return err From 3796c1f4a171d33b6622e118ae812435574fdadb Mon Sep 17 00:00:00 2001 From: Scott Rigby Date: Thu, 20 Feb 2025 12:37:41 -0500 Subject: [PATCH 07/12] remove comments about previous functionality Signed-off-by: Scott Rigby --- pkg/action/hooks.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 310e6d372..370aa9a67 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -103,8 +103,6 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, // under failed condition. If so, then clear the corresponding resource object in the hook if errDeleting := cfg.deleteHookByPolicy(h, release.HookFailed, timeout); err != nil { // We log the error here as we want to propagate the hook failure upwards to the release object. - // This is a change in behaviour as the edge case previously would lose the hook error and only - // raise the delete hook error. log.Printf("error the hook resource on hook failure: %v", errDeleting) } return err From e8a76bc3eb2a9e1ce9a246164d9d555031eb055e Mon Sep 17 00:00:00 2001 From: Scott Rigby Date: Thu, 20 Feb 2025 13:34:09 -0500 Subject: [PATCH 08/12] fix err check Co-authored-by: George Jenkins Signed-off-by: Scott Rigby --- pkg/action/hooks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 370aa9a67..ebe8be6ba 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -101,7 +101,7 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, } // If a hook is failed, check the annotation of the hook to determine whether the hook should be deleted // under failed condition. If so, then clear the corresponding resource object in the hook - if errDeleting := cfg.deleteHookByPolicy(h, release.HookFailed, timeout); err != nil { + if errDeleting := cfg.deleteHookByPolicy(h, release.HookFailed, timeout); errDeleting != nil { // We log the error here as we want to propagate the hook failure upwards to the release object. log.Printf("error the hook resource on hook failure: %v", errDeleting) } From 52ac92fb690bb6c4f8d09900330608b88f745ab2 Mon Sep 17 00:00:00 2001 From: Scott Rigby Date: Thu, 20 Feb 2025 13:34:54 -0500 Subject: [PATCH 09/12] clarify fix error message Signed-off-by: Scott Rigby --- pkg/action/hooks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index ebe8be6ba..dadcb27f6 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -103,7 +103,7 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, // under failed condition. If so, then clear the corresponding resource object in the hook if errDeleting := cfg.deleteHookByPolicy(h, release.HookFailed, timeout); errDeleting != nil { // We log the error here as we want to propagate the hook failure upwards to the release object. - log.Printf("error the hook resource on hook failure: %v", errDeleting) + log.Printf("error deleting the hook resource on hook failure: %v", errDeleting) } return err } From 6d30fa59904f1936613e0882d3e3c2608351da0b Mon Sep 17 00:00:00 2001 From: Chris Berry Date: Fri, 21 Feb 2025 12:33:12 +0000 Subject: [PATCH 10/12] Add HookOutputFunc and generic yaml unmarshaller Signed-off-by: Chris Berry --- pkg/action/action.go | 13 ++++++++- pkg/action/hooks.go | 57 ++++++++++++++++++---------------------- pkg/kube/client.go | 4 +-- pkg/kube/client_test.go | 3 ++- pkg/kube/fake/printer.go | 2 +- pkg/kube/interface.go | 2 +- 6 files changed, 43 insertions(+), 38 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index 8418a4c27..0d81a63cb 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -19,6 +19,8 @@ package action import ( "bytes" "fmt" + "io" + "log" "os" "path" "path/filepath" @@ -95,6 +97,9 @@ type Configuration struct { Capabilities *chartutil.Capabilities Log func(string, ...interface{}) + + // HookOutputFunc Called with container name and returns and expects writer that will receive the log output + HookOutputFunc func(namespace, pod, container string) io.Writer } // renderResources renders the templates in a chart @@ -122,7 +127,7 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu var err2 error // A `helm template` should not talk to the remote cluster. However, commands with the flag - //`--dry-run` with the value of `false`, `none`, or `server` should try to interact with the cluster. + // `--dry-run` with the value of `false`, `none`, or `server` should try to interact with the cluster. // It may break in interesting and exotic ways because other data (e.g. discovery) is mocked. if interactWithRemote && cfg.RESTClientGetter != nil { restConfig, err := cfg.RESTClientGetter.ToRESTConfig() @@ -422,6 +427,12 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp cfg.KubeClient = kc cfg.Releases = store cfg.Log = log + cfg.HookOutputFunc = defaultHookOutputWriter return nil } + +// defaultHookOutputWriter will write the Hook logs to log.Writer(). +func defaultHookOutputWriter(_, _, _ string) io.Writer { + return log.Writer() +} diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index dadcb27f6..b6c505807 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -19,16 +19,16 @@ import ( "bytes" "fmt" "log" + "slices" "sort" "time" "helm.sh/helm/v4/pkg/kube" - "helm.sh/helm/v4/pkg/chartutil" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/pkg/errors" + "gopkg.in/yaml.v3" "helm.sh/helm/v4/pkg/release" helmtime "helm.sh/helm/v4/pkg/time" @@ -179,22 +179,20 @@ func hookHasDeletePolicy(h *release.Hook, policy release.HookDeletePolicy) bool // outputLogsByPolicy outputs a pods logs if the hook policy instructs it to func (cfg *Configuration) outputLogsByPolicy(h *release.Hook, releaseNamespace string, policy release.HookOutputLogPolicy) error { if !hookHasOutputLogPolicy(h, policy) { - return nil + return nil } - namespace, err := cfg.deriveNamespace(h, releaseNamespace) - if err != nil { - return err - } - switch h.Kind { - case "Job": - return cfg.outputContainerLogsForListOptions(namespace, metav1.ListOptions{LabelSelector: fmt.Sprintf("job-name=%s", h.Name)}) - case "Pod": - return cfg.outputContainerLogsForListOptions(namespace, metav1.ListOptions{FieldSelector: fmt.Sprintf("metadata.name=%s", h.Name)}) - default: - return nil - } + namespace, err := cfg.deriveNamespace(h, releaseNamespace) + if err != nil { + return err + } + switch h.Kind { + case "Job": + return cfg.outputContainerLogsForListOptions(namespace, metav1.ListOptions{LabelSelector: fmt.Sprintf("job-name=%s", h.Name)}) + case "Pod": + return cfg.outputContainerLogsForListOptions(namespace, metav1.ListOptions{FieldSelector: fmt.Sprintf("metadata.name=%s", h.Name)}) + default: + return nil } - return nil } func (cfg *Configuration) outputContainerLogsForListOptions(namespace string, listOptions metav1.ListOptions) error { @@ -204,35 +202,30 @@ func (cfg *Configuration) outputContainerLogsForListOptions(namespace string, li if err != nil { return err } - err = kubeClient.OutputContainerLogsForPodList(podList, namespace, log.Writer()) + err = kubeClient.OutputContainerLogsForPodList(podList, namespace, cfg.HookOutputFunc) return err } return nil } func (cfg *Configuration) deriveNamespace(h *release.Hook, namespace string) (string, error) { - values, err := chartutil.ReadValues([]byte(h.Manifest)) + tmp := struct { + Metadata struct { + Namespace string + } + }{} + err := yaml.Unmarshal([]byte(h.Manifest), &tmp) if err != nil { - return "", errors.Wrapf(err, "unable to parse kubernetes manifest for output logs hook %s", h.Path) + return "", errors.Wrapf(err, "unable to parse metadata.namespace from kubernetes manifest for output logs hook %s", h.Path) } - value, err := values.PathValue("metadata.namespace") - switch err.(type) { - case nil: - return value.(string), nil - case chartutil.ErrNoValue: + if tmp.Metadata.Namespace == "" { return namespace, nil - default: - return "", errors.Wrapf(err, "unable to parse path of metadata.namespace in yaml for output logs hook %s", h.Path) } + return tmp.Metadata.Namespace, nil } // hookHasOutputLogPolicy determines whether the defined hook output log policy matches the hook output log policies // supported by helm. func hookHasOutputLogPolicy(h *release.Hook, policy release.HookOutputLogPolicy) bool { - for _, v := range h.OutputLogPolicies { - if policy == v { - return true - } - } - return false + return slices.Contains(h.OutputLogPolicies, policy) } diff --git a/pkg/kube/client.go b/pkg/kube/client.go index 361111fed..fd111c647 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -822,14 +822,14 @@ func (c *Client) GetPodList(namespace string, listOptions metav1.ListOptions) (* } // OutputContainerLogsForPodList is a helper that outputs logs for a list of pods -func (c *Client) OutputContainerLogsForPodList(podList *v1.PodList, namespace string, writer io.Writer) error { +func (c *Client) OutputContainerLogsForPodList(podList *v1.PodList, namespace string, writerFunc func(namespace, pod, container string) io.Writer) error { for _, pod := range podList.Items { for _, container := range pod.Spec.Containers { options := &v1.PodLogOptions{ Container: container.Name, } request := c.kubeClient.CoreV1().Pods(namespace).GetLogs(pod.Name, options) - err2 := copyRequestStreamToWriter(request, pod.Name, container.Name, writer) + err2 := copyRequestStreamToWriter(request, pod.Name, container.Name, writerFunc(namespace, pod.Name, container.Name)) if err2 != nil { return err2 } diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index d9bd72783..ff1335f0f 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -711,7 +711,8 @@ func TestOutputContainerLogsForPodList(t *testing.T) { kubeClient := k8sfake.NewSimpleClientset(&somePodList) c := Client{Namespace: namespace, kubeClient: kubeClient} outBuffer := &bytes.Buffer{} - err := c.OutputContainerLogsForPodList(&somePodList, namespace, outBuffer) + outBufferFunc := func(_, _, _ string) io.Writer { return outBuffer } + err := c.OutputContainerLogsForPodList(&somePodList, namespace, outBufferFunc) clientAssertions := assert.New(t) clientAssertions.NoError(err) clientAssertions.Equal("fake logsfake logsfake logs", outBuffer.String()) diff --git a/pkg/kube/fake/printer.go b/pkg/kube/fake/printer.go index 65515812e..dcce9a3be 100644 --- a/pkg/kube/fake/printer.go +++ b/pkg/kube/fake/printer.go @@ -124,7 +124,7 @@ func (p *PrintingKubeClient) GetPodList(_ string, _ metav1.ListOptions) (*v1.Pod } // OutputContainerLogsForPodList implements KubeClient OutputContainerLogsForPodList. -func (p *PrintingKubeClient) OutputContainerLogsForPodList(_ *v1.PodList, someNamespace string, _ io.Writer) error { +func (p *PrintingKubeClient) OutputContainerLogsForPodList(_ *v1.PodList, someNamespace string, _ func(namespace, pod, container string) io.Writer) error { _, err := io.Copy(p.LogOutput, strings.NewReader(fmt.Sprintf("attempted to output logs for namespace: %s", someNamespace))) return err } diff --git a/pkg/kube/interface.go b/pkg/kube/interface.go index f701690e3..c9776cacd 100644 --- a/pkg/kube/interface.go +++ b/pkg/kube/interface.go @@ -84,7 +84,7 @@ type InterfaceLogs interface { GetPodList(namespace string, listOptions metav1.ListOptions) (*v1.PodList, error) // OutputContainerLogsForPodList output the logs for a pod list - OutputContainerLogsForPodList(podList *v1.PodList, namespace string, writer io.Writer) error + OutputContainerLogsForPodList(podList *v1.PodList, namespace string, writerFunc func(namespace, pod, container string) io.Writer) error } // InterfaceDeletionPropagation is introduced to avoid breaking backwards compatibility for Interface implementers. From 9791767baa9d3e4d460c25e921f06172b2cec53f Mon Sep 17 00:00:00 2001 From: Chris Berry Date: Fri, 21 Feb 2025 16:16:26 +0000 Subject: [PATCH 11/12] Refactor based on review comment Signed-off-by: Chris Berry --- cmd/helm/helm.go | 7 ++++++- cmd/helm/list.go | 2 +- pkg/action/action.go | 19 ++++++++++--------- pkg/action/action_test.go | 2 +- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/cmd/helm/helm.go b/cmd/helm/helm.go index aa981740f..3d5c17030 100644 --- a/cmd/helm/helm.go +++ b/cmd/helm/helm.go @@ -57,6 +57,11 @@ func warning(format string, v ...interface{}) { fmt.Fprintf(os.Stderr, format, v...) } +// hookOutputWriter provides the writer for writing hook logs. +func hookOutputWriter(_, _, _ string) io.Writer { + return log.Writer() +} + func main() { // Setting the name of the app for managedFields in the Kubernetes client. // It is set here to the full name of "helm" so that renaming of helm to @@ -74,7 +79,7 @@ func main() { // run when each command's execute method is called cobra.OnInitialize(func() { helmDriver := os.Getenv("HELM_DRIVER") - if err := actionConfig.Init(settings.RESTClientGetter(), settings.Namespace(), helmDriver, debug); err != nil { + if err := actionConfig.Init(settings.RESTClientGetter(), settings.Namespace(), helmDriver, debug, hookOutputWriter); err != nil { log.Fatal(err) } if helmDriver == "memory" { diff --git a/cmd/helm/list.go b/cmd/helm/list.go index 67da22cdf..f9bf336d4 100644 --- a/cmd/helm/list.go +++ b/cmd/helm/list.go @@ -71,7 +71,7 @@ func newListCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { ValidArgsFunction: noMoreArgsCompFunc, RunE: func(cmd *cobra.Command, _ []string) error { if client.AllNamespaces { - if err := cfg.Init(settings.RESTClientGetter(), "", os.Getenv("HELM_DRIVER"), debug); err != nil { + if err := cfg.Init(settings.RESTClientGetter(), "", os.Getenv("HELM_DRIVER"), debug, nil); err != nil { return err } } diff --git a/pkg/action/action.go b/pkg/action/action.go index 0d81a63cb..dfb263269 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -20,7 +20,6 @@ import ( "bytes" "fmt" "io" - "log" "os" "path" "path/filepath" @@ -98,7 +97,7 @@ type Configuration struct { Log func(string, ...interface{}) - // HookOutputFunc Called with container name and returns and expects writer that will receive the log output + // HookOutputFunc called with container name and returns and expects writer that will receive the log output. HookOutputFunc func(namespace, pod, container string) io.Writer } @@ -247,6 +246,9 @@ type RESTClientGetter interface { // DebugLog sets the logger that writes debug strings type DebugLog func(format string, v ...interface{}) +// HookOutputFunc returns the io.Writer for outputting hook logs. +type HookOutputFunc func(namespace, pod, container string) io.Writer + // capabilities builds a Capabilities from discovery information. func (cfg *Configuration) getCapabilities() (*chartutil.Capabilities, error) { if cfg.Capabilities != nil { @@ -375,7 +377,7 @@ func (cfg *Configuration) recordRelease(r *release.Release) { } // Init initializes the action configuration -func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namespace, helmDriver string, log DebugLog) error { +func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namespace, helmDriver string, log DebugLog, outputFunc HookOutputFunc) error { kc := kube.New(getter) kc.Log = log @@ -427,12 +429,11 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp cfg.KubeClient = kc cfg.Releases = store cfg.Log = log - cfg.HookOutputFunc = defaultHookOutputWriter + if outputFunc != nil { + cfg.HookOutputFunc = outputFunc + } else { + cfg.HookOutputFunc = func(_, _, _ string) io.Writer { return io.Discard } + } return nil } - -// defaultHookOutputWriter will write the Hook logs to log.Writer(). -func defaultHookOutputWriter(_, _, _ string) io.Writer { - return log.Writer() -} diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index 47cff6ec1..2d1516edb 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -334,7 +334,7 @@ func TestConfiguration_Init(t *testing.T) { t.Run(tt.name, func(t *testing.T) { cfg := &Configuration{} - actualErr := cfg.Init(nil, "default", tt.helmDriver, nil) + actualErr := cfg.Init(nil, "default", tt.helmDriver, nil, nil) if tt.expectErr { assert.Error(t, actualErr) assert.Contains(t, actualErr.Error(), tt.errMsg) From e5bc21c56b5b384acf77dbc3589694d03477cb27 Mon Sep 17 00:00:00 2001 From: Chris Berry Date: Fri, 21 Feb 2025 16:33:31 +0000 Subject: [PATCH 12/12] Refactor based on review comment Signed-off-by: Chris Berry --- cmd/helm/helm.go | 3 ++- cmd/helm/list.go | 2 +- pkg/action/action.go | 16 +++++++--------- pkg/action/action_test.go | 2 +- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/cmd/helm/helm.go b/cmd/helm/helm.go index 3d5c17030..c8de18796 100644 --- a/cmd/helm/helm.go +++ b/cmd/helm/helm.go @@ -79,12 +79,13 @@ func main() { // run when each command's execute method is called cobra.OnInitialize(func() { helmDriver := os.Getenv("HELM_DRIVER") - if err := actionConfig.Init(settings.RESTClientGetter(), settings.Namespace(), helmDriver, debug, hookOutputWriter); err != nil { + if err := actionConfig.Init(settings.RESTClientGetter(), settings.Namespace(), helmDriver, debug); err != nil { log.Fatal(err) } if helmDriver == "memory" { loadReleasesInMemory(actionConfig) } + actionConfig.SetHookOutputFunc(hookOutputWriter) }) if err := cmd.Execute(); err != nil { diff --git a/cmd/helm/list.go b/cmd/helm/list.go index f9bf336d4..67da22cdf 100644 --- a/cmd/helm/list.go +++ b/cmd/helm/list.go @@ -71,7 +71,7 @@ func newListCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { ValidArgsFunction: noMoreArgsCompFunc, RunE: func(cmd *cobra.Command, _ []string) error { if client.AllNamespaces { - if err := cfg.Init(settings.RESTClientGetter(), "", os.Getenv("HELM_DRIVER"), debug, nil); err != nil { + if err := cfg.Init(settings.RESTClientGetter(), "", os.Getenv("HELM_DRIVER"), debug); err != nil { return err } } diff --git a/pkg/action/action.go b/pkg/action/action.go index dfb263269..745f02138 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -246,9 +246,6 @@ type RESTClientGetter interface { // DebugLog sets the logger that writes debug strings type DebugLog func(format string, v ...interface{}) -// HookOutputFunc returns the io.Writer for outputting hook logs. -type HookOutputFunc func(namespace, pod, container string) io.Writer - // capabilities builds a Capabilities from discovery information. func (cfg *Configuration) getCapabilities() (*chartutil.Capabilities, error) { if cfg.Capabilities != nil { @@ -377,7 +374,7 @@ func (cfg *Configuration) recordRelease(r *release.Release) { } // Init initializes the action configuration -func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namespace, helmDriver string, log DebugLog, outputFunc HookOutputFunc) error { +func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namespace, helmDriver string, log DebugLog) error { kc := kube.New(getter) kc.Log = log @@ -429,11 +426,12 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp cfg.KubeClient = kc cfg.Releases = store cfg.Log = log - if outputFunc != nil { - cfg.HookOutputFunc = outputFunc - } else { - cfg.HookOutputFunc = func(_, _, _ string) io.Writer { return io.Discard } - } + cfg.HookOutputFunc = func(_, _, _ string) io.Writer { return io.Discard } return nil } + +// SetHookOutputFunc sets the HookOutputFunc on the Configuration. +func (cfg *Configuration) SetHookOutputFunc(hookOutputFunc func(_, _, _ string) io.Writer) { + cfg.HookOutputFunc = hookOutputFunc +} diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index 2d1516edb..47cff6ec1 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -334,7 +334,7 @@ func TestConfiguration_Init(t *testing.T) { t.Run(tt.name, func(t *testing.T) { cfg := &Configuration{} - actualErr := cfg.Init(nil, "default", tt.helmDriver, nil, nil) + actualErr := cfg.Init(nil, "default", tt.helmDriver, nil) if tt.expectErr { assert.Error(t, actualErr) assert.Contains(t, actualErr.Error(), tt.errMsg)