From 088feb933f02c73780f1a44df28a8dc4c6290caf Mon Sep 17 00:00:00 2001 From: James Nugent Date: Thu, 7 Jul 2016 19:14:33 +0100 Subject: [PATCH 1/3] terraform: Add test case reproducing #7241 The reproduction of issue #7421 involves a list of maps being passed to a module, where one or more of the maps has a value which is computed (for example, from another resource). There is a failure at the point of use (via lookup interpolation) of the computed value of the form: ``` lookup: lookup failed to find 'elb' in: ${lookup(var.services[count.index], "elb")} ``` Where 'elb' is the key of the map. --- terraform/context_plan_test.go | 40 +++++++++++++++++++ terraform/terraform_test.go | 16 ++++++++ .../plan-computed-value-in-map/main.tf | 15 +++++++ .../plan-computed-value-in-map/mod/main.tf | 8 ++++ 4 files changed, 79 insertions(+) create mode 100644 terraform/test-fixtures/plan-computed-value-in-map/main.tf create mode 100644 terraform/test-fixtures/plan-computed-value-in-map/mod/main.tf diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 9ad2d1f19..09ed1bd82 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -2325,3 +2325,43 @@ func TestContext2Plan_moduleMapLiteral(t *testing.T) { t.Fatalf("err: %s", err) } } + +func TestContext2Plan_computedValueInMap(t *testing.T) { + m := testModule(t, "plan-computed-value-in-map") + p := testProvider("aws") + p.DiffFn = func(info *InstanceInfo, state *InstanceState, c *ResourceConfig) (*InstanceDiff, error) { + switch info.Type { + case "aws_computed_source": + return &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{ + "computed_read_only": &ResourceAttrDiff{ + NewComputed: true, + }, + }, + }, nil + } + + return testDiffFn(info, state, c) + } + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + plan, err := ctx.Plan() + if err != nil { + t.Fatalf("err: %s", err) + } + + actual := strings.TrimSpace(plan.String()) + expected := strings.TrimSpace(testTerraformPlanComputedValueInMap) + if actual != expected { + t.Fatalf("bad:\n%s\n\nexpected\n\n%s", actual, expected) + } +} diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index c608d3d73..333e72767 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -1355,3 +1355,19 @@ aws_instance.foo: ID = bar ami = ami-abcd1234 ` + +const testTerraformPlanComputedValueInMap = ` +DIFF: + +CREATE: aws_computed_source.intermediates + computed_read_only: "" => "" + +module.test_mod: + CREATE: aws_instance.inner2 + looked_up: "" => "" + type: "" => "aws_instance" + +STATE: + + +` diff --git a/terraform/test-fixtures/plan-computed-value-in-map/main.tf b/terraform/test-fixtures/plan-computed-value-in-map/main.tf new file mode 100644 index 000000000..b820b1705 --- /dev/null +++ b/terraform/test-fixtures/plan-computed-value-in-map/main.tf @@ -0,0 +1,15 @@ +resource "aws_computed_source" "intermediates" {} + +module "test_mod" { + source = "./mod" + + services { + "exists" = "true" + "elb" = "${aws_computed_source.intermediates.computed_read_only}" + } + + services { + "otherexists" = " true" + "elb" = "${aws_computed_source.intermediates.computed_read_only}" + } +} diff --git a/terraform/test-fixtures/plan-computed-value-in-map/mod/main.tf b/terraform/test-fixtures/plan-computed-value-in-map/mod/main.tf new file mode 100644 index 000000000..82ee1e494 --- /dev/null +++ b/terraform/test-fixtures/plan-computed-value-in-map/mod/main.tf @@ -0,0 +1,8 @@ +variable "services" { + type = "list" +} + +resource "aws_instance" "inner2" { + looked_up = "${lookup(var.services[0], "elb")}" +} + From c6e03cba96cea7f222a8c1cde74d7ed362d33415 Mon Sep 17 00:00:00 2001 From: James Nugent Date: Thu, 7 Jul 2016 19:15:32 +0100 Subject: [PATCH 2/3] core: Fix slice element keys on interpolateWalk Part of the interpolation walk is to detect keys which involve computed values and therefore cannot be resolved at this time. The interplation walker keeps sufficient state to be able to populate the ResourceConfig with a slice of such keys. Previously they didn't take slice indexes into account, so in the following case: ``` "services": []interface{}{ map[string]interface{}{ "elb": "___something computed___", }, map[string]interface{}{ "elb": "___something else computed___", }, map[string]interface{}{ "elb": "not computed", }, } ``` Unknown keys would be populated as follows: ``` services.elb services.elb ``` This is not sufficient information to be useful, as it is impossible to distinguish which of the `services.elb`s are unknown vs not. This commit therefore retains the slice indexes as part of the key for unknown keys - producing for the example above: ``` services.0.elb services.1.elb ``` --- config/interpolate_walk.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/config/interpolate_walk.go b/config/interpolate_walk.go index 143b96131..720a8b285 100644 --- a/config/interpolate_walk.go +++ b/config/interpolate_walk.go @@ -54,6 +54,9 @@ type interpolationWalkerContextFunc func(reflectwalk.Location, ast.Node) func (w *interpolationWalker) Enter(loc reflectwalk.Location) error { w.loc = loc + if loc == reflectwalk.WalkLoc { + w.sliceIndex = -1 + } return nil } @@ -72,6 +75,7 @@ func (w *interpolationWalker) Exit(loc reflectwalk.Location) error { w.cs = w.cs[:len(w.cs)-1] case reflectwalk.SliceElem: w.csKey = w.csKey[:len(w.csKey)-1] + w.sliceIndex = -1 } return nil @@ -85,7 +89,13 @@ func (w *interpolationWalker) Map(m reflect.Value) error { func (w *interpolationWalker) MapElem(m, k, v reflect.Value) error { w.csData = k w.csKey = append(w.csKey, k) - w.key = append(w.key, k.String()) + + if w.sliceIndex != -1 { + w.key = append(w.key, fmt.Sprintf("%d.%s", w.sliceIndex, k.String())) + } else { + w.key = append(w.key, k.String()) + } + w.lastValue = v return nil } @@ -164,6 +174,7 @@ func (w *interpolationWalker) Primitive(v reflect.Value) error { } else if replaceVal == UnknownVariableValue { remove = true } + if remove { w.removeCurrent() return nil From b6fff854a6e6dac2ebc99438acc05f052d9743cc Mon Sep 17 00:00:00 2001 From: James Nugent Date: Fri, 8 Jul 2016 10:11:25 +0100 Subject: [PATCH 3/3] core: Set all unknown keys to UnknownVariableValue As part of evaluating a variable block, there is a pass made on unknown keys setting them to the config.DefaultVariableValue sentinal value. Previously this only took into account one level of nesting and assumed all values were strings. This commit now traverses the unknown keys via lists and maps and sets unknown map keys surgically. Fixes #7241. --- terraform/eval_variable.go | 52 +++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/terraform/eval_variable.go b/terraform/eval_variable.go index ce6064706..47bd2ea2b 100644 --- a/terraform/eval_variable.go +++ b/terraform/eval_variable.go @@ -4,6 +4,7 @@ import ( "fmt" "log" "reflect" + "strconv" "strings" "github.com/hashicorp/terraform/config" @@ -143,15 +144,60 @@ func (n *EvalVariableBlock) Eval(ctx EvalContext) (interface{}, error) { return nil, fmt.Errorf("Variable value for %s is not a string, list or map type", k) } - for k, _ := range rc.Raw { - if _, ok := n.VariableValues[k]; !ok { - n.VariableValues[k] = config.UnknownVariableValue + + for _, path := range rc.ComputedKeys { + log.Printf("[DEBUG] Setting Unknown Variable Value for computed key: %s", path) + err := n.setUnknownVariableValueForPath(path) + if err != nil { + return nil, err } } return nil, nil } +func (n *EvalVariableBlock) setUnknownVariableValueForPath(path string) error { + pathComponents := strings.Split(path, ".") + + if len(pathComponents) < 1 { + return fmt.Errorf("No path comoponents in %s", path) + } + + if len(pathComponents) == 1 { + // Special case the "top level" since we know the type + if _, ok := n.VariableValues[pathComponents[0]]; !ok { + n.VariableValues[pathComponents[0]] = config.UnknownVariableValue + } + return nil + } + + // Otherwise find the correct point in the tree and then set to unknown + var current interface{} = n.VariableValues[pathComponents[0]] + for i := 1; i < len(pathComponents); i++ { + switch current.(type) { + case []interface{}, []map[string]interface{}: + tCurrent := current.([]interface{}) + index, err := strconv.Atoi(pathComponents[i]) + if err != nil { + return fmt.Errorf("Cannot convert %s to slice index in path %s", + pathComponents[i], path) + } + current = tCurrent[index] + case map[string]interface{}: + tCurrent := current.(map[string]interface{}) + if val, hasVal := tCurrent[pathComponents[i]]; hasVal { + current = val + continue + } + + tCurrent[pathComponents[i]] = config.UnknownVariableValue + break + } + } + + return nil +} + // EvalCoerceMapVariable is an EvalNode implementation that recognizes a // specific ambiguous HCL parsing situation and resolves it. In HCL parsing, a // bare map literal is indistinguishable from a list of maps w/ one element.