Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

import block might cause a destroy action to attempt destruction of an already-destroyed resource #1803

Closed
RLRabinowitz opened this issue Jul 11, 2024 · 4 comments · Fixed by #2214
Closed

import block might cause a destroy action to attempt destruction of an already-destroyed resource #1803

RLRabinowitz opened this issue Jul 11, 2024 · 4 comments · Fixed by #2214
Assignees
@Evi1Pumpkin
Labels
accepted This issue has been accepted for implementation. bug Something isn't working
Milestone
1.9.0

Comments

@RLRabinowitz
Copy link
Contributor

OpenTofu Version

1.8.0-beta

OpenTofu Configuration Files

resource "random_integer" "a" {
  max = 10
  min = 1
}

import {
  to = random_integer.a
  id = "3,1,10"
}

resource "random_integer" "b" {
  max = 10
  min = 1
}

import {
  to = random_integer.b
  id = "5,1,10"
}

Debug Output

.

Expected Behavior

When attempting to destroy the configuration, after random_integer.a has been destroyed as part of a targeted destroy, OpenTofu should attempt to only destroy random_integer.b

Actual Behavior

OpenTofu attempts to destroy both random_integer.a and random_integer.b

The logs you get from the tofu destroy action:

random_integer.a: Preparing import... [id=3,1,10]
random_integer.a: Refreshing state... [id=3]
random_integer.b: Refreshing state... [id=5]

OpenTofu used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  - destroy

OpenTofu will perform the following actions:

  # random_integer.a will be destroyed
  - resource "random_integer" "a" {
      - id     = "3" -> null
      - max    = 10 -> null
      - min    = 1 -> null
      - result = 3 -> null
    }

  # random_integer.b will be destroyed
  - resource "random_integer" "b" {
      - id     = "5" -> null
      - max    = 10 -> null
      - min    = 1 -> null
      - result = 5 -> null
    }

Plan: 0 to add, 0 to change, 2 to destroy.

It should only attempt to destroy random_integer.b

Steps to Reproduce

  1. tofu init
  2. tofu apply
  3. tofu destroy -target=random_integer.a
  4. tofu destroy

Additional Context

No response

References
@RLRabinowitz RLRabinowitz added bug Something isn't working pending-decision This issue has not been accepted for implementation nor rejected. It's still open to discussion. accepted This issue has been accepted for implementation. and removed pending-decision This issue has not been accepted for implementation nor rejected. It's still open to discussion. labels Jul 11, 2024
@cam72cam cam72cam added this to the 1.9.0 milestone Jul 26, 2024
@tony21921
Copy link
Contributor

I am working on this and will submit a PR

@nayang7
Copy link

nayang7 commented Aug 13, 2024

I am working on this and will submit a PR

going to submit a PR, Co-authored-by: @tony21921

@nayang7 nayang7 mentioned this issue Aug 20, 2024
Closed
9 tasks
@apparentlymart
Copy link
Contributor

I would perhaps describe this problem a little differently: tofu destroy in particular should not pay any attention to import blocks at all, and should instead focus only on generating "delete" actions for any resource instance objects already tracked in the prior state.

The tofu destroy command is essentially a shorthand for terraform apply -destroy, telling OpenTofu to create a plan in the "destroy" planning mode. The planning mode causes the language runtime's "Plan" operation to divert into one of three codepaths:

opentofu/internal/tofu/context_plan.go

Lines 211 to 220 in d396502

switch opts.Mode {
case plans.NormalMode:
plan, planDiags = c.plan(config, prevRunState, opts)
case plans.DestroyMode:
plan, planDiags = c.destroyPlan(config, prevRunState, opts)
case plans.RefreshOnlyMode:
plan, planDiags = c.refreshOnlyPlan(config, prevRunState, opts)
default:
panic(fmt.Sprintf("unsupported plan mode %s", opts.Mode))
}

The destroy mode in particular has an interesting quirk: it actually internally runs two plan phases, with the first in "normal" mode to make sure that the prior state represents the remote system as closely as possible, except that it sneakily sets the PreDestroyRefresh field in the planning options when creating that plan:

opentofu/internal/tofu/context_plan.go

Lines 399 to 449 in d396502

