From 7da1a39480ecf8a8d05a7385e0af896d6fd42d1b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 29 Jan 2018 16:16:41 -0500 Subject: [PATCH 1/9] 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 f620a43d7b3c..32338ab923e6 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 1f826e1d98cb..53c24dc3d006 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 e58f1f987d1c..d38722267eb2 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 000000000000..5818e7c7d53a --- /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 85a82a6517c2..5ee52481867d 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}) } } From 7ac0a46981234a77c4ef95d230dadf234e353459 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 29 Jan 2018 18:00:18 -0500 Subject: [PATCH 2/9] add destroy provisioner test with locals, outputs Add a complex destroy provisioner testcase using locals, outputs and variables. Add that pesky "id" attribute to the instance states for interpolation. --- terraform/context_apply_test.go | 64 ++++++++++++++++++- terraform/context_test.go | 3 + .../apply-provisioner-destroy-outputs/main.tf | 19 ++++++ .../mod/main.tf | 5 ++ .../mod2/main.tf | 10 +++ 5 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 terraform/test-fixtures/apply-provisioner-destroy-outputs/main.tf create mode 100644 terraform/test-fixtures/apply-provisioner-destroy-outputs/mod/main.tf create mode 100644 terraform/test-fixtures/apply-provisioner-destroy-outputs/mod2/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 32338ab923e6..ebd9ae4aeb8b 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -7623,7 +7623,7 @@ func TestContext2Apply_destroyProvisionerWithLocals(t *testing.T) { State: &State{ Modules: []*ModuleState{ &ModuleState{ - Path: rootModulePath, + Path: []string{"root", "mod"}, Resources: map[string]*ResourceState{ "aws_instance.foo": resourceState("aws_instance", "1234"), }, @@ -7646,6 +7646,68 @@ func TestContext2Apply_destroyProvisionerWithLocals(t *testing.T) { } } +func TestContext2Apply_destroyProvisionerWithOutput(t *testing.T) { + m := testModule(t, "apply-provisioner-destroy-outputs") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + pr := testProvisioner() + pr.ApplyFn = func(is *InstanceState, rc *ResourceConfig) error { + cmd, ok := rc.Get("command") + if !ok || cmd != "3" { + fmt.Printf("%#v\n", rc.Config) + return fmt.Errorf("provisioner for %s got %v:%s", is.ID, 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: []string{"root"}, + Resources: map[string]*ResourceState{ + "aws_instance.foo": resourceState("aws_instance", "1"), + }, + }, + &ModuleState{ + Path: []string{"root", "mod"}, + Resources: map[string]*ResourceState{ + "aws_instance.baz": resourceState("aws_instance", "3"), + }, + // state needs to be properly initialized + Outputs: map[string]*OutputState{}, + }, + &ModuleState{ + Path: []string{"root", "mod2"}, + Resources: map[string]*ResourceState{ + "aws_instance.bar": resourceState("aws_instance", "2"), + }, + }, + }, + }, + Destroy: true, + }) + + 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") diff --git a/terraform/context_test.go b/terraform/context_test.go index 7ef9245d7388..36ea61e89982 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -375,6 +375,9 @@ func resourceState(resourceType, resourceID string) *ResourceState { Type: resourceType, Primary: &InstanceState{ ID: resourceID, + Attributes: map[string]string{ + "id": resourceID, + }, }, } } diff --git a/terraform/test-fixtures/apply-provisioner-destroy-outputs/main.tf b/terraform/test-fixtures/apply-provisioner-destroy-outputs/main.tf new file mode 100644 index 000000000000..bb4e01db9ab2 --- /dev/null +++ b/terraform/test-fixtures/apply-provisioner-destroy-outputs/main.tf @@ -0,0 +1,19 @@ +module "mod" { + source = "./mod" +} + +locals { + value = "${module.mod.value}" +} + +resource "aws_instance" "foo" { + provisioner "shell" { + command = "${local.value}" + when = "destroy" + } +} + +module "mod2" { + source = "./mod2" + value = "${module.mod.value}" +} diff --git a/terraform/test-fixtures/apply-provisioner-destroy-outputs/mod/main.tf b/terraform/test-fixtures/apply-provisioner-destroy-outputs/mod/main.tf new file mode 100644 index 000000000000..1f4ad09c5449 --- /dev/null +++ b/terraform/test-fixtures/apply-provisioner-destroy-outputs/mod/main.tf @@ -0,0 +1,5 @@ +output "value" { + value = "${aws_instance.baz.id}" +} + +resource "aws_instance" "baz" {} diff --git a/terraform/test-fixtures/apply-provisioner-destroy-outputs/mod2/main.tf b/terraform/test-fixtures/apply-provisioner-destroy-outputs/mod2/main.tf new file mode 100644 index 000000000000..a476bb9207c1 --- /dev/null +++ b/terraform/test-fixtures/apply-provisioner-destroy-outputs/mod2/main.tf @@ -0,0 +1,10 @@ +variable "value" { +} + +resource "aws_instance" "bar" { + provisioner "shell" { + command = "${var.value}" + when = "destroy" + } +} + From 08139557f85cc24fb50849565b0814e4bcb3f6d2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 29 Jan 2018 18:02:59 -0500 Subject: [PATCH 3/9] always evaluate outputs too Always evaluate outputs during destroy, just like we did for locals. This breaks existing tests, which we will handle separately. Don't reverse output/local node evaluation order during destroy, as they are both being evaluated. --- terraform/node_output.go | 8 +------- terraform/transform_reference.go | 5 +++++ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/terraform/node_output.go b/terraform/node_output.go index 4fd246a10c70..191de839c634 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -83,19 +83,13 @@ func (n *NodeApplyableOutput) EvalTree() EvalNode { }, }, &EvalOpFilter{ - Ops: []walkOperation{walkRefresh, walkPlan, walkApply, walkValidate}, + Ops: []walkOperation{walkRefresh, walkPlan, walkApply, walkValidate, walkDestroy, walkPlanDestroy}, Node: &EvalWriteOutput{ Name: n.Config.Name, Sensitive: n.Config.Sensitive, Value: n.Config.RawConfig, }, }, - &EvalOpFilter{ - Ops: []walkOperation{walkDestroy, walkPlanDestroy}, - Node: &EvalDeleteOutput{ - Name: n.Config.Name, - }, - }, }, } } diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 5ee52481867d..b1c10d00c0bc 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -96,6 +96,11 @@ func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error { // reverse any outgoing edges so that the value is evaluated first. for _, e := range g.EdgesFrom(v) { target := e.Target() + switch target.(type) { + case *NodeApplyableOutput, *NodeLocal: + // don't reverse other values + continue + } log.Printf("[TRACE] output dep: %s", dag.VertexName(target)) g.RemoveEdge(e) From d31fe5ab9dc79d69d1ab41f92f50f73724c7caea Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 29 Jan 2018 19:17:31 -0500 Subject: [PATCH 4/9] delete outputs during destroy Now that outputs are always evaluated, we still need a way to remove them from state when they are destroyed. Previously, outputs were removed during destroy from the same "Applyable" node type that evaluates them. Now that we need to possibly both evaluate and remove output during an apply, we add a new node - NodeDestroyableOutput. This new node is added to the graph by the DestroyOutputTransformer, which make the new destroy node depend on all descendants of the output node. This ensures that the output remains in the state as long as everything which may interpolate the output still exists. --- terraform/context_apply_test.go | 14 ++------- terraform/graph_builder_apply.go | 5 +++ terraform/node_output.go | 52 ++++++++++++++++++++++++++++++++ terraform/transform_output.go | 46 +++++++++++++++++++++++++--- terraform/transform_reference.go | 7 +++-- 5 files changed, 104 insertions(+), 20 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index ebd9ae4aeb8b..e4d40fb6dcfc 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2590,24 +2590,14 @@ func TestContext2Apply_moduleDestroyOrder(t *testing.T) { &ModuleState{ Path: rootModulePath, Resources: map[string]*ResourceState{ - "aws_instance.b": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "b", - }, - }, + "aws_instance.b": resourceState("aws_instance", "b"), }, }, &ModuleState{ Path: []string{"root", "child"}, Resources: map[string]*ResourceState{ - "aws_instance.a": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "a", - }, - }, + "aws_instance.a": resourceState("aws_instance", "a"), }, Outputs: map[string]*OutputState{ "a_output": &OutputState{ diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 53c24dc3d006..62dc2d2755d5 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -126,6 +126,11 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { &DestroyValueReferenceTransformer{}, ), + GraphTransformIf( + func() bool { return b.Destroy }, + &DestroyOutputTransformer{}, + ), + // Add the node to fix the state count boundaries &CountBoundaryTransformer{}, diff --git a/terraform/node_output.go b/terraform/node_output.go index 191de839c634..be30911cf810 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -93,3 +93,55 @@ func (n *NodeApplyableOutput) EvalTree() EvalNode { }, } } + +// NodeDestroyableOutput represents an output that is "destroybale": +// its application will remove the output from the state. +type NodeDestroyableOutput struct { + PathValue []string + Config *config.Output // Config is the output in the config +} + +func (n *NodeDestroyableOutput) Name() string { + result := fmt.Sprintf("output.%s (destroy)", n.Config.Name) + if len(n.PathValue) > 1 { + result = fmt.Sprintf("%s.%s", modulePrefixStr(n.PathValue), result) + } + + return result +} + +// GraphNodeSubPath +func (n *NodeDestroyableOutput) Path() []string { + return n.PathValue +} + +// RemovableIfNotTargeted +func (n *NodeDestroyableOutput) RemoveIfNotTargeted() bool { + // We need to add this so that this node will be removed if + // it isn't targeted or a dependency of a target. + return true +} + +// GraphNodeReferencer +func (n *NodeDestroyableOutput) References() []string { + var result []string + result = append(result, n.Config.DependsOn...) + result = append(result, ReferencesFromConfig(n.Config.RawConfig)...) + for _, v := range result { + split := strings.Split(v, "/") + for i, s := range split { + split[i] = s + ".destroy" + } + + result = append(result, strings.Join(split, "/")) + } + + return result +} + +// GraphNodeEvalable +func (n *NodeDestroyableOutput) EvalTree() EvalNode { + return &EvalDeleteOutput{ + Name: n.Config.Name, + } +} diff --git a/terraform/transform_output.go b/terraform/transform_output.go index b260f4caa1bd..faa25e412cd9 100644 --- a/terraform/transform_output.go +++ b/terraform/transform_output.go @@ -1,7 +1,10 @@ package terraform import ( + "log" + "github.com/hashicorp/terraform/config/module" + "github.com/hashicorp/terraform/dag" ) // OutputTransformer is a GraphTransformer that adds all the outputs @@ -41,11 +44,6 @@ func (t *OutputTransformer) transform(g *Graph, m *module.Tree) error { // Add all outputs here for _, o := range os { - // Build the node. - // - // NOTE: For now this is just an "applyable" output. As we build - // new graph builders for the other operations I suspect we'll - // find a way to parameterize this, require new transforms, etc. node := &NodeApplyableOutput{ PathValue: normalizeModulePath(m.Path()), Config: o, @@ -57,3 +55,41 @@ func (t *OutputTransformer) transform(g *Graph, m *module.Tree) error { return nil } + +// DestroyOutputTransformer is a GraphTransformer that adds nodes to delete +// outputs during destroy. We need to do this to ensure that no stale outputs +// are ever left in the state. +type DestroyOutputTransformer struct { +} + +func (t *DestroyOutputTransformer) Transform(g *Graph) error { + for _, v := range g.Vertices() { + output, ok := v.(*NodeApplyableOutput) + if !ok { + continue + } + + // create the destroy node for this output + node := &NodeDestroyableOutput{ + PathValue: output.PathValue, + Config: output.Config, + } + + log.Printf("[TRACE] creating %s", node.Name()) + g.Add(node) + + deps, err := g.Descendents(v) + if err != nil { + return err + } + + // the destroy node must depend on the eval node + deps.Add(v) + + for _, d := range deps.List() { + log.Printf("[TRACE] %s depends on %s", node.Name(), dag.VertexName(d)) + g.Connect(dag.BasicEdge(node, d)) + } + } + return nil +} diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index b1c10d00c0bc..fa4f999890a2 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -96,11 +96,12 @@ func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error { // reverse any outgoing edges so that the value is evaluated first. for _, e := range g.EdgesFrom(v) { target := e.Target() - switch target.(type) { - case *NodeApplyableOutput, *NodeLocal: - // don't reverse other values + + // only destroy nodes will be evaluated in reverse + if _, ok := target.(GraphNodeDestroyer); !ok { continue } + log.Printf("[TRACE] output dep: %s", dag.VertexName(target)) g.RemoveEdge(e) From 2d138d99179967fce2b2393134c94a7084578dab Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 30 Jan 2018 10:05:41 -0500 Subject: [PATCH 5/9] add a more complex locals test Using destroy provisioners again for edge cases during destroy. --- terraform/context_apply_test.go | 75 ++++++++++++++++++- .../main.tf | 24 ++++++ 2 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 terraform/test-fixtures/apply-provisioner-destroy-multiple-locals/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index e4d40fb6dcfc..61a9ea81a26d 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2,6 +2,7 @@ package terraform import ( "bytes" + "errors" "fmt" "reflect" "runtime" @@ -7613,7 +7614,7 @@ func TestContext2Apply_destroyProvisionerWithLocals(t *testing.T) { State: &State{ Modules: []*ModuleState{ &ModuleState{ - Path: []string{"root", "mod"}, + Path: []string{"root"}, Resources: map[string]*ResourceState{ "aws_instance.foo": resourceState("aws_instance", "1234"), }, @@ -7634,6 +7635,75 @@ func TestContext2Apply_destroyProvisionerWithLocals(t *testing.T) { if _, err := ctx.Apply(); err != nil { t.Fatal(err) } + + if !pr.ApplyCalled { + t.Fatal("provisioner not called") + } +} + +func TestContext2Apply_destroyProvisionerWithMultipleLocals(t *testing.T) { + m := testModule(t, "apply-provisioner-destroy-multiple-locals") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + pr := testProvisioner() + pr.ApplyFn = func(is *InstanceState, rc *ResourceConfig) error { + cmd, ok := rc.Get("command") + if !ok { + return errors.New("no command in provisioner") + } + + switch is.ID { + case "1234": + if cmd != "local" { + return fmt.Errorf("provisioner %q got:%q", is.ID, cmd) + } + case "3456": + if cmd != "1234" { + return fmt.Errorf("provisioner %q got:%q", is.ID, cmd) + } + default: + t.Fatal("unknown instance") + } + 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: []string{"root"}, + Resources: map[string]*ResourceState{ + "aws_instance.foo": resourceState("aws_instance", "1234"), + "aws_instance.bar": resourceState("aws_instance", "3456"), + }, + }, + }, + }, + Destroy: true, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatal(err) + } + + if _, err := ctx.Apply(); err != nil { + t.Fatal(err) + } + + if !pr.ApplyCalled { + t.Fatal("provisioner not called") + } } func TestContext2Apply_destroyProvisionerWithOutput(t *testing.T) { @@ -7696,6 +7766,9 @@ func TestContext2Apply_destroyProvisionerWithOutput(t *testing.T) { if _, err := ctx.Apply(); err != nil { t.Fatal(err) } + if !pr.ApplyCalled { + t.Fatal("provisioner not called") + } } func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) { diff --git a/terraform/test-fixtures/apply-provisioner-destroy-multiple-locals/main.tf b/terraform/test-fixtures/apply-provisioner-destroy-multiple-locals/main.tf new file mode 100644 index 000000000000..437a6e7273de --- /dev/null +++ b/terraform/test-fixtures/apply-provisioner-destroy-multiple-locals/main.tf @@ -0,0 +1,24 @@ +locals { + value = "local" + foo_id = "${aws_instance.foo.id}" + + // baz is not in the state during destroy, but this is a valid config that + // should not fail. + baz_id = "${aws_instance.baz.id}" +} + +resource "aws_instance" "baz" {} + +resource "aws_instance" "foo" { + provisioner "shell" { + command = "${local.value}" + when = "destroy" + } +} + +resource "aws_instance" "bar" { + provisioner "shell" { + command = "${local.foo_id}" + when = "destroy" + } +} From 99867f0082f30fa19c5c5aefcbbfad66b9e38123 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 30 Jan 2018 10:24:15 -0500 Subject: [PATCH 6/9] add PruneUnusedValuesTransformer Since outputs and local nodes are always evaluated, if the reference a resource form the configuration that isn't in the state, the interpolation could fail. Prune any local or output values that have no references in the graph. --- terraform/context_apply_test.go | 2 ++ terraform/graph_builder_apply.go | 15 +++++++++------ terraform/transform_reference.go | 24 ++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 61a9ea81a26d..95ac647b289e 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -7641,6 +7641,8 @@ func TestContext2Apply_destroyProvisionerWithLocals(t *testing.T) { } } +// this also tests a local value in the config referencing a resource that +// wasn't in the state during destroy. func TestContext2Apply_destroyProvisionerWithMultipleLocals(t *testing.T) { m := testModule(t, "apply-provisioner-destroy-multiple-locals") p := testProvider("aws") diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 62dc2d2755d5..0c2b2332f4c9 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -119,16 +119,19 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Connect references so ordering is correct &ReferenceTransformer{}, + // Handle destroy time transformations for output and local values. // Reverse the edges from outputs and locals, so that // interpolations don't fail during destroy. + // Create a destroy node for outputs to remove them from the state. + // Prune unreferenced values, which may have interpolations that can't + // be resolved. GraphTransformIf( func() bool { return b.Destroy }, - &DestroyValueReferenceTransformer{}, - ), - - GraphTransformIf( - func() bool { return b.Destroy }, - &DestroyOutputTransformer{}, + GraphTransformMulti( + &DestroyValueReferenceTransformer{}, + &DestroyOutputTransformer{}, + &PruneUnusedValuesTransformer{}, + ), ), // Add the node to fix the state count boundaries diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index fa4f999890a2..403b7e424637 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -112,6 +112,30 @@ func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error { return nil } +// PruneUnusedValuesTransformer is s GraphTransformer that removes local and +// output values which are not referenced in the graph. Since outputs and +// locals always need to be evaluated, if they reference a resource that is not +// available in the state the interpolation could fail. +type PruneUnusedValuesTransformer struct{} + +func (t *PruneUnusedValuesTransformer) Transform(g *Graph) error { + vs := g.Vertices() + for _, v := range vs { + switch v.(type) { + case *NodeApplyableOutput, *NodeLocal: + // OK + default: + continue + } + + if len(g.EdgesTo(v)) == 0 { + g.Remove(v) + } + } + + return nil +} + // ReferenceMap is a structure that can be used to efficiently check // for references on a graph. type ReferenceMap struct { From a2f8482333ea99df923dd4795c4a95bd68f5baaa Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 30 Jan 2018 10:42:49 -0500 Subject: [PATCH 7/9] catch missing id attribute during interpolation The id attribute can be missing during the destroy operation. While the new destroy-time ordering of outputs and locals should prevent resources from having their id attributes set to an empty string, there's no reason to error out if we have the canonical ID field available. This still interrogates the attributes map first to retain any previous behavior, but in the future we should settle on a single ID location. --- terraform/interpolate.go | 15 ++++++++++++++ terraform/interpolate_test.go | 38 +++++++++++++++++++++++++++++++++-- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/terraform/interpolate.go b/terraform/interpolate.go index 456e7e3a23c7..1509a65fe0c0 100644 --- a/terraform/interpolate.go +++ b/terraform/interpolate.go @@ -518,6 +518,16 @@ func (i *Interpolater) computeResourceVariable( return &v, err } + // special case for the "id" field which is usually also an attribute + if v.Field == "id" && r.Primary.ID != "" { + // This is usually pulled from the attributes, but is sometimes missing + // during destroy. We can return the ID field in this case. + // FIXME: there should only be one ID to rule them all. + log.Printf("[WARN] resource %s missing 'id' attribute", v.ResourceId()) + v, err := hil.InterfaceToVariable(r.Primary.ID) + return &v, err + } + // computed list or map attribute _, isList = r.Primary.Attributes[v.Field+".#"] _, isMap = r.Primary.Attributes[v.Field+".%"] @@ -655,6 +665,11 @@ func (i *Interpolater) computeResourceMultiVariable( continue } + if v.Field == "id" && r.Primary.ID != "" { + log.Printf("[WARN] resource %s missing 'id' attribute", v.ResourceId()) + values = append(values, r.Primary.ID) + } + // computed list or map attribute _, isList := r.Primary.Attributes[v.Field+".#"] _, isMap := r.Primary.Attributes[v.Field+".%"] diff --git a/terraform/interpolate_test.go b/terraform/interpolate_test.go index c497b43b8e01..10f23a2788a4 100644 --- a/terraform/interpolate_test.go +++ b/terraform/interpolate_test.go @@ -129,6 +129,40 @@ func TestInterpolater_localVal(t *testing.T) { }) } +func TestInterpolater_missingID(t *testing.T) { + lock := new(sync.RWMutex) + state := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.web": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + }, + }, + }, + }, + }, + } + + i := &Interpolater{ + Module: testModule(t, "interpolate-resource-variable"), + State: state, + StateLock: lock, + } + + scope := &InterpolationScope{ + Path: rootModulePath, + } + + testInterpolate(t, i, scope, "aws_instance.web.id", ast.Variable{ + Value: "bar", + Type: ast.TypeString, + }) +} + func TestInterpolater_pathCwd(t *testing.T) { i := &Interpolater{} scope := &InterpolationScope{} @@ -314,8 +348,8 @@ func TestInterpolater_resourceVariableMissingDuringInput(t *testing.T) { &ModuleState{ Path: rootModulePath, Resources: map[string]*ResourceState{ - // No resources at all yet, because we're still dealing - // with input and so the resources haven't been created. + // No resources at all yet, because we're still dealing + // with input and so the resources haven't been created. }, }, }, From ca4178b9ec9a0405385d2c2ab6f829539b5d7434 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 30 Jan 2018 11:16:42 -0500 Subject: [PATCH 8/9] gofmt bug will be fixed in 1.10 --- terraform/interpolate_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/terraform/interpolate_test.go b/terraform/interpolate_test.go index 10f23a2788a4..00aec367da97 100644 --- a/terraform/interpolate_test.go +++ b/terraform/interpolate_test.go @@ -348,8 +348,8 @@ func TestInterpolater_resourceVariableMissingDuringInput(t *testing.T) { &ModuleState{ Path: rootModulePath, Resources: map[string]*ResourceState{ - // No resources at all yet, because we're still dealing - // with input and so the resources haven't been created. + // No resources at all yet, because we're still dealing + // with input and so the resources haven't been created. }, }, }, From 7fbc35a36c6da6e5b981c361f153fe2be17bb274 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Jan 2018 13:51:40 -0500 Subject: [PATCH 9/9] Make sure outputs are removed when targeting Similar to NodeApplyableOuptut, NodeDestroyableOutputs also need to stay in the graph if any ancestor nodes Use the same GraphNodeTargetDownstream method to keep them from being pruned, since they are dependent on the output node and all its descendants. --- terraform/context_apply_test.go | 20 ++++++++++++++++++- terraform/node_output.go | 6 ++++++ .../apply-provisioner-destroy-outputs/main.tf | 4 ++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 95ac647b289e..28e42b02e893 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -7741,6 +7741,12 @@ func TestContext2Apply_destroyProvisionerWithOutput(t *testing.T) { Resources: map[string]*ResourceState{ "aws_instance.foo": resourceState("aws_instance", "1"), }, + Outputs: map[string]*OutputState{ + "value": { + Type: "string", + Value: "3", + }, + }, }, &ModuleState{ Path: []string{"root", "mod"}, @@ -7759,18 +7765,30 @@ func TestContext2Apply_destroyProvisionerWithOutput(t *testing.T) { }, }, Destroy: true, + + // targeting the source of the value used by all resources shoudl still + // destroy them all. + Targets: []string{"module.mod.aws_instance.baz"}, }) if _, err := ctx.Plan(); err != nil { t.Fatal(err) } - if _, err := ctx.Apply(); err != nil { + state, err := ctx.Apply() + if err != nil { t.Fatal(err) } if !pr.ApplyCalled { t.Fatal("provisioner not called") } + + // confirm all outputs were removed too + for _, mod := range state.Modules { + if len(mod.Outputs) > 0 { + t.Fatalf("output left in module state: %#v\n", mod) + } + } } func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) { diff --git a/terraform/node_output.go b/terraform/node_output.go index be30911cf810..83e9925a1569 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -122,6 +122,12 @@ func (n *NodeDestroyableOutput) RemoveIfNotTargeted() bool { return true } +// This will keep the destroy node in the graph if its corresponding output +// node is also in the destroy graph. +func (n *NodeDestroyableOutput) TargetDownstream(targetedDeps, untargetedDeps *dag.Set) bool { + return true +} + // GraphNodeReferencer func (n *NodeDestroyableOutput) References() []string { var result []string diff --git a/terraform/test-fixtures/apply-provisioner-destroy-outputs/main.tf b/terraform/test-fixtures/apply-provisioner-destroy-outputs/main.tf index bb4e01db9ab2..9a5843aa3cc9 100644 --- a/terraform/test-fixtures/apply-provisioner-destroy-outputs/main.tf +++ b/terraform/test-fixtures/apply-provisioner-destroy-outputs/main.tf @@ -17,3 +17,7 @@ module "mod2" { source = "./mod2" value = "${module.mod.value}" } + +output "value" { + value = "${local.value}" +}