Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add formatting for hcl parsing error messages #5972

Merged
merged 4 commits into from
Jul 19, 2019
Merged

add formatting for hcl parsing error messages #5972

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
22732ab
add formatting for hcl parsing error messages
jazzyfresh Jul 17, 2019
c0bb2af
ParseHclInterface test for invalid labels
jazzyfresh Jul 17, 2019
b473b3b
fix task event errors
jazzyfresh Jul 17, 2019
90d72e0
fix broken test by adding TaskDriverFailure and refactor for code review
jazzyfresh Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 14 additions & 6 deletions client/allocrunner/taskrunner/task_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
nickethier marked this conversation as resolved.
Show resolved Hide resolved
tr.restartTracker.SetStartError(err)
goto RESTART
}
Expand Down Expand Up @@ -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()
Expand All @@ -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).SetDriverError(parseErr))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that in the snippet that this returns a generic Validation of task failed as the message. It's because TaskFailedValidation expects ValidationError[1] to be set instead of DriverError.

Also, using the multierror.Append generates very awkward messages, like:

2019-07-18T10:22:30+07:00  Failed Validation  2 error(s) occurred:

* failed to parse config
* Invalid label: No argument or block type is named "bad_key".

It'd be nice to format the errors ourselves and present something like the following

2019-07-18T10:22:30+07:00  Failed Validation  failed to parse config; found the following:

* Invalid label: No argument or block type is named "bad_key".

[1]

nomad/command/alloc_status.go

Lines 374 to 379 in 87d716e

case api.TaskFailedValidation:
if event.ValidationError != "" {
desc = event.ValidationError
} else {
desc = "Validation of task failed"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to address better multierror formatting in another PR

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).SetDriverError(encodeErr))
return encodeErr
}

// If there's already a task handle (eg from a Restore) there's nothing
Expand All @@ -734,12 +738,16 @@ 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
Expand Down
2 changes: 1 addition & 1 deletion helper/pluginutils/hclutils/testing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ 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, _ := ParseHclInterface(config, decSpec, b.vars)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use err instead of _ here, and require.Nil on the error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

require.Empty(t, diag)

// encode
Expand Down
33 changes: 24 additions & 9 deletions helper/pluginutils/hclutils/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package hclutils

import (
"bytes"
"errors"
"fmt"

"github.com/hashicorp/hcl2/hcl"
Expand All @@ -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(),
Expand All @@ -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.
Expand All @@ -72,3 +75,15 @@ func GetStdlibFuncs() map[string]function.Function {
"upper": stdlib.UpperFunc,
}
}

func formattedDiagnosticErrors(diag hcl.Diagnostics) []error {
var errs []error
for _, d := range diag {
if d.Summary == "Extraneous JSON object property" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a comment pointing to where this text originates, so future us and notice when it changes or if this is no longer needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Added the comment before the method

d.Summary = "Invalid label"
}
err := errors.New(fmt.Sprintf("%s: %s", d.Summary, d.Detail))
errs = append(errs, err)
}
return errs
}
42 changes: 38 additions & 4 deletions helper/pluginutils/hclutils/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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")
})
}
}
7 changes: 4 additions & 3 deletions helper/pluginutils/loader/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep the trailing colon here, as Prefix() prefixes all elements rather than just once on top. I have a sample output in https://play.golang.org/p/sj0JcGRRVGH .

Though, I'd suggest avoiding multierror formatting. I find it awkward and we can reuse the formatter from other callers.


}

// Marshal the value
Expand Down
9 changes: 3 additions & 6 deletions plugins/shared/cmd/launcher/command/device.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"github.com/hashicorp/go-multierror"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should appear in following grouping of imports, along with other non-stdlib imports. I'd suggest configuring your IDE to use goimports on save instead of plain gofmt.

"io/ioutil"
"os"
"os/exec"
Expand Down Expand Up @@ -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...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of all of these, I like this previous formatting the most :P.

}
c.logger.Trace("parsed hcl config", "config", hclog.Fmt("% #v", pretty.Formatter(val)))

Expand Down