From 02376e1e0e8519b4f02e50d5bbeb1030e01df4a0 Mon Sep 17 00:00:00 2001 From: James Nugent Date: Wed, 13 Apr 2016 10:18:20 -0500 Subject: [PATCH 1/3] core: Add test demonstrating module input types This test demonstrates a problem where the types to a module input are not checked. For example, if a module - inner - defines a variable "should_be_a_map" as a map, or with a default variable of map, we do not fail if the user sets the variable value in the outer module to a string value. --- terraform/context_plan_test.go | 18 ++++++++++++++++++ .../plan-module-wrong-var-type/inner/main.tf | 7 +++++++ .../plan-module-wrong-var-type/main.tf | 10 ++++++++++ 3 files changed, 35 insertions(+) create mode 100644 terraform/test-fixtures/plan-module-wrong-var-type/inner/main.tf create mode 100644 terraform/test-fixtures/plan-module-wrong-var-type/main.tf diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index ea45c1323665..ef1d7c7b5bfb 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -626,6 +626,23 @@ func TestContext2Plan_moduleVar(t *testing.T) { } } +func TestContext2Plan_moduleVarWrongType(t *testing.T) { + m := testModule(t, "plan-module-wrong-var-type") + p := testProvider("aws") + p.DiffFn = testDiffFn + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + }) + + _, err := ctx.Plan() + if err == nil { + t.Fatalf("should error") + } +} + func TestContext2Plan_moduleVarComputed(t *testing.T) { m := testModule(t, "plan-module-var-computed") p := testProvider("aws") @@ -2035,6 +2052,7 @@ func TestContext2Plan_varListErr(t *testing.T) { }) _, err := ctx.Plan() + if err == nil { t.Fatal("should error") } diff --git a/terraform/test-fixtures/plan-module-wrong-var-type/inner/main.tf b/terraform/test-fixtures/plan-module-wrong-var-type/inner/main.tf new file mode 100644 index 000000000000..8a9f380c7721 --- /dev/null +++ b/terraform/test-fixtures/plan-module-wrong-var-type/inner/main.tf @@ -0,0 +1,7 @@ +variable "map_in" { + type = "map" + default = { + us-west-1 = "ami-12345" + us-west-2 = "ami-67890" + } +} diff --git a/terraform/test-fixtures/plan-module-wrong-var-type/main.tf b/terraform/test-fixtures/plan-module-wrong-var-type/main.tf new file mode 100644 index 000000000000..4fc7f8a7c3fe --- /dev/null +++ b/terraform/test-fixtures/plan-module-wrong-var-type/main.tf @@ -0,0 +1,10 @@ +variable "input" { + type = "string" + default = "hello world" +} + +module "test" { + source = "./inner" + + map_in = "${var.input}" +} From 414910d4781df1d48860ddd4bd148cd909bc5df8 Mon Sep 17 00:00:00 2001 From: James Nugent Date: Thu, 14 Apr 2016 15:54:54 -0700 Subject: [PATCH 2/3] core: Add test demonstrating lack of type checking This adds a test where the assignment is in a nested module rather than the root. --- terraform/context_plan_test.go | 17 +++++++++++++++++ .../inner/main.tf | 13 +++++++++++++ .../plan-module-wrong-var-type-nested/main.tf | 3 +++ .../middle/main.tf | 19 +++++++++++++++++++ 4 files changed, 52 insertions(+) create mode 100644 terraform/test-fixtures/plan-module-wrong-var-type-nested/inner/main.tf create mode 100644 terraform/test-fixtures/plan-module-wrong-var-type-nested/main.tf create mode 100644 terraform/test-fixtures/plan-module-wrong-var-type-nested/middle/main.tf diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index ef1d7c7b5bfb..b6f696dd38a9 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -643,6 +643,23 @@ func TestContext2Plan_moduleVarWrongType(t *testing.T) { } } +func TestContext2Plan_moduleVarWrongTypeNested(t *testing.T) { + m := testModule(t, "plan-module-wrong-var-type-nested") + p := testProvider("aws") + p.DiffFn = testDiffFn + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + }) + + _, err := ctx.Plan() + if err == nil { + t.Fatalf("should error") + } +} + func TestContext2Plan_moduleVarComputed(t *testing.T) { m := testModule(t, "plan-module-var-computed") p := testProvider("aws") diff --git a/terraform/test-fixtures/plan-module-wrong-var-type-nested/inner/main.tf b/terraform/test-fixtures/plan-module-wrong-var-type-nested/inner/main.tf new file mode 100644 index 000000000000..88995119d7b2 --- /dev/null +++ b/terraform/test-fixtures/plan-module-wrong-var-type-nested/inner/main.tf @@ -0,0 +1,13 @@ +variable "inner_in" { + type = "map" + default = { + us-west-1 = "ami-12345" + us-west-2 = "ami-67890" + } +} + +resource "null_resource" "inner_noop" {} + +output "inner_out" { + value = "${lookup(var.inner_in, "us-west-1")}" +} diff --git a/terraform/test-fixtures/plan-module-wrong-var-type-nested/main.tf b/terraform/test-fixtures/plan-module-wrong-var-type-nested/main.tf new file mode 100644 index 000000000000..fe63fc5f8129 --- /dev/null +++ b/terraform/test-fixtures/plan-module-wrong-var-type-nested/main.tf @@ -0,0 +1,3 @@ +module "middle" { + source = "./middle" +} diff --git a/terraform/test-fixtures/plan-module-wrong-var-type-nested/middle/main.tf b/terraform/test-fixtures/plan-module-wrong-var-type-nested/middle/main.tf new file mode 100644 index 000000000000..1e823576196f --- /dev/null +++ b/terraform/test-fixtures/plan-module-wrong-var-type-nested/middle/main.tf @@ -0,0 +1,19 @@ +variable "middle_in" { + type = "map" + default = { + eu-west-1 = "ami-12345" + eu-west-2 = "ami-67890" + } +} + +module "inner" { + source = "../inner" + + inner_in = "hello" +} + +resource "null_resource" "middle_noop" {} + +output "middle_out" { + value = "${lookup(var.middle_in, "us-west-1")}" +} From 8b9896ea603524bf86144bd9d44312c94e9ea745 Mon Sep 17 00:00:00 2001 From: James Nugent Date: Thu, 14 Apr 2016 15:56:03 -0700 Subject: [PATCH 3/3] core: Check variable types between modules --- config/config.go | 11 +++++ terraform/eval_variable.go | 63 +++++++++++++++++++++++++ terraform/graph_config_node_variable.go | 10 +++- terraform/transform_config.go | 6 ++- 4 files changed, 88 insertions(+), 2 deletions(-) diff --git a/config/config.go b/config/config.go index 8d97d9f3c05a..6b8d1f80794d 100644 --- a/config/config.go +++ b/config/config.go @@ -162,6 +162,17 @@ const ( VariableTypeMap ) +func (v VariableType) Printable() string { + switch v { + case VariableTypeString: + return "string" + case VariableTypeMap: + return "map" + default: + return "unknown" + } +} + // ProviderConfigName returns the name of the provider configuration in // the given mapping that maps to the proper provider configuration // for this resource. diff --git a/terraform/eval_variable.go b/terraform/eval_variable.go index e6a9befbea1b..6b2130e20ee2 100644 --- a/terraform/eval_variable.go +++ b/terraform/eval_variable.go @@ -2,12 +2,75 @@ package terraform import ( "fmt" + "strings" "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform/config" + "github.com/hashicorp/terraform/config/module" "github.com/mitchellh/mapstructure" ) +// EvalTypeCheckVariable is an EvalNode which ensures that the variable +// values which are assigned as inputs to a module (including the root) +// match the types which are either declared for the variables explicitly +// or inferred from the default values. +// +// In order to achieve this three things are required: +// - a map of the proposed variable values +// - the configuration tree of the module in which the variable is +// 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]string + ModulePath []string + ModuleTree *module.Tree +} + +func (n *EvalTypeCheckVariable) 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 { + // 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 := n.Variables[name] + + 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. + continue + default: + // Only display a module if we are not in the root module + modulePathDescription := fmt.Sprintf(" in module %s", strings.Join(n.ModulePath[1:], ".")) + if len(n.ModulePath) == 1 { + modulePathDescription = "" + } + // 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()) + } + } + + return nil, nil +} + // EvalSetVariables is an EvalNode implementation that sets the variables // explicitly for interpolation later. type EvalSetVariables struct { diff --git a/terraform/graph_config_node_variable.go b/terraform/graph_config_node_variable.go index 9b3d77dbc4b1..e462070d022d 100644 --- a/terraform/graph_config_node_variable.go +++ b/terraform/graph_config_node_variable.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/hashicorp/terraform/config" + "github.com/hashicorp/terraform/config/module" "github.com/hashicorp/terraform/dag" ) @@ -18,7 +19,8 @@ type GraphNodeConfigVariable struct { Module string Value *config.RawConfig - depPrefix string + ModuleTree *module.Tree + ModulePath []string } func (n *GraphNodeConfigVariable) Name() string { @@ -125,6 +127,12 @@ func (n *GraphNodeConfigVariable) EvalTree() EvalNode { Variables: variables, }, + &EvalTypeCheckVariable{ + Variables: variables, + ModulePath: n.ModulePath, + ModuleTree: n.ModuleTree, + }, + &EvalSetVariables{ Module: &n.Module, Variables: variables, diff --git a/terraform/transform_config.go b/terraform/transform_config.go index a14e4f4342b9..bcfa1233e3a6 100644 --- a/terraform/transform_config.go +++ b/terraform/transform_config.go @@ -45,7 +45,11 @@ func (t *ConfigTransformer) Transform(g *Graph) error { // Write all the variables out for _, v := range config.Variables { - nodes = append(nodes, &GraphNodeConfigVariable{Variable: v}) + nodes = append(nodes, &GraphNodeConfigVariable{ + Variable: v, + ModuleTree: t.Module, + ModulePath: g.Path, + }) } // Write all the provider configs out