From 7da1a39480ecf8a8d05a7385e0af896d6fd42d1b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 29 Jan 2018 16:16:41 -0500 Subject: [PATCH] always evaluate locals, even during destroy Destroy-time provisioners require us to re-evaluate during destroy. Rather than destroying local values, which doesn't do much since they aren't persisted to state, we always evaluate them regardless of the type of apply. Since the destroy-time local node is no longer a "destroy" operation, the order of evaluation need to be reversed. Take the existing DestroyValueReferenceTransformer and change it to reverse the outgoing edges, rather than in incoming edges. This makes it so that any dependencies of a local or output node are destroyed after evaluation. Having locals evaluated during destroy failed one other test, but that was the odd case where we need `id` to exist as an attribute as well as a field. --- terraform/context_apply_test.go | 56 +++++++++++++++++++ terraform/graph_builder_apply.go | 2 +- terraform/node_local.go | 36 +----------- .../apply-provisioner-destroy-locals/main.tf | 14 +++++ terraform/transform_reference.go | 19 +++---- 5 files changed, 83 insertions(+), 44 deletions(-) create mode 100644 terraform/test-fixtures/apply-provisioner-destroy-locals/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index f620a43d7b..32338ab923 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -7594,6 +7594,58 @@ aws_instance.bar: `) } +func TestContext2Apply_destroyProvisionerWithLocals(t *testing.T) { + m := testModule(t, "apply-provisioner-destroy-locals") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + pr := testProvisioner() + pr.ApplyFn = func(_ *InstanceState, rc *ResourceConfig) error { + cmd, ok := rc.Get("command") + if !ok || cmd != "local" { + fmt.Printf("%#v\n", rc.Config) + return fmt.Errorf("provisioner got %v:%s", ok, cmd) + } + return nil + } + + ctx := testContext2(t, &ContextOpts{ + Module: m, + ProviderResolver: ResourceProviderResolverFixed( + map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + ), + Provisioners: map[string]ResourceProvisionerFactory{ + "shell": testProvisionerFuncFixed(pr), + }, + State: &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.foo": resourceState("aws_instance", "1234"), + }, + }, + }, + }, + Destroy: true, + // the test works without targeting, but this also tests that the local + // node isn't inadvertently pruned because of the wrong evaluation + // order. + Targets: []string{"aws_instance.foo"}, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatal(err) + } + + if _, err := ctx.Apply(); err != nil { + t.Fatal(err) + } +} + func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) { m := testModule(t, "apply-destroy-targeted-count") p := testProvider("aws") @@ -9000,6 +9052,10 @@ func TestContext2Apply_destroyWithLocals(t *testing.T) { Type: "aws_instance", Primary: &InstanceState{ ID: "foo", + // FIXME: id should only exist in one place + Attributes: map[string]string{ + "id": "foo", + }, }, }, }, diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 1f826e1d98..53c24dc3d0 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -119,7 +119,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Connect references so ordering is correct &ReferenceTransformer{}, - // Reverse the edges to outputs and locals, so that + // Reverse the edges from outputs and locals, so that // interpolations don't fail during destroy. GraphTransformIf( func() bool { return b.Destroy }, diff --git a/terraform/node_local.go b/terraform/node_local.go index e58f1f987d..d38722267e 100644 --- a/terraform/node_local.go +++ b/terraform/node_local.go @@ -59,38 +59,8 @@ func (n *NodeLocal) References() []string { // GraphNodeEvalable func (n *NodeLocal) EvalTree() EvalNode { - return &EvalSequence{ - Nodes: []EvalNode{ - &EvalOpFilter{ - Ops: []walkOperation{ - walkInput, - walkValidate, - walkRefresh, - walkPlan, - walkApply, - }, - Node: &EvalSequence{ - Nodes: []EvalNode{ - &EvalLocal{ - Name: n.Config.Name, - Value: n.Config.RawConfig, - }, - }, - }, - }, - &EvalOpFilter{ - Ops: []walkOperation{ - walkPlanDestroy, - walkDestroy, - }, - Node: &EvalSequence{ - Nodes: []EvalNode{ - &EvalDeleteLocal{ - Name: n.Config.Name, - }, - }, - }, - }, - }, + return &EvalLocal{ + Name: n.Config.Name, + Value: n.Config.RawConfig, } } diff --git a/terraform/test-fixtures/apply-provisioner-destroy-locals/main.tf b/terraform/test-fixtures/apply-provisioner-destroy-locals/main.tf new file mode 100644 index 0000000000..5818e7c7d5 --- /dev/null +++ b/terraform/test-fixtures/apply-provisioner-destroy-locals/main.tf @@ -0,0 +1,14 @@ +locals { + value = "local" +} + +resource "aws_instance" "foo" { + provisioner "shell" { + command = "${local.value}" + when = "create" + } + provisioner "shell" { + command = "${local.value}" + when = "destroy" + } +} diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 85a82a6517..5ee5248186 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -77,15 +77,14 @@ func (t *ReferenceTransformer) Transform(g *Graph) error { } // DestroyReferenceTransformer is a GraphTransformer that reverses the edges -// for nodes that depend on an Output or Local value. Output and local nodes are -// removed during destroy, so anything which depends on them must be evaluated -// first. These can't be interpolated during destroy, so the stored value must -// be used anyway hence they don't need to be re-evaluated. +// for locals and outputs that depend on other nodes which will be +// removed during destroy. If a destroy node is evaluated before the local or +// output value, it will be removed from the state, and the later interpolation +// will fail. type DestroyValueReferenceTransformer struct{} func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error { vs := g.Vertices() - for _, v := range vs { switch v.(type) { case *NodeApplyableOutput, *NodeLocal: @@ -94,13 +93,13 @@ func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error { continue } - // reverse any incoming edges so that the value is removed last - for _, e := range g.EdgesTo(v) { - source := e.Source() - log.Printf("[TRACE] output dep: %s", dag.VertexName(source)) + // reverse any outgoing edges so that the value is evaluated first. + for _, e := range g.EdgesFrom(v) { + target := e.Target() + log.Printf("[TRACE] output dep: %s", dag.VertexName(target)) g.RemoveEdge(e) - g.Connect(&DestroyEdge{S: v, T: source}) + g.Connect(&DestroyEdge{S: target, T: v}) } }