From 7a0ef561d2cd6c51c0bfd41d7fab028609e7251b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 23 Jan 2025 14:53:27 -0500 Subject: [PATCH 1/2] resources must be refreshed before plan The order of operations was incorrect here, with the refresh happening after the plan was already created. Invert the calls so the updated state is reflected in the plan output. We don't need to carry forward the defer response from refresh, since in the rare case that a read does need to be deferred, the PlanResourceChange call will return the same response. --- internal/terraform/context_plan2_test.go | 44 +++++++++++++++++++ .../terraform/node_resource_plan_orphan.go | 39 +++++++--------- 2 files changed, 61 insertions(+), 22 deletions(-) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 77cad9e845..f1c5efc976 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -6315,3 +6315,47 @@ resource "aws_instance" "foo" { t.Errorf("unexpected error message\ngot: %s\nwant substring: %s", got, want) } } + +func TestContext2Plan_orphanUpdateInstance(t *testing.T) { + // ean orphaned instance should still reflect the refreshed state in the plan + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "a" { + for_each = {} +} +`, + }) + + p := simpleMockProvider() + p.ReadResourceFn = func(req providers.ReadResourceRequest) (resp providers.ReadResourceResponse) { + state := req.PriorState.AsValueMap() + state["test_string"] = cty.StringVal("new") + resp.NewState = cty.ObjectVal(state) + return resp + } + + state := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent(mustResourceInstanceAddr(`test_object.a["old"]`), &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"test_string":"old"}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`)) + }) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan(m, state, DefaultPlanOpts) + assertNoErrors(t, diags) + + resourceType := p.GetProviderSchemaResponse.ResourceTypes["test_object"].Block.ImpliedType() + change, err := plan.Changes.ResourceInstance(mustResourceInstanceAddr(`test_object.a["old"]`)).Decode(resourceType) + if err != nil { + t.Fatal(err) + } + if change.Before.GetAttr("test_string").AsString() != "new" { + t.Fatalf("resource before value not refreshed in plan: %#v\n", change.Before) + } +} diff --git a/internal/terraform/node_resource_plan_orphan.go b/internal/terraform/node_resource_plan_orphan.go index 9e64977c19..51364049f2 100644 --- a/internal/terraform/node_resource_plan_orphan.go +++ b/internal/terraform/node_resource_plan_orphan.go @@ -122,22 +122,6 @@ func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalCon } } - shouldDefer := ctx.Deferrals().ShouldDeferResourceInstanceChanges(n.Addr, n.Dependencies) - - var change *plans.ResourceInstanceChange - var pDiags tfdiags.Diagnostics - var deferred *providers.Deferred - if forget { - change, pDiags = n.planForget(ctx, oldState, "") - diags = diags.Append(pDiags) - } else { - change, deferred, pDiags = n.planDestroy(ctx, oldState, "") - diags = diags.Append(pDiags) - } - if diags.HasErrors() { - return diags - } - if !n.skipRefresh && !forget { // Refresh this instance even though it is going to be destroyed, in // order to catch missing resources. If this is a normal plan, @@ -153,12 +137,7 @@ func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalCon oldState = refreshedState - if deferred == nil { - // set the overall deferred status if it wasn't already set. - deferred = refreshDeferred - } - - if deferred == nil && !shouldDefer { + if refreshDeferred == nil { // only update the state if we're not deferring the change diags = diags.Append(n.writeResourceInstanceState(ctx, refreshedState, refreshState)) if diags.HasErrors() { @@ -167,6 +146,22 @@ func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalCon } } + shouldDefer := ctx.Deferrals().ShouldDeferResourceInstanceChanges(n.Addr, n.Dependencies) + + var change *plans.ResourceInstanceChange + var pDiags tfdiags.Diagnostics + var deferred *providers.Deferred + if forget { + change, pDiags = n.planForget(ctx, oldState, "") + diags = diags.Append(pDiags) + } else { + change, deferred, pDiags = n.planDestroy(ctx, oldState, "") + diags = diags.Append(pDiags) + } + if diags.HasErrors() { + return diags + } + // We might be able to offer an approximate reason for why we are // planning to delete this object. (This is best-effort; we might // sometimes not have a reason.) From bfc6dc1a7c2ebd2caa0d87b89e0782d375aec652 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 23 Jan 2025 15:08:02 -0500 Subject: [PATCH 2/2] changelog --- .changes/unreleased/BUG FIXES-20250123-150746.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/unreleased/BUG FIXES-20250123-150746.yaml diff --git a/.changes/unreleased/BUG FIXES-20250123-150746.yaml b/.changes/unreleased/BUG FIXES-20250123-150746.yaml new file mode 100644 index 0000000000..1003a3ec9b --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20250123-150746.yaml @@ -0,0 +1,5 @@ +kind: BUG FIXES +body: Refreshed state was not used in the plan for orphaned resource instances +time: 2025-01-23T15:07:46.789595-05:00 +custom: + Issue: "36394"