From 748bc58cb2f31c70fba80ed9c75428bf6193ad4d Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 20 Dec 2018 11:37:46 -0800 Subject: [PATCH] taskenv: have maps take precedence over primitives **The Bug:** You may have seen log lines like this when running 0.9.0-dev: ``` ... client.alloc_runner.task_runner: some environment variables not available for rendering: ... keys="attr.driver.docker.volumes.enabled, attr.driver.docker.version, attr.driver.docker.bridge_ip, attr.driver.qemu.version" ``` Not only should we not be erroring on builtin driver attributes, but the results were nondeterministic due to map iteration order! The root cause is that we have an old root attribute for all drivers like: ``` attr.driver.docker = "1" ``` When attributes were opaque variable names it was fine to also have "nested" attributes like: ``` attr.driver.docker.version = "1.2.3" ``` However in the HCLv2 world the variable names are no longer opaque: they form an object tree. The `docker` object can no longer both hold a value (`"1"`) *and* nested attributes (`version = "1.2.3"`). **The Fix:** Since the old `attr.driver. = "1"` attribues are useless for task config interpolation, create a new precedence rule for creating the task config evaluation context: *Maps take precedence over primitives.* This means `attr.driver.docker.version` will always take precedence over `attr.driver.docker`. The results are determinstic and give users access to the more useful metadata. I made this a general precedence rule instead of special-casing driver attrs because it seemed like better default behavior than spamming WARNings to logs that were likely unactionable by users. --- client/taskenv/util.go | 42 ++++++------- client/taskenv/util_test.go | 115 +++++++++++++++++++++++++----------- 2 files changed, 97 insertions(+), 60 deletions(-) diff --git a/client/taskenv/util.go b/client/taskenv/util.go index 561b03709..1686a100d 100644 --- a/client/taskenv/util.go +++ b/client/taskenv/util.go @@ -14,28 +14,11 @@ var ( 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. +// Existing keys are overwritten. Map values take precedence over primitives. // // 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 @@ -64,13 +47,17 @@ func addNestedKey(dst map[string]interface{}, k, v string) error { 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) + if existing, ok := existingI.(map[string]interface{}); ok { + // Target already exists + target = existing + } else { + // Existing value is not a map. Maps should + // take precedence over primitive values (eg + // overwrite attr.driver.qemu = "1" with + // attr.driver.qemu.version = "...") + target = make(map[string]interface{}) + dst[newKey] = target } - target = existing } else { // Does not exist, create target = make(map[string]interface{}) @@ -96,6 +83,13 @@ func addNestedKey(dst map[string]interface{}, k, v string) error { return ErrInvalidObjectPath } + if existingI, ok := dst[newKey]; ok { + if _, ok := existingI.(map[string]interface{}); ok { + // Existing value is a map which takes precedence over + // a primitive value. Drop primitive. + return nil + } + } dst[newKey] = v return nil } diff --git a/client/taskenv/util_test.go b/client/taskenv/util_test.go index c9246713a..e97cc5716 100644 --- a/client/taskenv/util_test.go +++ b/client/taskenv/util_test.go @@ -53,7 +53,7 @@ func TestAddNestedKey_Ok(t *testing.T) { }, }, { - // Nested object b should get overwritten with "x" + // Nested object b should take precedence over values M: map[string]interface{}{ "a": map[string]interface{}{ "b": map[string]interface{}{ @@ -64,7 +64,9 @@ func TestAddNestedKey_Ok(t *testing.T) { K: "a.b", Result: map[string]interface{}{ "a": map[string]interface{}{ - "b": "x", + "b": map[string]interface{}{ + "c": "c", + }, }, }, }, @@ -123,6 +125,81 @@ func TestAddNestedKey_Ok(t *testing.T) { }, }, }, + // Regardless of whether attr.driver.qemu = "1" is added first + // or second, attr.driver.qemu.version = "..." should take + // precedence (nested maps take precedence over values) + { + M: map[string]interface{}{ + "attr": map[string]interface{}{ + "driver": map[string]interface{}{ + "qemu": "1", + }, + }, + }, + K: "attr.driver.qemu.version", + Result: map[string]interface{}{ + "attr": map[string]interface{}{ + "driver": map[string]interface{}{ + "qemu": map[string]interface{}{ + "version": "x", + }, + }, + }, + }, + }, + { + M: map[string]interface{}{ + "attr": map[string]interface{}{ + "driver": map[string]interface{}{ + "qemu": map[string]interface{}{ + "version": "1.2.3", + }, + }, + }, + }, + K: "attr.driver.qemu", + Result: map[string]interface{}{ + "attr": map[string]interface{}{ + "driver": map[string]interface{}{ + "qemu": map[string]interface{}{ + "version": "1.2.3", + }, + }, + }, + }, + }, + { + M: map[string]interface{}{ + "a": "a", + }, + K: "a.b", + Result: map[string]interface{}{ + "a": map[string]interface{}{ + "b": "x", + }, + }, + }, + { + M: map[string]interface{}{ + "a": "a", + "foo": map[string]interface{}{ + "b": "b", + "bar": "quux", + }, + "c": map[string]interface{}{}, + }, + K: "foo.bar.quux", + Result: map[string]interface{}{ + "a": "a", + "foo": map[string]interface{}{ + "b": "b", + "bar": map[string]interface{}{ + "quux": "x", + }, + }, + "c": map[string]interface{}{}, + }, + }, } for i := range cases { @@ -234,40 +311,6 @@ func TestAddNestedKey_Bad(t *testing.T) { 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 {