From 3db3ed03fbb004101389b367b6f5b17034771295 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 9 Nov 2022 15:45:52 -0500 Subject: [PATCH] ensure destroy plan contains valid state values Some prior refactors left the detroyPlan method a bit confusing, and ran into a case where the previous run state could be returned as nil. Get rid of the no longer used pendingPlan value, and track the prior and prev states directly, making sure we always have a value for both. --- internal/terraform/context_plan.go | 24 +++++++++++------------- internal/terraform/context_plan2_test.go | 10 +++++++++- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index ee0a4956fd07..ae446a9a8af9 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -325,7 +325,6 @@ func (c *Context) refreshOnlyPlan(config *configs.Config, prevRunState *states.S func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - pendingPlan := &plans.Plan{} if opts.Mode != plans.DestroyMode { panic(fmt.Sprintf("called Context.destroyPlan with %s", opts.Mode)) @@ -373,18 +372,17 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State return nil, diags } - // insert the refreshed state into the destroy plan result, and ignore - // the changes recorded from the refresh. - pendingPlan.PriorState = refreshPlan.PriorState.DeepCopy() - pendingPlan.PrevRunState = refreshPlan.PrevRunState.DeepCopy() - log.Printf("[TRACE] Context.destroyPlan: now _really_ creating a destroy plan") - // We'll use the refreshed state -- which is the "prior state" from - // the perspective of this "pending plan" -- as the starting state + // the perspective of this "destroy plan" -- as the starting state // for our destroy-plan walk, so it can take into account if we // detected during refreshing that anything was already deleted outside // of Terraform. - priorState = pendingPlan.PriorState + priorState = refreshPlan.PriorState.DeepCopy() + + // The refresh plan may have upgraded state for some resources, make + // sure we store the new version. + prevRunState = refreshPlan.PrevRunState.DeepCopy() + log.Printf("[TRACE] Context.destroyPlan: now _really_ creating a destroy plan") } destroyPlan, walkDiags := c.planWalk(config, priorState, opts) @@ -394,10 +392,10 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State } if !opts.SkipRefresh { - // If we didn't skip refreshing then we want the previous run state - // prior state to be the one we originally fed into the c.plan call - // above, not the refreshed version we used for the destroy walk. - destroyPlan.PrevRunState = pendingPlan.PrevRunState + // If we didn't skip refreshing then we want the previous run state to + // be the one we originally fed into the c.refreshOnlyPlan call above, + // not the refreshed version we used for the destroy planWalk. + destroyPlan.PrevRunState = prevRunState } relevantAttrs, rDiags := c.relevantResourceAttrsForPlan(config, destroyPlan) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 1f626bc45bf6..b3b9d6848210 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -3676,11 +3676,19 @@ output "out" { }, }) - _, diags := ctx.Plan(m, state, &PlanOpts{ + plan, diags := ctx.Plan(m, state, &PlanOpts{ Mode: plans.DestroyMode, }) assertNoErrors(t, diags) + + // ensure that the given states are valid and can be serialized + if plan.PrevRunState == nil { + t.Fatal("nil plan.PrevRunState") + } + if plan.PriorState == nil { + t.Fatal("nil plan.PriorState") + } } // A deposed instances which no longer exists during ReadResource creates NoOp