From 6abfdd61397e339e1d27604695d5a6ccdfda911a Mon Sep 17 00:00:00 2001 From: Drew Gonzales Date: Tue, 6 Jun 2023 16:58:22 -0700 Subject: [PATCH] check for unused values when tpl func is called Signed-off-by: Drew Gonzales --- .../templates/cluster-info-configmap.yaml | 1 + .../testcharts/unused-values/values.yaml | 3 ++ pkg/engine/engine.go | 49 ++++++++++++------- pkg/engine/engine_test.go | 8 ++- 4 files changed, 42 insertions(+), 19 deletions(-) diff --git a/cmd/helm/testdata/testcharts/unused-values/templates/cluster-info-configmap.yaml b/cmd/helm/testdata/testcharts/unused-values/templates/cluster-info-configmap.yaml index 6a1f371dd..cbcf22dce 100644 --- a/cmd/helm/testdata/testcharts/unused-values/templates/cluster-info-configmap.yaml +++ b/cmd/helm/testdata/testcharts/unused-values/templates/cluster-info-configmap.yaml @@ -9,3 +9,4 @@ metadata: data: cluster-name: {{ $.Values.cluster.name }} json: {{ toJson $.Values.resources }} + template: {{ tpl $.Values.template . }} diff --git a/cmd/helm/testdata/testcharts/unused-values/values.yaml b/cmd/helm/testdata/testcharts/unused-values/values.yaml index f1f410b7a..d5267aaeb 100644 --- a/cmd/helm/testdata/testcharts/unused-values/values.yaml +++ b/cmd/helm/testdata/testcharts/unused-values/values.yaml @@ -7,3 +7,6 @@ resources: limit: 64 unused: values + +usedByTemplate: test +template: "{{ .Values.usedByTemplate }}" diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index ef338d362..7cc1d9ca5 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -47,12 +47,18 @@ type Engine struct { config *rest.Config // EnableDNS tells the engine to allow DNS lookups when rendering templates EnableDNS bool + // usedValues is the set of values that are references in a chart + usedValues sets.Set[string] + // providedValues is a flattened map of every values given in the values files + providedValues sets.Set[string] } // New creates a new instance of Engine using the passed in rest config. func New(config *rest.Config) Engine { return Engine{ config: config, + usedValues: sets.New[string](), + providedValues: sets.New[string](), } } @@ -75,7 +81,7 @@ func New(config *rest.Config) Engine { // that section of the values will be passed into the "foo" chart. And if that // section contains a value named "bar", that value will be passed on to the // bar chart during render time. -func (e Engine) Render(chrt *chart.Chart, values chartutil.Values) (map[string]string, error) { +func (e *Engine) Render(chrt *chart.Chart, values chartutil.Values) (map[string]string, error) { tmap := allTemplates(chrt, values) return e.render(tmap) } @@ -90,9 +96,12 @@ func Render(chrt *chart.Chart, values chartutil.Values) (map[string]string, erro // render the Go templates using the default options. This engine is client aware and so can have template // functions that interact with the client func RenderWithClient(chrt *chart.Chart, values chartutil.Values, config *rest.Config) (map[string]string, error) { - return Engine{ + e := &Engine{ config: config, - }.Render(chrt, values) + usedValues: sets.New[string](), + providedValues: sets.New[string](), + } + return e.Render(chrt, values) } // renderable is an object that can be rendered. @@ -116,7 +125,7 @@ func warnWrap(warn string) string { } // initFunMap creates the Engine's FuncMap and adds context-specific functions. -func (e Engine) initFunMap(t *template.Template, referenceTpls map[string]renderable) { +func (e *Engine) initFunMap(t *template.Template, referenceTpls map[string]renderable) { funcMap := funcMap() includedNames := make(map[string]int) @@ -213,13 +222,26 @@ func (e Engine) initFunMap(t *template.Template, referenceTpls map[string]render } // render takes a map of templates/values and renders them. -func (e Engine) render(tpls map[string]renderable) (map[string]string, error) { - return e.renderWithReferences(tpls, tpls) +// When this function errors, it must return a map[string]string{}. +func (e *Engine) render(tpls map[string]renderable) (map[string]string, error) { + rendered, err := e.renderWithReferences(tpls, tpls) + if err != nil { + return map[string]string{}, err + } + + if e.Strict && e.LintMode { + unused := setDifferenceWithParents(e.providedValues, e.usedValues) + if unused.Len() > 0 { + return map[string]string{}, fmt.Errorf("there are unused fields in values files %+v", sets.List(unused)) + } + } + + return rendered, nil } // renderWithReferences takes a map of templates/values to render, and a map of // templates which can be referenced within them. -func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) (rendered map[string]string, err error) { +func (e *Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) (rendered map[string]string, err error) { // Basically, what we do here is start with an empty parent template and then // build up a list of templates -- one for each file. Once all of the templates // have been parsed, we loop through again and execute every template. @@ -266,8 +288,6 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) } } - usedValues := sets.New[string]() - providedValues := sets.New[string]() rendered = make(map[string]string, len(keys)) for _, filename := range keys { // Don't render partials. We don't care out the direct output of partials. @@ -283,8 +303,8 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) return map[string]string{}, cleanupExecError(filename, err) } - usedValues = usedValues.Union(traverse(t.Lookup(filename).Copy().Root)) - providedValues = providedValues.Union(getProvidedValues(vals)) + e.usedValues = e.usedValues.Union(traverse(t.Lookup(filename).Copy().Root)) + e.providedValues = e.providedValues.Union(getProvidedValues(vals)) // Work around the issue where Go will emit "" even if Options(missing=zero) // is set. Since missing=error will never get here, we do not need to handle @@ -292,13 +312,6 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) rendered[filename] = strings.ReplaceAll(buf.String(), "", "") } - if e.Strict && e.LintMode { - unused := setDifferenceWithParents(providedValues, usedValues) - if unused.Len() > 0 { - return map[string]string{}, fmt.Errorf("there are unused fields in values files %+v", sets.List(unused)) - } - } - return rendered, nil } diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 4f2e902fd..47f485447 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -26,6 +26,7 @@ import ( "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" "helm.sh/helm/v3/pkg/chartutil" + "k8s.io/apimachinery/pkg/util/sets" ) func TestSortTemplates(t *testing.T) { @@ -916,6 +917,7 @@ func TestReportUnusedValuesBasic(t *testing.T) { _, err = engine.Render(chart, v) if err == nil { t.Errorf("should have errored, .Values.not is not used") + t.FailNow() } if !strings.Contains(err.Error(), "[.Values.not]") { @@ -933,6 +935,8 @@ func TestReportUnusedValuesChartLoad(t *testing.T) { Strict: true, LintMode: true, config: nil, + usedValues: sets.New[string](), + providedValues: sets.New[string](), } options := chartutil.ReleaseOptions{ @@ -947,10 +951,11 @@ func TestReportUnusedValuesChartLoad(t *testing.T) { _, err = engine.Render(unusedValuesChart, vals) if err == nil { - t.Fatalf("there is an unused value in this chart") + t.Fatalf("there is an unused value in this chart, but we didn't find it") } if !strings.Contains(err.Error(), "[.Values.unused]") { + t.Log(err) t.Fatalf(".Values.unused is unused and should have been in error message") } } @@ -993,6 +998,7 @@ func TestReportUnusedValuesDollarSign(t *testing.T) { _, err = engine.Render(chart, v) if err == nil { t.Errorf("should have errored, $.Values.not is not used") + t.FailNow() } if !strings.Contains(err.Error(), "[.Values.not]") {