diff --git a/terraform/context_test.go b/terraform/context_test.go index f72e602c4..15bb62778 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -1155,6 +1155,58 @@ func TestContext2Plan_moduleDestroy(t *testing.T) { } } +// GH-1835 +func TestContext2Plan_moduleDestroyCycle(t *testing.T) { + m := testModule(t, "plan-module-destroy-gh-1835") + p := testProvider("aws") + p.DiffFn = testDiffFn + s := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: []string{"root", "a_module"}, + Resources: map[string]*ResourceState{ + "aws_instance.a": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "a", + }, + }, + }, + }, + &ModuleState{ + Path: []string{"root", "b_module"}, + Resources: map[string]*ResourceState{ + "aws_instance.b": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "b", + }, + }, + }, + }, + }, + } + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + State: s, + Destroy: true, + }) + + plan, err := ctx.Plan() + if err != nil { + t.Fatalf("err: %s", err) + } + + actual := strings.TrimSpace(plan.String()) + expected := strings.TrimSpace(testTerraformPlanModuleDestroyCycleStr) + if actual != expected { + t.Fatalf("bad:\n%s", actual) + } +} + func TestContext2Plan_moduleDestroyMultivar(t *testing.T) { m := testModule(t, "plan-module-destroy-multivar") p := testProvider("aws") diff --git a/terraform/diff.go b/terraform/diff.go index 96b8c654a..7cd907422 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -80,7 +80,18 @@ func (d *Diff) Empty() bool { func (d *Diff) String() string { var buf bytes.Buffer + + keys := make([]string, 0, len(d.Modules)) + lookup := make(map[string]*ModuleDiff) for _, m := range d.Modules { + key := fmt.Sprintf("module.%s", strings.Join(m.Path[1:], ".")) + keys = append(keys, key) + lookup[key] = m + } + sort.Strings(keys) + + for _, key := range keys { + m := lookup[key] mStr := m.String() // If we're the root module, we just write the output directly. @@ -89,7 +100,7 @@ func (d *Diff) String() string { continue } - buf.WriteString(fmt.Sprintf("module.%s:\n", strings.Join(m.Path[1:], "."))) + buf.WriteString(fmt.Sprintf("%s:\n", key)) s := bufio.NewScanner(strings.NewReader(mStr)) for s.Scan() { diff --git a/terraform/graph_builder.go b/terraform/graph_builder.go index 574181c42..28a76e513 100644 --- a/terraform/graph_builder.go +++ b/terraform/graph_builder.go @@ -153,7 +153,7 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer { if len(path) <= 1 { steps = append(steps, // Create the destruction nodes - &DestroyTransformer{}, + &DestroyTransformer{FullDestroy: b.Destroy}, &CreateBeforeDestroyTransformer{}, b.conditional(&conditionalOpts{ If: func() bool { return !b.Verbose }, diff --git a/terraform/graph_config_node_output.go b/terraform/graph_config_node_output.go index 0d84a7862..5b2d95fdc 100644 --- a/terraform/graph_config_node_output.go +++ b/terraform/graph_config_node_output.go @@ -62,7 +62,7 @@ func (n *GraphNodeConfigOutput) Proxy() bool { } // GraphNodeDestroyEdgeInclude impl. -func (n *GraphNodeConfigOutput) DestroyEdgeInclude() bool { +func (n *GraphNodeConfigOutput) DestroyEdgeInclude(dag.Vertex) bool { return false } diff --git a/terraform/graph_config_node_resource.go b/terraform/graph_config_node_resource.go index 298e90167..30667cb43 100644 --- a/terraform/graph_config_node_resource.go +++ b/terraform/graph_config_node_resource.go @@ -9,6 +9,12 @@ import ( "github.com/hashicorp/terraform/dot" ) +// GraphNodeCountDependent is implemented by resources for giving only +// the dependencies they have from the "count" field. +type GraphNodeCountDependent interface { + CountDependentOn() []string +} + // GraphNodeConfigResource represents a resource within the config graph. type GraphNodeConfigResource struct { Resource *config.Resource @@ -31,6 +37,18 @@ func (n *GraphNodeConfigResource) DependableName() []string { return []string{n.Resource.Id()} } +// GraphNodeCountDependent impl. +func (n *GraphNodeConfigResource) CountDependentOn() []string { + result := make([]string, 0, len(n.Resource.RawCount.Variables)) + for _, v := range n.Resource.RawCount.Variables { + if vn := varNameForVar(v); vn != "" { + result = append(result, vn) + } + } + + return result +} + // GraphNodeDependent impl. func (n *GraphNodeConfigResource) DependentOn() []string { result := make([]string, len(n.Resource.DependsOn), diff --git a/terraform/graph_config_node_variable.go b/terraform/graph_config_node_variable.go index 54c1c8343..91f126c49 100644 --- a/terraform/graph_config_node_variable.go +++ b/terraform/graph_config_node_variable.go @@ -55,6 +55,26 @@ func (n *GraphNodeConfigVariable) VariableName() string { return n.Variable.Name } +// GraphNodeDestroyEdgeInclude impl. +func (n *GraphNodeConfigVariable) DestroyEdgeInclude(v dag.Vertex) bool { + // Only include this variable in a destroy edge if the source vertex + // "v" has a count dependency on this variable. + cv, ok := v.(GraphNodeCountDependent) + if !ok { + return false + } + + for _, d := range cv.CountDependentOn() { + for _, d2 := range n.DependableName() { + if d == d2 { + return true + } + } + } + + return false +} + // GraphNodeProxy impl. func (n *GraphNodeConfigVariable) Proxy() bool { return true diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 878078668..e30d27a13 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -996,6 +996,26 @@ module.child: ID = bar ` +const testTerraformPlanModuleDestroyCycleStr = ` +DIFF: + +module.a_module: + DESTROY MODULE + DESTROY: aws_instance.a +module.b_module: + DESTROY MODULE + DESTROY: aws_instance.b + +STATE: + +module.a_module: + aws_instance.a: + ID = a +module.b_module: + aws_instance.b: + ID = b +` + const testTerraformPlanModuleDestroyMultivarStr = ` DIFF: diff --git a/terraform/test-fixtures/plan-module-destroy-gh-1835/a/main.tf b/terraform/test-fixtures/plan-module-destroy-gh-1835/a/main.tf new file mode 100644 index 000000000..ca44c757d --- /dev/null +++ b/terraform/test-fixtures/plan-module-destroy-gh-1835/a/main.tf @@ -0,0 +1,5 @@ +resource "aws_instance" "a" {} + +output "a_output" { + value = "${aws_instance.a.id}" +} diff --git a/terraform/test-fixtures/plan-module-destroy-gh-1835/b/main.tf b/terraform/test-fixtures/plan-module-destroy-gh-1835/b/main.tf new file mode 100644 index 000000000..c3b0270b0 --- /dev/null +++ b/terraform/test-fixtures/plan-module-destroy-gh-1835/b/main.tf @@ -0,0 +1,5 @@ +variable "a_id" {} + +resource "aws_instance" "b" { + command = "echo ${var.a_id}" +} diff --git a/terraform/test-fixtures/plan-module-destroy-gh-1835/main.tf b/terraform/test-fixtures/plan-module-destroy-gh-1835/main.tf new file mode 100644 index 000000000..c2f72c45e --- /dev/null +++ b/terraform/test-fixtures/plan-module-destroy-gh-1835/main.tf @@ -0,0 +1,8 @@ +module "a_module" { + source = "./a" +} + +module "b_module" { + source = "./b" + a_id = "${module.a_module.a_output}" +} diff --git a/terraform/transform_destroy.go b/terraform/transform_destroy.go index a3fe737af..3fe585664 100644 --- a/terraform/transform_destroy.go +++ b/terraform/transform_destroy.go @@ -49,12 +49,14 @@ type GraphNodeDestroyPrunable interface { // as an edge within the destroy graph. This is usually done because it // might cause unnecessary cycles. type GraphNodeDestroyEdgeInclude interface { - DestroyEdgeInclude() bool + DestroyEdgeInclude(dag.Vertex) bool } // DestroyTransformer is a GraphTransformer that creates the destruction // nodes for things that _might_ be destroyed. -type DestroyTransformer struct{} +type DestroyTransformer struct { + FullDestroy bool +} func (t *DestroyTransformer) Transform(g *Graph) error { var connect, remove []dag.Edge @@ -111,7 +113,8 @@ func (t *DestroyTransformer) transform( for _, edgeRaw := range downEdges { // If this thing specifically requests to not be depended on // by destroy nodes, then don't. - if i, ok := edgeRaw.(GraphNodeDestroyEdgeInclude); ok && !i.DestroyEdgeInclude() { + if i, ok := edgeRaw.(GraphNodeDestroyEdgeInclude); ok && + !i.DestroyEdgeInclude(v) { continue }