Repository: helm/helm
Pull request #13586 (2025-01-01 00:24:16)

fix: add formatting for errors to make multiline stacktraces in helm templates
<!--  Thanks for sending a pull request!  Here are some tips for you:
1. Make sure to read the Contributing Guide before submitting your PR: https://github.com/helm/helm/blob/main/CONTRIBUTING.md
2. If this PR closes another issue, add 'closes #<issue number>' somewhere in the PR summary. GitHub will automatically close that issue when this PR gets merged. Alternatively, adding 'refs #<issue number>' will not close the issue, but help provide the reviewer more context.-->

**What this PR does / why we need it**:

closes https://github.com/helm/helm/issues/13447

The purpose of this PR is to bring some structure into the error messages that helm will output. This structure is necessary so that readers of these error messages will naturally be able to identify important details like where the error occurred, and what the error actually is. For me, this problem comes about when you have complicated logic inside _helpers files, and is made worse when those _helpers call other _helpers.

This PR addresses this problem by adding whitespace to parts of the error message.  The structure of this format is : 
```
fileLocation:lineNumber:colNumber
  functionCallWithinFile
    contentOfError
```
and in the unit test I added, a real world example would look like this:
```
        NestedHelperFunctions/templates/svc.yaml:1:9
          executing "NestedHelperFunctions/templates/svc.yaml" at <include "nested_helper.name" .>: 
            error calling include:
        NestedHelperFunctions/templates/_helpers_1.tpl:1:39
          executing "nested_helper.name" at <include "common.names.get_name" .>: 
            error calling include:
        NestedHelperFunctions/charts/common/templates/_helpers_2.tpl:1:50
          executing "common.names.get_name" at <.Release.Name>: 
            nil pointer evaluating interface {}.Name
```

This change may introduce some risk that any added formatting logic may delete important parts of the error message.  As an attempt to address this, I use regex's to match against the known error messages from the parent golang text/template library, and if there is any part of the error that doesn't match those regex's,  the unformatted error message will be returned.


**Special notes for your reviewer**:

**If applicable**:
- [ ] this PR contains user facing changes (the `docs needed` label should be applied if so)
- [x] this PR contains unit tests
- [ ] this PR has been tested for backwards compatibility


robertsirc: Whats up yo. Thanks for the PR, we don't want to back port these changes but these are good for Helm 4. You do have errors that need to be addressed.
jessesimpson36: I added a commit to fix one test that was failing and to run `make format`. However, there are still some tests failing. I'll take some time to address later today.

jessesimpson36: Tests and linting should pass now. For the test that relied on `err.(type)`, I currently don't know how to handle it to preserve the type checking.  I'm open to suggestions.
jessesimpson36: These are good ideas. I'm willing to explore what it would take to upstream the formatting changes in text/template. and using regex's  also could be good at reducing the total line's added on this patch. It may take a few weeks for me to get the time to make these changes / start a conversation with the contributors of text/template. 

> Why not just ?
> 
```diff
-	return fmt.Errorf("%s", originalErr.Error())
+	return originalErr
```
:laughing: yeah that line looks silly now.
jessesimpson36: Here is my issue proposing additional fields to the ExecError type. https://github.com/golang/go/issues/71801
jessesimpson36: @gjenkins8 , I added a commit replacing much of the string manipulation logic with regex's. I also removed the logic that compares whether the old error message and the new are similar enough,  because if the regex's do not match, then the old error message will be returned anyway.

What do you think?
jessesimpson36: I'm sorry, I asked for a re-review and I'm still making refactoring changes. I'll update here when I'm done
jessesimpson36: I'm done with my refactorings. I won't make additional changes without re-review.
jessesimpson36: I rebased on latest main.
jessesimpson36: https://github.com/helm/helm/issues/13447#issuecomment-2725646423

