From 26211408a6cf66183eae732458e5610fcdf80a33 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 9 Nov 2018 11:27:18 -0800 Subject: [PATCH] client: turn env into nested objects for task configs --- client/driver/env/env.go | 66 +++++- client/driver/env/env_test.go | 57 ++++- client/driver/env/util.go | 131 +++++++++++ client/driver/env/util_test.go | 409 +++++++++++++++++++++++++++++++++ 4 files changed, 647 insertions(+), 16 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 a7872e6e7..b9acc5d01 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) - } - for k, v := range t.NodeAttrs { - m[k] = cty.StringVal(v) + if err := addNestedKey(allMap, k, v); err != nil { + errs[k] = err + } + envMap[k] = cty.StringVal(v) } - return m + // Prepare node-based variables (eg node.*, attr.*, meta.*) + for k, v := range t.NodeAttrs { + if err := addNestedKey(allMap, k, v); err != nil { + errs[k] = err + } + } + + // 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 51e9400d1..dd9dfde25 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 000000000..319b3b797 --- /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 000000000..c25e710e4 --- /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) + }) + } +}