From 68dbcb5e198654fc92d0ee0f08db4cc6dfb46434 Mon Sep 17 00:00:00 2001 From: Drew Gonzales Date: Wed, 5 Apr 2023 17:23:26 -0700 Subject: [PATCH] Add tests and check for parent usage Signed-off-by: Drew Gonzales --- .../testcharts/unused-values/Chart.yaml | 4 + .../templates/cluster-info-configmap.yaml | 11 ++ .../testcharts/unused-values/values.yaml | 9 ++ pkg/engine/engine.go | 57 +++++++-- pkg/engine/engine_test.go | 114 +++++++++++++++--- 5 files changed, 168 insertions(+), 27 deletions(-) create mode 100644 cmd/helm/testdata/testcharts/unused-values/Chart.yaml create mode 100644 cmd/helm/testdata/testcharts/unused-values/templates/cluster-info-configmap.yaml create mode 100644 cmd/helm/testdata/testcharts/unused-values/values.yaml diff --git a/cmd/helm/testdata/testcharts/unused-values/Chart.yaml b/cmd/helm/testdata/testcharts/unused-values/Chart.yaml new file mode 100644 index 000000000..963172eaa --- /dev/null +++ b/cmd/helm/testdata/testcharts/unused-values/Chart.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +name: k8s-platform-cluster-info +version: 1.0.16 +icon: https://bitnami.com/assets/stacks/wordpress/img/wordpress-stack-220x234.png 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 new file mode 100644 index 000000000..6a1f371dd --- /dev/null +++ b/cmd/helm/testdata/testcharts/unused-values/templates/cluster-info-configmap.yaml @@ -0,0 +1,11 @@ +--- +kind: ConfigMap +apiVersion: v1 +metadata: + name: unused-values + namespace: default + labels: + team: infrastructure +data: + cluster-name: {{ $.Values.cluster.name }} + json: {{ toJson $.Values.resources }} diff --git a/cmd/helm/testdata/testcharts/unused-values/values.yaml b/cmd/helm/testdata/testcharts/unused-values/values.yaml new file mode 100644 index 000000000..f1f410b7a --- /dev/null +++ b/cmd/helm/testdata/testcharts/unused-values/values.yaml @@ -0,0 +1,9 @@ +cluster: + name: silk-sonic + +resources: + cpu: + request: 32 + limit: 64 + +unused: values diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index dc5b5f544..05a86b88d 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -21,6 +21,7 @@ import ( "log" "path" "path/filepath" + "reflect" "regexp" "sort" "strings" @@ -284,7 +285,10 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) } usedValues = usedValues.Union(traverse(t.Lookup(filename).Copy().Root)) - pv := getProvidedValues(vals) + pv, err := getProvidedValues(vals) + if err != nil { + return nil, err + } providedValues = providedValues.Union(pv) // Work around the issue where Go will emit "" even if Options(missing=zero) @@ -293,8 +297,8 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) rendered[filename] = strings.ReplaceAll(buf.String(), "", "") } - if e.Strict { - unused := providedValues.Difference(usedValues) + 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)) } @@ -501,7 +505,7 @@ func traverse(cur parse.Node) sets.Set[string] { vars = vars.Insert(fmt.Sprintf(".%s", strings.Join(node.Ident, "."))) case *parse.VariableNode: - vars = vars.Insert(strings.Join(node.Ident, ".")) + vars = vars.Insert(strings.TrimPrefix(strings.Join(node.Ident, "."), "$")) default: panic("uncaught node type in go template") } @@ -509,15 +513,27 @@ func traverse(cur parse.Node) sets.Set[string] { return vars } -func getProvidedValues(vals chartutil.Values) sets.Set[string] { - v, ok := vals["Values"].(chartutil.Values) +// getProvidedValues grabs the Values part, ignoring the Chart and Release data. +// I'm explicitly ignoring those because we don't care about fields that aren't used +// like $.Chart.name or $.Release.name because we don't set them in the cluster specific +// values. +func getProvidedValues(vals chartutil.Values) (sets.Set[string], error) { + chartUtilValues, keyExists := vals["Values"] + if !keyExists { + return nil, fmt.Errorf("no values key found") + } + + if chartUtilValues == nil { + return nil, fmt.Errorf("values key is nil") + } + + values, ok := chartUtilValues.(chartutil.Values) if !ok { - // When checking for unused values, if no values are found, we - // swallow the error here. - return sets.New[string]() + return nil, fmt.Errorf("could not typecast chart values %s", reflect.TypeOf(chartUtilValues)) } - f := flattenMapKeys(".Values", v) - return sets.New(f...) + + f := flattenMapKeys(".Values", values) + return sets.New(f...), nil } // flattenMapKeys turns an interface into a list of variable paths to make it easy to log and @@ -537,3 +553,22 @@ func flattenMapKeys(root string, values chartutil.Values) []string { return keys } + +// setDifferenceWithParents checks for unused variables. However, there are instances where the +// parent is used and the children are transitively used, not explicitly used. For example, a common +// pattern in charts is to use the helm function {{ toJson $.Values.resources }} and put the limits +// and requests nested in $.Values.resources. We never use $.Values.resources.cpu.limit explicitly, +// but it is used. +func setDifferenceWithParents(providedValues sets.Set[string], usedValues sets.Set[string]) sets.Set[string] { + unused := providedValues.Difference(usedValues) + + for usedValue := range usedValues { + for unusedValue := range unused { + if strings.HasPrefix(unusedValue, usedValue) { + unused.Delete(unusedValue) + } + } + } + + return unused +} diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 4ae86b5ac..4f2e902fd 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -24,6 +24,7 @@ import ( "testing" "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chart/loader" "helm.sh/helm/v3/pkg/chartutil" ) @@ -869,52 +870,133 @@ func TestRenderRecursionLimit(t *testing.T) { } } -func TestReportUnusedValues(t *testing.T) { - c := &chart.Chart{ +func TestReportUnusedValuesBasic(t *testing.T) { + chart := &chart.Chart{ Metadata: &chart.Metadata{ Name: "UnusedValues", Version: "4.2.0", }, Templates: []*chart.File{ - {Name: "templates/test1", Data: []byte("{{.Values.very.used | title }} {{.Values.extremely.used | title}}")}, - {Name: "templates/test2", Data: []byte("{{.Values.super.used | lower }}")}, - {Name: "templates/test3", Data: []byte("{{.Values.definitely.used}}")}, - {Name: "templates/test4", Data: []byte("{{toJson .Values}}")}, + {Name: "templates/test1", Data: []byte("{{$.Values.very.used | title }} {{$.Values.extremely.used | title}}")}, + {Name: "templates/test2", Data: []byte("{{$.Values.super.used | lower }}")}, + {Name: "templates/test3", Data: []byte("{{$.Values.definitely.used}}")}, }, - Values: map[string]interface{}{"outer": "DEFAULT", "inner": "DEFAULT"}, + Values: chartutil.Values{"outer": "DEFAULT", "inner": "DEFAULT"}, } - vals := map[string]interface{}{ - "Values": map[string]interface{}{ - "very": map[string]interface{}{ + vals := chartutil.Values{ + "Values": chartutil.Values{ + "very": chartutil.Values{ "used": "used", }, "extremely": chartutil.Values{ "used": "used", }, - "definitely": map[string]interface{}{ + "definitely": chartutil.Values{ "used": "used", }, - "super": map[string]interface{}{ + "super": chartutil.Values{ "used": "used", }, "not": "not used", }, } - v, err := chartutil.CoalesceValues(c, vals) + v, err := chartutil.CoalesceValues(chart, vals) + if err != nil { + t.Fatalf("Failed to coalesce values: %s", err) + } + + engine := Engine{ + Strict: true, + LintMode: true, + config: nil, + } + + _, err = engine.Render(chart, v) + if err == nil { + t.Errorf("should have errored, .Values.not is not used") + } + + if !strings.Contains(err.Error(), "[.Values.not]") { + t.Errorf(".Values.not is unused and should have been in error message") + } +} + +func TestReportUnusedValuesChartLoad(t *testing.T) { + unusedValuesChart, err := loader.Load("../../cmd/helm/testdata/testcharts/unused-values/") + if err != nil { + t.Fatal(err) + } + + engine := Engine{ + Strict: true, + LintMode: true, + config: nil, + } + + options := chartutil.ReleaseOptions{ + Name: unusedValuesChart.Name(), + Namespace: "test", + } + + vals, err := chartutil.ToRenderValues(unusedValuesChart, unusedValuesChart.Values, options, nil) + if err != nil { + t.Fatal(err) + } + + _, err = engine.Render(unusedValuesChart, vals) + if err == nil { + t.Fatalf("there is an unused value in this chart") + } + + if !strings.Contains(err.Error(), "[.Values.unused]") { + t.Fatalf(".Values.unused is unused and should have been in error message") + } +} + +func TestReportUnusedValuesDollarSign(t *testing.T) { + chart := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "UnusedValues", + Version: "4.2.0", + }, + Templates: []*chart.File{ + {Name: "templates/test1", Data: []byte("{{ .Values.super.used }}")}, + {Name: "templates/test2", Data: []byte("{{ $.Values.definitely.used }}")}, + }, + } + + vals := chartutil.Values{ + "Values": chartutil.Values{ + "definitely": chartutil.Values{ + "used": "used", + }, + "super": chartutil.Values{ + "used": "used", + }, + "not": "not used", + }, + } + + v, err := chartutil.CoalesceValues(chart, vals) if err != nil { t.Fatalf("Failed to coalesce values: %s", err) } engine := Engine{ Strict: true, - LintMode: false, + LintMode: true, config: nil, } - _, err = engine.Render(c, v) + _, err = engine.Render(chart, v) if err == nil { - t.Errorf("Render should have returned an error for an unused variable: %s", err) + t.Errorf("should have errored, $.Values.not is not used") + } + + if !strings.Contains(err.Error(), "[.Values.not]") { + t.Error(err) + t.Errorf(".Values.not is the ONLY unused value") } }