linking my comment from the issue since it's relevant to this implementation
gjenkins8: > Here is my issue proposing additional fields to the ExecError type. [golang/go#71801](https://github.com/golang/go/issues/71801)

Nice!
jessesimpson36: I went through one round of implementing based on the feedback, but I would like a followup response on the topic of the unresolved comments (converting from for loop to unbounded while loop, error deduplication logic) before making any commits.
jessesimpson36: > I went through one round of implementing based on the feedback, but I would like a followup response on the topic of the unresolved comments (converting from for loop to unbounded while loop, error deduplication logic) before making any commits.

I think I addressed both of those topics now. Please see the messages in the "Review" threads.
jessesimpson36: @gjenkins8 may I have a re-review? (just a reminder in case this PR got lost in the noise)
jessesimpson36: rebased on main. Sorry for the extra noise this causes.
jessesimpson36: @gjenkins8 your review got invalidated since my 2nd reviewer asked for a change. :sweat_smile: 
Could you re-review?
jessesimpson36: @scottrigby directed me towards a 3rd type of error possibility that looks like this:

> template: no template "nested_helper.name" associated with template "gotpl"

upstream it's found here:
https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=191

so I added a unit test and extra regex to match. 
jessesimpson36: what's interesting about that third type of error is that it's the only place in exec.go 
https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=191
that uses fmt.Errorf  instead of the usual state.errorf (the two regex's I covered).  

Patch:
diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go
index 839ad4a31..009b5e432 100644
--- a/pkg/engine/engine.go
+++ b/pkg/engine/engine.go
@@ -33,6 +33,18 @@ import (
 	chartutil "helm.sh/helm/v4/pkg/chart/v2/util"
 )
 
+// taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=141
+// > "template: %s: executing %q at <%s>: %s"
+var execErrFmt = regexp.MustCompile(`^template: (?P<templateName>(?U).+): executing (?P<functionName>(?U).+) at (?P<location>(?U).+): (?P<errMsg>(?U).+)(?P<nextErr>( template:.*)?)$`)
+
+// taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=138
+// > "template: %s: %s"
+var execErrFmtWithoutTemplate = regexp.MustCompile(`^template: (?P<templateName>(?U).+): (?P<errMsg>.*)(?P<nextErr>( template:.*)?)$`)
+
+// taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=191
+// > "template: no template %q associated with template %q"
+var execErrNoTemplateAssociated = regexp.MustCompile(`^template: no template (?P<location>.*) associated with template (?P<functionName>(.*)?)$`)
+
 // Engine is an implementation of the Helm rendering implementation for templates.
 type Engine struct {
 	// If strict is enabled, template rendering will fail if a template references
@@ -304,7 +316,7 @@ func (e Engine) render(tpls map[string]renderable) (rendered map[string]string,
 		vals["Template"] = chartutil.Values{"Name": filename, "BasePath": tpls[filename].basePath}
 		var buf strings.Builder
 		if err := t.ExecuteTemplate(&buf, filename, vals); err != nil {
-			return map[string]string{}, cleanupExecError(filename, err)
+			return map[string]string{}, reformatExecErrorMsg(filename, err)
 		}
 
 		// Work around the issue where Go will emit "<no value>" even if Options(missing=zero)
@@ -330,7 +342,33 @@ func cleanupParseError(filename string, err error) error {
 	return fmt.Errorf("parse error at (%s): %s", string(location), errMsg)
 }
 
-func cleanupExecError(filename string, err error) error {
+type TraceableError struct {
+	location         string
+	message          string
+	executedFunction string
+}
+
+func (t TraceableError) String() string {
+	var errorString strings.Builder
+	if t.location != "" {
+		fmt.Fprintf(&errorString, "%s\n  ", t.location)
+	}
+	if t.executedFunction != "" {
+		fmt.Fprintf(&errorString, "%s\n    ", t.executedFunction)
+	}
+	if t.message != "" {
+		fmt.Fprintf(&errorString, "%s\n", t.message)
+	}
+	return errorString.String()
+}
+
+// reformatExecErrorMsg takes an error message for template rendering and formats it into a formatted
+// multi-line error string
+func reformatExecErrorMsg(filename string, err error) error {
+	// This function matches the error message against regex's for the text/template package.
+	// If the regex's can parse out details from that error message such as the line number, template it failed on,
+	// and error description, then it will construct a new error that displays these details in a structured way.
+	// If there are issues with parsing the error message, the err passed into the function should return instead.
 	if _, isExecError := err.(template.ExecError); !isExecError {
 		return err
 	}
@@ -349,8 +387,46 @@ func cleanupExecError(filename string, err error) error {
 	if len(parts) >= 2 {
 		return fmt.Errorf("execution error at (%s): %s", string(location), parts[1])
 	}
+	current := err
+	fileLocations := []TraceableError{}
+	for current != nil {
+		var traceable TraceableError
+		if matches := execErrFmt.FindStringSubmatch(current.Error()); matches != nil {
+			templateName := matches[execErrFmt.SubexpIndex("templateName")]
+			functionName := matches[execErrFmt.SubexpIndex("functionName")]
+			locationName := matches[execErrFmt.SubexpIndex("location")]
+			errMsg := matches[execErrFmt.SubexpIndex("errMsg")]
+			traceable = TraceableError{
+				location:         templateName,
+				message:          errMsg,
+				executedFunction: "executing " + functionName + " at " + locationName + ":",
+			}
+		} else if matches := execErrFmtWithoutTemplate.FindStringSubmatch(current.Error()); matches != nil {
+			templateName := matches[execErrFmt.SubexpIndex("templateName")]
+			errMsg := matches[execErrFmt.SubexpIndex("errMsg")]
+			traceable = TraceableError{
+				location: templateName,
+				message:  errMsg,
+			}
+		} else if matches := execErrNoTemplateAssociated.FindStringSubmatch(current.Error()); matches != nil {
+			traceable = TraceableError{
+				message: current.Error(),
+			}
+		} else {
+			return err
+		}
+		if len(fileLocations) == 0 || fileLocations[len(fileLocations)-1] != traceable {
+			fileLocations = append(fileLocations, traceable)
+		}
+		current = errors.Unwrap(current)
+	}
+
+	var finalErrorString strings.Builder
+	for _, fileLocation := range fileLocations {
+		fmt.Fprintf(&finalErrorString, "%s", fileLocation.String())
+	}
 
-	return err
+	return errors.New(strings.TrimSpace(finalErrorString.String()))
 }
 
 func sortTemplates(tpls map[string]renderable) []string {


Test patch:
diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go
index e39674c80..61cc2946f 100644
--- a/pkg/action/install_test.go
+++ b/pkg/action/install_test.go
@@ -446,7 +446,9 @@ func TestInstallReleaseIncorrectTemplate_DryRun(t *testing.T) {
 	instAction.DryRun = true
 	vals := map[string]interface{}{}
 	_, err := instAction.Run(buildChart(withSampleIncludingIncorrectTemplates()), vals)
-	expectedErr := "\"hello/templates/incorrect\" at <.Values.bad.doh>: nil pointer evaluating interface {}.doh"
+	expectedErr := `hello/templates/incorrect:1:10
+  executing "hello/templates/incorrect" at <.Values.bad.doh>:
+    nil pointer evaluating interface {}.doh`
 	if err == nil {
 		t.Fatalf("Install should fail containing error: %s", expectedErr)
 	}
diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go
index 68e0158fa..f4228fbd7 100644
--- a/pkg/engine/engine_test.go
+++ b/pkg/engine/engine_test.go
@@ -24,6 +24,8 @@ import (
 	"testing"
 	"text/template"
 
+	"github.com/stretchr/testify/assert"
+
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -1289,16 +1291,82 @@ func TestRenderTplMissingKeyString(t *testing.T) {
 		t.Errorf("Expected error, got %v", out)
 		return
 	}
-	switch err.(type) {
-	case (template.ExecError):
-		errTxt := fmt.Sprint(err)
-		if !strings.Contains(errTxt, "noSuchKey") {
-			t.Errorf("Expected error to contain 'noSuchKey', got %s", errTxt)
-		}
-	default:
-		// Some unexpected error.
-		t.Fatal(err)
+	errTxt := fmt.Sprint(err)
+	if !strings.Contains(errTxt, "noSuchKey") {
+		t.Errorf("Expected error to contain 'noSuchKey', got %s", errTxt)
+	}
+
+}
+
+func TestNestedHelpersProducesMultilineStacktrace(t *testing.T) {
+	c := &chart.Chart{
+		Metadata: &chart.Metadata{Name: "NestedHelperFunctions"},
+		Templates: []*chart.File{
+			{Name: "templates/svc.yaml", Data: []byte(
+				`name: {{ include "nested_helper.name" . }}`,
+			)},
+			{Name: "templates/_helpers_1.tpl", Data: []byte(
+				`{{- define "nested_helper.name" -}}{{- include "common.names.get_name" . -}}{{- end -}}`,
+			)},
+			{Name: "charts/common/templates/_helpers_2.tpl", Data: []byte(
+				`{{- define "common.names.get_name" -}}{{- .Values.nonexistant.key | trunc 63 | trimSuffix "-" -}}{{- end -}}`,
+			)},
+		},
+	}
+
+	expectedErrorMessage := `NestedHelperFunctions/templates/svc.yaml:1:9
+  executing "NestedHelperFunctions/templates/svc.yaml" at <include "nested_helper.name" .>:
+    error calling include:
+NestedHelperFunctions/templates/_helpers_1.tpl:1:39
+  executing "nested_helper.name" at <include "common.names.get_name" .>:
+    error calling include:
+NestedHelperFunctions/charts/common/templates/_helpers_2.tpl:1:49
+  executing "common.names.get_name" at <.Values.nonexistant.key>:
+    nil pointer evaluating interface {}.key`
+
+	v := chartutil.Values{}
+
+	val, _ := chartutil.CoalesceValues(c, v)
+	vals := map[string]interface{}{
+		"Values": val.AsMap(),
 	}
+	_, err := Render(c, vals)
+
+	assert.NotNil(t, err)
+	assert.Equal(t, expectedErrorMessage, err.Error())
+}
+
+func TestMultilineNoTemplateAssociatedError(t *testing.T) {
+	c := &chart.Chart{
+		Metadata: &chart.Metadata{Name: "multiline"},
+		Templates: []*chart.File{
+			{Name: "templates/svc.yaml", Data: []byte(
+				`name: {{ include "nested_helper.name" . }}`,
+			)},
+			{Name: "templates/test.yaml", Data: []byte(
+				`{{ toYaml .Values }}`,
+			)},
+			{Name: "charts/common/templates/_helpers_2.tpl", Data: []byte(
+				`{{ toYaml .Values }}`,
+			)},
+		},
+	}
+
+	expectedErrorMessage := `multiline/templates/svc.yaml:1:9
+  executing "multiline/templates/svc.yaml" at <include "nested_helper.name" .>:
+    error calling include:
+template: no template "nested_helper.name" associated with template "gotpl"`
+
+	v := chartutil.Values{}
+
+	val, _ := chartutil.CoalesceValues(c, v)
+	vals := map[string]interface{}{
+		"Values": val.AsMap(),
+	}
+	_, err := Render(c, vals)
+
+	assert.NotNil(t, err)
+	assert.Equal(t, expectedErrorMessage, err.Error())
 }
 
 func TestRenderCustomTemplateFuncs(t *testing.T) {