Merge pull request #9898 from hashicorp/b-new-ref-existing

terraform: remove pruning of module vars if they ref non-existing nodes

terraform/context_apply_test.go

@@ -1858,6 +1858,60 @@ func TestContext2Apply_moduleProviderCloseNested(t *testing.T) {
 	}
 }
 
+// Tests that variables used as module vars that reference data that
+// already exists in the state and requires no diff works properly. This
+// fixes an issue faced where module variables were pruned because they were
+// accessing "non-existent" resources (they existed, just not in the graph
+// cause they weren't in the diff).
+func TestContext2Apply_moduleVarRefExisting(t *testing.T) {
+	m := testModule(t, "apply-ref-existing")
+	p := testProvider("aws")
+	p.ApplyFn = testApplyFn
+	p.DiffFn = testDiffFn
+
+	state := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+				Resources: map[string]*ResourceState{
+					"aws_instance.foo": &ResourceState{
+						Type: "aws_instance",
+						Primary: &InstanceState{
+							ID: "foo",
+							Attributes: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		State: state,
+	})
+
+	if _, err := ctx.Plan(); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	state, err := ctx.Apply()
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(state.String())
+	expected := strings.TrimSpace(testTerraformApplyModuleVarRefExistingStr)
+	if actual != expected {
+		t.Fatalf("bad: \n%s", actual)
+	}
+}
+
 func TestContext2Apply_moduleVarResourceCount(t *testing.T) {
 	m := testModule(t, "apply-module-var-resource-count")
 	p := testProvider("aws")

terraform/graph_builder_apply.go

@@ -105,12 +105,12 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 		// Add root variables
 		&RootVariableTransformer{Module: b.Module},
 
-		// Add module variables
-		&ModuleVariableTransformer{Module: b.Module},
-
 		// Add the outputs
 		&OutputTransformer{Module: b.Module},
 
+		// Add module variables
+		&ModuleVariableTransformer{Module: b.Module},
+
 		// Connect references so ordering is correct
 		&ReferenceTransformer{},
 

terraform/terraform_test.go

@@ -494,6 +494,18 @@ module.child:
     provider = aws.eu
 `
 
+const testTerraformApplyModuleVarRefExistingStr = `
+aws_instance.foo:
+  ID = foo
+  foo = bar
+
+module.child:
+  aws_instance.foo:
+    ID = foo
+    type = aws_instance
+    value = bar
+`
+
 const testTerraformApplyOutputOrphanStr = `
 <no state>
 Outputs:

terraform/test-fixtures/apply-ref-existing/child/main.tf

@@ -0,0 +1,5 @@
+variable "var" {}
+
+resource "aws_instance" "foo" {
+    value = "${var.var}"
+}

terraform/test-fixtures/apply-ref-existing/main.tf

@@ -0,0 +1,7 @@
+resource "aws_instance" "foo" { foo = "bar" }
+
+module "child" {
+    source = "./child"
+
+    var = "${aws_instance.foo.foo}"
+}

terraform/transform_module_variable.go

@@ -11,11 +11,12 @@ import (
 // ModuleVariableTransformer is a GraphTransformer that adds all the variables
 // in the configuration to the graph.
 //
-// This only adds variables that either have no dependencies (and therefore
-// always succeed) or has dependencies that are 100% represented in the
-// graph.
+// This only adds variables that are referenced by other thigns in the graph.
+// If a module variable is not referenced, it won't be added to the graph.
 type ModuleVariableTransformer struct {
 	Module *module.Tree
+
+	DisablePrune bool // True if pruning unreferenced should be disabled
 }
 
 func (t *ModuleVariableTransformer) Transform(g *Graph) error {
@@ -28,18 +29,18 @@ func (t *ModuleVariableTransformer) transform(g *Graph, parent, m *module.Tree)
 		return nil
 	}
 
-	// If we have a parent, we can determine if a module variable is being
-	// used, so we transform this.
-	if parent != nil {
-		if err := t.transformSingle(g, parent, m); err != nil {
+	// Transform all the children. This must be done BEFORE the transform
+	// above since child module variables can reference parent module variables.
+	for _, c := range m.Children() {
+		if err := t.transform(g, m, c); err != nil {
 			return err
 		}
 	}
 
-	// Transform all the children. This must be done AFTER the transform
-	// above since child module variables can reference parent module variables.
-	for _, c := range m.Children() {
-		if err := t.transform(g, m, c); err != nil {
+	// If we have a parent, we can determine if a module variable is being
+	// used, so we transform this.
+	if parent != nil {
+		if err := t.transformSingle(g, parent, m); err != nil {
 			return err
 		}
 	}
@@ -100,18 +101,15 @@ func (t *ModuleVariableTransformer) transformSingle(g *Graph, parent, m *module.
 			Module:    t.Module,
 		}
 
-		// If the node references something, then we check to make sure
-		// that the thing it references is in the graph. If it isn't, then
-		// we don't add it because we may not be able to compute the output.
-		//
-		// If the node references nothing, we always include it since there
-		// is no other clear time to compute it.
-		matches, missing := refMap.References(node)
-		if len(missing) > 0 {
-			log.Printf(
-				"[INFO] Not including %q in graph, matches: %v, missing: %s",
-				dag.VertexName(node), matches, missing)
-			continue
+		if !t.DisablePrune {
+			// If the node is not referenced by anything, then we don't need
+			// to include it since it won't be used.
+			if matches := refMap.ReferencedBy(node); len(matches) == 0 {
+				log.Printf(
+					"[INFO] Not including %q in graph, nothing depends on it",
+					dag.VertexName(node))
+				continue
+			}
 		}
 
 		// Add it!

terraform/transform_module_variable_test.go

@@ -17,7 +17,7 @@ func TestModuleVariableTransformer(t *testing.T) {
 	}
 
 	{
-		tf := &ModuleVariableTransformer{Module: module}
+		tf := &ModuleVariableTransformer{Module: module, DisablePrune: true}
 		if err := tf.Transform(&g); err != nil {
 			t.Fatalf("err: %s", err)
 		}
@@ -42,7 +42,7 @@ func TestModuleVariableTransformer_nested(t *testing.T) {
 	}
 
 	{
-		tf := &ModuleVariableTransformer{Module: module}
+		tf := &ModuleVariableTransformer{Module: module, DisablePrune: true}
 		if err := tf.Transform(&g); err != nil {
 			t.Fatalf("err: %s", err)
 		}

terraform/transform_reference.go

@@ -66,7 +66,8 @@ func (t *ReferenceTransformer) Transform(g *Graph) error {
 type ReferenceMap struct {
 	// m is the mapping of referenceable name to list of verticies that
 	// implement that name. This is built on initialization.
-	m map[string][]dag.Vertex
+	references   map[string][]dag.Vertex
+	referencedBy map[string][]dag.Vertex
 }
 
 // References returns the list of vertices that this vertex
@@ -82,7 +83,7 @@ func (m *ReferenceMap) References(v dag.Vertex) ([]dag.Vertex, []string) {
 	prefix := m.prefix(v)
 	for _, n := range rn.References() {
 		n = prefix + n
-		parents, ok := m.m[n]
+		parents, ok := m.references[n]
 		if !ok {
 			missing = append(missing, n)
 			continue
@@ -106,6 +107,41 @@ func (m *ReferenceMap) References(v dag.Vertex) ([]dag.Vertex, []string) {
 	return matches, missing
 }
 
+// ReferencedBy returns the list of vertices that reference the
+// vertex passed in.
+func (m *ReferenceMap) ReferencedBy(v dag.Vertex) []dag.Vertex {
+	rn, ok := v.(GraphNodeReferenceable)
+	if !ok {
+		return nil
+	}
+
+	var matches []dag.Vertex
+	prefix := m.prefix(v)
+	for _, n := range rn.ReferenceableName() {
+		n = prefix + n
+		children, ok := m.referencedBy[n]
+		if !ok {
+			continue
+		}
+
+		// Make sure this isn't a self reference, which isn't included
+		selfRef := false
+		for _, p := range children {
+			if p == v {
+				selfRef = true
+				break
+			}
+		}
+		if selfRef {
+			continue
+		}
+
+		matches = append(matches, children...)
+	}
+
+	return matches
+}
+
 func (m *ReferenceMap) prefix(v dag.Vertex) string {
 	// If the node is stating it is already fully qualified then
 	// we don't have to create the prefix!
@@ -146,7 +182,25 @@ func NewReferenceMap(vs []dag.Vertex) *ReferenceMap {
 		}
 	}
 
-	m.m = refMap
+	// Build the lookup table for referenced by
+	refByMap := make(map[string][]dag.Vertex)
+	for _, v := range vs {
+		// We're only looking for referenceable nodes
+		rn, ok := v.(GraphNodeReferencer)
+		if !ok {
+			continue
+		}
+
+		// Go through and cache them
+		prefix := m.prefix(v)
+		for _, n := range rn.References() {
+			n = prefix + n
+			refByMap[n] = append(refByMap[n], v)
+		}
+	}
+
+	m.references = refMap
+	m.referencedBy = refByMap
 	return &m
 }
 

terraform/transform_reference_test.go

@@ -1,8 +1,12 @@
 package terraform
 
 import (
+	"reflect"
+	"sort"
 	"strings"
 	"testing"
+
+	"github.com/hashicorp/terraform/dag"
 )
 
 func TestReferenceTransformer_simple(t *testing.T) {
@@ -84,6 +88,94 @@ func TestReferenceTransformer_path(t *testing.T) {
 	}
 }
 
+func TestReferenceMapReferences(t *testing.T) {
+	cases := map[string]struct {
+		Nodes  []dag.Vertex
+		Check  dag.Vertex
+		Result []string
+	}{
+		"simple": {
+			Nodes: []dag.Vertex{
+				&graphNodeRefParentTest{
+					NameValue: "A",
+					Names:     []string{"A"},
+				},
+			},
+			Check: &graphNodeRefChildTest{
+				NameValue: "foo",
+				Refs:      []string{"A"},
+			},
+			Result: []string{"A"},
+		},
+	}
+
+	for tn, tc := range cases {
+		t.Run(tn, func(t *testing.T) {
+			rm := NewReferenceMap(tc.Nodes)
+			result, _ := rm.References(tc.Check)
+
+			var resultStr []string
+			for _, v := range result {
+				resultStr = append(resultStr, dag.VertexName(v))
+			}
+
+			sort.Strings(resultStr)
+			sort.Strings(tc.Result)
+			if !reflect.DeepEqual(resultStr, tc.Result) {
+				t.Fatalf("bad: %#v", resultStr)
+			}
+		})
+	}
+}
+
+func TestReferenceMapReferencedBy(t *testing.T) {
+	cases := map[string]struct {
+		Nodes  []dag.Vertex
+		Check  dag.Vertex
+		Result []string
+	}{
+		"simple": {
+			Nodes: []dag.Vertex{
+				&graphNodeRefChildTest{
+					NameValue: "A",
+					Refs:      []string{"A"},
+				},
+				&graphNodeRefChildTest{
+					NameValue: "B",
+					Refs:      []string{"A"},
+				},
+				&graphNodeRefChildTest{
+					NameValue: "C",
+					Refs:      []string{"B"},
+				},
+			},
+			Check: &graphNodeRefParentTest{
+				NameValue: "foo",
+				Names:     []string{"A"},
+			},
+			Result: []string{"A", "B"},
+		},
+	}
+
+	for tn, tc := range cases {
+		t.Run(tn, func(t *testing.T) {
+			rm := NewReferenceMap(tc.Nodes)
+			result := rm.ReferencedBy(tc.Check)
+
+			var resultStr []string
+			for _, v := range result {
+				resultStr = append(resultStr, dag.VertexName(v))
+			}
+
+			sort.Strings(resultStr)
+			sort.Strings(tc.Result)
+			if !reflect.DeepEqual(resultStr, tc.Result) {
+				t.Fatalf("bad: %#v", resultStr)
+			}
+		})
+	}
+}
+
 type graphNodeRefParentTest struct {
 	NameValue string
 	PathValue []string