From 43b359914b36aec6985130194a57b2eb538f1f2a Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 5 Nov 2018 17:39:02 -0800 Subject: [PATCH 1/8] client: interpolate driver configurations Also add missing SetDriverNetwork calls. --- client/allocrunner/taskrunner/task_runner.go | 2 +- .../taskrunner/task_runner_getters.go | 6 +- .../taskrunner/task_runner_test.go | 60 ++++++++++++ client/config/testing.go | 8 +- client/driver/env/env.go | 15 +++ client/driver/env/env_test.go | 95 +++++++++++++++++++ drivers/mock/driver.go | 25 +++++ 7 files changed, 203 insertions(+), 8 deletions(-) diff --git a/client/allocrunner/taskrunner/task_runner.go b/client/allocrunner/taskrunner/task_runner.go index b90f45457b7..699245eb5b9 100644 --- a/client/allocrunner/taskrunner/task_runner.go +++ b/client/allocrunner/taskrunner/task_runner.go @@ -462,8 +462,8 @@ func (tr *TaskRunner) runDriver() error { // TODO(nickethier): make sure this uses alloc.AllocatedResources once #4750 is rebased taskConfig := tr.buildTaskConfig() - // TODO: load variables evalCtx := &hcl.EvalContext{ + Variables: tr.envBuilder.Build().AllValues(), Functions: shared.GetStdlibFuncs(), } diff --git a/client/allocrunner/taskrunner/task_runner_getters.go b/client/allocrunner/taskrunner/task_runner_getters.go index 3bce6ffcd77..89d67a99ebb 100644 --- a/client/allocrunner/taskrunner/task_runner_getters.go +++ b/client/allocrunner/taskrunner/task_runner_getters.go @@ -61,11 +61,15 @@ func (tr *TaskRunner) getDriverHandle() *DriverHandle { return tr.handle } -// setDriverHanlde sets the driver handle and creates a new result proxy. +// setDriverHanlde sets the driver handle, creates a new result proxy, and +// updates the driver network in the task's environment. func (tr *TaskRunner) setDriverHandle(handle *DriverHandle) { tr.handleLock.Lock() defer tr.handleLock.Unlock() tr.handle = handle + + // Update the environment's driver network + tr.envBuilder.SetDriverNetwork(handle.net) } func (tr *TaskRunner) clearDriverHandle() { diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go index 88e596d78a7..4e1496286ce 100644 --- a/client/allocrunner/taskrunner/task_runner_test.go +++ b/client/allocrunner/taskrunner/task_runner_test.go @@ -5,12 +5,14 @@ import ( "fmt" "path/filepath" "testing" + "time" "github.com/hashicorp/nomad/client/allocdir" "github.com/hashicorp/nomad/client/config" consulapi "github.com/hashicorp/nomad/client/consul" cstate "github.com/hashicorp/nomad/client/state" "github.com/hashicorp/nomad/client/vaultclient" + mockdriver "github.com/hashicorp/nomad/drivers/mock" "github.com/hashicorp/nomad/helper/testlog" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" @@ -154,3 +156,61 @@ func TestTaskRunner_Restore_Running(t *testing.T) { } assert.Equal(t, 1, started) } + +// TestTaskRunner_TaskEnv asserts driver configurations are interpolated. +func TestTaskRunner_TaskEnv(t *testing.T) { + t.Parallel() + require := require.New(t) + + alloc := mock.BatchAlloc() + alloc.Job.TaskGroups[0].Meta = map[string]string{ + "common_user": "somebody", + } + task := alloc.Job.TaskGroups[0].Tasks[0] + task.Name = "testtask_taskenv" + task.Driver = "mock_driver" + task.Meta = map[string]string{ + "foo": "bar", + } + + // Use interpolation from both node attributes and meta vars + task.Config = map[string]interface{}{ + "run_for": time.Millisecond, + "stdout_string": `${node.region} ${NOMAD_META_foo}`, + } + task.User = "${NOMAD_META_common_user}" + + conf, cleanup := testTaskRunnerConfig(t, alloc, task.Name) + defer cleanup() + + fmt.Println(conf.ClientConfig.Node) + + // Run the first TaskRunner + tr, err := NewTaskRunner(conf) + require.NoError(err) + go tr.Run() + defer tr.Kill(context.Background(), structs.NewTaskEvent("cleanup")) + + // Wait for task to complete + select { + case <-tr.WaitCh(): + case <-time.After(3 * time.Second): + } + + // Get the mock driver plugin + driverPlugin, err := conf.PluginSingletonLoader.Dispense( + mockdriver.PluginID.Name, + mockdriver.PluginID.PluginType, + nil, + conf.Logger, + ) + require.NoError(err) + mockDriver := driverPlugin.Plugin().(*mockdriver.Driver) + + // Assert its config has been properly interpolated + driverCfg, mockCfg := mockDriver.GetTaskConfig() + require.NotNil(driverCfg) + require.NotNil(mockCfg) + assert.Equal(t, "somebody", driverCfg.User) + assert.Equal(t, "global bar", mockCfg.StdoutString) +} diff --git a/client/config/testing.go b/client/config/testing.go index 73ab82d2d46..1097e5bcde4 100644 --- a/client/config/testing.go +++ b/client/config/testing.go @@ -7,7 +7,7 @@ import ( "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/testlog" - "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/nomad/mock" "github.com/mitchellh/go-testing-interface" ) @@ -15,6 +15,7 @@ import ( // a cleanup func to remove the state and alloc dirs when finished. func TestClientConfig(t testing.T) (*Config, func()) { conf := DefaultConfig() + conf.Node = mock.Node() conf.Logger = testlog.HCLogger(t) // Create a tempdir to hold state and alloc subdirs @@ -42,11 +43,6 @@ func TestClientConfig(t testing.T) (*Config, func()) { conf.VaultConfig.Enabled = helper.BoolToPtr(false) conf.DevMode = true - conf.Node = &structs.Node{ - Reserved: &structs.Resources{ - DiskMB: 0, - }, - } // Loosen GC threshold conf.GCDiskUsageThreshold = 98.0 diff --git a/client/driver/env/env.go b/client/driver/env/env.go index 013623168d3..a7872e6e7d1 100644 --- a/client/driver/env/env.go +++ b/client/driver/env/env.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/nomad/helper" hargs "github.com/hashicorp/nomad/helper/args" "github.com/hashicorp/nomad/nomad/structs" + "github.com/zclconf/go-cty/cty" ) // A set of environment variables that are exported by each driver. @@ -159,6 +160,20 @@ func (t *TaskEnv) All() map[string]string { return m } +// AllValues is a map of the task's environment variables and the node's +// attributes with cty.Value (String) values. +func (t *TaskEnv) AllValues() map[string]cty.Value { + m := make(map[string]cty.Value, len(t.EnvMap)+len(t.NodeAttrs)) + for k, v := range t.EnvMap { + m[k] = cty.StringVal(v) + } + for k, v := range t.NodeAttrs { + m[k] = cty.StringVal(v) + } + + return m +} + // ParseAndReplace takes the user supplied args replaces any instance of an // environment variable or Nomad variable in the args with the actual value. func (t *TaskEnv) ParseAndReplace(args []string) []string { diff --git a/client/driver/env/env_test.go b/client/driver/env/env_test.go index db696cc218a..51e9400d115 100644 --- a/client/driver/env/env_test.go +++ b/client/driver/env/env_test.go @@ -318,6 +318,101 @@ func TestEnvironment_AsList_Old(t *testing.T) { require.Equal(t, exp, act) } +func TestEnvironment_AllValues(t *testing.T) { + n := mock.Node() + n.Meta = map[string]string{ + "metaKey": "metaVal", + } + a := mock.Alloc() + a.AllocatedResources.Tasks["web"].Networks[0] = &structs.NetworkResource{ + Device: "eth0", + IP: "127.0.0.1", + ReservedPorts: []structs.Port{{Label: "https", Value: 8080}}, + MBits: 50, + DynamicPorts: []structs.Port{{Label: "http", Value: 80}}, + } + a.AllocatedResources.Tasks["ssh"] = &structs.AllocatedTaskResources{ + Networks: []*structs.NetworkResource{ + { + Device: "eth0", + IP: "192.168.0.100", + MBits: 50, + ReservedPorts: []structs.Port{ + {Label: "ssh", Value: 22}, + {Label: "other", Value: 1234}, + }, + }, + }, + } + task := a.Job.TaskGroups[0].Tasks[0] + task.Env = map[string]string{ + "taskEnvKey": "taskEnvVal", + } + env := NewBuilder(n, a, task, "global").SetDriverNetwork( + &cstructs.DriverNetwork{PortMap: map[string]int{"https": 443}}, + ) + + act := env.Build().AllValues() + exp := map[string]string{ + // Node + "node.unique.id": n.ID, + "node.region": "global", + "node.datacenter": n.Datacenter, + "node.unique.name": n.Name, + "node.class": n.NodeClass, + "meta.metaKey": "metaVal", + "attr.arch": "x86", + "attr.driver.exec": "1", + "attr.driver.mock_driver": "1", + "attr.kernel.name": "linux", + "attr.nomad.version": "0.5.0", + + // Env + "taskEnvKey": "taskEnvVal", + "NOMAD_ADDR_http": "127.0.0.1:80", + "NOMAD_PORT_http": "80", + "NOMAD_IP_http": "127.0.0.1", + "NOMAD_ADDR_https": "127.0.0.1:8080", + "NOMAD_PORT_https": "443", + "NOMAD_IP_https": "127.0.0.1", + "NOMAD_HOST_PORT_http": "80", + "NOMAD_HOST_PORT_https": "8080", + "NOMAD_TASK_NAME": "web", + "NOMAD_GROUP_NAME": "web", + "NOMAD_ADDR_ssh_other": "192.168.0.100:1234", + "NOMAD_ADDR_ssh_ssh": "192.168.0.100:22", + "NOMAD_IP_ssh_other": "192.168.0.100", + "NOMAD_IP_ssh_ssh": "192.168.0.100", + "NOMAD_PORT_ssh_other": "1234", + "NOMAD_PORT_ssh_ssh": "22", + "NOMAD_CPU_LIMIT": "500", + "NOMAD_DC": "dc1", + "NOMAD_REGION": "global", + "NOMAD_MEMORY_LIMIT": "256", + "NOMAD_META_ELB_CHECK_INTERVAL": "30s", + "NOMAD_META_ELB_CHECK_MIN": "3", + "NOMAD_META_ELB_CHECK_TYPE": "http", + "NOMAD_META_FOO": "bar", + "NOMAD_META_OWNER": "armon", + "NOMAD_META_elb_check_interval": "30s", + "NOMAD_META_elb_check_min": "3", + "NOMAD_META_elb_check_type": "http", + "NOMAD_META_foo": "bar", + "NOMAD_META_owner": "armon", + "NOMAD_JOB_NAME": "my-job", + "NOMAD_ALLOC_ID": a.ID, + "NOMAD_ALLOC_INDEX": "0", + } + + // Should be able to convert all values back to strings + actStr := make(map[string]string, len(act)) + for k, v := range act { + actStr[k] = v.AsString() + } + + require.Equal(t, exp, actStr) +} + func TestEnvironment_VaultToken(t *testing.T) { n := mock.Node() a := mock.Alloc() diff --git a/drivers/mock/driver.go b/drivers/mock/driver.go index 019f931f4d9..8a21843b5d3 100644 --- a/drivers/mock/driver.go +++ b/drivers/mock/driver.go @@ -6,6 +6,7 @@ import ( "fmt" "strconv" "strings" + "sync" "time" hclog "github.com/hashicorp/go-hclog" @@ -109,6 +110,15 @@ type Driver struct { shutdownFingerprintTime time.Time + // lastDriverTaskConfig is the last *drivers.TaskConfig passed to StartTask + lastDriverTaskConfig *drivers.TaskConfig + + // lastTaskConfig is the last decoded *TaskConfig created by StartTask + lastTaskConfig *TaskConfig + + // lastMu guards access to last[Driver]TaskConfig + lastMu sync.Mutex + // logger will log to the Nomad agent logger hclog.Logger } @@ -318,6 +328,12 @@ func (d *Driver) StartTask(cfg *drivers.TaskConfig) (*drivers.TaskHandle, *cstru time.Sleep(driverConfig.startBlockForDuration) } + // Store last configs + d.lastMu.Lock() + d.lastDriverTaskConfig = cfg + d.lastTaskConfig = &driverConfig + d.lastMu.Unlock() + if driverConfig.StartErr != "" { return nil, nil, structs.NewRecoverableError(errors.New(driverConfig.StartErr), driverConfig.StartErrRecoverable) } @@ -471,3 +487,12 @@ func (d *Driver) ExecTask(taskID string, cmd []string, timeout time.Duration) (* } return &res, nil } + +// GetTaskConfig is unique to the mock driver and for testing purposes only. It +// returns the *drivers.TaskConfig passed to StartTask and the decoded +// *mock.TaskConfig created by the last StartTask call. +func (d *Driver) GetTaskConfig() (*drivers.TaskConfig, *TaskConfig) { + d.lastMu.Lock() + defer d.lastMu.Unlock() + return d.lastDriverTaskConfig, d.lastTaskConfig +} From 26211408a6cf66183eae732458e5610fcdf80a33 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 9 Nov 2018 11:27:18 -0800 Subject: [PATCH 2/8] client: turn env into nested objects for task configs --- client/driver/env/env.go | 62 ++++- client/driver/env/env_test.go | 57 ++++- client/driver/env/util.go | 131 +++++++++++ client/driver/env/util_test.go | 409 +++++++++++++++++++++++++++++++++ 4 files changed, 645 insertions(+), 14 deletions(-) create mode 100644 client/driver/env/util.go create mode 100644 client/driver/env/util_test.go diff --git a/client/driver/env/env.go b/client/driver/env/env.go index a7872e6e7d1..b9acc5d017c 100644 --- a/client/driver/env/env.go +++ b/client/driver/env/env.go @@ -161,17 +161,67 @@ func (t *TaskEnv) All() map[string]string { } // AllValues is a map of the task's environment variables and the node's -// attributes with cty.Value (String) values. -func (t *TaskEnv) AllValues() map[string]cty.Value { - m := make(map[string]cty.Value, len(t.EnvMap)+len(t.NodeAttrs)) +// attributes with cty.Value (String) values. Errors including keys are +// returned in a map by key name. +// +// In the rare case of a fatal error, only an error value is returned. This is +// likely a programming error as user input should not be able to cause a fatal +// error. +func (t *TaskEnv) AllValues() (map[string]cty.Value, map[string]error, error) { + errs := make(map[string]error) + + // Intermediate map for building up nested go types + allMap := make(map[string]interface{}, len(t.EnvMap)+len(t.NodeAttrs)) + + // Intermediate map for all env vars including those whose keys that + // cannot be nested (eg foo...bar) + envMap := make(map[string]cty.Value, len(t.EnvMap)) + + // Prepare job-based variables (eg job.meta, job.group.task.env, etc) for k, v := range t.EnvMap { - m[k] = cty.StringVal(v) + if err := addNestedKey(allMap, k, v); err != nil { + errs[k] = err + } + envMap[k] = cty.StringVal(v) } + + // Prepare node-based variables (eg node.*, attr.*, meta.*) for k, v := range t.NodeAttrs { - m[k] = cty.StringVal(v) + if err := addNestedKey(allMap, k, v); err != nil { + errs[k] = err + } } - return m + // Add flat envMap as a Map to allMap so users can access any key via + // HCL2's indexing syntax: ${env["foo...bar"]} + allMap["env"] = cty.MapVal(envMap) + + // Add meta and attr to node if they exist to properly namespace things + // a bit. + nodeMapI, ok := allMap["node"] + if !ok { + return nil, nil, fmt.Errorf("missing node variable") + } + nodeMap, ok := nodeMapI.(map[string]interface{}) + if !ok { + return nil, nil, fmt.Errorf("invalid type for node variable: %T", nodeMapI) + } + if attrMap, ok := allMap["attr"]; ok { + nodeMap["attr"] = attrMap + } + if metaMap, ok := allMap["meta"]; ok { + nodeMap["meta"] = metaMap + } + + // ctyify the entire tree of strings and maps + tree, err := ctyify(allMap) + if err != nil { + // This should not be possible and is likely a programming + // error. Invalid user input should be cleaned earlier. + return nil, nil, err + } + + return tree, errs, nil } // ParseAndReplace takes the user supplied args replaces any instance of an diff --git a/client/driver/env/env_test.go b/client/driver/env/env_test.go index 51e9400d115..dd9dfde25a5 100644 --- a/client/driver/env/env_test.go +++ b/client/driver/env/env_test.go @@ -8,6 +8,9 @@ import ( "strings" "testing" + "github.com/hashicorp/hcl2/gohcl" + "github.com/hashicorp/hcl2/hcl" + "github.com/hashicorp/hcl2/hcl/hclsyntax" cstructs "github.com/hashicorp/nomad/client/structs" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" @@ -319,9 +322,13 @@ func TestEnvironment_AsList_Old(t *testing.T) { } func TestEnvironment_AllValues(t *testing.T) { + t.Parallel() + n := mock.Node() n.Meta = map[string]string{ - "metaKey": "metaVal", + "metaKey": "metaVal", + "nested.meta.key": "a", + "invalid...metakey": "b", } a := mock.Alloc() a.AllocatedResources.Tasks["web"].Networks[0] = &structs.NetworkResource{ @@ -346,13 +353,22 @@ func TestEnvironment_AllValues(t *testing.T) { } task := a.Job.TaskGroups[0].Tasks[0] task.Env = map[string]string{ - "taskEnvKey": "taskEnvVal", + "taskEnvKey": "taskEnvVal", + "nested.task.key": "x", + "invalid...taskkey": "y", } env := NewBuilder(n, a, task, "global").SetDriverNetwork( &cstructs.DriverNetwork{PortMap: map[string]int{"https": 443}}, ) - act := env.Build().AllValues() + values, errs, err := env.Build().AllValues() + require.NoError(t, err) + + // Assert the keys we couldn't nest were reported + require.Len(t, errs, 2) + require.Contains(t, errs, "invalid...taskkey") + require.Contains(t, errs, "meta.invalid...metakey") + exp := map[string]string{ // Node "node.unique.id": n.ID, @@ -367,6 +383,14 @@ func TestEnvironment_AllValues(t *testing.T) { "attr.kernel.name": "linux", "attr.nomad.version": "0.5.0", + // 0.9 style meta and attr + "node.meta.metaKey": "metaVal", + "node.attr.arch": "x86", + "node.attr.driver.exec": "1", + "node.attr.driver.mock_driver": "1", + "node.attr.kernel.name": "linux", + "node.attr.nomad.version": "0.5.0", + // Env "taskEnvKey": "taskEnvVal", "NOMAD_ADDR_http": "127.0.0.1:80", @@ -402,15 +426,32 @@ func TestEnvironment_AllValues(t *testing.T) { "NOMAD_JOB_NAME": "my-job", "NOMAD_ALLOC_ID": a.ID, "NOMAD_ALLOC_INDEX": "0", + + // 0.9 style env map + `env["taskEnvKey"]`: "taskEnvVal", + `env["NOMAD_ADDR_http"]`: "127.0.0.1:80", + `env["nested.task.key"]`: "x", + `env["invalid...taskkey"]`: "y", } - // Should be able to convert all values back to strings - actStr := make(map[string]string, len(act)) - for k, v := range act { - actStr[k] = v.AsString() + evalCtx := &hcl.EvalContext{ + Variables: values, } - require.Equal(t, exp, actStr) + for k, expectedVal := range exp { + t.Run(k, func(t *testing.T) { + // Parse HCL containing the test key + hclStr := fmt.Sprintf(`"${%s}"`, k) + expr, diag := hclsyntax.ParseExpression([]byte(hclStr), "test.hcl", hcl.Pos{}) + require.Empty(t, diag) + + // Decode with the TaskEnv values + out := "" + diag = gohcl.DecodeExpression(expr, evalCtx, &out) + require.Empty(t, diag) + require.Equal(t, out, expectedVal) + }) + } } func TestEnvironment_VaultToken(t *testing.T) { diff --git a/client/driver/env/util.go b/client/driver/env/util.go new file mode 100644 index 00000000000..319b3b79779 --- /dev/null +++ b/client/driver/env/util.go @@ -0,0 +1,131 @@ +package env + +import ( + "errors" + "fmt" + "strings" + + "github.com/zclconf/go-cty/cty" +) + +var ( + // ErrInvalidObjectPath is returned when a key cannot be converted into + // a nested object path like "foo...bar", ".foo", or "foo." + ErrInvalidObjectPath = errors.New("invalid object path") +) + +type ErrKeyExists struct { + msg string +} + +func NewErrKeyExists(newKey, oldKey string) *ErrKeyExists { + return &ErrKeyExists{ + msg: fmt.Sprintf( + "cannot add key %q because %q already exists with a different type", + newKey, oldKey, + ), + } +} + +func (e *ErrKeyExists) Error() string { + return e.msg +} + +// addNestedKey expands keys into their nested form: +// +// k="foo.bar", v="quux" -> {"foo": {"bar": "quux"}} +// +// Existing keys are overwritten. +// +// If the key has dots but cannot be converted to a valid nested data structure +// (eg "foo...bar", "foo.", or non-object value exists for key), an error is +// returned. +func addNestedKey(dst map[string]interface{}, k, v string) error { + // createdParent and Key capture the parent object of the first created + // object and the first created object's key respectively. The cleanup + // func deletes them to prevent side-effects when returning errors. + var createdParent map[string]interface{} + var createdKey string + cleanup := func() { + if createdParent != nil { + delete(createdParent, createdKey) + } + } + + segments := strings.Split(k, ".") + for _, newKey := range segments[:len(segments)-1] { + if newKey == "" { + // String either begins with a dot (.foo) or has at + // least two consecutive dots (foo..bar); either way + // it's an invalid object path. + cleanup() + return ErrInvalidObjectPath + } + + var target map[string]interface{} + if existingI, ok := dst[newKey]; ok { + existing, ok := existingI.(map[string]interface{}) + if !ok { + // Existing value is not a map, unable to support this key + cleanup() + return NewErrKeyExists(k, newKey) + } + target = existing + } else { + // Does not exist, create + target = make(map[string]interface{}) + dst[newKey] = target + + // If this is the first created key, capture it for + // cleanup if there is an error later. + if createdParent == nil { + createdParent = dst + createdKey = newKey + } + } + + // Descend into new m + dst = target + } + + // See if the final segment is a valid key + newKey := segments[len(segments)-1] + if newKey == "" { + // String ends in a dot + cleanup() + return ErrInvalidObjectPath + } + + dst[newKey] = v + return nil +} + +// ctyify converts nested map[string]interfaces to a map[string]cty.Value. An +// error is returned if an unsupported type is encountered. +// +// Currently only strings, cty.Values, and nested maps are supported. +func ctyify(src map[string]interface{}) (map[string]cty.Value, error) { + dst := make(map[string]cty.Value, len(src)) + + for k, vI := range src { + switch v := vI.(type) { + case string: + dst[k] = cty.StringVal(v) + + case cty.Value: + dst[k] = v + + case map[string]interface{}: + o, err := ctyify(v) + if err != nil { + return nil, err + } + dst[k] = cty.ObjectVal(o) + + default: + return nil, fmt.Errorf("key %q has invalid type %T", k, v) + } + } + + return dst, nil +} diff --git a/client/driver/env/util_test.go b/client/driver/env/util_test.go new file mode 100644 index 00000000000..c25e710e48a --- /dev/null +++ b/client/driver/env/util_test.go @@ -0,0 +1,409 @@ +package env + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" + "github.com/zclconf/go-cty/cty" +) + +// TestaddNestedKey_Ok asserts test cases that succeed when passed to +// addNestedKey. +func TestaddNestedKey_Ok(t *testing.T) { + cases := []struct { + // M will be initialized if unset + M map[string]interface{} + K string + // Value is always "x" + Result map[string]interface{} + }{ + { + K: "foo", + Result: map[string]interface{}{ + "foo": "x", + }, + }, + { + K: "foo.bar", + Result: map[string]interface{}{ + "foo": map[string]interface{}{ + "bar": "x", + }, + }, + }, + { + K: "foo.bar.quux", + Result: map[string]interface{}{ + "foo": map[string]interface{}{ + "bar": map[string]interface{}{ + "quux": "x", + }, + }, + }, + }, + { + K: "a.b.c", + Result: map[string]interface{}{ + "a": map[string]interface{}{ + "b": map[string]interface{}{ + "c": "x", + }, + }, + }, + }, + { + // Nested object b should get overwritten with "x" + M: map[string]interface{}{ + "a": map[string]interface{}{ + "b": map[string]interface{}{ + "c": "c", + }, + }, + }, + K: "a.b", + Result: map[string]interface{}{ + "a": map[string]interface{}{ + "b": "x", + }, + }, + }, + { + M: map[string]interface{}{ + "a": map[string]interface{}{ + "x": "x", + }, + "z": "z", + }, + K: "a.b.c", + Result: map[string]interface{}{ + "a": map[string]interface{}{ + "b": map[string]interface{}{ + "c": "x", + }, + "x": "x", + }, + "z": "z", + }, + }, + { + M: map[string]interface{}{ + "foo": map[string]interface{}{ + "bar": map[string]interface{}{ + "a": "z", + "quux": "z", + }, + }, + }, + K: "foo.bar.quux", + Result: map[string]interface{}{ + "foo": map[string]interface{}{ + "bar": map[string]interface{}{ + "a": "z", + "quux": "x", + }, + }, + }, + }, + { + M: map[string]interface{}{ + "foo": "1", + "bar": "2", + "quux": "3", + }, + K: "a.bbbbbb.c", + Result: map[string]interface{}{ + "foo": "1", + "bar": "2", + "quux": "3", + "a": map[string]interface{}{ + "bbbbbb": map[string]interface{}{ + "c": "x", + }, + }, + }, + }, + } + + for i := range cases { + tc := cases[i] + name := tc.K + if len(tc.M) > 0 { + name = fmt.Sprintf("%s-%d", name, len(tc.M)) + } + t.Run(name, func(t *testing.T) { + t.Parallel() + if tc.M == nil { + tc.M = map[string]interface{}{} + } + require.NoError(t, addNestedKey(tc.M, tc.K, "x")) + require.Equal(t, tc.Result, tc.M) + }) + } +} + +// TestaddNestedKey_Bad asserts test cases return an error when passed to +// addNestedKey. +func TestaddNestedKey_Bad(t *testing.T) { + cases := []struct { + // M will be initialized if unset + M func() map[string]interface{} + K string + // Value is always "x" + // Result is compared by Error() string equality + Result error + }{ + { + K: ".", + Result: ErrInvalidObjectPath, + }, + { + K: ".foo", + Result: ErrInvalidObjectPath, + }, + { + K: "foo.", + Result: ErrInvalidObjectPath, + }, + { + K: ".a.", + Result: ErrInvalidObjectPath, + }, + { + K: "foo..bar", + Result: ErrInvalidObjectPath, + }, + { + K: "foo...bar", + Result: ErrInvalidObjectPath, + }, + { + K: "foo.bar..quux", + Result: ErrInvalidObjectPath, + }, + { + K: "foo..bar.quux", + Result: ErrInvalidObjectPath, + }, + { + K: "foo.bar.quux.", + Result: ErrInvalidObjectPath, + }, + { + M: func() map[string]interface{} { + return map[string]interface{}{ + "a": "a", + "foo": map[string]interface{}{ + "b": "b", + "bar": map[string]interface{}{ + "c": "c", + }, + }, + } + }, + K: "foo.bar.quux.", + Result: ErrInvalidObjectPath, + }, + { + M: func() map[string]interface{} { + return map[string]interface{}{ + "a": "a", + "foo": map[string]interface{}{ + "b": "b", + "bar": map[string]interface{}{ + "c": "c", + }, + }, + } + }, + K: "foo.bar..quux", + Result: ErrInvalidObjectPath, + }, + { + M: func() map[string]interface{} { + return map[string]interface{}{ + "a": "a", + "foo": map[string]interface{}{ + "b": "b", + "bar": map[string]interface{}{ + "c": "c", + }, + }, + } + }, + K: "foo.bar..quux", + Result: ErrInvalidObjectPath, + }, + { + M: func() map[string]interface{} { + return map[string]interface{}{ + "a": "a", + } + }, + K: "a.b", + Result: NewErrKeyExists("a.b", "a"), + }, + { + M: func() map[string]interface{} { + return map[string]interface{}{ + "a": "a", + "foo": map[string]interface{}{ + "b": "b", + "bar": "quux", + }, + "c": map[string]interface{}{}, + } + }, + K: "foo.bar.quux", + Result: NewErrKeyExists("foo.bar.quux", "bar"), + }, + { + M: func() map[string]interface{} { + return map[string]interface{}{ + "a": map[string]interface{}{ + "b": "b", + }, + } + }, + K: "a.b.c.", + Result: NewErrKeyExists("a.b.c.", "b"), + }, + } + + for i := range cases { + tc := cases[i] + name := tc.K + if tc.M != nil { + name += "-cleanup" + } + t.Run(name, func(t *testing.T) { + t.Parallel() + + // Copy original M value to ensure it doesn't get altered + if tc.M == nil { + tc.M = func() map[string]interface{} { + return map[string]interface{}{} + } + } + + // Call func and assert error + m := tc.M() + err := addNestedKey(m, tc.K, "x") + require.EqualError(t, err, tc.Result.Error()) + + // Ensure M wasn't altered + require.Equal(t, tc.M(), m) + }) + } +} + +func TestCtyify_Ok(t *testing.T) { + cases := []struct { + Name string + In map[string]interface{} + Out map[string]cty.Value + }{ + { + Name: "OneVal", + In: map[string]interface{}{ + "a": "b", + }, + Out: map[string]cty.Value{ + "a": cty.StringVal("b"), + }, + }, + { + Name: "MultiVal", + In: map[string]interface{}{ + "a": "b", + "foo": "bar", + }, + Out: map[string]cty.Value{ + "a": cty.StringVal("b"), + "foo": cty.StringVal("bar"), + }, + }, + { + Name: "NestedVals", + In: map[string]interface{}{ + "a": "b", + "foo": map[string]interface{}{ + "c": "d", + "bar": map[string]interface{}{ + "quux": "z", + }, + }, + "123": map[string]interface{}{ + "bar": map[string]interface{}{ + "456": "789", + }, + }, + }, + Out: map[string]cty.Value{ + "a": cty.StringVal("b"), + "foo": cty.ObjectVal(map[string]cty.Value{ + "c": cty.StringVal("d"), + "bar": cty.ObjectVal(map[string]cty.Value{ + "quux": cty.StringVal("z"), + }), + }), + "123": cty.ObjectVal(map[string]cty.Value{ + "bar": cty.ObjectVal(map[string]cty.Value{ + "456": cty.StringVal("789"), + }), + }), + }, + }, + } + + for i := range cases { + tc := cases[i] + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + + // ctiyif and check for errors + result, err := ctyify(tc.In) + require.NoError(t, err) + + // convert results to ObjectVals and compare with RawEquals + resultObj := cty.ObjectVal(result) + OutObj := cty.ObjectVal(tc.Out) + require.True(t, OutObj.RawEquals(resultObj)) + }) + } +} + +func TestCtyify_Bad(t *testing.T) { + cases := []struct { + Name string + In map[string]interface{} + Out map[string]cty.Value + }{ + { + Name: "NonStringVal", + In: map[string]interface{}{ + "a": 1, + }, + }, + { + Name: "NestedNonString", + In: map[string]interface{}{ + "foo": map[string]interface{}{ + "c": 1, + }, + }, + }, + } + + for i := range cases { + tc := cases[i] + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + + // ctiyif and check for errors + result, err := ctyify(tc.In) + require.Error(t, err) + require.Nil(t, result) + }) + } +} From c4c0869a2e7d974762ec96f06547360831f1339f Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 9 Nov 2018 11:27:48 -0800 Subject: [PATCH 3/8] docs: document new interpolation behavior and options --- CHANGELOG.md | 3 ++ .../docs/runtime/interpolation.html.md.erb | 35 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fa7539f21c..d7086c5dbd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,9 @@ __BACKWARDS INCOMPATIBILITIES:__ * core: Switch to structured logging using [go-hclog](https://github.com/hashicorp/go-hclog) * core: Allow the != constraint to match against keys that do not exist [[GH-4875](https://github.com/hashicorp/nomad/pull/4875)] + * client: Task config interpolation requires names to be valid identifiers + (`node.region` or `NOMAD_DC`). Interpolating other variables requires a new + indexing syntax: `env[".invalid.identifier."]`. [[GH-4843](https://github.com/hashicorp/nomad/issues/4843)] IMPROVEMENTS: * core: Added advertise address to client node meta data [[GH-4390](https://github.com/hashicorp/nomad/issues/4390)] diff --git a/website/source/docs/runtime/interpolation.html.md.erb b/website/source/docs/runtime/interpolation.html.md.erb index 35e03d79f57..33c33fbcfef 100644 --- a/website/source/docs/runtime/interpolation.html.md.erb +++ b/website/source/docs/runtime/interpolation.html.md.erb @@ -201,4 +201,39 @@ a particular node and as such can not be used in constraints. Environment variables should be enclosed in brackets `${...}` for interpolation. +### Dots in Variables + +Starting in Nomad 0.9, task configuration interpolation requires variables to +be valid identifiers. While this does not affect default variables or common +custom variables, it is possible to define a variable that is not a valid +identifier: + +```hcl +env { + "valid.name" = "ok" + "invalid...name" = "not a valid identifier" +} +``` + +The environment variable `invalid...name` cannot be interpolated using the +standard `"${invalid...name}"` syntax. The dots wil be interpreted as object +notation so multiple consecutive dots are invalid. + +To continue supporting all user environment variables Nomad 0.9 added a new +`env` variable which allows accessing any environment variable through index +syntax: + +```hcl +task "redis" { + driver = "docker" + config { + image = "redis:3.2" + labels { + label1 = "${env["invalid...name"]}" + label2 = "${env["valid.name"]}" + } + } +} +``` + <%= partial "envvars.html.md" %> From b878bbf3f741705799be8e3df78fca5bcefeced5 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 12 Nov 2018 10:13:25 -0800 Subject: [PATCH 4/8] client: add new nested variables to task's hcl ctx The error messages are really bad, but it's extremely difficult to produce good error messages without the original HCL. --- client/allocrunner/taskrunner/task_runner.go | 23 ++++++++++++++++++- .../taskrunner/task_runner_test.go | 6 ++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/client/allocrunner/taskrunner/task_runner.go b/client/allocrunner/taskrunner/task_runner.go index 699245eb5b9..9ea342d621c 100644 --- a/client/allocrunner/taskrunner/task_runner.go +++ b/client/allocrunner/taskrunner/task_runner.go @@ -462,8 +462,29 @@ func (tr *TaskRunner) runDriver() error { // TODO(nickethier): make sure this uses alloc.AllocatedResources once #4750 is rebased taskConfig := tr.buildTaskConfig() + // Build hcl context variables + vars, errs, err := tr.envBuilder.Build().AllValues() + if err != nil { + return err + } + + // Handle per-key errors + if len(errs) > 0 { + keys := make([]string, 0, len(errs)) + for k, err := range errs { + keys = append(keys, k) + + if tr.logger.IsTrace() { + // Verbosely log every diagnostic for debugging + tr.logger.Trace("error building environment variables", "key", k, "error", err) + } + } + + tr.logger.Warn("some environment variables not available for rendering", "keys", strings.Join(keys, ", ")) + } + evalCtx := &hcl.EvalContext{ - Variables: tr.envBuilder.Build().AllValues(), + Variables: vars, Functions: shared.GetStdlibFuncs(), } diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go index 4e1496286ce..d3c97ef8be7 100644 --- a/client/allocrunner/taskrunner/task_runner_test.go +++ b/client/allocrunner/taskrunner/task_runner_test.go @@ -176,9 +176,8 @@ func TestTaskRunner_TaskEnv(t *testing.T) { // Use interpolation from both node attributes and meta vars task.Config = map[string]interface{}{ "run_for": time.Millisecond, - "stdout_string": `${node.region} ${NOMAD_META_foo}`, + "stdout_string": `${node.region} ${NOMAD_META_foo} ${NOMAD_META_common_user}`, } - task.User = "${NOMAD_META_common_user}" conf, cleanup := testTaskRunnerConfig(t, alloc, task.Name) defer cleanup() @@ -211,6 +210,5 @@ func TestTaskRunner_TaskEnv(t *testing.T) { driverCfg, mockCfg := mockDriver.GetTaskConfig() require.NotNil(driverCfg) require.NotNil(mockCfg) - assert.Equal(t, "somebody", driverCfg.User) - assert.Equal(t, "global bar", mockCfg.StdoutString) + assert.Equal(t, "global bar somebody", mockCfg.StdoutString) } From 926e3dc7061ca82f606f52a07b39ff5f7bcb7324 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 15 Nov 2018 16:07:16 -0800 Subject: [PATCH 5/8] client: test more env key variations --- client/driver/env/env_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/client/driver/env/env_test.go b/client/driver/env/env_test.go index dd9dfde25a5..9fb27a1c19a 100644 --- a/client/driver/env/env_test.go +++ b/client/driver/env/env_test.go @@ -356,6 +356,9 @@ func TestEnvironment_AllValues(t *testing.T) { "taskEnvKey": "taskEnvVal", "nested.task.key": "x", "invalid...taskkey": "y", + ".a": "a", + "b.": "b", + ".": "c", } env := NewBuilder(n, a, task, "global").SetDriverNetwork( &cstructs.DriverNetwork{PortMap: map[string]int{"https": 443}}, @@ -365,9 +368,12 @@ func TestEnvironment_AllValues(t *testing.T) { require.NoError(t, err) // Assert the keys we couldn't nest were reported - require.Len(t, errs, 2) + require.Len(t, errs, 5) require.Contains(t, errs, "invalid...taskkey") require.Contains(t, errs, "meta.invalid...metakey") + require.Contains(t, errs, ".a") + require.Contains(t, errs, "b.") + require.Contains(t, errs, ".") exp := map[string]string{ // Node @@ -432,6 +438,9 @@ func TestEnvironment_AllValues(t *testing.T) { `env["NOMAD_ADDR_http"]`: "127.0.0.1:80", `env["nested.task.key"]`: "x", `env["invalid...taskkey"]`: "y", + `env[".a"]`: "a", + `env["b."]`: "b", + `env["."]`: "c", } evalCtx := &hcl.EvalContext{ From 2283c8fff3c090ddc8fe15323688d7d054fa9174 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 15 Nov 2018 16:07:35 -0800 Subject: [PATCH 6/8] client: remove old proxy references from comments --- client/allocrunner/taskrunner/task_runner_getters.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/client/allocrunner/taskrunner/task_runner_getters.go b/client/allocrunner/taskrunner/task_runner_getters.go index 89d67a99ebb..f5b1594366e 100644 --- a/client/allocrunner/taskrunner/task_runner_getters.go +++ b/client/allocrunner/taskrunner/task_runner_getters.go @@ -53,16 +53,15 @@ func (tr *TaskRunner) setVaultToken(token string) { tr.envBuilder.SetVaultToken(token, tr.task.Vault.Env) } -// getDriverHandle returns a driver handle and its result proxy. Use the -// result proxy instead of the handle's WaitCh. +// getDriverHandle returns a driver handle. func (tr *TaskRunner) getDriverHandle() *DriverHandle { tr.handleLock.Lock() defer tr.handleLock.Unlock() return tr.handle } -// setDriverHanlde sets the driver handle, creates a new result proxy, and -// updates the driver network in the task's environment. +// setDriverHandle sets the driver handle and updates the driver network in the +// task's environment. func (tr *TaskRunner) setDriverHandle(handle *DriverHandle) { tr.handleLock.Lock() defer tr.handleLock.Unlock() From a552a0c42c05a85a93367be975f3bd5c165c5c70 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 15 Nov 2018 16:07:56 -0800 Subject: [PATCH 7/8] client/tr: add a bit of context to envbuilder errors --- client/allocrunner/taskrunner/task_runner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/allocrunner/taskrunner/task_runner.go b/client/allocrunner/taskrunner/task_runner.go index 9ea342d621c..7a3b7aac736 100644 --- a/client/allocrunner/taskrunner/task_runner.go +++ b/client/allocrunner/taskrunner/task_runner.go @@ -465,7 +465,7 @@ func (tr *TaskRunner) runDriver() error { // Build hcl context variables vars, errs, err := tr.envBuilder.Build().AllValues() if err != nil { - return err + return fmt.Errorf("error building environment variables: %v", err) } // Handle per-key errors From 91891736496c9b17e28293393522364dd65f43f4 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 15 Nov 2018 17:39:45 -0800 Subject: [PATCH 8/8] tests: fix tests post-rebase --- client/allocrunner/taskrunner/task_runner_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go index d3c97ef8be7..2f0d7290bfd 100644 --- a/client/allocrunner/taskrunner/task_runner_test.go +++ b/client/allocrunner/taskrunner/task_runner_test.go @@ -175,15 +175,13 @@ func TestTaskRunner_TaskEnv(t *testing.T) { // Use interpolation from both node attributes and meta vars task.Config = map[string]interface{}{ - "run_for": time.Millisecond, + "run_for": "1ms", "stdout_string": `${node.region} ${NOMAD_META_foo} ${NOMAD_META_common_user}`, } conf, cleanup := testTaskRunnerConfig(t, alloc, task.Name) defer cleanup() - fmt.Println(conf.ClientConfig.Node) - // Run the first TaskRunner tr, err := NewTaskRunner(conf) require.NoError(err)