feat: `inherit` field for release template inheritance (#606)

* feat: `inherit` field for release template inheritance

Ref https://github.com/helmfile/helmfile/issues/435#issuecomment-1364749414

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

* Fix wording

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

* Comment on releaseWithInheritedTemplate

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

* Update Release Template doc with the new `inherit` feature

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

* Fix a typo in code comment

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

docs/writing-helmfile.md

@@ -85,7 +85,7 @@ It allows you to abstract away the repetitions in releases into a template, whic
 
 ```yaml
 templates:
-  default: &default
+  default:
     chart: stable/{{`{{ .Release.Name }}`}}
     namespace: kube-system
     # This prevents helmfile exiting when it encounters a missing file
@@ -102,10 +102,12 @@ templates:
 releases:
 - name: heapster
   version: 0.3.2
-  <<: *default
+  inherit:
+    template: default
 - name: kubernetes-dashboard
   version: 0.10.0
-  <<: *default
+  inherit:
+    template: default
 ```
 
 Release Templating supports the following parts of release definition:
@@ -144,7 +146,13 @@ Release Templating supports the following parts of release definition:
   # ...
   ```
 
-See the [issue 428](https://github.com/roboll/helmfile/issues/428) for more context on how this is supposed to work.
+Previously, we've been using YAML anchors for release template inheritance.
+It turned out not work well when you wanted to nest templates for complex use cases and/or you want a fine control over which fields to inherit or not.
+Thus we added a new way for inheritance, which uses the `inherit` field we introduced above.
+
+See [issue helmfile/helmfile#435](https://github.com/helmfile/helmfile/issues/435#issuecomment-1362177510) for more context.
+
+You might also find [issue roboll/helmfile#428](https://github.com/roboll/helmfile/issues/428) useful for more context on how we originally designed the relase template and what it's supposed to solve.
 
 ## Layering Release Values
 

pkg/app/app_template_test.go

@@ -392,3 +392,94 @@ releases:
 		})
 	})
 }
+
+func TestTemplate_CyclicInheritance(t *testing.T) {
+	type testcase struct {
+		ns    string
+		error string
+	}
+
+	check := func(t *testing.T, tc testcase) {
+		t.Helper()
+
+		var helm = &exectest.Helm{
+			FailOnUnexpectedList: true,
+			FailOnUnexpectedDiff: true,
+			DiffMutex:            &sync.Mutex{},
+			ChartsMutex:          &sync.Mutex{},
+			ReleasesMutex:        &sync.Mutex{},
+		}
+
+		_ = runWithLogCapture(t, "debug", func(t *testing.T, logger *zap.SugaredLogger) {
+			t.Helper()
+
+			valsRuntime, err := vals.New(vals.Options{CacheSize: 32})
+			if err != nil {
+				t.Errorf("unexpected error creating vals runtime: %v", err)
+			}
+
+			files := map[string]string{
+				"/path/to/helmfile.yaml": `
+templates:
+  a:
+    inherit:
+      template: b
+    values:
+    - a.yaml
+  b:
+    inherit:
+      template: c
+    values:
+    - b.yaml
+  c:
+    inherit:
+      template: a
+    values:
+    - c.yaml
+releases:
+- name: app1
+  inherit:
+    template: a
+  chart: incubator/raw
+`,
+			}
+
+			app := appWithFs(&App{
+				OverrideHelmBinary:  DefaultHelmBinary,
+				fs:                  &ffs.FileSystem{Glob: filepath.Glob},
+				OverrideKubeContext: "default",
+				Env:                 "default",
+				Logger:              logger,
+				helms: map[helmKey]helmexec.Interface{
+					createHelmKey("helm", "default"): helm,
+				},
+				valsRuntime: valsRuntime,
+			}, files)
+
+			if tc.ns != "" {
+				app.Namespace = tc.ns
+			}
+
+			tmplErr := app.Template(applyConfig{
+				// if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic.
+				concurrency: 1,
+				logger:      logger,
+			})
+
+			var gotErr string
+			if tmplErr != nil {
+				gotErr = tmplErr.Error()
+			}
+
+			if d := cmp.Diff(tc.error, gotErr); d != "" {
+				t.Fatalf("unexpected error: want (-), got (+): %s", d)
+			}
+		})
+	}
+
+	t.Run("fail due to cyclic inheritance", func(t *testing.T) {
+		check(t, testcase{
+			error: `in ./helmfile.yaml: failed executing release templates in "helmfile.yaml": unable to load release "app1" with template: cyclic inheritance detected: a->b->c->a`,
+		})
+	})
+}

pkg/state/state.go

@@ -198,6 +198,11 @@ type RepositorySpec struct {
 	SkipTLSVerify   string `yaml:"skipTLSVerify,omitempty"`
 }
 
+type Inherit struct {
+	Template string   `yaml:"template,omitempty"`
+	Except   []string `yaml:"except,omitempty"`
+}
+
 // ReleaseSpec defines the structure of a helm release
 type ReleaseSpec struct {
 	// Chart is the name of the chart being installed to create this release
@@ -340,6 +345,9 @@ type ReleaseSpec struct {
 
 	// Propagate '--post-renderer' to helmv3 template and helm install
 	PostRenderer *string `yaml:"postRenderer,omitempty"`
+
+	// Inherit is used to inherit a release template from a release or another release template
+	Inherit Inherit `yaml:"inherit,omitempty"`
 }
 
 // ChartPathOrName returns ChartPath if it is non-empty, and returns Chart otherwise.

pkg/state/state_exec_tmpl.go

@@ -1,8 +1,12 @@
 package state
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
+	"strings"
+
+	"github.com/imdario/mergo"
 
 	"github.com/helmfile/helmfile/pkg/tmpl"
 	"github.com/helmfile/helmfile/pkg/yaml"
@@ -89,7 +93,15 @@ func (st *HelmState) ExecuteTemplates() (*HelmState, error) {
 	vals := st.Values()
 
 	for i, rt := range st.Releases {
-		release := rt
+		release, err := st.releaseWithInheritedTemplate(&rt, nil)
+		if err != nil {
+			var cyclicInheritanceErr CyclicReleaseTemplateInheritanceError
+			if errors.As(err, &cyclicInheritanceErr) {
+				return nil, fmt.Errorf("unable to load release %q with template: %w", rt.Name, cyclicInheritanceErr)
+			}
+			return nil, err
+		}
+
 		if release.KubeContext == "" {
 			release.KubeContext = r.HelmDefaults.KubeContext
 		}
@@ -107,7 +119,7 @@ func (st *HelmState) ExecuteTemplates() (*HelmState, error) {
 		}
 
 		successFlag := false
-		for it, prev := 0, &release; it < 6; it++ {
+		for it, prev := 0, release; it < 6; it++ {
 			tmplData := st.createReleaseTemplateData(prev, vals)
 			renderer := tmpl.NewFileRenderer(st.fs, st.basePath, tmplData)
 			r, err := release.ExecuteTemplateExpressions(renderer)
@@ -132,3 +144,81 @@ func (st *HelmState) ExecuteTemplates() (*HelmState, error) {
 
 	return &r, nil
 }
+
+type CyclicReleaseTemplateInheritanceError struct {
+	Message string
+}
+
+func (e CyclicReleaseTemplateInheritanceError) Error() string {
+	return e.Message
+}
+
+// releaseWithInheritedTemplate generates a new ReleaseSpec from a ReleaseSpec, by recursively inheriting
+// release templates referenced by the spec's `inherit` field.
+// The third parameter retains the current state of the recursive call, to detect a cyclic dependency a.k.a
+// a cyclic relese template inheritance.
+// This functions fails with a CyclicReleaseTemplateInheritanceError if it finds a cyclic inheritance.
+func (st *HelmState) releaseWithInheritedTemplate(r *ReleaseSpec, inheritancePath []string) (*ReleaseSpec, error) {
+	templateName := r.Inherit.Template
+	if templateName == "" {
+		return r, nil
+	}
+
+	path := append([]string{}, inheritancePath...)
+	path = append(path, templateName)
+
+	var cycleFound bool
+	for _, t := range inheritancePath {
+		if t == templateName {
+			cycleFound = true
+			break
+		}
+	}
+
+	if cycleFound {
+		return nil, CyclicReleaseTemplateInheritanceError{Message: fmt.Sprintf("cyclic inheritance detected: %s", strings.Join(path, "->"))}
+	}
+
+	template, defined := st.Templates[templateName]
+	if !defined {
+		return nil, fmt.Errorf("release %q tried to inherit inexistent release template %q", r.Name, templateName)
+	}
+
+	src, err := st.releaseWithInheritedTemplate(&template.ReleaseSpec, path)
+	if err != nil {
+		return nil, fmt.Errorf("unable to load release template %q: %w", templateName, err)
+	}
+
+	for _, k := range r.Inherit.Except {
+		switch k {
+		case "labels":
+			src.Labels = map[string]string{}
+		case "values":
+			src.Values = nil
+		case "valuesTemplate":
+			src.ValuesTemplate = nil
+		case "setTemplate":
+			src.SetValuesTemplate = nil
+		case "set":
+			src.SetValues = nil
+		default:
+			return nil, fmt.Errorf("%q is not allowed under `inherit`. Allowed values are \"set\", \"setTemplate\", \"values\", \"valuesTemplate\", and \"labels\"", k)
+		}
+
+		st.logger.Debugf("excluded field %q when inheriting template %q to release %q", k, templateName, r.Name)
+	}
+
+	var merged ReleaseSpec
+
+	if err := mergo.Merge(&merged, src, mergo.WithAppendSlice, mergo.WithSliceDeepCopy); err != nil {
+		return nil, fmt.Errorf("unable to inherit release template %q: %w", templateName, err)
+	}
+
+	if err := mergo.Merge(&merged, r, mergo.WithAppendSlice, mergo.WithSliceDeepCopy); err != nil {
+		return nil, fmt.Errorf("unable to load release %q: %w", r.Name, err)
+	}
+
+	merged.Inherit = Inherit{}
+
+	return &merged, nil
+}

pkg/state/temp_test.go

@@ -38,39 +38,39 @@ func TestGenerateID(t *testing.T) {
 	run(testcase{
 		subject: "baseline",
 		release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"},
-		want:    "foo-values-8496665478",
+		want:    "foo-values-648b77cdd4",
 	})
 
 	run(testcase{
 		subject: "different bytes content",
 		release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"},
 		data:    []byte(`{"k":"v"}`),
-		want:    "foo-values-5c4468ff65",
+		want:    "foo-values-5dfbf8fdb7",
 	})
 
 	run(testcase{
 		subject: "different map content",
 		release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"},
 		data:    map[string]interface{}{"k": "v"},
-		want:    "foo-values-7b656f7c67",
+		want:    "foo-values-7565d47dd9",
 	})
 
 	run(testcase{
 		subject: "different chart",
 		release: ReleaseSpec{Name: "foo", Chart: "stable/envoy"},
-		want:    "foo-values-675b4dffc9",
+		want:    "foo-values-7c4f76c445",
 	})
 
 	run(testcase{
 		subject: "different name",
 		release: ReleaseSpec{Name: "bar", Chart: "incubator/raw"},
-		want:    "bar-values-5fb8b9599",
+		want:    "bar-values-644fb47865",
 	})
 
 	run(testcase{
 		subject: "specific ns",
 		release: ReleaseSpec{Name: "foo", Chart: "incubator/raw", Namespace: "myns"},
-		want:    "myns-foo-values-64948d6f45",
+		want:    "myns-foo-values-c5ddcc795",
 	})
 
 	for id, n := range ids {

test/e2e/template/helmfile/testdata/snapshot/pr_560/output.yaml

@@ -82,7 +82,7 @@ remote> getter dest: values/https_github_com_helmfile_helmfile_git.ref=main
 remote> cached dir: /home/runner/.cache/helmfile/values/https_github_com_helmfile_helmfile_git.ref=main
 skipping missing secrets file matching "git::https://github.com/helmfile/helmfile.git@test/e2e/template/helmfile/testdata/snapshot/pr_560/secrets.yaml?ref=main"
 Templating release=foo, chart=../../charts/raw-0.1.0
-exec: helm template foo ../../charts/raw-0.1.0 --values /tmp/helmfile/foo-values-649697bc75 --debug
+exec: helm template foo ../../charts/raw-0.1.0 --values /tmp/helmfile/foo-values-d459bc67c --debug
 helm> install.go:192: [debug] Original chart version: ""
 helm> 
 helm> install.go:209: [debug] CHART PATH: /home/runner/work/helmfile/helmfile/test/e2e/template/helmfile/testdata/charts/raw-0.1.0
@@ -108,6 +108,6 @@ metadata:
 data:
   foo: FOO
 
-Removed /tmp/helmfile/foo-values-649697bc75
+Removed /tmp/helmfile/foo-values-d459bc67c
 Removed /tmp/helmfile
 changing working directory back to "/home/runner/work/helmfile/helmfile/test/e2e/template/helmfile"

test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/config.yaml

@@ -0,0 +1,5 @@
+localChartRepoServer:
+  enabled: true
+  port: 18084
+helmfileArgs:
+- template

test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/input.yaml

@@ -0,0 +1,63 @@
+repositories:
+- name: myrepo
+  url: http://localhost:18084/
+
+templates:
+  base:
+    valuesTemplate:
+    - base: base
+    labels:
+      base: base
+  template1:
+    values:
+    - template1: template1
+    valuesTemplate:
+    - template1Label: "{{` '{{ .Release.Labels.template1 }}' `}}"
+    labels:
+      template1: template1
+    inherit:
+      template: base
+      except:
+      - labels
+  template2:
+    values:
+    - template2: template2
+    valuesTemplate:
+    - inheritedBaseLabel: "{{` '{{ .Release.Labels.base }}' `}}"
+      template2Label: "{{` '{{ .Release.Labels.template2 }}' `}}"
+    labels:
+      template2: template2
+    inherit:
+      template: base
+      except:
+      - valuesTemplate
+
+releases:
+- name: foo1
+  chart: ../../charts/raw-0.1.0
+  inherit:
+    template: template1
+  values:
+  - templates:
+    - |
+      apiVersion: v1
+      kind: ConfigMap
+      metadata:
+        name: {{`{{ .Release.Name }}`}}-1
+        namespace: {{`{{ .Release.Namespace }}`}}
+      data:
+        {{` {{ .Values | toYaml | nindent 2 }} `}}
+- name: foo2
+  chart: ../../charts/raw-0.1.0
+  inherit:
+    template: template2
+  values:
+  - templates:
+    - |
+      apiVersion: v1
+      kind: ConfigMap
+      metadata:
+        name: {{`{{ .Release.Name }}`}}-1
+        namespace: {{`{{ .Release.Namespace }}`}}
+      data:
+        {{` {{ .Values | toYaml | nindent 2 }} `}}

test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/output.yaml

@@ -0,0 +1,45 @@
+Adding repo myrepo http://localhost:18084/
+"myrepo" has been added to your repositories
+
+Building dependency release=foo1, chart=../../charts/raw-0.1.0
+Building dependency release=foo2, chart=../../charts/raw-0.1.0
+Templating release=foo1, chart=../../charts/raw-0.1.0
+---
+# Source: raw/templates/resources.yaml
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: foo1-1
+  namespace: default
+data:
+   
+  base: base
+  template1: template1
+  template1Label: template1
+  templates:
+  - "apiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: {{ .Release.Name }}-1\n  namespace:
+    {{ .Release.Namespace }}\ndata:\n   {{ .Values | toYaml | nindent 2 }} \n"
+
+Templating release=foo2, chart=../../charts/raw-0.1.0
+---
+# Source: raw/templates/resources.yaml
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: foo2-1
+  namespace: default
+data:
+   
+  inheritedBaseLabel: base
+  template2: template2
+  template2Label: template2
+  templates:
+  - |-
+    apiVersion: v1
+    kind: ConfigMap
+    metadata:
+      name: {{ .Release.Name }}-1
+      namespace: {{ .Release.Namespace }}
+    data:
+       {{ .Values | toYaml | nindent 2 }}
+