From 9d8ab5565841912873c22e78e7b0c176bfbd0c3c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 28 Sep 2017 11:06:37 -0400 Subject: [PATCH 1/3] Add failing test for destroy with locals --- terraform/context_apply_test.go | 47 +++++++++++++++++++ .../apply-destroy-with-locals/main.tf | 8 ++++ 2 files changed, 55 insertions(+) create mode 100644 terraform/test-fixtures/apply-destroy-with-locals/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index fc45c6ac03..32ce763918 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -8809,3 +8809,50 @@ module.child: t.Fatalf("wrong final state\ngot:\n%s\nwant:\n%s", got, want) } } + +func TestContext2Apply_destroyWithLocals(t *testing.T) { + m := testModule(t, "apply-destroy-with-locals") + ctx := testContext2(t, &ContextOpts{ + Module: m, + ProviderResolver: ResourceProviderResolverFixed( + map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(testProvider("aws")), + }, + ), + State: &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Outputs: map[string]*OutputState{ + "name": &OutputState{ + Type: "string", + Value: "test-bar", + }, + }, + Resources: map[string]*ResourceState{ + "aws_instance.foo": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "foo", + }, + }, + }, + }, + }, + }, + Destroy: true, + }) + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("error during apply: %s", err) + } + + got := strings.TrimSpace(state.String()) + want := strings.TrimSpace(` + TODO +`) + if got != want { + t.Fatalf("wrong final state\ngot:\n%s\nwant:\n%s", got, want) + } +} diff --git a/terraform/test-fixtures/apply-destroy-with-locals/main.tf b/terraform/test-fixtures/apply-destroy-with-locals/main.tf new file mode 100644 index 0000000000..1ab7518715 --- /dev/null +++ b/terraform/test-fixtures/apply-destroy-with-locals/main.tf @@ -0,0 +1,8 @@ +locals { + name = "test-${aws_instance.foo.id}" +} +resource "aws_instance" "foo" {} + +output "name" { + value = "${local.name}" +} From 77396107c4363227bb581d698a79300deaebc004 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 28 Sep 2017 12:43:18 -0400 Subject: [PATCH 2/3] don't evaluate locals during destroy Locals don't need to be evaluated during destroy. Rather than simply skipping them, remove them from the state as they are encountered. Even though they are not persisted in the state, it keeps the state up to date as the destroy happens, and we reduce the chance of other inconstancies later on. --- terraform/context_apply_test.go | 62 ++++++++++++++++++++------------- terraform/eval_local.go | 28 +++++++++++++++ terraform/node_local.go | 44 +++++++++++++++-------- 3 files changed, 95 insertions(+), 39 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 32ce763918..c5bc754b67 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -8812,46 +8812,58 @@ module.child: func TestContext2Apply_destroyWithLocals(t *testing.T) { m := testModule(t, "apply-destroy-with-locals") - ctx := testContext2(t, &ContextOpts{ - Module: m, - ProviderResolver: ResourceProviderResolverFixed( - map[string]ResourceProviderFactory{ - "aws": testProviderFuncFixed(testProvider("aws")), - }, - ), - State: &State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Outputs: map[string]*OutputState{ - "name": &OutputState{ - Type: "string", - Value: "test-bar", - }, + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = func(info *InstanceInfo, s *InstanceState, c *ResourceConfig) (*InstanceDiff, error) { + d, err := testDiffFn(info, s, c) + fmt.Println("DIFF:", d) + return d, err + } + + s := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Outputs: map[string]*OutputState{ + "name": &OutputState{ + Type: "string", + Value: "test-bar", }, - Resources: map[string]*ResourceState{ - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "foo", - }, + }, + Resources: map[string]*ResourceState{ + "aws_instance.foo": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "foo", }, }, }, }, }, + } + + ctx := testContext2(t, &ContextOpts{ + Module: m, + ProviderResolver: ResourceProviderResolverFixed( + map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + ), + State: s, Destroy: true, }) + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + state, err := ctx.Apply() if err != nil { t.Fatalf("error during apply: %s", err) } got := strings.TrimSpace(state.String()) - want := strings.TrimSpace(` - TODO -`) + want := strings.TrimSpace(``) if got != want { t.Fatalf("wrong final state\ngot:\n%s\nwant:\n%s", got, want) } diff --git a/terraform/eval_local.go b/terraform/eval_local.go index 1b63bf4f4f..a4b2a50598 100644 --- a/terraform/eval_local.go +++ b/terraform/eval_local.go @@ -56,3 +56,31 @@ func (n *EvalLocal) Eval(ctx EvalContext) (interface{}, error) { return nil, nil } + +// EvalDeleteLocal is an EvalNode implementation that deletes a Local value +// from the state. Locals aren't persisted, but we don't need to evaluate them +// during destroy. +type EvalDeleteLocal struct { + Name string +} + +func (n *EvalDeleteLocal) Eval(ctx EvalContext) (interface{}, error) { + state, lock := ctx.State() + if state == nil { + return nil, nil + } + + // Get a write lock so we can access this instance + lock.Lock() + defer lock.Unlock() + + // Look for the module state. If we don't have one, create it. + mod := state.ModuleByPath(ctx.Path()) + if mod == nil { + return nil, nil + } + + delete(mod.Locals, n.Name) + + return nil, nil +} diff --git a/terraform/node_local.go b/terraform/node_local.go index da1564e390..e58f1f987d 100644 --- a/terraform/node_local.go +++ b/terraform/node_local.go @@ -59,20 +59,36 @@ func (n *NodeLocal) References() []string { // GraphNodeEvalable func (n *NodeLocal) EvalTree() EvalNode { - return &EvalOpFilter{ - Ops: []walkOperation{ - walkInput, - walkValidate, - walkRefresh, - walkPlan, - walkApply, - walkDestroy, - }, - Node: &EvalSequence{ - Nodes: []EvalNode{ - &EvalLocal{ - Name: n.Config.Name, - Value: n.Config.RawConfig, + 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, + }, + }, }, }, }, From 061597304cdcf95c200e9fb53f9392676134da6e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 28 Sep 2017 15:09:42 -0400 Subject: [PATCH 3/3] add test for destroying with locals in a provider Verify that locals aren't removed from the state before providers are evaluated during destroy. --- terraform/context_apply_test.go | 48 +++++++++++++++++++ .../provider-with-locals/main.tf | 11 +++++ 2 files changed, 59 insertions(+) create mode 100644 terraform/test-fixtures/provider-with-locals/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index c5bc754b67..c22e87a1c6 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -8868,3 +8868,51 @@ func TestContext2Apply_destroyWithLocals(t *testing.T) { t.Fatalf("wrong final state\ngot:\n%s\nwant:\n%s", got, want) } } + +func TestContext2Apply_providerWithLocals(t *testing.T) { + m := testModule(t, "provider-with-locals") + p := testProvider("aws") + p.DiffFn = testDiffFn + p.ApplyFn = testApplyFn + ctx := testContext2(t, &ContextOpts{ + Module: m, + ProviderResolver: ResourceProviderResolverFixed( + map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + ), + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + ctx = testContext2(t, &ContextOpts{ + Module: m, + ProviderResolver: ResourceProviderResolverFixed( + map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + ), + State: state, + Destroy: true, + }) + + if _, err = ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + state, err = ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + if state.HasResources() { + t.Fatal("expected no state, got:", state) + } +} diff --git a/terraform/test-fixtures/provider-with-locals/main.tf b/terraform/test-fixtures/provider-with-locals/main.tf new file mode 100644 index 0000000000..44b6d46b37 --- /dev/null +++ b/terraform/test-fixtures/provider-with-locals/main.tf @@ -0,0 +1,11 @@ +provider "aws" { + alias = "${local.foo}" +} + +locals { + foo = "bar" +} + +resource "aws_instance" "foo" { + value = "${local.foo}" +}