Merge pull request #1062 from iverberk/f-validate-driver-config

Validate driver configuration

client/driver/docker.go

@@ -23,6 +23,7 @@ import (
 	"github.com/hashicorp/nomad/client/driver/executor"
 	cstructs "github.com/hashicorp/nomad/client/driver/structs"
 	"github.com/hashicorp/nomad/helper/discover"
+	"github.com/hashicorp/nomad/helper/fields"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/mitchellh/mapstructure"
 )
@@ -126,6 +127,76 @@ func NewDockerDriver(ctx *DriverContext) Driver {
 	return &DockerDriver{DriverContext: *ctx}
 }
 
+// Validate is used to validate the driver configuration
+func (d *DockerDriver) Validate(config map[string]interface{}) error {
+	fd := &fields.FieldData{
+		Raw: config,
+		Schema: map[string]*fields.FieldSchema{
+			"image": &fields.FieldSchema{
+				Type:     fields.TypeString,
+				Required: true,
+			},
+			"load": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+			"command": &fields.FieldSchema{
+				Type: fields.TypeString,
+			},
+			"args": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+			"ipc_mode": &fields.FieldSchema{
+				Type: fields.TypeString,
+			},
+			"network_mode": &fields.FieldSchema{
+				Type: fields.TypeString,
+			},
+			"pid_mode": &fields.FieldSchema{
+				Type: fields.TypeString,
+			},
+			"uts_mode": &fields.FieldSchema{
+				Type: fields.TypeString,
+			},
+			"port_map": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+			"privileged": &fields.FieldSchema{
+				Type: fields.TypeBool,
+			},
+			"dns_servers": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+			"dns_search_domains": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+			"hostname": &fields.FieldSchema{
+				Type: fields.TypeString,
+			},
+			"labels": &fields.FieldSchema{
+				Type: fields.TypeMap,
+			},
+			"auth": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+			"ssl": &fields.FieldSchema{
+				Type: fields.TypeBool,
+			},
+			"tty": &fields.FieldSchema{
+				Type: fields.TypeBool,
+			},
+			"interactive": &fields.FieldSchema{
+				Type: fields.TypeBool,
+			},
+		},
+	}
+
+	if err := fd.Validate(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 // dockerClient creates *docker.Client. In test / dev mode we can use ENV vars
 // to connect to the docker daemon. In production mode we will read
 // docker.endpoint from the config file.

client/driver/driver.go

@@ -55,6 +55,9 @@ type Driver interface {
 
 	// Open is used to re-open a handle to a task
 	Open(ctx *ExecContext, handleID string) (DriverHandle, error)
+
+	// Drivers must validate their configuration
+	Validate(map[string]interface{}) error
 }
 
 // DriverContext is a means to inject dependencies such as loggers, configs, and
@@ -68,6 +71,18 @@ type DriverContext struct {
 	taskEnv  *env.TaskEnvironment
 }
 
+// NewEmptyDriverContext returns a DriverContext with all fields set to their
+// zero value.
+func NewEmptyDriverContext() *DriverContext {
+	return &DriverContext{
+		taskName: "",
+		config:   nil,
+		node:     nil,
+		logger:   nil,
+		taskEnv:  nil,
+	}
+}
+
 // NewDriverContext initializes a new DriverContext with the specified fields.
 // This enables other packages to create DriverContexts but keeps the fields
 // private to the driver. If we want to change this later we can gorename all of

client/driver/exec.go

@@ -17,6 +17,7 @@ import (
 	"github.com/hashicorp/nomad/client/driver/executor"
 	cstructs "github.com/hashicorp/nomad/client/driver/structs"
 	"github.com/hashicorp/nomad/helper/discover"
+	"github.com/hashicorp/nomad/helper/fields"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/mitchellh/mapstructure"
 )
@@ -58,6 +59,28 @@ func NewExecDriver(ctx *DriverContext) Driver {
 	return &ExecDriver{DriverContext: *ctx}
 }
 
+// Validate is used to validate the driver configuration
+func (d *ExecDriver) Validate(config map[string]interface{}) error {
+	fd := &fields.FieldData{
+		Raw: config,
+		Schema: map[string]*fields.FieldSchema{
+			"command": &fields.FieldSchema{
+				Type:     fields.TypeString,
+				Required: true,
+			},
+			"args": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+		},
+	}
+
+	if err := fd.Validate(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func (d *ExecDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, error) {
 	// Get the current status so that we can log any debug messages only if the
 	// state changes

client/driver/java.go

@@ -22,6 +22,7 @@ import (
 	cstructs "github.com/hashicorp/nomad/client/driver/structs"
 	"github.com/hashicorp/nomad/client/fingerprint"
 	"github.com/hashicorp/nomad/helper/discover"
+	"github.com/hashicorp/nomad/helper/fields"
 	"github.com/hashicorp/nomad/nomad/structs"
 )
 
@@ -66,6 +67,31 @@ func NewJavaDriver(ctx *DriverContext) Driver {
 	return &JavaDriver{DriverContext: *ctx}
 }
 
+// Validate is used to validate the driver configuration
+func (d *JavaDriver) Validate(config map[string]interface{}) error {
+	fd := &fields.FieldData{
+		Raw: config,
+		Schema: map[string]*fields.FieldSchema{
+			"jar_path": &fields.FieldSchema{
+				Type:     fields.TypeString,
+				Required: true,
+			},
+			"jvm_options": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+			"args": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+		},
+	}
+
+	if err := fd.Validate(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func (d *JavaDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, error) {
 	// Get the current status so that we can log any debug messages only if the
 	// state changes

client/driver/qemu.go

@@ -18,6 +18,7 @@ import (
 	cstructs "github.com/hashicorp/nomad/client/driver/structs"
 	"github.com/hashicorp/nomad/client/fingerprint"
 	"github.com/hashicorp/nomad/helper/discover"
+	"github.com/hashicorp/nomad/helper/fields"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/mitchellh/mapstructure"
 )
@@ -65,6 +66,31 @@ func NewQemuDriver(ctx *DriverContext) Driver {
 	return &QemuDriver{DriverContext: *ctx}
 }
 
+// Validate is used to validate the driver configuration
+func (d *QemuDriver) Validate(config map[string]interface{}) error {
+	fd := &fields.FieldData{
+		Raw: config,
+		Schema: map[string]*fields.FieldSchema{
+			"image_path": &fields.FieldSchema{
+				Type:     fields.TypeString,
+				Required: true,
+			},
+			"accelerator": &fields.FieldSchema{
+				Type: fields.TypeString,
+			},
+			"port_map": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+		},
+	}
+
+	if err := fd.Validate(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func (d *QemuDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, error) {
 	// Get the current status so that we can log any debug messages only if the
 	// state changes

client/driver/raw_exec.go

@@ -16,6 +16,7 @@ import (
 	cstructs "github.com/hashicorp/nomad/client/driver/structs"
 	"github.com/hashicorp/nomad/client/fingerprint"
 	"github.com/hashicorp/nomad/helper/discover"
+	"github.com/hashicorp/nomad/helper/fields"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/mitchellh/mapstructure"
 )
@@ -56,6 +57,28 @@ func NewRawExecDriver(ctx *DriverContext) Driver {
 	return &RawExecDriver{DriverContext: *ctx}
 }
 
+// Validate is used to validate the driver configuration
+func (d *RawExecDriver) Validate(config map[string]interface{}) error {
+	fd := &fields.FieldData{
+		Raw: config,
+		Schema: map[string]*fields.FieldSchema{
+			"command": &fields.FieldSchema{
+				Type:     fields.TypeString,
+				Required: true,
+			},
+			"args": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+		},
+	}
+
+	if err := fd.Validate(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func (d *RawExecDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, error) {
 	// Get the current status so that we can log any debug messages only if the
 	// state changes

client/driver/rkt.go

@@ -22,6 +22,7 @@ import (
 	cstructs "github.com/hashicorp/nomad/client/driver/structs"
 	"github.com/hashicorp/nomad/client/fingerprint"
 	"github.com/hashicorp/nomad/helper/discover"
+	"github.com/hashicorp/nomad/helper/fields"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/mitchellh/mapstructure"
 )
@@ -88,6 +89,34 @@ func NewRktDriver(ctx *DriverContext) Driver {
 	return &RktDriver{DriverContext: *ctx}
 }
 
+// Validate is used to validate the driver configuration
+func (d *RktDriver) Validate(config map[string]interface{}) error {
+	fd := &fields.FieldData{
+		Raw: config,
+		Schema: map[string]*fields.FieldSchema{
+			"image": &fields.FieldSchema{
+				Type:     fields.TypeString,
+				Required: true,
+			},
+			"args": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+			"dns_servers": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+			"dns_search_domains": &fields.FieldSchema{
+				Type: fields.TypeArray,
+			},
+		},
+	}
+
+	if err := fd.Validate(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func (d *RktDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, error) {
 	// Get the current status so that we can log any debug messages only if the
 	// state changes

helper/fields/data.go

@@ -0,0 +1,169 @@
+package fields
+
+import (
+	"fmt"
+
+	"github.com/hashicorp/go-multierror"
+	"github.com/mitchellh/mapstructure"
+)
+
+// FieldData contains the raw data and the schema that the data should adhere to
+type FieldData struct {
+	Raw    map[string]interface{}
+	Schema map[string]*FieldSchema
+}
+
+// Validate cycles through the raw data and validates conversions in the schema.
+// It also checks for the existence and value of required fields.
+func (d *FieldData) Validate() error {
+	var result *multierror.Error
+
+	// Scan for missing required fields
+	for field, schema := range d.Schema {
+		if schema.Required {
+			_, ok := d.Raw[field]
+			if !ok {
+				result = multierror.Append(result, fmt.Errorf(
+					"field %q is required", field))
+			}
+		}
+	}
+
+	// Validate field type and value
+	for field, value := range d.Raw {
+		schema, ok := d.Schema[field]
+		if !ok {
+			result = multierror.Append(result, fmt.Errorf(
+				"%q is an invalid field", field))
+			continue
+		}
+
+		switch schema.Type {
+		case TypeBool, TypeInt, TypeMap, TypeArray, TypeString:
+			val, _, err := d.getPrimitive(field, schema)
+			if err != nil {
+				result = multierror.Append(result, fmt.Errorf(
+					"field %q with input %q doesn't seem to be of type %s",
+					field, value, schema.Type))
+			}
+			// Check that we don't have an empty value for required fields
+			if schema.Required && val == schema.Type.Zero() {
+				result = multierror.Append(result, fmt.Errorf(
+					"field %q is required, but no value was found", field))
+			}
+		default:
+			result = multierror.Append(result, fmt.Errorf(
+				"unknown field type %s for field %s", schema.Type, field))
+		}
+	}
+
+	return result.ErrorOrNil()
+}
+
+// Get gets the value for the given field. If the key is an invalid field,
+// FieldData will panic. If you want a safer version of this method, use
+// GetOk. If the field k is not set, the default value (if set) will be
+// returned, otherwise the zero value will be returned.
+func (d *FieldData) Get(k string) interface{} {
+	schema, ok := d.Schema[k]
+	if !ok {
+		panic(fmt.Sprintf("field %s not in the schema", k))
+	}
+
+	value, ok := d.GetOk(k)
+	if !ok {
+		value = schema.DefaultOrZero()
+	}
+
+	return value
+}
+
+// GetOk gets the value for the given field. The second return value
+// will be false if the key is invalid or the key is not set at all.
+func (d *FieldData) GetOk(k string) (interface{}, bool) {
+	schema, ok := d.Schema[k]
+	if !ok {
+		return nil, false
+	}
+
+	result, ok, err := d.GetOkErr(k)
+	if err != nil {
+		panic(fmt.Sprintf("error reading %s: %s", k, err))
+	}
+
+	if ok && result == nil {
+		result = schema.DefaultOrZero()
+	}
+
+	return result, ok
+}
+
+// GetOkErr is the most conservative of all the Get methods. It returns
+// whether key is set or not, but also an error value. The error value is
+// non-nil if the field doesn't exist or there was an error parsing the
+// field value.
+func (d *FieldData) GetOkErr(k string) (interface{}, bool, error) {
+	schema, ok := d.Schema[k]
+	if !ok {
+		return nil, false, fmt.Errorf("unknown field: %s", k)
+	}
+
+	switch schema.Type {
+	case TypeBool, TypeInt, TypeMap, TypeArray, TypeString:
+		return d.getPrimitive(k, schema)
+	default:
+		return nil, false,
+			fmt.Errorf("unknown field type %s for field %s", schema.Type, k)
+	}
+}
+
+// getPrimitive tries to convert the raw value of a field to its data type as
+// defined in the schema. It does strict type checking, so the value will need
+// to be able to convert to the appropriate type directly.
+func (d *FieldData) getPrimitive(
+	k string, schema *FieldSchema) (interface{}, bool, error) {
+	raw, ok := d.Raw[k]
+	if !ok {
+		return nil, false, nil
+	}
+
+	switch schema.Type {
+	case TypeBool:
+		var result bool
+		if err := mapstructure.Decode(raw, &result); err != nil {
+			return nil, true, err
+		}
+		return result, true, nil
+
+	case TypeInt:
+		var result int
+		if err := mapstructure.Decode(raw, &result); err != nil {
+			return nil, true, err
+		}
+		return result, true, nil
+
+	case TypeString:
+		var result string
+		if err := mapstructure.Decode(raw, &result); err != nil {
+			return nil, true, err
+		}
+		return result, true, nil
+
+	case TypeMap:
+		var result map[string]interface{}
+		if err := mapstructure.Decode(raw, &result); err != nil {
+			return nil, true, err
+		}
+		return result, true, nil
+
+	case TypeArray:
+		var result []interface{}
+		if err := mapstructure.Decode(raw, &result); err != nil {
+			return nil, true, err
+		}
+		return result, true, nil
+
+	default:
+		panic(fmt.Sprintf("Unknown type: %s", schema.Type))
+	}
+}

helper/fields/data_test.go

@@ -0,0 +1,120 @@
+package fields
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestFieldDataGet(t *testing.T) {
+	cases := map[string]struct {
+		Schema map[string]*FieldSchema
+		Raw    map[string]interface{}
+		Key    string
+		Value  interface{}
+	}{
+		"string type, string value": {
+			map[string]*FieldSchema{
+				"foo": &FieldSchema{Type: TypeString},
+			},
+			map[string]interface{}{
+				"foo": "bar",
+			},
+			"foo",
+			"bar",
+		},
+
+		"string type, int value": {
+			map[string]*FieldSchema{
+				"foo": &FieldSchema{Type: TypeInt},
+			},
+			map[string]interface{}{
+				"foo": 42,
+			},
+			"foo",
+			42,
+		},
+
+		"string type, unset value": {
+			map[string]*FieldSchema{
+				"foo": &FieldSchema{Type: TypeString},
+			},
+			map[string]interface{}{},
+			"foo",
+			"",
+		},
+
+		"string type, unset value with default": {
+			map[string]*FieldSchema{
+				"foo": &FieldSchema{
+					Type:    TypeString,
+					Default: "bar",
+				},
+			},
+			map[string]interface{}{},
+			"foo",
+			"bar",
+		},
+
+		"int type, int value": {
+			map[string]*FieldSchema{
+				"foo": &FieldSchema{Type: TypeInt},
+			},
+			map[string]interface{}{
+				"foo": 42,
+			},
+			"foo",
+			42,
+		},
+
+		"bool type, bool value": {
+			map[string]*FieldSchema{
+				"foo": &FieldSchema{Type: TypeBool},
+			},
+			map[string]interface{}{
+				"foo": false,
+			},
+			"foo",
+			false,
+		},
+
+		"map type, map value": {
+			map[string]*FieldSchema{
+				"foo": &FieldSchema{Type: TypeMap},
+			},
+			map[string]interface{}{
+				"foo": map[string]interface{}{
+					"child": true,
+				},
+			},
+			"foo",
+			map[string]interface{}{
+				"child": true,
+			},
+		},
+
+		"array type, array value": {
+			map[string]*FieldSchema{
+				"foo": &FieldSchema{Type: TypeArray},
+			},
+			map[string]interface{}{
+				"foo": []interface{}{},
+			},
+			"foo",
+			[]interface{}{},
+		},
+	}
+
+	for name, tc := range cases {
+		data := &FieldData{
+			Raw:    tc.Raw,
+			Schema: tc.Schema,
+		}
+
+		actual := data.Get(tc.Key)
+		if !reflect.DeepEqual(actual, tc.Value) {
+			t.Fatalf(
+				"bad: %s\n\nExpected: %#v\nGot: %#v",
+				name, tc.Value, actual)
+		}
+	}
+}

helper/fields/schema.go

@@ -0,0 +1,19 @@
+package fields
+
+// FieldSchema is a basic schema to describe the format of a configuration field
+type FieldSchema struct {
+	Type        FieldType
+	Default     interface{}
+	Description string
+	Required    bool
+}
+
+// DefaultOrZero returns the default value if it is set, or otherwise
+// the zero value of the type.
+func (s *FieldSchema) DefaultOrZero() interface{} {
+	if s.Default != nil {
+		return s.Default
+	}
+
+	return s.Type.Zero()
+}

helper/fields/schema_test.go

@@ -0,0 +1,22 @@
+package fields
+
+import "testing"
+
+func TestFieldSchemaDefaultOrZero(t *testing.T) {
+	fs := &FieldSchema{
+		Type:    TypeString,
+		Default: "default",
+	}
+
+	if d := fs.DefaultOrZero(); d != "default" {
+		t.Fatalf("bad: Expected: default Got: %s", d)
+	}
+
+	fs = &FieldSchema{
+		Type: TypeString,
+	}
+
+	if d := fs.DefaultOrZero(); d != "" {
+		t.Fatalf("bad: Expected: \"\" Got: %s", d)
+	}
+}

helper/fields/type.go

@@ -0,0 +1,47 @@
+package fields
+
+// FieldType is the enum of types that a field can be.
+type FieldType uint
+
+const (
+	TypeInvalid FieldType = 0
+	TypeString  FieldType = iota
+	TypeInt
+	TypeBool
+	TypeMap
+	TypeArray
+)
+
+func (t FieldType) String() string {
+	switch t {
+	case TypeString:
+		return "string"
+	case TypeInt:
+		return "integer"
+	case TypeBool:
+		return "boolean"
+	case TypeMap:
+		return "map"
+	case TypeArray:
+		return "array"
+	default:
+		return "unknown type"
+	}
+}
+
+func (t FieldType) Zero() interface{} {
+	switch t {
+	case TypeString:
+		return ""
+	case TypeInt:
+		return 0
+	case TypeBool:
+		return false
+	case TypeMap:
+		return map[string]interface{}{}
+	case TypeArray:
+		return []interface{}{}
+	default:
+		panic("unknown type: " + t.String())
+	}
+}

helper/fields/type_test.go

@@ -0,0 +1,61 @@
+package fields
+
+import "testing"
+
+func TestFieldTypeString(t *testing.T) {
+	if s := TypeString.String(); s != "string" {
+		t.Fatalf("bad: expected 'string' got: %s", s)
+	}
+
+	if s := TypeInt.String(); s != "integer" {
+		t.Fatalf("bad: expected 'integer' got: %s", s)
+	}
+
+	if s := TypeBool.String(); s != "boolean" {
+		t.Fatalf("bad: expected 'boolean' got: %s", s)
+	}
+
+	if s := TypeMap.String(); s != "map" {
+		t.Fatalf("bad: expected 'map' got: %v", s)
+	}
+
+	if s := TypeArray.String(); s != "array" {
+		t.Fatalf("bad: expected 'array' got: %v", s)
+	}
+
+	if s := TypeInvalid.String(); s != "unknown type" {
+		t.Fatalf("bad: expected 'unkown type' got: %v", s)
+	}
+}
+
+func TestFieldTypeZero(t *testing.T) {
+	if z := TypeString.Zero(); z != "" {
+		t.Fatalf("bad: expected \"\" got: %v", z)
+	}
+
+	if z := TypeInt.Zero(); z != 0 {
+		t.Fatalf("bad: expected 0 got: %v", z)
+	}
+
+	if z := TypeBool.Zero(); z != false {
+		t.Fatalf("bad: expected false got: %v", z)
+	}
+
+	z := TypeMap.Zero()
+	if _, ok := z.(map[string]interface{}); !ok {
+		t.Fatalf("bad: expected map[string]interface{} got: %v", z)
+	}
+
+	z = TypeArray.Zero()
+	if _, ok := z.([]interface{}); !ok {
+		t.Fatalf("bad: expected []interface{} got: %v", z)
+	}
+
+	defer func() {
+		if r := recover(); r == nil {
+			t.Errorf("The code did not panic")
+		}
+	}()
+
+	TypeInvalid.Zero()
+}

jobspec/parse.go

@@ -13,6 +13,7 @@ import (
 	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/hcl"
 	"github.com/hashicorp/hcl/hcl/ast"
+	"github.com/hashicorp/nomad/client/driver"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/mitchellh/mapstructure"
 )
@@ -537,6 +538,22 @@ func parseTasks(jobName string, taskGroupName string, result *[]*structs.Task, l
 					return err
 				}
 			}
+
+			// Instantiate a driver to validate the configuration
+			d, err := driver.NewDriver(
+				t.Driver,
+				driver.NewEmptyDriverContext(),
+			)
+
+			if err != nil {
+				return multierror.Prefix(err,
+					fmt.Sprintf("'%s', config ->", n))
+			}
+
+			if err := d.Validate(t.Config); err != nil {
+				return multierror.Prefix(err,
+					fmt.Sprintf("'%s', config ->", n))
+			}
 		}
 
 		// Parse constraints

jobspec/parse_test.go

@@ -53,7 +53,7 @@ func TestParse(t *testing.T) {
 								Name:   "outside",
 								Driver: "java",
 								Config: map[string]interface{}{
-									"jar": "s3://my-cool-store/foo.jar",
+									"jar_path": "s3://my-cool-store/foo.jar",
 								},
 								Meta: map[string]string{
 									"my-cool-key": "foobar",
@@ -148,7 +148,7 @@ func TestParse(t *testing.T) {
 							},
 							&structs.Task{
 								Name:   "storagelocker",
-								Driver: "java",
+								Driver: "docker",
 								User:   "",
 								Config: map[string]interface{}{
 									"image": "hashicorp/storagelocker",
@@ -302,6 +302,7 @@ func TestParse(t *testing.T) {
 								Name:   "bar",
 								Driver: "docker",
 								Config: map[string]interface{}{
+									"image": "hashicorp/image",
 									"port_map": []map[string]interface{}{
 										map[string]interface{}{
 											"db": 1234,
@@ -319,6 +320,7 @@ func TestParse(t *testing.T) {
 			},
 			false,
 		},
+
 		{
 			"bad-artifact.hcl",
 			nil,
@@ -399,10 +401,65 @@ func TestParse(t *testing.T) {
 	}
 }
 
+func TestBadConfigEmpty(t *testing.T) {
+	path, err := filepath.Abs(filepath.Join("./test-fixtures", "bad-config-empty.hcl"))
+	if err != nil {
+		t.Fatalf("Can't get absolute path for file: %s", err)
+	}
+
+	_, err = ParseFile(path)
+
+	if !strings.Contains(err.Error(), "field \"image\" is required, but no value was found") {
+		t.Fatalf("\nExpected error\n  %s\ngot\n  %v",
+			"field \"image\" is required, but no value was found",
+			err,
+		)
+	}
+}
+
+func TestBadConfigMissing(t *testing.T) {
+	path, err := filepath.Abs(filepath.Join("./test-fixtures", "bad-config-missing.hcl"))
+	if err != nil {
+		t.Fatalf("Can't get absolute path for file: %s", err)
+	}
+
+	_, err = ParseFile(path)
+
+	if !strings.Contains(err.Error(), "field \"image\" is required") {
+		t.Fatalf("\nExpected error\n  %s\ngot\n  %v",
+			"field \"image\" is required",
+			err,
+		)
+	}
+}
+
+func TestBadConfig(t *testing.T) {
+	path, err := filepath.Abs(filepath.Join("./test-fixtures", "bad-config.hcl"))
+	if err != nil {
+		t.Fatalf("Can't get absolute path for file: %s", err)
+	}
+
+	_, err = ParseFile(path)
+
+	if !strings.Contains(err.Error(), "seem to be of type boolean") {
+		t.Fatalf("\nExpected error\n  %s\ngot\n  %v",
+			"seem to be of type boolean",
+			err,
+		)
+	}
+
+	if !strings.Contains(err.Error(), "\"foo\" is an invalid field") {
+		t.Fatalf("\nExpected error\n  %s\ngot\n  %v",
+			"\"foo\" is an invalid field",
+			err,
+		)
+	}
+}
+
 func TestBadPorts(t *testing.T) {
 	path, err := filepath.Abs(filepath.Join("./test-fixtures", "bad-ports.hcl"))
 	if err != nil {
-		t.Fatalf("Can't get absoluate path for file: %s", err)
+		t.Fatalf("Can't get absolute path for file: %s", err)
 	}
 
 	_, err = ParseFile(path)

jobspec/test-fixtures/bad-config-empty.hcl

@@ -0,0 +1,16 @@
+job "binstore-storagelocker" {
+  group "binsl" {
+    count = 5
+
+    task "binstore" {
+      driver = "docker"
+
+      config {
+        image = ""
+      }
+
+      resources {
+      }
+    }
+  }
+}

jobspec/test-fixtures/bad-config-missing.hcl

@@ -0,0 +1,15 @@
+job "binstore-storagelocker" {
+  group "binsl" {
+    count = 5
+
+    task "binstore" {
+      driver = "docker"
+
+      config {
+      }
+
+      resources {
+      }
+    }
+  }
+}

jobspec/test-fixtures/bad-config.hcl

@@ -0,0 +1,18 @@
+job "binstore-storagelocker" {
+  group "binsl" {
+    count = 5
+
+    task "binstore" {
+      driver = "docker"
+
+      config {
+        image      = "hashicorp/image"
+        privileged = "false"
+        foo        = "bar"
+      }
+
+      resources {
+      }
+    }
+  }
+}

jobspec/test-fixtures/bad-ports.hcl

@@ -1,86 +1,101 @@
 job "binstore-storagelocker" {
-    region = "global"
-    type = "service"
-    priority = 50
-    all_at_once = true
-    datacenters = ["us2", "eu1"]
+  region      = "global"
+  type        = "service"
+  priority    = 50
+  all_at_once = true
+  datacenters = ["us2", "eu1"]
+
+  meta {
+    foo = "bar"
+  }
+
+  constraint {
+    attribute = "kernel.os"
+    value     = "windows"
+  }
+
+  update {
+    stagger      = "60s"
+    max_parallel = 2
+  }
+
+  task "outside" {
+    driver = "java"
+
+    config {
+      jar_path = "s3://my-cool-store/foo.jar"
+    }
 
     meta {
-        foo = "bar"
+      my-cool-key = "foobar"
+    }
+  }
+
+  group "binsl" {
+    count = 5
+
+    task "binstore" {
+      driver = "docker"
+
+      config {
+        image = "hashicorp/binstore"
+      }
+
+      resources {
+        cpu    = 500
+        memory = 128
+
+        network {
+          mbits = "100"
+
+          port "one" {
+            static = 1
+          }
+
+          port "two" {
+            static = 2
+          }
+
+          port "three" {
+            static = 3
+          }
+
+          port "this_is_aport" {
+          }
+
+          port "" {
+          }
+        }
+      }
+    }
+
+    task "storagelocker" {
+      driver = "docker"
+
+      config {
+        image = "hashicorp/storagelocker"
+      }
+
+      resources {
+        cpu    = 500
+        memory = 128
+      }
+
+      constraint {
+        attribute = "kernel.arch"
+        value     = "amd64"
+      }
     }
 
     constraint {
-        attribute = "kernel.os"
-        value = "windows"
+      attribute = "kernel.os"
+      value     = "linux"
     }
 
-    update {
-        stagger = "60s"
-        max_parallel = 2
-    }
-
-    task "outside" {
-        driver = "java"
-        config {
-           jar = "s3://my-cool-store/foo.jar"
-        }
-        meta {
-           my-cool-key = "foobar"
-        }
-    }
-
-    group "binsl" {
-        count = 5
-        task "binstore" {
-            driver = "docker"
-            config {
-                image = "hashicorp/binstore"
-            }
-            resources {
-                cpu = 500
-                memory = 128
-
-                network {
-                    mbits = "100"
-                    port "one" {
-                        static = 1
-                    }
-                    port "two" {
-                        static = 2
-                    }
-                    port "three" {
-                        static = 3
-                    }
-                    port "this_is_aport" {}
-                    port ""{}
-                }
-            }
-        }
-
-        task "storagelocker" {
-            driver = "java"
-            config {
-                image = "hashicorp/storagelocker"
-            }
-            resources {
-                cpu = 500
-                memory = 128
-            }
-            constraint {
-                attribute = "kernel.arch"
-                value = "amd64"
-            }
-        }
-
-        constraint {
-            attribute = "kernel.os"
-            value = "linux"
-        }
-
-        meta {
-            elb_mode = "tcp"
-            elb_interval = 10
-            elb_checks = 3
-        }
+    meta {
+      elb_mode     = "tcp"
+      elb_interval = 10
+      elb_checks   = 3
     }
+  }
 }

jobspec/test-fixtures/basic.hcl

@@ -1,129 +1,153 @@
 job "binstore-storagelocker" {
-    region = "global"
-    type = "service"
-    priority = 50
-    all_at_once = true
-    datacenters = ["us2", "eu1"]
+  region      = "global"
+  type        = "service"
+  priority    = 50
+  all_at_once = true
+  datacenters = ["us2", "eu1"]
+
+  meta {
+    foo = "bar"
+  }
+
+  constraint {
+    attribute = "kernel.os"
+    value     = "windows"
+  }
+
+  update {
+    stagger      = "60s"
+    max_parallel = 2
+  }
+
+  task "outside" {
+    driver = "java"
+
+    config {
+      jar_path = "s3://my-cool-store/foo.jar"
+    }
 
     meta {
-        foo = "bar"
+      my-cool-key = "foobar"
+    }
+  }
+
+  group "binsl" {
+    count = 5
+
+    restart {
+      attempts = 5
+      interval = "10m"
+      delay    = "15s"
+      mode     = "delay"
+    }
+
+    task "binstore" {
+      driver = "docker"
+      user   = "bob"
+
+      config {
+        image = "hashicorp/binstore"
+      }
+
+      logs {
+        max_files     = 10
+        max_file_size = 100
+      }
+
+      env {
+        HELLO = "world"
+        LOREM = "ipsum"
+      }
+
+      service {
+        tags = ["foo", "bar"]
+        port = "http"
+
+        check {
+          name     = "check-name"
+          type     = "tcp"
+          interval = "10s"
+          timeout  = "2s"
+        }
+      }
+
+      resources {
+        cpu    = 500
+        memory = 128
+
+        network {
+          mbits = "100"
+
+          port "one" {
+            static = 1
+          }
+
+          port "two" {
+            static = 2
+          }
+
+          port "three" {
+            static = 3
+          }
+
+          port "http" {
+          }
+
+          port "https" {
+          }
+
+          port "admin" {
+          }
+        }
+      }
+
+      kill_timeout = "22s"
+
+      artifact {
+        source = "http://foo.com/artifact"
+
+        options {
+          checksum = "md5:b8a4f3f72ecab0510a6a31e997461c5f"
+        }
+      }
+
+      artifact {
+        source = "http://bar.com/artifact"
+
+        options {
+          checksum = "md5:ff1cc0d3432dad54d607c1505fb7245c"
+        }
+      }
+    }
+
+    task "storagelocker" {
+      driver = "docker"
+
+      config {
+        image = "hashicorp/storagelocker"
+      }
+
+      resources {
+        cpu    = 500
+        memory = 128
+        iops   = 30
+      }
+
+      constraint {
+        attribute = "kernel.arch"
+        value     = "amd64"
+      }
     }
 
     constraint {
-        attribute = "kernel.os"
-        value = "windows"
+      attribute = "kernel.os"
+      value     = "linux"
     }
 
-    update {
-        stagger = "60s"
-        max_parallel = 2
-    }
-
-    task "outside" {
-        driver = "java"
-        config {
-           jar = "s3://my-cool-store/foo.jar"
-        }
-        meta {
-           my-cool-key = "foobar"
-        }
-    }
-
-    group "binsl" {
-        count = 5
-        restart {
-            attempts = 5
-            interval = "10m"
-            delay = "15s"
-            mode = "delay"
-        }
-        task "binstore" {
-            driver = "docker"
-            user = "bob"
-            config {
-                image = "hashicorp/binstore"
-            }
-            logs {
-                max_files = 10
-                max_file_size = 100
-            }
-            env {
-              HELLO = "world"
-              LOREM = "ipsum"
-            }
-            service {
-                tags = ["foo", "bar"]
-                port = "http"
-                check {
-                    name = "check-name"
-                    type = "tcp"
-                    interval = "10s"
-                    timeout = "2s"
-                }
-            }
-            resources {
-                cpu = 500
-                memory = 128
-
-                network {
-                    mbits = "100"
-                    port "one" {
-                        static = 1
-                    }
-                    port "two" {
-                        static = 2
-                    }
-                    port "three" {
-                        static = 3
-                    }
-                    port "http" {}
-                    port "https" {}
-                    port "admin" {}
-                }
-            }
-
-            kill_timeout = "22s"
-
-            artifact {
-                source = "http://foo.com/artifact"
-                options {
-                    checksum = "md5:b8a4f3f72ecab0510a6a31e997461c5f"
-                }
-            }
-
-            artifact {
-                source = "http://bar.com/artifact"
-                options {
-                    checksum = "md5:ff1cc0d3432dad54d607c1505fb7245c"
-                }
-            }
-        }
-
-        task "storagelocker" {
-            driver = "java"
-            config {
-                image = "hashicorp/storagelocker"
-            }
-            resources {
-                cpu = 500
-                memory = 128
-                iops = 30
-            }
-            constraint {
-                attribute = "kernel.arch"
-                value = "amd64"
-            }
-        }
-
-        constraint {
-            attribute = "kernel.os"
-            value = "linux"
-        }
-
-        meta {
-            elb_mode = "tcp"
-            elb_interval = 10
-            elb_checks = 3
-        }
+    meta {
+      elb_mode     = "tcp"
+      elb_interval = 10
+      elb_checks   = 3
     }
+  }
 }

jobspec/test-fixtures/basic_wrong_key.hcl

@@ -1,114 +1,136 @@
 job "binstore-storagelocker" {
-    region = "global"
-    type = "service"
-    priority = 50
-    all_at_once = true
-    datacenters = ["us2", "eu1"]
+  region      = "global"
+  type        = "service"
+  priority    = 50
+  all_at_once = true
+  datacenters = ["us2", "eu1"]
+
+  meta {
+    foo = "bar"
+  }
+
+  constraint {
+    attribute = "kernel.os"
+    value     = "windows"
+  }
+
+  update {
+    stagger      = "60s"
+    max_parallel = 2
+  }
+
+  task "outside" {
+    driver = "java"
+
+    config {
+      jar_path = "s3://my-cool-store/foo.jar"
+    }
 
     meta {
-        foo = "bar"
+      my-cool-key = "foobar"
+    }
+  }
+
+  group "binsl" {
+    count = 5
+
+    restart {
+      attempts = 5
+      interval = "10m"
+      delay    = "15s"
+      mode     = "delay"
+    }
+
+    task "binstore" {
+      driver = "docker"
+
+      config {
+        image = "hashicorp/binstore"
+      }
+
+      logs {
+        max_files     = 10
+        max_file_size = 100
+      }
+
+      env {
+        HELLO = "world"
+        LOREM = "ipsum"
+      }
+
+      service {
+        tags = ["foo", "bar"]
+        port = "http"
+
+        check {
+          name    = "check-name"
+          type    = "tcp"
+          nterval = "10s"
+          timeout = "2s"
+        }
+      }
+
+      resources {
+        cpu    = 500
+        memory = 128
+
+        network {
+          mbits = "100"
+
+          port "one" {
+            static = 1
+          }
+
+          port "two" {
+            static = 2
+          }
+
+          port "three" {
+            static = 3
+          }
+
+          port "http" {
+          }
+
+          port "https" {
+          }
+
+          port "admin" {
+          }
+        }
+      }
+
+      kill_timeout = "22s"
+    }
+
+    task "storagelocker" {
+      driver = "docker"
+
+      config {
+        image = "hashicorp/storagelocker"
+      }
+
+      resources {
+        cpu    = 500
+        memory = 128
+        iops   = 30
+      }
+
+      constraint {
+        attribute = "kernel.arch"
+        value     = "amd64"
+      }
     }
 
     constraint {
-        attribute = "kernel.os"
-        value = "windows"
+      attribute = "kernel.os"
+      value     = "linux"
     }
 
-    update {
-        stagger = "60s"
-        max_parallel = 2
-    }
-
-    task "outside" {
-        driver = "java"
-        config {
-           jar = "s3://my-cool-store/foo.jar"
-        }
-        meta {
-           my-cool-key = "foobar"
-        }
-    }
-
-    group "binsl" {
-        count = 5
-        restart {
-            attempts = 5
-            interval = "10m"
-            delay = "15s"
-            mode = "delay"
-        }
-        task "binstore" {
-            driver = "docker"
-            config {
-                image = "hashicorp/binstore"
-            }
-            logs {
-                max_files = 10
-                max_file_size = 100
-            }
-            env {
-              HELLO = "world"
-              LOREM = "ipsum"
-            }
-            service {
-                tags = ["foo", "bar"]
-                port = "http"
-                check {
-                    name = "check-name"
-                    type = "tcp"
-                    nterval = "10s"
-                    timeout = "2s"
-                }
-            }
-            resources {
-                cpu = 500
-                memory = 128
-
-                network {
-                    mbits = "100"
-                    port "one" {
-                        static = 1
-                    }
-                    port "two" {
-                        static = 2
-                    }
-                    port "three" {
-                        static = 3
-                    }
-                    port "http" {}
-                    port "https" {}
-                    port "admin" {}
-                }
-            }
-
-            kill_timeout = "22s"
-        }
-
-        task "storagelocker" {
-            driver = "java"
-            config {
-                image = "hashicorp/storagelocker"
-            }
-            resources {
-                cpu = 500
-                memory = 128
-                iops = 30
-            }
-            constraint {
-                attribute = "kernel.arch"
-                value = "amd64"
-            }
-        }
-
-        constraint {
-            attribute = "kernel.os"
-            value = "linux"
-        }
-
-        meta {
-            elb_mode = "tcp"
-            elb_interval = 10
-            elb_checks = 3
-        }
+    meta {
+      elb_mode     = "tcp"
+      elb_interval = 10
+      elb_checks   = 3
     }
+  }
 }

jobspec/test-fixtures/incorrect-service-def.hcl

@@ -1,78 +1,92 @@
 job "binstore-storagelocker" {
-    region = "global"
-    type = "service"
-    priority = 50
-    all_at_once = true
-    datacenters = ["us2", "eu1"]
+  region      = "global"
+  type        = "service"
+  priority    = 50
+  all_at_once = true
+  datacenters = ["us2", "eu1"]
+
+  meta {
+    foo = "bar"
+  }
+
+  constraint {
+    attribute = "kernel.os"
+    value     = "windows"
+  }
+
+  update {
+    stagger      = "60s"
+    max_parallel = 2
+  }
+
+  task "outside" {
+    driver = "java"
+
+    config {
+      jar_path = "s3://my-cool-store/foo.jar"
+    }
 
     meta {
-        foo = "bar"
+      my-cool-key = "foobar"
+    }
+  }
+
+  group "binsl" {
+    count = 5
+
+    restart {
+      attempts = 5
+      interval = "10m"
+      delay    = "15s"
     }
 
-    constraint {
-        attribute = "kernel.os"
-        value = "windows"
-    }
+    task "binstore" {
+      driver = "docker"
 
-    update {
-        stagger = "60s"
-        max_parallel = 2
-    }
+      config {
+        image = "hashicorp/binstore"
+      }
 
-    task "outside" {
-        driver = "java"
-        config {
-           jar = "s3://my-cool-store/foo.jar"
+      env {
+        HELLO = "world"
+        LOREM = "ipsum"
+      }
+
+      service {
+        tags = ["foo", "bar"]
+        port = "http"
+
+        check {
+          name     = "check-name"
+          type     = "http"
+          interval = "10s"
+          timeout  = "2s"
         }
-        meta {
-           my-cool-key = "foobar"
-        }
-    }
+      }
 
-    group "binsl" {
-        count = 5
-        restart {
-            attempts = 5
-            interval = "10m"
-            delay = "15s"
-        }
-        task "binstore" {
-            driver = "docker"
-            config {
-                image = "hashicorp/binstore"
-            }
-            env {
-              HELLO = "world"
-              LOREM = "ipsum"
-            }
-            service {
-                tags = ["foo", "bar"]
-                port = "http"
-                check {
-                    name = "check-name"
-                    type = "http"
-                    interval = "10s"
-                    timeout = "2s"
-                }
-            }
-            service {
-                port = "one"
-            }
-            resources {
-                cpu = 500
-                memory = 128
+      service {
+        port = "one"
+      }
 
-                network {
-                    mbits = "100"
-                    port "one" {
-                        static = 1
-                    }
-                    port "three" {
-                        static = 3
-                    }
-                    port "http" {}
-                }
-            }
+      resources {
+        cpu    = 500
+        memory = 128
+
+        network {
+          mbits = "100"
+
+          port "one" {
+            static = 1
+          }
+
+          port "three" {
+            static = 3
+          }
+
+          port "http" {
+          }
         }
+      }
     }
+  }
 }

jobspec/test-fixtures/overlapping-ports.hcl

@@ -1,87 +1,102 @@
 job "binstore-storagelocker" {
-    region = "global"
-    type = "service"
-    priority = 50
-    all_at_once = true
-    datacenters = ["us2", "eu1"]
+  region      = "global"
+  type        = "service"
+  priority    = 50
+  all_at_once = true
+  datacenters = ["us2", "eu1"]
+
+  meta {
+    foo = "bar"
+  }
+
+  constraint {
+    attribute = "kernel.os"
+    value     = "windows"
+  }
+
+  update {
+    stagger      = "60s"
+    max_parallel = 2
+  }
+
+  task "outside" {
+    driver = "java"
+
+    config {
+      jar_path = "s3://my-cool-store/foo.jar"
+    }
 
     meta {
-        foo = "bar"
+      my-cool-key = "foobar"
+    }
+  }
+
+  group "binsl" {
+    count = 5
+
+    task "binstore" {
+      driver = "docker"
+
+      config {
+        image = "hashicorp/binstore"
+      }
+
+      resources {
+        cpu    = 500
+        memory = 128
+
+        network {
+          mbits = "100"
+
+          port "one" {
+            static = 1
+          }
+
+          port "two" {
+            static = 2
+          }
+
+          port "three" {
+            static = 3
+          }
+
+          port "Http"{
+          }
+          port "http"{
+          }
+          port "HTTP"{
+          }
+        }
+      }
+    }
+
+    task "storagelocker" {
+      driver = "docker"
+
+      config {
+        image = "hashicorp/storagelocker"
+      }
+
+      resources {
+        cpu    = 500
+        memory = 128
+      }
+
+      constraint {
+        attribute = "kernel.arch"
+        value     = "amd64"
+      }
     }
 
     constraint {
-        attribute = "kernel.os"
-        value = "windows"
+      attribute = "kernel.os"
+      value     = "linux"
     }
 
-    update {
-        stagger = "60s"
-        max_parallel = 2
-    }
-
-    task "outside" {
-        driver = "java"
-        config {
-           jar = "s3://my-cool-store/foo.jar"
-        }
-        meta {
-           my-cool-key = "foobar"
-        }
-    }
-
-    group "binsl" {
-        count = 5
-        task "binstore" {
-            driver = "docker"
-            config {
-                image = "hashicorp/binstore"
-            }
-            resources {
-                cpu = 500
-                memory = 128
-
-                network {
-                    mbits = "100"
-                    port "one" {
-                        static = 1
-                    }
-                    port "two" {
-                        static = 2
-                    }
-                    port "three" {
-                        static = 3
-                    }
-                    port "Http" {}
-                    port "http" {}
-                    port "HTTP" {}
-                }
-            }
-        }
-
-        task "storagelocker" {
-            driver = "java"
-            config {
-                image = "hashicorp/storagelocker"
-            }
-            resources {
-                cpu = 500
-                memory = 128
-            }
-            constraint {
-                attribute = "kernel.arch"
-                value = "amd64"
-            }
-        }
-
-        constraint {
-            attribute = "kernel.os"
-            value = "linux"
-        }
-
-        meta {
-            elb_mode = "tcp"
-            elb_interval = 10
-            elb_checks = 3
-        }
+    meta {
+      elb_mode     = "tcp"
+      elb_interval = 10
+      elb_checks   = 3
     }
+  }
 }

jobspec/test-fixtures/task-nested-config.hcl

@@ -1,10 +1,13 @@
 job "foo" {
-    task "bar" {
-        driver = "docker"
-        config {
-            port_map {
-                db = 1234
-            }
-        }
+  task "bar" {
+    driver = "docker"
+
+    config {
+      image = "hashicorp/image"
+
+      port_map {
+        db = 1234
+      }
     }
+  }
 }