From 559f14c3fa2d943d2d0f61ee24e96f899e57c376 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Wed, 6 Jul 2016 09:11:46 -0500 Subject: [PATCH] terraform: allow literal maps to be passed to modules Passing a literal map to a module looks like this in HCL: module "foo" { source = "./foo" somemap { somekey = "somevalue" } } The HCL parser always wraps an extra list around the map, so we need to remove that extra list wrapper when the parameter is indeed of type "map". Fixes #7140 --- terraform/context_plan_test.go | 39 +++++ terraform/eval_variable.go | 61 ++++++-- terraform/eval_variable_test.go | 142 ++++++++++++++++++ terraform/graph_config_node_variable.go | 6 + .../plan-module-map-literal/child/main.tf | 12 ++ .../plan-module-map-literal/main.tf | 7 + 6 files changed, 255 insertions(+), 12 deletions(-) create mode 100644 terraform/eval_variable_test.go create mode 100644 terraform/test-fixtures/plan-module-map-literal/child/main.tf create mode 100644 terraform/test-fixtures/plan-module-map-literal/main.tf diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 18cd7efc6b..9ad2d1f194 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -2286,3 +2286,42 @@ func TestContext2Plan_ignoreChanges(t *testing.T) { t.Fatalf("bad:\n%s\n\nexpected\n\n%s", actual, expected) } } + +func TestContext2Plan_moduleMapLiteral(t *testing.T) { + m := testModule(t, "plan-module-map-literal") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = func(i *InstanceInfo, s *InstanceState, c *ResourceConfig) (*InstanceDiff, error) { + // Here we verify that both the populated and empty map literals made it + // through to the resource attributes + val, _ := c.Get("tags") + m, ok := val.(map[string]interface{}) + if !ok { + t.Fatalf("Tags attr not map: %#v", val) + } + if m["foo"] != "bar" { + t.Fatalf("Bad value in tags attr: %#v", m) + } + { + val, _ := c.Get("meta") + m, ok := val.(map[string]interface{}) + if !ok { + t.Fatalf("Meta attr not map: %#v", val) + } + if len(m) != 0 { + t.Fatalf("Meta attr not empty: %#v", val) + } + } + return nil, nil + } + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } +} diff --git a/terraform/eval_variable.go b/terraform/eval_variable.go index 22d93da7fb..ce6064706c 100644 --- a/terraform/eval_variable.go +++ b/terraform/eval_variable.go @@ -2,6 +2,7 @@ package terraform import ( "fmt" + "log" "reflect" "strings" @@ -21,10 +22,6 @@ import ( // declared // - the path to the module (so we know which part of the tree to // compare the values against). -// -// Currently since the type system is simple, we currently do not make -// use of the values since it is only valid to pass string values. The -// structure is in place for extension of the type system, however. type EvalTypeCheckVariable struct { Variables map[string]interface{} ModulePath []string @@ -50,10 +47,6 @@ func (n *EvalTypeCheckVariable) Eval(ctx EvalContext) (interface{}, error) { } for name, declaredType := range prototypes { - // This is only necessary when we _actually_ check. It is left as a reminder - // that at the current time we are dealing with a type system consisting only - // of strings and maps - where the only valid inter-module variable type is - // string. proposedValue, ok := n.Variables[name] if !ok { // This means the default value should be used as no overriding value @@ -67,8 +60,6 @@ func (n *EvalTypeCheckVariable) Eval(ctx EvalContext) (interface{}, error) { switch declaredType { case config.VariableTypeString: - // This will need actual verification once we aren't dealing with - // a map[string]string but this is sufficient for now. switch proposedValue.(type) { case string: continue @@ -93,8 +84,6 @@ func (n *EvalTypeCheckVariable) Eval(ctx EvalContext) (interface{}, error) { name, modulePathDescription, declaredType.Printable(), hclTypeName(proposedValue)) } default: - // This will need the actual type substituting when we have more than - // just strings and maps. return nil, fmt.Errorf("variable %s%s should be type %s, got type string", name, modulePathDescription, declaredType.Printable()) } @@ -163,6 +152,54 @@ func (n *EvalVariableBlock) Eval(ctx EvalContext) (interface{}, error) { return nil, 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. +// +// We take all the same inputs as EvalTypeCheckVariable above, since we need +// both the target type and the proposed value in order to properly coerce. +type EvalCoerceMapVariable struct { + Variables map[string]interface{} + ModulePath []string + ModuleTree *module.Tree +} + +// Eval implements the EvalNode interface. See EvalCoerceMapVariable for +// details. +func (n *EvalCoerceMapVariable) Eval(ctx EvalContext) (interface{}, error) { + currentTree := n.ModuleTree + for _, pathComponent := range n.ModulePath[1:] { + currentTree = currentTree.Children()[pathComponent] + } + targetConfig := currentTree.Config() + + prototypes := make(map[string]config.VariableType) + for _, variable := range targetConfig.Variables { + prototypes[variable.Name] = variable.Type() + } + + for name, declaredType := range prototypes { + if declaredType != config.VariableTypeMap { + continue + } + + proposedValue, ok := n.Variables[name] + if !ok { + continue + } + + if list, ok := proposedValue.([]interface{}); ok && len(list) == 1 { + if m, ok := list[0].(map[string]interface{}); ok { + log.Printf("[DEBUG] EvalCoerceMapVariable: "+ + "Coercing single element list into map: %#v", m) + n.Variables[name] = m + } + } + } + + return nil, nil +} + // hclTypeName returns the name of the type that would represent this value in // a config file, or falls back to the Go type name if there's no corresponding // HCL type. This is used for formatted output, not for comparing types. diff --git a/terraform/eval_variable_test.go b/terraform/eval_variable_test.go new file mode 100644 index 0000000000..05fc2b850c --- /dev/null +++ b/terraform/eval_variable_test.go @@ -0,0 +1,142 @@ +package terraform + +import ( + "reflect" + "testing" +) + +func TestCoerceMapVariable(t *testing.T) { + cases := map[string]struct { + Input *EvalCoerceMapVariable + ExpectVars map[string]interface{} + }{ + "a valid map is untouched": { + Input: &EvalCoerceMapVariable{ + Variables: map[string]interface{}{ + "amap": map[string]interface{}{"foo": "bar"}, + }, + ModulePath: []string{"root"}, + ModuleTree: testModuleInline(t, map[string]string{ + "main.tf": ` + variable "amap" { + type = "map" + } + `, + }), + }, + ExpectVars: map[string]interface{}{ + "amap": map[string]interface{}{"foo": "bar"}, + }, + }, + "a list w/ a single map element is coerced": { + Input: &EvalCoerceMapVariable{ + Variables: map[string]interface{}{ + "amap": []interface{}{ + map[string]interface{}{"foo": "bar"}, + }, + }, + ModulePath: []string{"root"}, + ModuleTree: testModuleInline(t, map[string]string{ + "main.tf": ` + variable "amap" { + type = "map" + } + `, + }), + }, + ExpectVars: map[string]interface{}{ + "amap": map[string]interface{}{"foo": "bar"}, + }, + }, + "a list w/ more than one map element is untouched": { + Input: &EvalCoerceMapVariable{ + Variables: map[string]interface{}{ + "amap": []interface{}{ + map[string]interface{}{"foo": "bar"}, + map[string]interface{}{"baz": "qux"}, + }, + }, + ModulePath: []string{"root"}, + ModuleTree: testModuleInline(t, map[string]string{ + "main.tf": ` + variable "amap" { + type = "map" + } + `, + }), + }, + ExpectVars: map[string]interface{}{ + "amap": []interface{}{ + map[string]interface{}{"foo": "bar"}, + map[string]interface{}{"baz": "qux"}, + }, + }, + }, + "list coercion also works in a module": { + Input: &EvalCoerceMapVariable{ + Variables: map[string]interface{}{ + "amap": []interface{}{ + map[string]interface{}{"foo": "bar"}, + }, + }, + ModulePath: []string{"root", "middle", "bottom"}, + ModuleTree: testModuleInline(t, map[string]string{ + "top.tf": ` + module "middle" { + source = "./middle" + } + `, + "middle/mid.tf": ` + module "bottom" { + source = "./bottom" + amap { + foo = "bar" + } + } + `, + "middle/bottom/bot.tf": ` + variable "amap" { + type = "map" + } + `, + }), + }, + ExpectVars: map[string]interface{}{ + "amap": map[string]interface{}{"foo": "bar"}, + }, + }, + "coercion only occurs when target var is a map": { + Input: &EvalCoerceMapVariable{ + Variables: map[string]interface{}{ + "alist": []interface{}{ + map[string]interface{}{"foo": "bar"}, + }, + }, + ModulePath: []string{"root"}, + ModuleTree: testModuleInline(t, map[string]string{ + "main.tf": ` + variable "alist" { + type = "list" + } + `, + }), + }, + ExpectVars: map[string]interface{}{ + "alist": []interface{}{ + map[string]interface{}{"foo": "bar"}, + }, + }, + }, + } + + for tn, tc := range cases { + _, err := tc.Input.Eval(&MockEvalContext{}) + if err != nil { + t.Fatalf("%s: Unexpected err: %s", tn, err) + } + if !reflect.DeepEqual(tc.Input.Variables, tc.ExpectVars) { + t.Fatalf("%s: Expected variables:\n\n%#v\n\nGot:\n\n%#v", + tn, tc.ExpectVars, tc.Input.Variables) + } + } +} diff --git a/terraform/graph_config_node_variable.go b/terraform/graph_config_node_variable.go index 0a86c48815..35222a547c 100644 --- a/terraform/graph_config_node_variable.go +++ b/terraform/graph_config_node_variable.go @@ -176,6 +176,12 @@ func (n *GraphNodeConfigVariable) EvalTree() EvalNode { VariableValues: variables, }, + &EvalCoerceMapVariable{ + Variables: variables, + ModulePath: n.ModulePath, + ModuleTree: n.ModuleTree, + }, + &EvalTypeCheckVariable{ Variables: variables, ModulePath: n.ModulePath, diff --git a/terraform/test-fixtures/plan-module-map-literal/child/main.tf b/terraform/test-fixtures/plan-module-map-literal/child/main.tf new file mode 100644 index 0000000000..5dcb7bec4a --- /dev/null +++ b/terraform/test-fixtures/plan-module-map-literal/child/main.tf @@ -0,0 +1,12 @@ +variable "amap" { + type = "map" +} + +variable "othermap" { + type = "map" +} + +resource "aws_instance" "foo" { + tags = "${var.amap}" + meta = "${var.othermap}" +} diff --git a/terraform/test-fixtures/plan-module-map-literal/main.tf b/terraform/test-fixtures/plan-module-map-literal/main.tf new file mode 100644 index 0000000000..f9a3f201a7 --- /dev/null +++ b/terraform/test-fixtures/plan-module-map-literal/main.tf @@ -0,0 +1,7 @@ +module "child" { + source = "./child" + amap { + foo = "bar" + } + othermap {} +}