diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index f620a43d7b3c..28e42b02e893 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" @@ -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", + }, }, }, }, 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/graph_builder_apply.go b/terraform/graph_builder_apply.go index 1f826e1d98cb..0c2b2332f4c9 100644 --- a/terraform/graph_builder_apply.go +++ b/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 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..00aec367da97 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{} 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/node_output.go b/terraform/node_output.go index 4fd246a10c70..83e9925a1569 100644 --- a/terraform/node_output.go +++ b/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, + } +} 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/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" + } +} 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..9a5843aa3cc9 --- /dev/null +++ b/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}" +} 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" + } +} + 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 85a82a6517c2..403b7e424637 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,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) } }