Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix destroy-time handling of outputs and local values #17241

Merged
merged 9 commits into from
Jan 31, 2018
225 changes: 213 additions & 12 deletions terraform/context_apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package terraform

import (
"bytes"
"errors"
"fmt"
"reflect"
"runtime"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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",
},
},
},
},
Expand Down
3 changes: 3 additions & 0 deletions terraform/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,9 @@ func resourceState(resourceType, resourceID string) *ResourceState {
Type: resourceType,
Primary: &InstanceState{
ID: resourceID,
Attributes: map[string]string{
"id": resourceID,
},
},
}
}
Expand Down
12 changes: 10 additions & 2 deletions terraform/graph_builder_apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions terraform/interpolate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that can really happen in practice, or was this more just a result of us tending to be lazy about fully-constructing state in tests?

Not objecting to doing it to be safe in the mean time, but just curious as to what tripped this up and prompted you to add the branch here.

(The new provider protocols and state format no longer have this special ID field, as you know, so an end to this odd special-case is not too far in the future but of course we'll need to watch out during that change for situations like this were we're doing something special to keep the two aligned.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it's a destroy-time issue, and we do run into it quite often. I'm actually not sure what order of operations through helper/schema get us in this situation, but as you said it is hopefully going away so this would only be temporary. I added the check now because the extra local/output evaluations during destroy are much more likely to encounter the situation.

// 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+".%"]
Expand Down Expand Up @@ -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+".%"]
Expand Down
34 changes: 34 additions & 0 deletions terraform/interpolate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down
Loading