Add tests and check for parent usage

Signed-off-by: Drew Gonzales <drew.gonzales@datadoghq.com>

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

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 }}

cmd/helm/testdata/testcharts/unused-values/values.yaml

@@ -0,0 +1,9 @@
+cluster:
+  name: silk-sonic
+
+resources:
+  cpu:
+    request: 32
+    limit: 64
+
+unused: values

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 "<no value>" even if Options(missing=zero)
@@ -293,8 +297,8 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable)
 		rendered[filename] = strings.ReplaceAll(buf.String(), "<no value>", "")
 	}
 
-	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
+}

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")
 	}
 }