From 3ea704ef81bf5e8bc28f49bc14bf4e626fb74fcb Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 11 Nov 2022 11:46:56 -0500 Subject: [PATCH] Make the pre-destroy refresh a full plan In order to complete the terraform destroy command, a refresh must first be done to update state and remove any instances which have already been deleted externally. This was being done with a refresh plan, which will avoid any condition evaluations and avoid planning new instances. That however can fail due to invalid references from resources that are already missing from the state. A new plan type to handle the concept of the pre-destroy-refresh is needed here, which should probably be incorporated directly into the destroy plan, just like the original refresh walk was incorporated into the normal planning process. That however is major refactoring that is not appropriate for a patch release. Instead we make two discrete changes here to prevent blocking a destroy plan. The first is to use a normal plan to refresh, which will enable evaluation because missing and inconsistent instances will be planned for creation and updates, allowing them to be evaluated. That is not optimal of course, but does revert to the method used by previous Terraform releases until a better method can be implemented. The second change is adding a preDestroyRefresh flag to the planning process. This is checked in any location which evalCheckRules is called, and lets us change the diagnosticSeverity of the output to only be warnings, matching the behavior of a normal refresh plan. --- internal/terraform/context_plan.go | 23 +++- internal/terraform/context_plan2_test.go | 100 ++++++++++++++++++ internal/terraform/graph_builder_plan.go | 8 +- .../node_resource_abstract_instance.go | 9 +- internal/terraform/node_resource_plan.go | 3 + .../terraform/node_resource_plan_instance.go | 13 ++- 6 files changed, 148 insertions(+), 8 deletions(-) diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index 9b35ea756237..ee0a4956fd07 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -33,6 +33,16 @@ type PlanOpts struct { // instance using its corresponding provider. SkipRefresh bool + // PreDestroyRefresh indicated that this is being passed to a plan used to + // refresh the state immediately before a destroy plan. + // FIXME: This is a temporary fix to allow the pre-destroy refresh to + // succeed. The refreshing operation during destroy must be a special case, + // which can allow for missing instances in the state, and avoid blocking + // on failing condition tests. The destroy plan itself should be + // responsible for this special case of refreshing, and the separate + // pre-destroy plan removed entirely. + PreDestroyRefresh bool + // SetVariables are the raw values for root module variables as provided // by the user who is requesting the run, prior to any normalization or // substitution of defaults. See the documentation for the InputValue @@ -337,8 +347,16 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State if !opts.SkipRefresh && !prevRunState.Empty() { log.Printf("[TRACE] Context.destroyPlan: calling Context.plan to get the effect of refreshing the prior state") refreshOpts := *opts - refreshOpts.Mode = plans.RefreshOnlyMode - refreshPlan, refreshDiags := c.refreshOnlyPlan(config, prevRunState, &refreshOpts) + refreshOpts.Mode = plans.NormalMode + refreshOpts.PreDestroyRefresh = true + + // FIXME: A normal plan is required here to refresh the state, because + // the state and configuration may not match during a destroy, and a + // normal refresh plan can fail with evaluation errors. In the future + // the destroy plan should take care of refreshing instances itself, + // where the special cases of evaluation and skipping condition checks + // can be done. + refreshPlan, refreshDiags := c.plan(config, prevRunState, &refreshOpts) if refreshDiags.HasErrors() { // NOTE: Normally we'd append diagnostics regardless of whether // there are errors, just in case there are warnings we'd want to @@ -558,6 +576,7 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State, Targets: opts.Targets, ForceReplace: opts.ForceReplace, skipRefresh: opts.SkipRefresh, + preDestroyRefresh: opts.PreDestroyRefresh, Operation: walkPlan, }).Build(addrs.RootModuleInstance) return graph, walkPlan, diags diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 70f614fb14e5..1f626bc45bf6 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -3827,3 +3827,103 @@ resource "test_object" "b" { _, diags = ctx.Plan(m, state, opts) assertNoErrors(t, diags) } + +func TestContext2Plan_destroyPartialState(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "a" { +} + +output "out" { + value = module.mod.out +} + +module "mod" { + source = "./mod" +} +`, + + "./mod/main.tf": ` +resource "test_object" "a" { + count = 2 + + lifecycle { + precondition { + # test_object_b has already been destroyed, so referencing the first + # instance must not fail during a destroy plan. + condition = test_object.b[0].test_string == "invalid" + error_message = "should not block destroy" + } + precondition { + # this failing condition should bot block a destroy plan + condition = !local.continue + error_message = "should not block destroy" + } + } +} + +resource "test_object" "b" { + count = 2 +} + +locals { + continue = true +} + +output "out" { + # the reference to test_object.b[0] may not be valid during a destroy plan, + # but should not fail. + value = local.continue ? test_object.a[1].test_string != "invalid" && test_object.b[0].test_string != "invalid" : false + + precondition { + # test_object_b has already been destroyed, so referencing the first + # instance must not fail during a destroy plan. + condition = test_object.b[0].test_string == "invalid" + error_message = "should not block destroy" + } + precondition { + # this failing condition should bot block a destroy plan + condition = test_object.a[0].test_string == "invalid" + error_message = "should not block destroy" + } +} +`}) + + p := simpleMockProvider() + + // This state could be the result of a failed destroy, leaving only 2 + // remaining instances. We want to be able to continue the destroy to + // remove everything without blocking on invalid references or failing + // conditions. + state := states.NewState() + mod := state.EnsureModule(addrs.RootModuleInstance.Child("mod", addrs.NoKey)) + mod.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.a[0]").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectTainted, + AttrsJSON: []byte(`{"test_string":"current"}`), + Dependencies: []addrs.ConfigResource{}, + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + mod.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.a[1]").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectTainted, + AttrsJSON: []byte(`{"test_string":"current"}`), + Dependencies: []addrs.ConfigResource{}, + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + _, diags := ctx.Plan(m, state, &PlanOpts{ + Mode: plans.DestroyMode, + }) + assertNoErrors(t, diags) +} diff --git a/internal/terraform/graph_builder_plan.go b/internal/terraform/graph_builder_plan.go index 4195a7445a79..2c706647b450 100644 --- a/internal/terraform/graph_builder_plan.go +++ b/internal/terraform/graph_builder_plan.go @@ -49,6 +49,11 @@ type PlanGraphBuilder struct { // skipRefresh indicates that we should skip refreshing managed resources skipRefresh bool + // preDestroyRefresh indicates that we are executing the refresh which + // happens immediately before a destroy plan, which happens to use the + // normal planing mode so skipPlanChanges cannot be set. + preDestroyRefresh bool + // skipPlanChanges indicates that we should skip the step of comparing // prior state with configuration and generating planned changes to // resource instances. (This is for the "refresh only" planning mode, @@ -111,7 +116,7 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { &LocalTransformer{Config: b.Config}, &OutputTransformer{ Config: b.Config, - RefreshOnly: b.skipPlanChanges, + RefreshOnly: b.skipPlanChanges || b.preDestroyRefresh, PlanDestroy: b.Operation == walkPlanDestroy, // NOTE: We currently treat anything built with the plan graph @@ -214,6 +219,7 @@ func (b *PlanGraphBuilder) initPlan() { NodeAbstractResource: a, skipRefresh: b.skipRefresh, skipPlanChanges: b.skipPlanChanges, + preDestroyRefresh: b.preDestroyRefresh, forceReplace: b.ForceReplace, } } diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 9ff15b840804..5d6dcc43f328 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -37,6 +37,8 @@ type NodeAbstractResourceInstance struct { storedProviderConfig addrs.AbsProviderConfig Dependencies []addrs.ConfigResource + + preDestroyRefresh bool } // NewNodeAbstractResourceInstance creates an abstract resource instance graph @@ -682,6 +684,11 @@ func (n *NodeAbstractResourceInstance) plan( return plan, state, keyData, diags.Append(err) } + checkRuleSeverity := tfdiags.Error + if n.preDestroyRefresh { + checkRuleSeverity = tfdiags.Warning + } + if plannedChange != nil { // If we already planned the action, we stick to that plan createBeforeDestroy = plannedChange.Action == plans.CreateThenDelete @@ -708,7 +715,7 @@ func (n *NodeAbstractResourceInstance) plan( addrs.ResourcePrecondition, n.Config.Preconditions, ctx, n.Addr, keyData, - tfdiags.Error, + checkRuleSeverity, ) diags = diags.Append(checkDiags) if diags.HasErrors() { diff --git a/internal/terraform/node_resource_plan.go b/internal/terraform/node_resource_plan.go index 3c6e94d8bf9d..3d9d201f349c 100644 --- a/internal/terraform/node_resource_plan.go +++ b/internal/terraform/node_resource_plan.go @@ -25,6 +25,8 @@ type nodeExpandPlannableResource struct { // skipRefresh indicates that we should skip refreshing individual instances skipRefresh bool + preDestroyRefresh bool + // skipPlanChanges indicates we should skip trying to plan change actions // for any instances. skipPlanChanges bool @@ -328,6 +330,7 @@ func (n *nodeExpandPlannableResource) resourceInstanceSubgraph(ctx EvalContext, a.ProviderMetas = n.ProviderMetas a.dependsOn = n.dependsOn a.Dependencies = n.dependencies + a.preDestroyRefresh = n.preDestroyRefresh return &NodePlannableResourceInstance{ NodeAbstractResourceInstance: a, diff --git a/internal/terraform/node_resource_plan_instance.go b/internal/terraform/node_resource_plan_instance.go index d50b59958450..aac13bedc82d 100644 --- a/internal/terraform/node_resource_plan_instance.go +++ b/internal/terraform/node_resource_plan_instance.go @@ -83,7 +83,7 @@ func (n *NodePlannableResourceInstance) dataResourceExecute(ctx EvalContext) (di } checkRuleSeverity := tfdiags.Error - if n.skipPlanChanges { + if n.skipPlanChanges || n.preDestroyRefresh { checkRuleSeverity = tfdiags.Warning } @@ -128,6 +128,11 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) var change *plans.ResourceInstanceChange var instanceRefreshState *states.ResourceInstanceObject + checkRuleSeverity := tfdiags.Error + if n.skipPlanChanges || n.preDestroyRefresh { + checkRuleSeverity = tfdiags.Warning + } + _, providerSchema, err := getProvider(ctx, n.ResolvedProvider) diags = diags.Append(err) if diags.HasErrors() { @@ -280,7 +285,7 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) addrs.ResourcePostcondition, n.Config.Postconditions, ctx, n.ResourceInstanceAddr(), repeatData, - tfdiags.Error, + checkRuleSeverity, ) diags = diags.Append(checkDiags) } else { @@ -298,7 +303,7 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) addrs.ResourcePrecondition, n.Config.Preconditions, ctx, addr, repeatData, - tfdiags.Warning, + checkRuleSeverity, ) diags = diags.Append(checkDiags) @@ -321,7 +326,7 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) addrs.ResourcePostcondition, n.Config.Postconditions, ctx, addr, repeatData, - tfdiags.Warning, + checkRuleSeverity, ) diags = diags.Append(checkDiags) }