Type check variables between modules (#6185)

These tests 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. This is also a problem in nested modules.

The implementation changes add a type checking step into the graph
evaluation process to ensure invalid types are not passed.

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.

terraform/context_plan_test.go

@@ -626,6 +626,40 @@ 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_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")
@@ -2035,6 +2069,7 @@ func TestContext2Plan_varListErr(t *testing.T) {
 	})
 
 	_, err := ctx.Plan()
+
 	if err == nil {
 		t.Fatal("should error")
 	}

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 {

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,

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")}"
+}

terraform/test-fixtures/plan-module-wrong-var-type-nested/main.tf

@@ -0,0 +1,3 @@
+module "middle" {
+    source = "./middle"
+}

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")}"
+}

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"
+    }
+}

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}"
+}   

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