add formatting for hcl parsing error messages (#5972)

client/allocrunner/taskrunner/task_runner.go

@@ -487,7 +487,6 @@ MAIN:
 		// Run the task
 		if err := tr.runDriver(); err != nil {
 			tr.logger.Error("running driver failed", "error", err)
-			tr.EmitEvent(structs.NewTaskEvent(structs.TaskDriverFailure).SetDriverError(err))
 			tr.restartTracker.SetStartError(err)
 			goto RESTART
 		}
@@ -680,6 +679,7 @@ func (tr *TaskRunner) shouldRestart() (bool, time.Duration) {
 }
 
 // runDriver runs the driver and waits for it to exit
+// runDriver emits an appropriate task event on success/failure
 func (tr *TaskRunner) runDriver() error {
 
 	taskConfig := tr.buildTaskConfig()
@@ -705,13 +705,17 @@ func (tr *TaskRunner) runDriver() error {
 		tr.logger.Warn("some environment variables not available for rendering", "keys", strings.Join(keys, ", "))
 	}
 
-	val, diag := hclutils.ParseHclInterface(tr.task.Config, tr.taskSchema, vars)
+	val, diag, diagErrs := hclutils.ParseHclInterface(tr.task.Config, tr.taskSchema, vars)
 	if diag.HasErrors() {
-		return multierror.Append(errors.New("failed to parse config"), diag.Errs()...)
+		parseErr := multierror.Append(errors.New("failed to parse config: "), diagErrs...)
+		tr.EmitEvent(structs.NewTaskEvent(structs.TaskFailedValidation).SetValidationError(parseErr))
+		return parseErr
 	}
 
 	if err := taskConfig.EncodeDriverConfig(val); err != nil {
-		return fmt.Errorf("failed to encode driver config: %v", err)
+		encodeErr := fmt.Errorf("failed to encode driver config: %v", err)
+		tr.EmitEvent(structs.NewTaskEvent(structs.TaskFailedValidation).SetValidationError(encodeErr))
+		return encodeErr
 	}
 
 	// If there's already a task handle (eg from a Restore) there's nothing
@@ -734,16 +738,21 @@ func (tr *TaskRunner) runDriver() error {
 		if err == bstructs.ErrPluginShutdown {
 			tr.logger.Info("failed to start task because plugin shutdown unexpectedly; attempting to recover")
 			if err := tr.initDriver(); err != nil {
-				return fmt.Errorf("failed to initialize driver after it exited unexpectedly: %v", err)
+				taskErr := fmt.Errorf("failed to initialize driver after it exited unexpectedly: %v", err)
+				tr.EmitEvent(structs.NewTaskEvent(structs.TaskDriverFailure).SetDriverError(taskErr))
+				return taskErr
 			}
 
 			handle, net, err = tr.driver.StartTask(taskConfig)
 			if err != nil {
-				return fmt.Errorf("failed to start task after driver exited unexpectedly: %v", err)
+				taskErr := fmt.Errorf("failed to start task after driver exited unexpectedly: %v", err)
+				tr.EmitEvent(structs.NewTaskEvent(structs.TaskDriverFailure).SetDriverError(taskErr))
+				return taskErr
 			}
 		} else {
-			// Do *NOT* wrap the error here without maintaining
-			// whether or not is Recoverable.
+			// Do *NOT* wrap the error here without maintaining whether or not is Recoverable.
+			// You must emit a task event failure to be considered Recoverable
+			tr.EmitEvent(structs.NewTaskEvent(structs.TaskDriverFailure).SetDriverError(err))
 			return err
 		}
 	}

helper/pluginutils/hclutils/testing.go

@@ -70,7 +70,8 @@ func (b *HCLParser) parse(t *testing.T, config, out interface{}) {
 	decSpec, diags := hclspecutils.Convert(b.spec)
 	require.Empty(t, diags)
 
-	ctyValue, diag := ParseHclInterface(config, decSpec, b.vars)
+	ctyValue, diag, errs := ParseHclInterface(config, decSpec, b.vars)
+	require.Nil(t, errs)
 	require.Empty(t, diag)
 
 	// encode

helper/pluginutils/hclutils/util.go

@@ -2,6 +2,7 @@ package hclutils
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 
 	"github.com/hashicorp/hcl2/hcl"
@@ -17,7 +18,7 @@ import (
 // ParseHclInterface is used to convert an interface value representing a hcl2
 // body and return the interpolated value. Vars may be nil if there are no
 // variables to interpolate.
-func ParseHclInterface(val interface{}, spec hcldec.Spec, vars map[string]cty.Value) (cty.Value, hcl.Diagnostics) {
+func ParseHclInterface(val interface{}, spec hcldec.Spec, vars map[string]cty.Value) (cty.Value, hcl.Diagnostics, []error) {
 	evalCtx := &hcl.EvalContext{
 		Variables: vars,
 		Functions: GetStdlibFuncs(),
@@ -29,27 +30,29 @@ func ParseHclInterface(val interface{}, spec hcldec.Spec, vars map[string]cty.Va
 	err := enc.Encode(val)
 	if err != nil {
 		// Convert to a hcl diagnostics message
-		return cty.NilVal, hcl.Diagnostics([]*hcl.Diagnostic{
-			{
+		errorMessage := fmt.Sprintf("Label encoding failed: %v", err)
+		return cty.NilVal,
+			hcl.Diagnostics([]*hcl.Diagnostic{{
 				Severity: hcl.DiagError,
-				Summary:  "Failed to JSON encode value",
-				Detail:   fmt.Sprintf("JSON encoding failed: %v", err),
-			}})
+				Summary:  "Failed to encode label value",
+				Detail:   errorMessage,
+			}}),
+			[]error{errors.New(errorMessage)}
 	}
 
 	// Parse the json as hcl2
 	hclFile, diag := hjson.Parse(buf.Bytes(), "")
 	if diag.HasErrors() {
-		return cty.NilVal, diag
+		return cty.NilVal, diag, formattedDiagnosticErrors(diag)
 	}
 
 	value, decDiag := hcldec.Decode(hclFile.Body, spec, evalCtx)
 	diag = diag.Extend(decDiag)
 	if diag.HasErrors() {
-		return cty.NilVal, diag
+		return cty.NilVal, diag, formattedDiagnosticErrors(diag)
 	}
 
-	return value, diag
+	return value, diag, nil
 }
 
 // GetStdlibFuncs returns the set of stdlib functions.
@@ -72,3 +75,18 @@ func GetStdlibFuncs() map[string]function.Function {
 		"upper":      stdlib.UpperFunc,
 	}
 }
+
+// TODO: update hcl2 library with better diagnostics formatting for streamed configs
+// - should be arbitrary labels not JSON https://github.com/hashicorp/hcl2/blob/4fba5e1a75e382aed7f7a7993f2c4836a5e1cd52/hcl/json/structure.go#L66
+// - should not print diagnostic subject https://github.com/hashicorp/hcl2/blob/4fba5e1a75e382aed7f7a7993f2c4836a5e1cd52/hcl/diagnostic.go#L77
+func formattedDiagnosticErrors(diag hcl.Diagnostics) []error {
+	var errs []error
+	for _, d := range diag {
+		if d.Summary == "Extraneous JSON object property" {
+			d.Summary = "Invalid label"
+		}
+		err := errors.New(fmt.Sprintf("%s: %s", d.Summary, d.Detail))
+		errs = append(errs, err)
+	}
+	return errs
+}

helper/pluginutils/hclutils/util_test.go

@@ -358,9 +358,9 @@ func TestParseHclInterface_Hcl(t *testing.T) {
 		t.Run(c.name, func(t *testing.T) {
 			t.Logf("Val: % #v", pretty.Formatter(c.config))
 			// Parse the interface
-			ctyValue, diag := hclutils.ParseHclInterface(c.config, c.spec, c.vars)
+			ctyValue, diag, errs := hclutils.ParseHclInterface(c.config, c.spec, c.vars)
 			if diag.HasErrors() {
-				for _, err := range diag.Errs() {
+				for _, err := range errs {
 					t.Error(err)
 				}
 				t.FailNow()
@@ -497,11 +497,45 @@ func TestParseUnknown(t *testing.T) {
 		t.Run(c.name, func(t *testing.T) {
 			inter := hclutils.HclConfigToInterface(t, c.hcl)
 
-			ctyValue, diag := hclutils.ParseHclInterface(inter, cSpec, vars)
+			ctyValue, diag, errs := hclutils.ParseHclInterface(inter, cSpec, vars)
 			t.Logf("parsed: %# v", pretty.Formatter(ctyValue))
 
+			require.NotNil(t, errs)
 			require.True(t, diag.HasErrors())
-			require.Contains(t, diag.Errs()[0].Error(), "no variable named")
+			require.Contains(t, errs[0].Error(), "no variable named")
+		})
+	}
+}
+
+func TestParseInvalid(t *testing.T) {
+	dockerDriver := new(docker.Driver)
+	dockerSpec, err := dockerDriver.TaskConfigSchema()
+	require.NoError(t, err)
+	spec, diags := hclspecutils.Convert(dockerSpec)
+	require.False(t, diags.HasErrors())
+
+	cases := []struct {
+		name string
+		hcl  string
+	}{
+		{
+			"invalid_field",
+			`config { image = "redis:3.2" bad_key = "whatever"}`,
+		},
+	}
+
+	vars := map[string]cty.Value{}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			inter := hclutils.HclConfigToInterface(t, c.hcl)
+
+			ctyValue, diag, errs := hclutils.ParseHclInterface(inter, spec, vars)
+			t.Logf("parsed: %# v", pretty.Formatter(ctyValue))
+
+			require.NotNil(t, errs)
+			require.True(t, diag.HasErrors())
+			require.Contains(t, errs[0].Error(), "Invalid label")
 		})
 	}
 }

helper/pluginutils/loader/init.go

@@ -457,10 +457,11 @@ func (l *PluginLoader) validatePluginConfig(id PluginID, info *pluginInfo) error
 	}
 
 	// Parse the config using the spec
-	val, diag := hclutils.ParseHclInterface(info.config, spec, nil)
+	val, diag, diagErrs := hclutils.ParseHclInterface(info.config, spec, nil)
 	if diag.HasErrors() {
-		multierror.Append(&mErr, diag.Errs()...)
-		return multierror.Prefix(&mErr, "failed parsing config:")
+		multierror.Append(&mErr, diagErrs...)
+		return multierror.Prefix(&mErr, "failed to parse config: ")
+
 	}
 
 	// Marshal the value

nomad/structs/structs.go

@@ -5937,15 +5937,15 @@ const (
 	TaskSetupFailure = "Setup Failure"
 
 	// TaskDriveFailure indicates that the task could not be started due to a
-	// failure in the driver.
+	// failure in the driver. TaskDriverFailure is considered Recoverable.
 	TaskDriverFailure = "Driver Failure"
 
 	// TaskReceived signals that the task has been pulled by the client at the
 	// given timestamp.
 	TaskReceived = "Received"
 
-	// TaskFailedValidation indicates the task was invalid and as such was not
-	// run.
+	// TaskFailedValidation indicates the task was invalid and as such was not run.
+	// TaskFailedValidation is not considered Recoverable.
 	TaskFailedValidation = "Failed Validation"
 
 	// TaskStarted signals that the task was started and its timestamp can be

plugins/shared/cmd/launcher/command/device.go

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	hclog "github.com/hashicorp/go-hclog"
+	multierror "github.com/hashicorp/go-multierror"
 	plugin "github.com/hashicorp/go-plugin"
 	"github.com/hashicorp/hcl"
 	"github.com/hashicorp/hcl/hcl/ast"
@@ -196,13 +197,9 @@ func (c *Device) setConfig(spec hcldec.Spec, apiVersion string, config []byte, n
 
 	c.logger.Trace("raw hcl config", "config", hclog.Fmt("% #v", pretty.Formatter(configVal)))
 
-	val, diag := hclutils.ParseHclInterface(configVal, spec, nil)
+	val, diag, diagErrs := hclutils.ParseHclInterface(configVal, spec, nil)
 	if diag.HasErrors() {
-		errStr := "failed to parse config"
-		for _, err := range diag.Errs() {
-			errStr = fmt.Sprintf("%s\n* %s", errStr, err.Error())
-		}
-		return errors.New(errStr)
+		return multierror.Append(errors.New("failed to parse config: "), diagErrs...)
 	}
 	c.logger.Trace("parsed hcl config", "config", hclog.Fmt("% #v", pretty.Formatter(val)))