support for more helm template args (#4926)

* support for more helm template args

* move templateArgs and unit tests to api/types

* undo package name change

* use our own simple helm chart instead of forking one

* add argument to AsHelmArgs

* code review

* lint errors

api/internal/builtins/HelmChartInflationGenerator.go

@@ -16,6 +16,7 @@ import (
 	"sigs.k8s.io/kustomize/api/resmap"
 	"sigs.k8s.io/kustomize/api/types"
 	"sigs.k8s.io/kustomize/kyaml/errors"
+	"sigs.k8s.io/kustomize/kyaml/kio"
 	"sigs.k8s.io/yaml"
 )
 
@@ -86,12 +87,20 @@ func (p *HelmChartInflationGeneratorPlugin) validateArgs() (err error) {
 		p.ChartHome = types.HelmDefaultHome
 	}
 
-	// The ValuesFile may be consulted by the plugin, so it must
+	// The ValuesFile(s) may be consulted by the plugin, so it must
 	// be under the loader root (unless root restrictions are
 	// disabled).
 	if p.ValuesFile == "" {
 		p.ValuesFile = filepath.Join(p.ChartHome, p.Name, "values.yaml")
 	}
+	for i, file := range p.AdditionalValuesFiles {
+		// use Load() to enforce root restrictions
+		if _, err := p.h.Loader().Load(file); err != nil {
+			return errors.WrapPrefixf(err, "could not load additionalValuesFile")
+		}
+		// the additional values filepaths must be relative to the kust root
+		p.AdditionalValuesFiles[i] = filepath.Join(p.h.Loader().Root(), file)
+	}
 
 	if err = p.errIfIllegalValuesMerge(); err != nil {
 		return err
@@ -240,49 +249,28 @@ func (p *HelmChartInflationGeneratorPlugin) Generate() (rm resmap.ResMap, err er
 		return nil, err
 	}
 	var stdout []byte
-	stdout, err = p.runHelmCommand(p.templateCommand())
+	stdout, err = p.runHelmCommand(p.AsHelmArgs(p.absChartHome()))
 	if err != nil {
 		return nil, err
 	}
 
-	rm, err = p.h.ResmapFactory().NewResMapFromBytes(stdout)
-	if err == nil {
+	rm, resMapErr := p.h.ResmapFactory().NewResMapFromBytes(stdout)
+	if resMapErr == nil {
 		return rm, nil
 	}
 	// try to remove the contents before first "---" because
 	// helm may produce messages to stdout before it
-	stdoutStr := string(stdout)
-	if idx := strings.Index(stdoutStr, "\n---\n"); idx != -1 {
-		return p.h.ResmapFactory().NewResMapFromBytes([]byte(stdoutStr[idx:]))
-	}
-	return nil, err
-}
+	r := &kio.ByteReader{Reader: bytes.NewBufferString(string(stdout)), OmitReaderAnnotations: true}
+	nodes, err := r.Read()
 
-func (p *HelmChartInflationGeneratorPlugin) templateCommand() []string {
-	args := []string{"template"}
-	if p.ReleaseName != "" {
-		args = append(args, p.ReleaseName)
+	if len(nodes) != 0 {
+		rm, err = p.h.ResmapFactory().NewResMapFromRNodeSlice(nodes)
+		if err != nil {
+			return nil, fmt.Errorf("could not parse rnode slice into resource map: %w\n", err)
+		}
+		return rm, nil
 	}
-	if p.Namespace != "" {
-		args = append(args, "--namespace", p.Namespace)
-	}
-	args = append(args, filepath.Join(p.absChartHome(), p.Name))
-	if p.ValuesFile != "" {
-		args = append(args, "--values", p.ValuesFile)
-	}
-	if p.ReleaseName == "" {
-		// AFAICT, this doesn't work as intended due to a bug in helm.
-		// See https://github.com/helm/helm/issues/6019
-		// I've tried placing the flag before and after the name argument.
-		args = append(args, "--generate-name")
-	}
-	if p.IncludeCRDs {
-		args = append(args, "--include-crds")
-	}
-	if p.SkipHooks {
-		args = append(args, "--no-hooks")
-	}
-	return args
+	return nil, fmt.Errorf("could not parse bytes into resource map: %w\n", resMapErr)
 }
 
 func (p *HelmChartInflationGeneratorPlugin) pullCommand() []string {

api/krusty/helmchartinflationgenerator_test.go

@@ -4,9 +4,12 @@
 package krusty_test
 
 import (
+	"path/filepath"
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
+	"sigs.k8s.io/kustomize/kyaml/copyutil"
 )
 
 const expectedHelm = `
@@ -233,3 +236,193 @@ spec:
   type: ClusterIP
 `)
 }
+
+func TestHelmChartInflationGeneratorMultipleValuesFiles(t *testing.T) {
+	th := kusttest_test.MakeEnhancedHarnessWithTmpRoot(t)
+	defer th.Reset()
+	if err := th.ErrIfNoHelm(); err != nil {
+		t.Skip("skipping: " + err.Error())
+	}
+
+	copyValuesFilesTestChartsIntoHarness(t, th)
+
+	th.WriteK(th.GetRoot(), `
+helmCharts:
+  - name: test-chart
+    releaseName: test-chart
+    additionalValuesFiles:
+    - charts/valuesFiles/file1.yaml
+    - charts/valuesFiles/file2.yaml
+`)
+
+	m := th.Run(th.GetRoot(), th.MakeOptionsPluginsEnabled())
+	asYaml, err := m.AsYaml()
+	require.NoError(t, err)
+	require.Equal(t, string(asYaml), `apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    chart: test-1.0.0
+  name: my-deploy
+  namespace: file-2
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: test
+  template:
+    spec:
+      containers:
+      - image: test-image-file1:file1
+        imagePullPolicy: Never
+---
+apiVersion: apps/v1
+kind: Pod
+metadata:
+  annotations:
+    helm.sh/hook: test
+  name: test-chart
+`)
+}
+
+func TestHelmChartInflationGeneratorApiVersions(t *testing.T) {
+	th := kusttest_test.MakeEnhancedHarnessWithTmpRoot(t)
+	defer th.Reset()
+	if err := th.ErrIfNoHelm(); err != nil {
+		t.Skip("skipping: " + err.Error())
+	}
+
+	copyValuesFilesTestChartsIntoHarness(t, th)
+
+	th.WriteK(th.GetRoot(), `
+helmCharts:
+  - name: test-chart
+    releaseName: test-chart
+    apiVersions:
+    - foo/v1
+`)
+
+	m := th.Run(th.GetRoot(), th.MakeOptionsPluginsEnabled())
+	asYaml, err := m.AsYaml()
+	require.NoError(t, err)
+	require.Equal(t, string(asYaml), `apiVersion: foo/v1
+kind: Deployment
+metadata:
+  labels:
+    chart: test-1.0.0
+  name: my-deploy
+  namespace: default
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: test
+  template:
+    spec:
+      containers:
+      - image: test-image:v1.0.0
+        imagePullPolicy: Always
+---
+apiVersion: foo/v1
+kind: Pod
+metadata:
+  annotations:
+    helm.sh/hook: test
+  name: test-chart
+`)
+}
+
+func TestHelmChartInflationGeneratorSkipTests(t *testing.T) {
+	th := kusttest_test.MakeEnhancedHarnessWithTmpRoot(t)
+	defer th.Reset()
+	if err := th.ErrIfNoHelm(); err != nil {
+		t.Skip("skipping: " + err.Error())
+	}
+
+	copyValuesFilesTestChartsIntoHarness(t, th)
+
+	th.WriteK(th.GetRoot(), `
+helmCharts:
+  - name: test-chart
+    releaseName: test-chart
+    skipTests: true
+`)
+
+	m := th.Run(th.GetRoot(), th.MakeOptionsPluginsEnabled())
+	asYaml, err := m.AsYaml()
+	require.NoError(t, err)
+	require.Equal(t, string(asYaml), `apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    chart: test-1.0.0
+  name: my-deploy
+  namespace: default
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: test
+  template:
+    spec:
+      containers:
+      - image: test-image:v1.0.0
+        imagePullPolicy: Always
+`)
+}
+
+func TestHelmChartInflationGeneratorNameTemplate(t *testing.T) {
+	th := kusttest_test.MakeEnhancedHarnessWithTmpRoot(t)
+	defer th.Reset()
+	if err := th.ErrIfNoHelm(); err != nil {
+		t.Skip("skipping: " + err.Error())
+	}
+
+	copyValuesFilesTestChartsIntoHarness(t, th)
+
+	th.WriteK(th.GetRoot(), `
+helmCharts:
+  - name: test-chart
+    nameTemplate: name-template
+`)
+
+	m := th.Run(th.GetRoot(), th.MakeOptionsPluginsEnabled())
+	asYaml, err := m.AsYaml()
+	require.NoError(t, err)
+	require.Equal(t, string(asYaml), `apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    chart: test-1.0.0
+  name: my-deploy
+  namespace: default
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: test
+  template:
+    spec:
+      containers:
+      - image: test-image:v1.0.0
+        imagePullPolicy: Always
+---
+apiVersion: apps/v1
+kind: Pod
+metadata:
+  annotations:
+    helm.sh/hook: test
+  name: name-template
+`)
+}
+
+func copyValuesFilesTestChartsIntoHarness(t *testing.T, th *kusttest_test.HarnessEnhanced) {
+	t.Helper()
+
+	thDir := filepath.Join(th.GetRoot(), "charts")
+	chartDir := "testdata/helmcharts"
+
+	fs := th.GetFSys()
+	require.NoError(t, fs.MkdirAll(filepath.Join(thDir, "templates")))
+	require.NoError(t, copyutil.CopyDir(th.GetFSys(), chartDir, thDir))
+}

api/krusty/testdata/helmcharts/test-chart/Chart.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+appVersion: "1.0"
+description: A simple test helm chart.
+name: test
+version: 1.0.0

api/krusty/testdata/helmcharts/test-chart/README.md

@@ -0,0 +1 @@
+This is a simple test chart.

api/krusty/testdata/helmcharts/test-chart/templates/_helpers.tpl

@@ -0,0 +1,7 @@
+{{- define "apiversion" -}}
+{{- if .Capabilities.APIVersions.Has "foo/v1" -}}
+foo/v1
+{{- else -}}
+apps/v1
+{{- end -}}
+{{- end -}}

api/krusty/testdata/helmcharts/test-chart/templates/deployment.yaml

@@ -0,0 +1,18 @@
+---
+apiVersion: {{ template "apiversion" . }}
+kind: Deployment
+metadata:
+  labels:
+    chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+  name: my-deploy
+  namespace: {{ .Values.data.namespace }}
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: {{ .Chart.Name }}
+  template:
+    spec:
+      containers:
+        - image: "{{ .Values.data.image.name }}:{{ .Values.data.image.tag }}"
+          imagePullPolicy: {{ .Values.data.image.imagePullPolicy }}

api/krusty/testdata/helmcharts/test-chart/templates/tests/test-pod.yaml

@@ -0,0 +1,6 @@
+apiVersion: {{ template "apiversion" . }}
+kind: Pod
+metadata:
+  name: {{ .Release.Name }}
+  annotations:
+    "helm.sh/hook": test

api/krusty/testdata/helmcharts/test-chart/values.yaml

@@ -0,0 +1,6 @@
+data:
+  namespace: default
+  image:
+    name: test-image
+    tag: v1.0.0
+    imagePullPolicy: Always

api/krusty/testdata/helmcharts/valuesFiles/file1.yaml

@@ -0,0 +1,5 @@
+data:
+  image:
+    name: test-image-file1
+    tag: file1
+    imagePullPolicy: Never

api/krusty/testdata/helmcharts/valuesFiles/file2.yaml

@@ -0,0 +1,2 @@
+data:
+  namespace: file-2

api/types/helmchartargs.go

@@ -3,6 +3,8 @@
 
 package types
 
+import "path/filepath"
+
 const HelmDefaultHome = "charts"
 
 type HelmGlobals struct {
@@ -57,7 +59,11 @@ type HelmChart struct {
 	// in the helm template
 	Namespace string `json:"namespace,omitempty" yaml:"namespace,omitempty"`
 
-	// ValuesFile is local file path to a values file to use _instead of_
+	// AdditionalValuesFiles are local file paths to values files to be used in
+	// addition to either the default values file or the values specified in ValuesFile.
+	AdditionalValuesFiles []string `json:"additionalValuesFiles,omitempty" yaml:"additionalValuesFiles,omitempty"`
+
+	// ValuesFile is a local file path to a values file to use _instead of_
 	// the default values that accompanied the chart.
 	// The default values are in '{ChartHome}/{Name}/values.yaml'.
 	ValuesFile string `json:"valuesFile,omitempty" yaml:"valuesFile,omitempty"`
@@ -78,6 +84,15 @@ type HelmChart struct {
 	// SkipHooks sets the --no-hooks flag when calling helm template. This prevents
 	// helm from erroneously rendering test templates.
 	SkipHooks bool `json:"skipHooks,omitempty" yaml:"skipHooks,omitempty"`
+
+	// ApiVersions is the kubernetes apiversions used for Capabilities.APIVersions
+	ApiVersions []string `json:"apiVersions,omitempty" yaml:"apiVersions,omitempty"`
+
+	// NameTemplate is for specifying the name template used to name the release.
+	NameTemplate string `json:"nameTemplate,omitempty" yaml:"nameTemplate,omitempty"`
+
+	// SkipTests skips tests from templated output.
+	SkipTests bool `json:"skipTests,omitempty" yaml:"skipTests,omitempty"`
 }
 
 // HelmChartArgs contains arguments to helm.
@@ -126,3 +141,45 @@ func makeHelmChartFromHca(old *HelmChartArgs) (c HelmChart) {
 	c.ReleaseName = old.ReleaseName
 	return
 }
+
+func (h HelmChart) AsHelmArgs(absChartHome string) []string {
+	args := []string{"template"}
+	if h.ReleaseName != "" {
+		args = append(args, h.ReleaseName)
+	} else {
+		// AFAICT, this doesn't work as intended due to a bug in helm.
+		// See https://github.com/helm/helm/issues/6019
+		// I've tried placing the flag before and after the name argument.
+		args = append(args, "--generate-name")
+	}
+	if h.Name != "" {
+		args = append(args, filepath.Join(absChartHome, h.Name))
+	}
+	if h.Namespace != "" {
+		args = append(args, "--namespace", h.Namespace)
+	}
+	if h.NameTemplate != "" {
+		args = append(args, "--name-template", h.NameTemplate)
+	}
+
+	if h.ValuesFile != "" {
+		args = append(args, "-f", h.ValuesFile)
+	}
+	for _, valuesFile := range h.AdditionalValuesFiles {
+		args = append(args, "-f", valuesFile)
+	}
+
+	for _, apiVer := range h.ApiVersions {
+		args = append(args, "--api-versions", apiVer)
+	}
+	if h.IncludeCRDs {
+		args = append(args, "--include-crds")
+	}
+	if h.SkipTests {
+		args = append(args, "--skip-tests")
+	}
+	if h.SkipHooks {
+		args = append(args, "--no-hooks")
+	}
+	return args
+}

api/types/helmchartargs_test.go

@@ -0,0 +1,61 @@
+// Copyright 2019 The Kubernetes Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package types_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"sigs.k8s.io/kustomize/api/types"
+)
+
+func TestAsHelmArgs(t *testing.T) {
+	t.Run("use generate-name", func(t *testing.T) {
+		p := types.HelmChart{
+			Name:                  "chart-name",
+			Version:               "1.0.0",
+			Repo:                  "https://helm.releases.hashicorp.com",
+			ApiVersions:           []string{"foo", "bar"},
+			NameTemplate:          "template",
+			SkipTests:             true,
+			IncludeCRDs:           true,
+			SkipHooks:             true,
+			ValuesFile:            "values",
+			AdditionalValuesFiles: []string{"values1", "values2"},
+			Namespace:             "my-ns",
+		}
+		require.Equal(t, p.AsHelmArgs("/home/charts"),
+			[]string{"template", "--generate-name",
+				"/home/charts/chart-name",
+				"--namespace", "my-ns",
+				"--name-template", "template",
+				"-f", "values",
+				"-f", "values1", "-f", "values2",
+				"--api-versions", "foo", "--api-versions", "bar",
+				"--include-crds",
+				"--skip-tests",
+				"--no-hooks"})
+	})
+
+	t.Run("use release-name", func(t *testing.T) {
+		p := types.HelmChart{
+			Name:                  "chart-name",
+			Version:               "1.0.0",
+			Repo:                  "https://helm.releases.hashicorp.com",
+			ApiVersions:           []string{"foo", "bar"},
+			NameTemplate:          "template",
+			ValuesFile:            "values",
+			AdditionalValuesFiles: []string{"values1", "values2"},
+			Namespace:             "my-ns",
+			ReleaseName:           "test",
+		}
+		require.Equal(t, p.AsHelmArgs("/home/charts"),
+			[]string{"template", "test", "/home/charts/chart-name",
+				"--namespace", "my-ns",
+				"--name-template", "template",
+				"-f", "values",
+				"-f", "values1", "-f", "values2",
+				"--api-versions", "foo", "--api-versions", "bar"})
+	})
+}