// A destroy plan starts by running Refresh to read any pending data
// sources, and remove missing managed resources. This is required because
// a "destroy plan" is only creating delete changes, and is essentially a
// local operation.
//
// NOTE: if skipRefresh _is_ set then we'll rely on the destroy-plan walk
// below to upgrade the prevRunState and priorState both to the latest
// resource type schemas, so NodePlanDestroyableResourceInstance.Execute
// must coordinate with this by taking that action only when c.skipRefresh
// _is_ set. This coupling between the two is unfortunate but necessary
// to work within our current structure.
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.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
// preserve, but we're intentionally _not_ doing that here because
// if the first plan succeeded then we'll be running another plan
// in DestroyMode below, and we don't want to double-up any
// warnings that both plan walks would generate.
// (This does mean we won't show any warnings that would've been
// unique to only this walk, but we're assuming here that if the
// warnings aren't also applicable to a destroy plan then we'd
// rather not show them here, because this non-destroy plan for
// refreshing is largely an implementation detail.)
diags = diags.Append(refreshDiags)
return nil, diags
}
// We'll use the refreshed state -- which is the "prior state" from
// 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 OpenTofu.
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")
}

If that succeeds, it then performs the "real" destroy-mode plan phase (with opts here still carrying the caller's request to plan in destroy mode):

opentofu/internal/tofu/context_plan.go

Lines 451 to 458 in d396502

destroyPlan, walkDiags := c.planWalk(config, priorState, opts)
diags = diags.Append(walkDiags)
if walkDiags.HasErrors() {
// Non-nil plan along with errors indicates a non-applyable partial
// plan that's only suitable to be shown to the user as extra context
// to help understand the errors.
return destroyPlan, diags
}

The main handling of the planning mode happens during graph construction:

opentofu/internal/tofu/context_plan.go

Lines 840 to 893 in d396502

func (c *Context) planGraph(config *configs.Config, prevRunState *states.State, opts *PlanOpts, providerFunctionTracker ProviderFunctionMapping) (*Graph, walkOperation, tfdiags.Diagnostics) {
switch mode := opts.Mode; mode {
case plans.NormalMode:
graph, diags := (&PlanGraphBuilder{
Config: config,
State: prevRunState,
RootVariableValues: opts.SetVariables,
Plugins: c.plugins,
Targets: opts.Targets,
Excludes: opts.Excludes,
ForceReplace: opts.ForceReplace,
skipRefresh: opts.SkipRefresh,
preDestroyRefresh: opts.PreDestroyRefresh,
Operation: walkPlan,
ExternalReferences: opts.ExternalReferences,
ImportTargets: opts.ImportTargets,
GenerateConfigPath: opts.GenerateConfigPath,
EndpointsToRemove: opts.EndpointsToRemove,
ProviderFunctionTracker: providerFunctionTracker,
}).Build(addrs.RootModuleInstance)
return graph, walkPlan, diags
case plans.RefreshOnlyMode:
graph, diags := (&PlanGraphBuilder{
Config: config,
State: prevRunState,
RootVariableValues: opts.SetVariables,
Plugins: c.plugins,
Targets: opts.Targets,
Excludes: opts.Excludes,
skipRefresh: opts.SkipRefresh,
skipPlanChanges: true, // this activates "refresh only" mode.
Operation: walkPlan,
ExternalReferences: opts.ExternalReferences,
ProviderFunctionTracker: providerFunctionTracker,
}).Build(addrs.RootModuleInstance)
return graph, walkPlan, diags
case plans.DestroyMode:
graph, diags := (&PlanGraphBuilder{
Config: config,
State: prevRunState,
RootVariableValues: opts.SetVariables,
Plugins: c.plugins,
Targets: opts.Targets,
Excludes: opts.Excludes,
skipRefresh: opts.SkipRefresh,
Operation: walkPlanDestroy,
ProviderFunctionTracker: providerFunctionTracker,
}).Build(addrs.RootModuleInstance)
return graph, walkPlanDestroy, diags
default:
// The above should cover all plans.Mode values
panic(fmt.Sprintf("unsupported plan mode %s", mode))
}
}

From reading the trace logs I've confirmed that the import is happening during that secret initial "normal mode" plan, when it visits the NodePlannableResourceInstance representing random_integer.a:

[TRACE] vertex "random_integer.a": starting visit (*tofu.NodePlannableResourceInstance)
[TRACE] Resolving provider key for random_integer.a
[TRACE] Resolved provider key for random_integer.a as %!s(<nil>)
[TRACE] tofu.contextPlugins: Serving provider "registry.opentofu.org/hashicorp/random" schema from global schema cache
[TRACE] GRPCProvider: ImportResourceState
[TRACE] GRPCProvider: GetProviderSchema
[TRACE] GRPCProvider: GetProviderSchema: serving from global schema cache: address=registry.opentofu.org/hashicorp/random
[TRACE] graphNodeImportState: import random_integer.a "3,1,10" produced instance object of type random_integer
[TRACE] NodeAbstractResourceInstance.refresh for random_integer.a
[TRACE] tofu.contextPlugins: Serving provider "registry.opentofu.org/hashicorp/random" schema from global schema cache
[TRACE] tofu.contextPlugins: Serving provider "registry.opentofu.org/hashicorp/random" schema from global schema cache
[TRACE] GRPCProvider: ReadResource

Therefore it seems like probably the least invasive change would be to make the PlanGraphBuilder use its existing preDestroyRefresh flag also to set an additional flag on the concrete nodeExpandPlannableResource objects it creates which tells them that any import blocks ought to be ignored:

opentofu/internal/tofu/graph_builder_plan.go

Lines 271 to 279 in d396502

b.ConcreteResource = func(a *NodeAbstractResource) dag.Vertex {
return &nodeExpandPlannableResource{
NodeAbstractResource: a,
skipRefresh: b.skipRefresh,
skipPlanChanges: b.skipPlanChanges,
preDestroyRefresh: b.preDestroyRefresh,
forceReplace: b.ForceReplace,
}
}

The expansion node can then in turn skip resolving imports when its own "skip imports" flag is set:

opentofu/internal/tofu/node_resource_plan.go

Lines 329 to 377 in d396502

// The concrete resource factory we'll use
concreteResource := func(a *NodeAbstractResourceInstance) dag.Vertex {
var m *NodePlannableResourceInstance
// If we're in the `tofu import` CLI command, we only need
// to return the import node, not a plannable resource node.
for _, c := range commandLineImportTargets {
if c.Addr.Equal(a.Addr) {
return &graphNodeImportState{
Addr: c.Addr,
ID: c.ID,
ResolvedProvider: n.ResolvedProvider,
Schema: n.Schema,
SchemaVersion: n.SchemaVersion,
Config: n.Config,
}
}
}
// Add the config and state since we don't do that via transforms
a.Config = n.Config
a.ResolvedProvider = n.ResolvedProvider
a.Schema = n.Schema
a.ProvisionerSchemas = n.ProvisionerSchemas
a.ProviderMetas = n.ProviderMetas
a.dependsOn = n.dependsOn
a.Dependencies = n.dependencies
a.preDestroyRefresh = n.preDestroyRefresh
a.generateConfigPath = n.generateConfigPath
m = &NodePlannableResourceInstance{
NodeAbstractResourceInstance: a,
// By the time we're walking, we've figured out whether we need
// to force on CreateBeforeDestroy due to dependencies on other
// nodes that have it.
ForceCreateBeforeDestroy: n.CreateBeforeDestroy(),
skipRefresh: n.skipRefresh,
skipPlanChanges: n.skipPlanChanges,
forceReplace: n.forceReplace,
}
resolvedImportTarget := ctx.ImportResolver().GetImport(a.Addr)
if resolvedImportTarget != nil {
m.importTarget = *resolvedImportTarget
}
return m
}

In an ideal world we wouldn't have this secret "normal-ish plan" hidden inside the destroy plan implementation with various little special cases attached to it, but it's risky to replace it because the destroy plan implementation is very focused only on the problem of generating "destroy" actions for objects in the state and so doesn't really know how to deal with anything else.

@Evi1Pumpkin Evi1Pumpkin mentioned this issue Nov 22, 2024
Merged
9 tasks
@Evi1Pumpkin
Copy link
Contributor

I followed your approach, @apparentlymart. Thanks for your help!
I made a few simple changes. Instead of skipping the resolved import in resourceInstanceSubgraph(), I decided to skip the expansion and resolution logic of the import block targets altogether in an earlier phase.

Let me know if you have a different opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Evi1Pumpkin Evi1Pumpkin

Labels
accepted This issue has been accepted for implementation. bug Something isn't working
Projects
None yet
Milestone
1.9.0
6 participants
@apparentlymart @cam72cam @Evi1Pumpkin @RLRabinowitz @tony21921 @nayang7