From cf6bc7163a678f9a49fce649049369cb11815308 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 7695d1cefe4c18220c06cbbdb4e76874aba1fec0 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 ec231c76167566d9ed53741df4823a5fe56639a7 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{