Repository: helm/helm
Pull request #8527 (2020-07-28 14:00:53)

Fix issue with install and upgrade running all hooks
When #8156 was merged it had the side effect that all hooks were
run all the time. All the hooks were put in the flow of the
content rendered and sent to Kubernetes on every command.

For example, if you ran the following 2 commands the test hooks
would run:

    helm create foo
    helm install foo ./foo

This should not run any hooks. But, the generated test hook is run.

The change in this commit moves the writing of the hooks to output
or disk back into the template command rather than in a private
function within the actions. This is where it was for v3.2.

One side effect is that post renderers will not work on hooks. This
was the case in v3.2. Since this bug is blocking the release of v3.3.0
it is being rolled back. A refactor effort is underway for this section
of code. post renderer for hooks should be added back as part of that
work. Since post renderer hooks did not make it into a release it
is ok to roll it back for now.

There is code in the cmd/helm package that has been duplicated from
pkg/action. This is a temporary measure to fix the immediate bug
with plans to correct the situation as part of a refactor
of renderResources.

**What this PR does / why we need it**:

It fixes a release blocking bug to v3.3.0. v3.3.0-rc.1 currently runs all hooks on all commands (e.g., `helm install`).

**Special notes for your reviewer**:

@technosophos and I discussed this. He's volunteered to refactor this section of code for v3.4. If he doesn't, I will. There are mixes of concerns and some other bugs (e.g., post renderers don't work when helm template writes to an output directory). The other bugs are not regressions and were there in v3.2 so they are not release blockers.



Patch:
diff --git a/Makefile b/Makefile
index 97f99fd86..f8325480a 100644
--- a/Makefile
+++ b/Makefile
@@ -60,7 +60,7 @@ all: build
 build: $(BINDIR)/$(BINNAME)
 
 $(BINDIR)/$(BINNAME): $(SRC)
-	GO111MODULE=on go build $(GOFLAGS) -tags '$(TAGS)' -ldflags '$(LDFLAGS)' -o '$(BINDIR)'/$(BINNAME) ./cmd/helm
+	GO111MODULE=on go build $(GOFLAGS) -tags '$(TAGS)' -ldflags '$(LDFLAGS)' -o $(BINDIR)/$(BINNAME) ./cmd/helm
 
 # ------------------------------------------------------------------------------
 #  test
@@ -97,7 +97,7 @@ test-acceptance: TARGETS = linux/amd64
 test-acceptance: build build-cross
 	@if [ -d "${ACCEPTANCE_DIR}" ]; then \
 		cd ${ACCEPTANCE_DIR} && \
-			ROBOT_RUN_TESTS=$(ACCEPTANCE_RUN_TESTS) ROBOT_HELM_PATH='$(BINDIR)' make acceptance; \
+			ROBOT_RUN_TESTS=$(ACCEPTANCE_RUN_TESTS) ROBOT_HELM_PATH=$(BINDIR) make acceptance; \
 	else \
 		echo "You must clone the acceptance_testing repo under $(ACCEPTANCE_DIR)"; \
 		echo "You can find the acceptance_testing repo at https://github.com/helm/acceptance-testing"; \
@@ -178,7 +178,7 @@ checksum:
 
 .PHONY: clean
 clean:
-	@rm -rf '$(BINDIR)' ./_dist
+	@rm -rf $(BINDIR) ./_dist
 
 .PHONY: release-notes
 release-notes:
diff --git a/cmd/helm/template.go b/cmd/helm/template.go
index 5a8a25102..6123d29d4 100644
--- a/cmd/helm/template.go
+++ b/cmd/helm/template.go
@@ -20,6 +20,8 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"os"
+	"path"
 	"path/filepath"
 	"regexp"
 	"sort"
@@ -79,6 +81,25 @@ func newTemplateCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 			if rel != nil {
 				var manifests bytes.Buffer
 				fmt.Fprintln(&manifests, strings.TrimSpace(rel.Manifest))
+				if !client.DisableHooks {
+					fileWritten := make(map[string]bool)
+					for _, m := range rel.Hooks {
+						if client.OutputDir == "" {
+							fmt.Fprintf(&manifests, "---\n# Source: %s\n%s\n", m.Path, m.Manifest)
+						} else {
+							newDir := client.OutputDir
+							if client.UseReleaseName {
+								newDir = filepath.Join(client.OutputDir, client.ReleaseName)
+							}
+							err = writeToFile(newDir, m.Path, m.Manifest, fileWritten[m.Path])
+							if err != nil {
+								return err
+							}
+							fileWritten[m.Path] = true
+						}
+
+					}
+				}
 
 				// if we have a list of files to render, then check that each of the
 				// provided files exists in the chart.
@@ -149,3 +170,50 @@ func newTemplateCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 
 	return cmd
 }
+
+// The following functions (writeToFile, createOrOpenFile, and ensureDirectoryForFile)
+// are coppied from the actions package. This is part of a change to correct a
+// bug introduced by #8156. As part of the todo to refactor renderResources
+// this duplicate code should be removed. It is added here so that the API
+// surface area is as minimally impacted as possible in fixing the issue.
+func writeToFile(outputDir string, name string, data string, append bool) error {
+	outfileName := strings.Join([]string{outputDir, name}, string(filepath.Separator))
+
+	err := ensureDirectoryForFile(outfileName)
+	if err != nil {
+		return err
+	}
+
+	f, err := createOrOpenFile(outfileName, append)
+	if err != nil {
+		return err
+	}
+
+	defer f.Close()
+
+	_, err = f.WriteString(fmt.Sprintf("---\n# Source: %s\n%s\n", name, data))
+
+	if err != nil {
+		return err
+	}
+
+	fmt.Printf("wrote %s\n", outfileName)
+	return nil
+}
+
+func createOrOpenFile(filename string, append bool) (*os.File, error) {
+	if append {
+		return os.OpenFile(filename, os.O_APPEND|os.O_WRONLY, 0600)
+	}
+	return os.Create(filename)
+}
+
+func ensureDirectoryForFile(file string) error {
+	baseDir := path.Dir(file)
+	_, err := os.Stat(baseDir)
+	if err != nil && !os.IsNotExist(err) {
+		return err
+	}
+
+	return os.MkdirAll(baseDir, 0755)
+}
diff --git a/pkg/action/action.go b/pkg/action/action.go
index fec86cd42..3c47cc6df 100644
--- a/pkg/action/action.go
+++ b/pkg/action/action.go
@@ -99,7 +99,9 @@ type Configuration struct {
 // renderResources renders the templates in a chart
 //
 // TODO: This function is badly in need of a refactor.
-func (c *Configuration) renderResources(ch *chart.Chart, values chartutil.Values, releaseName, outputDir string, subNotes, useReleaseName, includeCrds bool, disableHooks bool, pr postrender.PostRenderer, dryRun bool) ([]*release.Hook, *bytes.Buffer, string, error) {
+// TODO: As part of the refactor the duplicate code in cmd/helm/template.go should be removed
+//       This code has to do with writing files to dick.
+func (c *Configuration) renderResources(ch *chart.Chart, values chartutil.Values, releaseName, outputDir string, subNotes, useReleaseName, includeCrds bool, pr postrender.PostRenderer, dryRun bool) ([]*release.Hook, *bytes.Buffer, string, error) {
 	hs := []*release.Hook{}
 	b := bytes.NewBuffer(nil)
 
@@ -212,24 +214,6 @@ func (c *Configuration) renderResources(ch *chart.Chart, values chartutil.Values
 		}
 	}
 
-	if !disableHooks && len(hs) > 0 {
-		for _, h := range hs {
-			if outputDir == "" {
-				fmt.Fprintf(b, "---\n# Source: %s\n%s\n", h.Path, h.Manifest)
-			} else {
-				newDir := outputDir
-				if useReleaseName {
-					newDir = filepath.Join(outputDir, releaseName)
-				}
-				err = writeToFile(newDir, h.Path, h.Manifest, fileWritten[h.Path])
-				if err != nil {
-					return hs, b, "", err
-				}
-				fileWritten[h.Path] = true
-			}
-		}
-	}
-
 	if pr != nil {
 		b, err = pr.Run(b)
 		if err != nil {
diff --git a/pkg/action/install.go b/pkg/action/install.go
index 48a3aeeca..00fb208b0 100644
--- a/pkg/action/install.go
+++ b/pkg/action/install.go
@@ -236,7 +236,7 @@ func (i *Install) Run(chrt *chart.Chart, vals map[string]interface{}) (*release.
 	rel := i.createRelease(chrt, vals)
 
 	var manifestDoc *bytes.Buffer
-	rel.Hooks, manifestDoc, rel.Info.Notes, err = i.cfg.renderResources(chrt, valuesToRender, i.ReleaseName, i.OutputDir, i.SubNotes, i.UseReleaseName, i.IncludeCRDs, i.DisableHooks, i.PostRenderer, i.DryRun)
+	rel.Hooks, manifestDoc, rel.Info.Notes, err = i.cfg.renderResources(chrt, valuesToRender, i.ReleaseName, i.OutputDir, i.SubNotes, i.UseReleaseName, i.IncludeCRDs, i.PostRenderer, i.DryRun)
 	// Even for errors, attach this if available
 	if manifestDoc != nil {
 		rel.Manifest = manifestDoc.String()
diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go
index 949aebcae..e7c2aec25 100644
--- a/pkg/action/upgrade.go
+++ b/pkg/action/upgrade.go
@@ -222,7 +222,7 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin
 		return nil, nil, err
 	}
 
-	hooks, manifestDoc, notesTxt, err := u.cfg.renderResources(chart, valuesToRender, "", "", u.SubNotes, false, false, false, u.PostRenderer, u.DryRun)
+	hooks, manifestDoc, notesTxt, err := u.cfg.renderResources(chart, valuesToRender, "", "", u.SubNotes, false, false, u.PostRenderer, u.DryRun)
 	if err != nil {
 		return nil, nil, err
 	}


Test patch:
diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go
index 4366889ce..6c4012cfd 100644
--- a/pkg/action/install_test.go
+++ b/pkg/action/install_test.go
@@ -499,9 +499,6 @@ func TestInstallReleaseOutputDir(t *testing.T) {
 	_, err = os.Stat(filepath.Join(dir, "hello/templates/with-partials"))
 	is.NoError(err)
 
-	_, err = os.Stat(filepath.Join(dir, "hello/templates/hooks"))
-	is.NoError(err)
-
 	_, err = os.Stat(filepath.Join(dir, "hello/templates/rbac"))
 	is.NoError(err)
 
@@ -542,9 +539,6 @@ func TestInstallOutputDirWithReleaseName(t *testing.T) {
 	_, err = os.Stat(filepath.Join(newDir, "hello/templates/with-partials"))
 	is.NoError(err)
 
-	_, err = os.Stat(filepath.Join(newDir, "hello/templates/hooks"))
-	is.NoError(err)
-
 	_, err = os.Stat(filepath.Join(newDir, "hello/templates/rbac"))
 	is.NoError(err)