diff --git a/client/allocrunner/taskrunner/task_runner.go b/client/allocrunner/taskrunner/task_runner.go index ac9d7a1c2..e56bb7b3d 100644 --- a/client/allocrunner/taskrunner/task_runner.go +++ b/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 } } diff --git a/helper/pluginutils/hclutils/testing.go b/helper/pluginutils/hclutils/testing.go index 177992117..b552af494 100644 --- a/helper/pluginutils/hclutils/testing.go +++ b/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 diff --git a/helper/pluginutils/hclutils/util.go b/helper/pluginutils/hclutils/util.go index 6e39a5a6c..f75012d8b 100644 --- a/helper/pluginutils/hclutils/util.go +++ b/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 +} diff --git a/helper/pluginutils/hclutils/util_test.go b/helper/pluginutils/hclutils/util_test.go index 67575ea51..4faae2738 100644 --- a/helper/pluginutils/hclutils/util_test.go +++ b/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") }) } } diff --git a/helper/pluginutils/loader/init.go b/helper/pluginutils/loader/init.go index 2bf714ecc..babecd50e 100644 --- a/helper/pluginutils/loader/init.go +++ b/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 diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 8a1b91daf..8abb1a325 100644 --- a/nomad/structs/structs.go +++ b/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 diff --git a/plugins/shared/cmd/launcher/command/device.go b/plugins/shared/cmd/launcher/command/device.go index 031137e1e..2a89881bf 100644 --- a/plugins/shared/cmd/launcher/command/device.go +++ b/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)))