From 490bb5d1474fa3a79bcffabf2d8984a77e3862cc Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Wed, 28 Dec 2022 10:01:04 +0900 Subject: [PATCH] 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 * Fix wording Signed-off-by: Yusuke Kuoka * Comment on releaseWithInheritedTemplate Signed-off-by: Yusuke Kuoka * Update Release Template doc with the new `inherit` feature Signed-off-by: Yusuke Kuoka * Fix a typo in code comment Signed-off-by: Yusuke Kuoka Signed-off-by: Yusuke Kuoka --- docs/writing-helmfile.md | 16 +++- pkg/app/app_template_test.go | 91 ++++++++++++++++++ pkg/state/state.go | 8 ++ pkg/state/state_exec_tmpl.go | 94 ++++++++++++++++++- pkg/state/temp_test.go | 12 +-- .../testdata/snapshot/pr_560/output.yaml | 4 +- .../release_template_inheritance/config.yaml | 5 + .../release_template_inheritance/input.yaml | 63 +++++++++++++ .../release_template_inheritance/output.yaml | 45 +++++++++ 9 files changed, 324 insertions(+), 14 deletions(-) create mode 100644 test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/config.yaml create mode 100644 test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/input.yaml create mode 100644 test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/output.yaml diff --git a/docs/writing-helmfile.md b/docs/writing-helmfile.md index b3f21c6d..9ee4c992 100644 --- a/docs/writing-helmfile.md +++ b/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 diff --git a/pkg/app/app_template_test.go b/pkg/app/app_template_test.go index 921cc4c7..1b8ffc66 100644 --- a/pkg/app/app_template_test.go +++ b/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`, + }) + }) +} diff --git a/pkg/state/state.go b/pkg/state/state.go index c2e90801..8ef6fef2 100644 --- a/pkg/state/state.go +++ b/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. diff --git a/pkg/state/state_exec_tmpl.go b/pkg/state/state_exec_tmpl.go index e09e7ca4..666674a3 100644 --- a/pkg/state/state_exec_tmpl.go +++ b/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 +} diff --git a/pkg/state/temp_test.go b/pkg/state/temp_test.go index bcd52586..9bb3d434 100644 --- a/pkg/state/temp_test.go +++ b/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 { diff --git a/test/e2e/template/helmfile/testdata/snapshot/pr_560/output.yaml b/test/e2e/template/helmfile/testdata/snapshot/pr_560/output.yaml index ed4c66ef..ed9dea41 100644 --- a/test/e2e/template/helmfile/testdata/snapshot/pr_560/output.yaml +++ b/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" diff --git a/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/config.yaml b/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/config.yaml new file mode 100644 index 00000000..a7207392 --- /dev/null +++ b/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/config.yaml @@ -0,0 +1,5 @@ +localChartRepoServer: + enabled: true + port: 18084 +helmfileArgs: +- template diff --git a/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/input.yaml b/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/input.yaml new file mode 100644 index 00000000..f94955ee --- /dev/null +++ b/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 }} `}} \ No newline at end of file diff --git a/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/output.yaml b/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/output.yaml new file mode 100644 index 00000000..5cb1b25e --- /dev/null +++ b/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 }} +