From e4e50617aa86097d694d68662ae2a31c8f2a57b0 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 21 Jan 2021 12:43:50 -0500 Subject: [PATCH 1/2] add failing test, and start new test files The existing context test files are becoming quite unwieldy. Start new ones both to make editing easier, and to help discourage the copy+pasting of older test patterns we no longer need. --- terraform/context_apply2_test.go | 1 + terraform/context_apply_test.go | 5 +++ terraform/context_plan2_test.go | 61 ++++++++++++++++++++++++++++++++ terraform/context_plan_test.go | 5 +++ 4 files changed, 72 insertions(+) create mode 100644 terraform/context_apply2_test.go create mode 100644 terraform/context_plan2_test.go diff --git a/terraform/context_apply2_test.go b/terraform/context_apply2_test.go new file mode 100644 index 000000000..cc3ee2f47 --- /dev/null +++ b/terraform/context_apply2_test.go @@ -0,0 +1 @@ +package terraform diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 537bd289b..1f80ae7d3 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -12564,3 +12564,8 @@ resource "test_object" "a" { t.Fatalf("error missing expected info: %q", errString) } } + +//////////////////////////////////////////////////////////////////////////////// +// NOTE: Due to the size of this file, new tests should be added to +// context_apply2_test.go. +//////////////////////////////////////////////////////////////////////////////// diff --git a/terraform/context_plan2_test.go b/terraform/context_plan2_test.go new file mode 100644 index 000000000..ecf2a36f8 --- /dev/null +++ b/terraform/context_plan2_test.go @@ -0,0 +1,61 @@ +package terraform + +import ( + "testing" + + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/plans" + "github.com/hashicorp/terraform/providers" + "github.com/hashicorp/terraform/states" + "github.com/zclconf/go-cty/cty" +) + +func TestContext2Plan_removedDuringRefresh(t *testing.T) { + // The resource was added to state but actually failed to create and was + // left tainted. This should be removed during plan and result in a Create + // action. + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "a" { +} +`, + }) + + p := simpleMockProvider() + p.ReadResourceFn = func(req providers.ReadResourceRequest) (resp providers.ReadResourceResponse) { + resp.NewState = cty.NullVal(req.PriorState.Type()) + return resp + } + + addr := mustResourceInstanceAddr("test_object.a") + state := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent(addr, &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"test_string":"foo"}`), + Status: states.ObjectTainted, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`)) + }) + + ctx := testContext2(t, &ContextOpts{ + Config: m, + State: state, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.Err()) + } + + for _, c := range plan.Changes.Resources { + if c.Action != plans.Create { + t.Fatalf("expected Create action for missing %s, got %s", c.Addr, c.Action) + } + } + + _, diags = ctx.Apply() + if diags.HasErrors() { + t.Fatal(diags.Err()) + } +} diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 099fff1c1..5507cc08d 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -6732,3 +6732,8 @@ output "planned" { } } } + +//////////////////////////////////////////////////////////////////////////////// +// NOTE: Due to the size of this file, new tests should be added to +// context_plan2_test.go. +//////////////////////////////////////////////////////////////////////////////// From cb7a08c691b72337faff400480b777d605aba5ac Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 21 Jan 2021 12:46:37 -0500 Subject: [PATCH 2/2] fix null check and shadowed state variable The tainted state was checked against `cty.NilVal`, however it was always being set to a null value. The refreshed state value was being shadowed, and not used in the following plan. --- terraform/node_resource_abstract_instance.go | 2 +- terraform/node_resource_plan_instance.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index 1393d1abe..ad5574c85 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -905,7 +905,7 @@ func (n *NodeAbstractResourceInstance) plan( // If our prior value was tainted then we actually want this to appear // as a replace change, even though so far we've been treating it as a // create. - if action == plans.Create && priorValTainted != cty.NilVal { + if action == plans.Create && !priorValTainted.IsNull() { if createBeforeDestroy { action = plans.CreateThenDelete } else { diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index e6cfc1dc7..1f57e85c8 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -128,11 +128,12 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) // Refresh, maybe if !n.skipRefresh { - instanceRefreshState, refreshDiags := n.refresh(ctx, instanceRefreshState) + s, refreshDiags := n.refresh(ctx, instanceRefreshState) diags = diags.Append(refreshDiags) if diags.HasErrors() { return diags } + instanceRefreshState = s diags = diags.Append(n.writeResourceInstanceState(ctx, instanceRefreshState, n.Dependencies, refreshState)) if diags.HasErrors() {