diff --git a/terraform/context_test.go b/terraform/context_test.go index 60da26deb..907f21c4b 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -4053,6 +4053,88 @@ func TestContext2Apply_module(t *testing.T) { } } +func TestContext2Apply_moduleDestroyOrder(t *testing.T) { + m := testModule(t, "apply-module-destroy-order") + p := testProvider("aws") + p.DiffFn = testDiffFn + + // Create a custom apply function to track the order they were destroyed + var order []string + var orderLock sync.Mutex + p.ApplyFn = func( + info *InstanceInfo, + is *InstanceState, + id *InstanceDiff) (*InstanceState, error) { + orderLock.Lock() + defer orderLock.Unlock() + + order = append(order, is.ID) + return nil, nil + } + + state := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.b": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "b", + }, + }, + }, + }, + + &ModuleState{ + Path: []string{"root", "child"}, + Resources: map[string]*ResourceState{ + "aws_instance.a": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "a", + }, + }, + }, + Outputs: map[string]string{ + "a_output": "a", + }, + }, + }, + } + + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + State: state, + Destroy: true, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + expected := []string{"b", "a"} + if !reflect.DeepEqual(order, expected) { + t.Fatalf("bad: %#v", order) + } + + { + actual := strings.TrimSpace(state.String()) + expected := strings.TrimSpace(testTerraformApplyModuleDestroyOrderStr) + 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/graph_builder.go b/terraform/graph_builder.go index addb8baca..574181c42 100644 --- a/terraform/graph_builder.go +++ b/terraform/graph_builder.go @@ -91,7 +91,7 @@ type BuiltinGraphBuilder struct { // Build builds the graph according to the steps returned by Steps. func (b *BuiltinGraphBuilder) Build(path []string) (*Graph, error) { basic := &BasicGraphBuilder{ - Steps: b.Steps(), + Steps: b.Steps(path), Validate: b.Validate, } @@ -100,7 +100,7 @@ func (b *BuiltinGraphBuilder) Build(path []string) (*Graph, error) { // Steps returns the ordered list of GraphTransformers that must be executed // to build a complete graph. -func (b *BuiltinGraphBuilder) Steps() []GraphTransformer { +func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer { steps := []GraphTransformer{ // Create all our resources from the configuration and state &ConfigTransformer{Module: b.Root}, @@ -144,20 +144,31 @@ func (b *BuiltinGraphBuilder) Steps() []GraphTransformer { // their dependencies. &TargetsTransformer{Targets: b.Targets, Destroy: b.Destroy}, - // Create the destruction nodes - &DestroyTransformer{}, - &CreateBeforeDestroyTransformer{}, - b.conditional(&conditionalOpts{ - If: func() bool { return !b.Verbose }, - Then: &PruneDestroyTransformer{Diff: b.Diff, State: b.State}, - }), - - // Make sure we create one root + // Make sure we have a single root &RootTransformer{}, + } - // Perform the transitive reduction to make our graph a bit - // more sane if possible (it usually is possible). - &TransitiveReductionTransformer{}, + // If we're on the root path, then we do a bunch of other stuff. + // We don't do the following for modules. + if len(path) <= 1 { + steps = append(steps, + // Create the destruction nodes + &DestroyTransformer{}, + &CreateBeforeDestroyTransformer{}, + b.conditional(&conditionalOpts{ + If: func() bool { return !b.Verbose }, + Then: &PruneDestroyTransformer{Diff: b.Diff, State: b.State}, + }), + + // Make sure we have a single root after the above changes. + // This is the 2nd root transformer. In practice this shouldn't + // actually matter as the RootTransformer is idempotent. + &RootTransformer{}, + + // Perform the transitive reduction to make our graph a bit + // more sane if possible (it usually is possible). + &TransitiveReductionTransformer{}, + ) } // Remove nils diff --git a/terraform/graph_config_node_resource.go b/terraform/graph_config_node_resource.go index 821f89738..7ef59f8fd 100644 --- a/terraform/graph_config_node_resource.go +++ b/terraform/graph_config_node_resource.go @@ -268,10 +268,38 @@ func (n *GraphNodeConfigResourceFlat) ProvidedBy() []string { } // GraphNodeDestroyable impl. -func (n *GraphNodeConfigResourceFlat) DestroyNode(GraphNodeDestroyMode) GraphNodeDestroy { - // Disable destroy mode for flattened resources since we already - // did this within the original subgraph. - return nil +func (n *GraphNodeConfigResourceFlat) DestroyNode(mode GraphNodeDestroyMode) GraphNodeDestroy { + // Get our parent destroy node. If we don't have any, just return + raw := n.GraphNodeConfigResource.DestroyNode(mode) + if raw == nil { + return nil + } + + node, ok := raw.(*graphNodeResourceDestroy) + if !ok { + panic(fmt.Sprintf("unknown destroy node: %s %T", dag.VertexName(raw), raw)) + } + + // Otherwise, wrap it so that it gets the proper module treatment. + return &graphNodeResourceDestroyFlat{ + graphNodeResourceDestroy: node, + PathValue: n.PathValue, + } +} + +type graphNodeResourceDestroyFlat struct { + *graphNodeResourceDestroy + + PathValue []string +} + +func (n *graphNodeResourceDestroyFlat) Name() string { + return fmt.Sprintf( + "%s.%s", modulePrefixStr(n.PathValue), n.graphNodeResourceDestroy.Name()) +} + +func (n *graphNodeResourceDestroyFlat) Path() []string { + return n.PathValue } // graphNodeResourceDestroy represents the logical destruction of a diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 09c330792..878078668 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -362,6 +362,12 @@ module.child: leader = 1 ` +const testTerraformApplyModuleDestroyOrderStr = ` + +module.child: + +` + const testTerraformApplyMultiProviderStr = ` aws_instance.bar: ID = foo diff --git a/terraform/test-fixtures/apply-module-destroy-order/child/main.tf b/terraform/test-fixtures/apply-module-destroy-order/child/main.tf new file mode 100644 index 000000000..c3a5aecd6 --- /dev/null +++ b/terraform/test-fixtures/apply-module-destroy-order/child/main.tf @@ -0,0 +1,5 @@ +resource "aws_instance" "a" {} + +output "a_output" { + value = "${aws_instance.a.id}" +} diff --git a/terraform/test-fixtures/apply-module-destroy-order/main.tf b/terraform/test-fixtures/apply-module-destroy-order/main.tf new file mode 100644 index 000000000..2a6bcce37 --- /dev/null +++ b/terraform/test-fixtures/apply-module-destroy-order/main.tf @@ -0,0 +1,7 @@ +module "child" { + source = "./child" +} + +resource "aws_instance" "b" { + blah = "${module.child.a_output}" +} diff --git a/terraform/transform_destroy.go b/terraform/transform_destroy.go index 7d4293ef3..b64f930d6 100644 --- a/terraform/transform_destroy.go +++ b/terraform/transform_destroy.go @@ -102,6 +102,14 @@ func (t *DestroyTransformer) transform( // Inherit all the edges from the old node downEdges := g.DownEdges(v).List() for _, edgeRaw := range downEdges { + // Don't inherit proxies. These are currently variables and + // outputs and don't affect destroys. In the future we should + // probably make this more obvious somehow (another interface?). + // Right now I'm not sure how. + if _, ok := edgeRaw.(GraphNodeProxy); ok { + continue + } + g.Connect(dag.BasicEdge(n, edgeRaw.(dag.Vertex))) } @@ -204,15 +212,6 @@ type PruneDestroyTransformer struct { } func (t *PruneDestroyTransformer) Transform(g *Graph) error { - var modDiff *ModuleDiff - var modState *ModuleState - if t.Diff != nil { - modDiff = t.Diff.ModuleByPath(g.Path) - } - if t.State != nil { - modState = t.State.ModuleByPath(g.Path) - } - for _, v := range g.Vertices() { // If it is not a destroyer, we don't care dn, ok := v.(GraphNodeDestroyPrunable) @@ -220,6 +219,20 @@ func (t *PruneDestroyTransformer) Transform(g *Graph) error { continue } + path := g.Path + if pn, ok := v.(GraphNodeSubPath); ok { + path = pn.Path() + } + + var modDiff *ModuleDiff + var modState *ModuleState + if t.Diff != nil { + modDiff = t.Diff.ModuleByPath(path) + } + if t.State != nil { + modState = t.State.ModuleByPath(path) + } + // Remove it if we should if !dn.DestroyInclude(modDiff, modState) { g.Remove(v)