Add configuration for hook log output

pkg/action/action.go

@@ -19,6 +19,7 @@ package action
 import (
 	"bytes"
 	"fmt"
+	"io"
 	"os"
 	"path"
 	"path/filepath"
@@ -94,6 +95,9 @@ type Configuration struct {
 	// Capabilities describes the capabilities of the Kubernetes cluster.
 	Capabilities *chartutil.Capabilities
 
+	// Called with container name and returns and expects writer that will receive the log output
+	HookOutputFunc func(namespace string, pod string, container string) io.Writer
+
 	Log func(string, ...interface{})
 }
 
@@ -422,5 +426,10 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp
 	cfg.Releases = store
 	cfg.Log = log
 
+	cfg.HookOutputFunc = func(namespace string, pod string, container string) io.Writer {
+		fmt.Fprint(os.Stdout, fmt.Sprintf("Logs for pod: %s, container: %s:\n", pod, container))
+		return os.Stdout
+	}
+
 	return nil
 }

pkg/action/action_test.go

@@ -109,6 +109,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.
@@ -117,18 +125,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
 }
 

pkg/action/hooks.go

@@ -17,9 +17,17 @@ package action
 
 import (
 	"bytes"
+	"fmt"
+	"log"
 	"sort"
 	"time"
 
+	"helm.sh/helm/v3/pkg/kube"
+
+	"helm.sh/helm/v3/pkg/chartutil"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	"github.com/pkg/errors"
 
 	"helm.sh/helm/v3/pkg/release"
@@ -86,19 +94,31 @@ 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); err != nil {
-				return err
+			if errDeleting := cfg.deleteHookByPolicy(h, release.HookFailed); errDeleting != 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
 		}
 		h.LastRun.Phase = release.HookPhaseSucceeded
 	}
 
 	// 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 _, h := range executingHooks {
+		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); err != nil {
 			return err
 		}
@@ -149,3 +169,63 @@ 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
+		}
+		logWriterFunc := cfg.HookOutputFunc
+		err = kubeClient.OutputContainerLogsForPodList(podList, namespace, logWriterFunc)
+		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
+}

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"
+	"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: io.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: io.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)
+}