From 14d079f91456d7cb15de39ab79a6786083e4684f Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 10 Dec 2016 20:11:24 -0500 Subject: [PATCH 1/2] terraform: destroy resources in dependent providers first Fixes #4645 This is something that never worked (even in legacy graphs), but as we push forward towards encouraging multi-provider usage especially with things like the Vault data source, I want to make sure we have this right for 0.8. When you have a config like this: ``` resource "foo_type" "name" {} provider "bar" { attr = "${foo_type.name.value}" } resource "bar_type" "name" {} ``` Then the destruction ordering MUST be: 1. `bar_type` 2. `foo_type` Since configuring the client for `bar_type` requires accessing data from `foo_type`. Prior to this PR, these two would be done in parallel. This properly pushes forward the dependency. There are more cases I want to test but this is a basic case that is fixed. --- terraform/context_apply_test.go | 96 +++++++++++++++++++ terraform/graph_builder_apply.go | 14 +-- .../apply-multi-provider-destroy/main.tf | 9 ++ terraform/transform_destroy_edge.go | 22 ++++- terraform/transform_provider.go | 14 ++- 5 files changed, 140 insertions(+), 15 deletions(-) create mode 100644 terraform/test-fixtures/apply-multi-provider-destroy/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 6458b82d7..3988c58e6 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2747,6 +2747,102 @@ func TestContext2Apply_multiProvider(t *testing.T) { } } +func TestContext2Apply_multiProviderDestroy(t *testing.T) { + m := testModule(t, "apply-multi-provider-destroy") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + p2 := testProvider("do") + p2.ApplyFn = testApplyFn + p2.DiffFn = testDiffFn + + var state *State + + // First, create the instances + { + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + "vault": testProviderFuncFixed(p2), + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + s, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + state = s + } + + // Destroy them + { + // Verify that aws_instance.bar is destroyed first + var checked bool + var called int32 + var lock sync.Mutex + applyFn := func( + info *InstanceInfo, + is *InstanceState, + id *InstanceDiff) (*InstanceState, error) { + lock.Lock() + defer lock.Unlock() + + if info.HumanId() == "aws_instance.bar" { + checked = true + + // Sleep to allow parallel execution + time.Sleep(50 * time.Millisecond) + + // Verify that called is 0 (dep not called) + if atomic.LoadInt32(&called) != 0 { + return nil, fmt.Errorf("nothing else should be called") + } + } + + atomic.AddInt32(&called, 1) + return testApplyFn(info, is, id) + } + + // Set the apply functions + p.ApplyFn = applyFn + p2.ApplyFn = applyFn + + ctx := testContext2(t, &ContextOpts{ + Destroy: true, + State: state, + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + "vault": testProviderFuncFixed(p2), + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + s, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + if !checked { + t.Fatal("should be checked") + } + + state = s + } + + checkStateString(t, state, ``) +} + func TestContext2Apply_multiVar(t *testing.T) { m := testModule(t, "apply-multi-var") p := testProvider("aws") diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index e268204c4..75de53757 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -81,13 +81,6 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Attach the state &AttachStateTransformer{State: b.State}, - // Destruction ordering - &DestroyEdgeTransformer{Module: b.Module, State: b.State}, - GraphTransformIf( - func() bool { return !b.Destroy }, - &CBDEdgeTransformer{Module: b.Module, State: b.State}, - ), - // Create all the providers &MissingProviderTransformer{Providers: b.Providers, Concrete: concreteProvider}, &ProviderTransformer{}, @@ -95,6 +88,13 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { &ParentProviderTransformer{}, &AttachProviderConfigTransformer{Module: b.Module}, + // Destruction ordering + &DestroyEdgeTransformer{Module: b.Module, State: b.State}, + GraphTransformIf( + func() bool { return !b.Destroy }, + &CBDEdgeTransformer{Module: b.Module, State: b.State}, + ), + // Provisioner-related transformations GraphTransformIf( func() bool { return !b.Destroy }, diff --git a/terraform/test-fixtures/apply-multi-provider-destroy/main.tf b/terraform/test-fixtures/apply-multi-provider-destroy/main.tf new file mode 100644 index 000000000..4a315a60e --- /dev/null +++ b/terraform/test-fixtures/apply-multi-provider-destroy/main.tf @@ -0,0 +1,9 @@ +resource "vault_instance" "foo" {} + +provider "aws" { + addr = "${vault_instance.foo.id}" +} + +resource "aws_instance" "bar" { + foo = "bar" +} diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index 789586ef0..76e3fd659 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -38,8 +38,8 @@ type GraphNodeCreator interface { // example: VPC with subnets, the VPC can't be deleted while there are // still subnets. type DestroyEdgeTransformer struct { - // Module and State are only needed to look up dependencies in - // any way possible. Either can be nil if not availabile. + // These are needed to properly build the graph of dependencies + // to determine what a destroy node depends on. Any of these can be nil. Module *module.Tree State *State } @@ -115,10 +115,20 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { // Example: resource A is force new, then destroy A AND create A are // in the graph. BUT if resource A is just pure destroy, then only // destroy A is in the graph, and create A is not. + providerFn := func(a *NodeAbstractProvider) dag.Vertex { + return &NodeApplyableProvider{NodeAbstractProvider: a} + } steps := []GraphTransformer{ + // Add outputs and metadata &OutputTransformer{Module: t.Module}, &AttachResourceConfigTransformer{Module: t.Module}, &AttachStateTransformer{State: t.State}, + + // Add providers since they can affect destroy order as well + &MissingProviderTransformer{AllowAny: true, Concrete: providerFn}, + &ProviderTransformer{}, + &AttachProviderConfigTransformer{Module: t.Module}, + &ReferenceTransformer{}, } @@ -170,9 +180,13 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { refs = append(refs, raw.(dag.Vertex)) } + refNames := make([]string, len(refs)) + for i, ref := range refs { + refNames[i] = dag.VertexName(ref) + } log.Printf( - "[TRACE] DestroyEdgeTransformer: creation node %q references %v", - dag.VertexName(v), refs) + "[TRACE] DestroyEdgeTransformer: creation node %q references %s", + dag.VertexName(v), refNames) // If we have no references, then we won't need to do anything if len(refs) == 0 { diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index e4eba60c4..ac13bb9a5 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -114,6 +114,10 @@ type MissingProviderTransformer struct { // Providers is the list of providers we support. Providers []string + // AllowAny will not check that a provider is supported before adding + // it to the graph. + AllowAny bool + // Concrete, if set, overrides how the providers are made. Concrete ConcreteProviderNodeFunc } @@ -170,10 +174,12 @@ func (t *MissingProviderTransformer) Transform(g *Graph) error { ptype = p[:idx] } - if _, ok := supported[ptype]; !ok { - // If we don't support the provider type, skip it. - // Validation later will catch this as an error. - continue + if !t.AllowAny { + if _, ok := supported[ptype]; !ok { + // If we don't support the provider type, skip it. + // Validation later will catch this as an error. + continue + } } // Add the missing provider node to the graph From b346ba32d16e970259ba8e380b2fa71f9ed191d6 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 10 Dec 2016 20:22:12 -0500 Subject: [PATCH 2/2] terraform: dependent provider resources are destroyed first in modules This extends the prior commit to also verify (and fix) that resources of dependent providers are destroyed first even when they're within modules. --- terraform/context_apply_test.go | 103 ++++++++++++++++++ .../child/main.tf | 3 + .../main.tf | 9 ++ terraform/transform_destroy_edge.go | 2 + 4 files changed, 117 insertions(+) create mode 100644 terraform/test-fixtures/apply-multi-provider-destroy-child/child/main.tf create mode 100644 terraform/test-fixtures/apply-multi-provider-destroy-child/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 3988c58e6..7f14e98c1 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2843,6 +2843,109 @@ func TestContext2Apply_multiProviderDestroy(t *testing.T) { checkStateString(t, state, ``) } +// This is like the multiProviderDestroy test except it tests that +// dependent resources within a child module that inherit provider +// configuration are still destroyed first. +func TestContext2Apply_multiProviderDestroyChild(t *testing.T) { + m := testModule(t, "apply-multi-provider-destroy-child") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + p2 := testProvider("do") + p2.ApplyFn = testApplyFn + p2.DiffFn = testDiffFn + + var state *State + + // First, create the instances + { + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + "vault": testProviderFuncFixed(p2), + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + s, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + state = s + } + + // Destroy them + { + // Verify that aws_instance.bar is destroyed first + var checked bool + var called int32 + var lock sync.Mutex + applyFn := func( + info *InstanceInfo, + is *InstanceState, + id *InstanceDiff) (*InstanceState, error) { + lock.Lock() + defer lock.Unlock() + + if info.HumanId() == "module.child.aws_instance.bar" { + checked = true + + // Sleep to allow parallel execution + time.Sleep(50 * time.Millisecond) + + // Verify that called is 0 (dep not called) + if atomic.LoadInt32(&called) != 0 { + return nil, fmt.Errorf("nothing else should be called") + } + } + + atomic.AddInt32(&called, 1) + return testApplyFn(info, is, id) + } + + // Set the apply functions + p.ApplyFn = applyFn + p2.ApplyFn = applyFn + + ctx := testContext2(t, &ContextOpts{ + Destroy: true, + State: state, + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + "vault": testProviderFuncFixed(p2), + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + s, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + if !checked { + t.Fatal("should be checked") + } + + state = s + } + + checkStateString(t, state, ` + +module.child: + +`) +} + func TestContext2Apply_multiVar(t *testing.T) { m := testModule(t, "apply-multi-var") p := testProvider("aws") diff --git a/terraform/test-fixtures/apply-multi-provider-destroy-child/child/main.tf b/terraform/test-fixtures/apply-multi-provider-destroy-child/child/main.tf new file mode 100644 index 000000000..ae1bc8ee4 --- /dev/null +++ b/terraform/test-fixtures/apply-multi-provider-destroy-child/child/main.tf @@ -0,0 +1,3 @@ +resource "aws_instance" "bar" { + foo = "bar" +} diff --git a/terraform/test-fixtures/apply-multi-provider-destroy-child/main.tf b/terraform/test-fixtures/apply-multi-provider-destroy-child/main.tf new file mode 100644 index 000000000..61e69e816 --- /dev/null +++ b/terraform/test-fixtures/apply-multi-provider-destroy-child/main.tf @@ -0,0 +1,9 @@ +resource "vault_instance" "foo" {} + +provider "aws" { + addr = "${vault_instance.foo.id}" +} + +module "child" { + source = "./child" +} diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index 76e3fd659..8c067892c 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -127,6 +127,8 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { // Add providers since they can affect destroy order as well &MissingProviderTransformer{AllowAny: true, Concrete: providerFn}, &ProviderTransformer{}, + &DisableProviderTransformer{}, + &ParentProviderTransformer{}, &AttachProviderConfigTransformer{Module: t.Module}, &ReferenceTransformer{},