From b7954a42fea1cf2ee0d84368f1ad273fbe6ac965 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 4 Nov 2016 17:47:20 -0700 Subject: [PATCH 1/3] terraform: remove pruning of module vars if they ref non-existing nodes Fixes a shadow graph error found during usage. The new apply graph was only adding module variables that referenced data that existed _in the graph_. This isn't a valid optimization since the data it is referencing may be in the state with no diff, and therefore available but not in the graph. This just removes that optimization logic, which causes no failing tests. It also adds a test that exposes the bug if we had the pruning logic. --- terraform/context_apply_test.go | 54 +++++++++++++++++++ terraform/terraform_test.go | 12 +++++ .../apply-ref-existing/child/main.tf | 5 ++ .../test-fixtures/apply-ref-existing/main.tf | 7 +++ terraform/transform_module_variable.go | 18 ------- 5 files changed, 78 insertions(+), 18 deletions(-) create mode 100644 terraform/test-fixtures/apply-ref-existing/child/main.tf create mode 100644 terraform/test-fixtures/apply-ref-existing/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 337261616c..bcec45cfcb 100644 --- a/terraform/context_apply_test.go +++ b/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") diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 23fe5aeeba..60b26b23b5 100644 --- a/terraform/terraform_test.go +++ b/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 = ` Outputs: diff --git a/terraform/test-fixtures/apply-ref-existing/child/main.tf b/terraform/test-fixtures/apply-ref-existing/child/main.tf new file mode 100644 index 0000000000..cd1e56eec9 --- /dev/null +++ b/terraform/test-fixtures/apply-ref-existing/child/main.tf @@ -0,0 +1,5 @@ +variable "var" {} + +resource "aws_instance" "foo" { + value = "${var.var}" +} diff --git a/terraform/test-fixtures/apply-ref-existing/main.tf b/terraform/test-fixtures/apply-ref-existing/main.tf new file mode 100644 index 0000000000..8aa356391f --- /dev/null +++ b/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}" +} diff --git a/terraform/transform_module_variable.go b/terraform/transform_module_variable.go index 1e035107cb..e685b918e6 100644 --- a/terraform/transform_module_variable.go +++ b/terraform/transform_module_variable.go @@ -5,7 +5,6 @@ import ( "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/config/module" - "github.com/hashicorp/terraform/dag" ) // ModuleVariableTransformer is a GraphTransformer that adds all the variables @@ -68,9 +67,6 @@ func (t *ModuleVariableTransformer) transformSingle(g *Graph, parent, m *module. return nil } - // Build the reference map so we can determine if we're referencing things. - refMap := NewReferenceMap(g.Vertices()) - // Add all variables here for _, v := range vars { // Determine the value of the variable. If it isn't in the @@ -100,20 +96,6 @@ 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 - } - // Add it! g.Add(node) } From b488e51f56a57a4c3a86e23be560c6efa0871d7d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 4 Nov 2016 18:40:09 -0700 Subject: [PATCH 2/3] terraform: tests for ReferenceMap.References --- terraform/transform_reference_test.go | 47 +++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/terraform/transform_reference_test.go b/terraform/transform_reference_test.go index 525add6bdf..ae3b7ce799 100644 --- a/terraform/transform_reference_test.go +++ b/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,49 @@ 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, err := rm.References(tc.Check) + if err != nil { + t.Fatalf("err: %s", err) + } + + 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 From a5df3973a471b675c757b46b9540c2f0f9f1635a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 4 Nov 2016 18:58:03 -0700 Subject: [PATCH 3/3] terraform: module variables should be pruned if nothing depends on them --- terraform/graph_builder_apply.go | 6 +-- terraform/transform_module_variable.go | 38 +++++++++---- terraform/transform_module_variable_test.go | 4 +- terraform/transform_reference.go | 60 +++++++++++++++++++-- terraform/transform_reference_test.go | 51 ++++++++++++++++-- 5 files changed, 137 insertions(+), 22 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 1d88b706d3..601792721a 100644 --- a/terraform/graph_builder_apply.go +++ b/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{}, diff --git a/terraform/transform_module_variable.go b/terraform/transform_module_variable.go index e685b918e6..50f00bff1c 100644 --- a/terraform/transform_module_variable.go +++ b/terraform/transform_module_variable.go @@ -5,16 +5,18 @@ import ( "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/config/module" + "github.com/hashicorp/terraform/dag" ) // 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 { @@ -27,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 } } @@ -67,6 +69,9 @@ func (t *ModuleVariableTransformer) transformSingle(g *Graph, parent, m *module. return nil } + // Build the reference map so we can determine if we're referencing things. + refMap := NewReferenceMap(g.Vertices()) + // Add all variables here for _, v := range vars { // Determine the value of the variable. If it isn't in the @@ -96,6 +101,17 @@ func (t *ModuleVariableTransformer) transformSingle(g *Graph, parent, m *module. Module: t.Module, } + 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! g.Add(node) } diff --git a/terraform/transform_module_variable_test.go b/terraform/transform_module_variable_test.go index 6842b325c7..bd1f9525b9 100644 --- a/terraform/transform_module_variable_test.go +++ b/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) } diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index f848274aa1..613f1484c0 100644 --- a/terraform/transform_reference.go +++ b/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 } diff --git a/terraform/transform_reference_test.go b/terraform/transform_reference_test.go index ae3b7ce799..31cf4664de 100644 --- a/terraform/transform_reference_test.go +++ b/terraform/transform_reference_test.go @@ -112,11 +112,56 @@ func TestReferenceMapReferences(t *testing.T) { for tn, tc := range cases { t.Run(tn, func(t *testing.T) { rm := NewReferenceMap(tc.Nodes) - result, err := rm.References(tc.Check) - if err != nil { - t.Fatalf("err: %s", err) + 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))