From 765edc28a4f33123b96782c86103725df29bbf07 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 9 Sep 2020 15:36:46 -0400 Subject: [PATCH 1/3] not all plan action changes are provider bugs A provider cannot influence CreateThenDelete vs DeleteThenCreate, so we shouldn't attribute this to the provider in the error. --- terraform/eval_diff.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 509bd5bbcd..c8777b436e 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -59,6 +59,18 @@ func (n *EvalCheckPlannedChange) Eval(ctx EvalContext) (interface{}, error) { // all of the unknown values, since the final values might actually // match what was there before after all. log.Printf("[DEBUG] After incorporating new values learned so far during apply, %s change has become NoOp", absAddr) + + case (plannedChange.Action == plans.CreateThenDelete && actualChange.Action == plans.DeleteThenCreate) || + (plannedChange.Action == plans.DeleteThenCreate && actualChange.Action == plans.CreateThenDelete): + // If the order of replacement changed, then that is a bug in terraform + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Terraform produced inconsistent final plan", + fmt.Sprintf( + "When expanding the plan for %s to include new values learned so far during apply, the planned action changed from %s to %s.\n\nThis is a bug in Terraform and should be reported.", + absAddr, plannedChange.Action, actualChange.Action, + ), + )) default: diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, From 98cbfee2a26dda11eb4ea6a0f5417387183fba72 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 9 Sep 2020 16:38:01 -0400 Subject: [PATCH 2/3] add test for forced cbd with no other changes If a resource is forced CreateBeforeDestroy from a dependent resource, and that dependent has no changes, the plan is changed from CreateThenDelete to DeleteThenCreate causing an apply error. --- terraform/context_apply_test.go | 65 +++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 2c7025f9d5..e33f1a466d 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -11847,3 +11847,68 @@ resource "test_resource" "a" { t.Fatalf("apply errors: %s", diags.Err()) } } + +func TestContext2Apply_forcedCBD(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "v" {} + +resource "test_instance" "a" { + require_new = var.v +} + +resource "test_instance" "b" { + depends_on = [test_instance.a] + lifecycle { + create_before_destroy = true + } +} +`}) + + p := testProvider("test") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + Variables: InputValues{ + "v": &InputValue{ + Value: cty.StringVal("A"), + }, + }, + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + state, diags := ctx.Apply() + if diags.HasErrors() { + t.Fatalf("apply errors: %s", diags.Err()) + } + + ctx = testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + Variables: InputValues{ + "v": &InputValue{ + Value: cty.StringVal("B"), + }, + }, + State: state, + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + _, diags = ctx.Apply() + if diags.HasErrors() { + t.Fatalf("apply errors: %s", diags.Err()) + } +} From a16e6173138c79721e8180b4d4b989c0ef330eff Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 9 Sep 2020 17:02:28 -0400 Subject: [PATCH 3/3] apply the stored plan CreateThenDelete action When applying a plan, a forced CreateBeforeDestroy may not be set during the apply walk when downstream resources are no longer present in the graph. We still need to stick to that plan, and both the NodeApplyableResourceInstance EvalTree and the individual Eval nodes need to operate on that planned value. Ensure that we always check for an existing plan when determining CreateBeforeDestroy status. This must happen in 2 different code paths due to the eval node pattern currently in-use. Future refactoring may be able to unify these code-paths to make this less fragile. --- terraform/eval_diff.go | 10 ++++++++-- terraform/node_resource_apply_instance.go | 5 +++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index c8777b436e..58fc76b05c 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -130,6 +130,12 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { provider := *n.Provider providerSchema := *n.ProviderSchema + createBeforeDestroy := n.CreateBeforeDestroy + if n.PreviousDiff != nil { + // If we already planned the action, we stick to that plan + createBeforeDestroy = (*n.PreviousDiff).Action == plans.CreateThenDelete + } + if providerSchema == nil { return nil, fmt.Errorf("provider schema is unavailable for %s", n.Addr) } @@ -384,7 +390,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { case !reqRep.Empty(): // If there are any "requires replace" paths left _after our filtering // above_ then this is a replace action. - if n.CreateBeforeDestroy { + if createBeforeDestroy { action = plans.CreateThenDelete } else { action = plans.DeleteThenCreate @@ -450,7 +456,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { // as a replace change, even though so far we've been treating it as a // create. if action == plans.Create && priorValTainted != cty.NilVal { - if n.CreateBeforeDestroy { + if createBeforeDestroy { action = plans.CreateThenDelete } else { action = plans.DeleteThenCreate diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index 1282f8a11c..baf9422365 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -263,10 +263,15 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe destroy := false if diffApply != nil { destroy = (diffApply.Action == plans.Delete || diffApply.Action.IsReplace()) + + // Get the stored action for CBD if we have a plan already + createBeforeDestroyEnabled = diffApply.Change.Action == plans.CreateThenDelete } + if destroy && n.CreateBeforeDestroy() { createBeforeDestroyEnabled = true } + return createBeforeDestroyEnabled, nil }, Then: &EvalDeposeState{