Merge pull request #17241 from hashicorp/jbardin/destroy-with-locals

Fix destroy-time handling of outputs and local values

terraform/context_apply_test.go

@@ -2,6 +2,7 @@ package terraform
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"reflect"
 	"runtime"
@@ -2590,24 +2591,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{
@@ -7594,6 +7585,212 @@ 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: []string{"root"},
+					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)
+	}
+
+	if !pr.ApplyCalled {
+		t.Fatal("provisioner not called")
+	}
+}
+
+// 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")
+	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) {
+	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"),
+					},
+					Outputs: map[string]*OutputState{
+						"value": {
+							Type:  "string",
+							Value: "3",
+						},
+					},
+				},
+				&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,
+
+		// 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)
+	}
+
+	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) {
 	m := testModule(t, "apply-destroy-targeted-count")
 	p := testProvider("aws")
@@ -9000,6 +9197,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",
+							},
 						},
 					},
 				},

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,
+			},
 		},
 	}
 }

terraform/graph_builder_apply.go

@@ -119,11 +119,19 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 		// Connect references so ordering is correct
 		&ReferenceTransformer{},
 
-		// Reverse the edges to outputs and locals, so that
+		// 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{},
+			GraphTransformMulti(
+				&DestroyValueReferenceTransformer{},
+				&DestroyOutputTransformer{},
+				&PruneUnusedValuesTransformer{},
+			),
 		),
 
 		// Add the node to fix the state count boundaries

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+".%"]

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{}

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,
 	}
 }

terraform/node_output.go

@@ -83,19 +83,71 @@ 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,
-				},
-			},
 		},
 	}
 }
+
+// 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
+}
+
+// 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
+	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,
+	}
+}

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"
+    }
+}

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"
+    }
+}

terraform/test-fixtures/apply-provisioner-destroy-outputs/main.tf

@@ -0,0 +1,23 @@
+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}"
+}
+
+output "value" {
+  value = "${local.value}"
+}

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" {}

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"
+    }
+}
+

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
+}

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,43 @@ 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()
+
+			// 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)
-			g.Connect(&DestroyEdge{S: v, T: source})
+			g.Connect(&DestroyEdge{S: target, T: v})
+		}
+	}
+
+	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)
 		}
 	}