From b7954a42fea1cf2ee0d84368f1ad273fbe6ac965 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 4 Nov 2016 17:47:20 -0700 Subject: [PATCH] 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 337261616..bcec45cfc 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 23fe5aeeb..60b26b23b 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 000000000..cd1e56eec --- /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 000000000..8aa356391 --- /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 1e035107c..e685b918e 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) }