diff --git a/internal/tofu/context_plan_test.go b/internal/tofu/context_plan_test.go index 00202f226e..18d21f383c 100644 --- a/internal/tofu/context_plan_test.go +++ b/internal/tofu/context_plan_test.go @@ -1819,6 +1819,93 @@ func TestContext2Plan_preventDestroy_dynamicFromDataResource(t *testing.T) { }) } +func TestContext2Plan_preventDestroy_dynamicFromManagedResourceDestroyMode(t *testing.T) { + // This test is verifying that a prevent_destroy argument's dependencies + // are respected even when we're in plans.DestroyMode, which has separate + // rules for how its graph gets built. + // + // With the design of the system at the time of writing this comment, the + // likely failure mode is that no dependency from test_instance.foo to + // var.prevent_destroy is created in the child module, and so the variable + // itself is eligible to be "pruned" from the graph as apparently irrelevant + // to the destroy phase. That's incorrect because, unlike most of the rest + // of a resource configuration, prevent_destroy is relevant during destroy + // planning just like in normal planning. + m := testModuleInline(t, map[string]string{ + "main.tf": ` + module "child" { + source = "./child" + + prevent_destroy = true + } + `, + "child/main.tf": ` + variable "prevent_destroy" { + type = bool + } + + resource "test_instance" "foo" { + require_new = "yes" + + lifecycle { + prevent_destroy = var.prevent_destroy + } + } + `, + }) + mainP := testProvider("test") + mainP.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + // Because we're simulating a "replace" situation here, the + // plan function actually gets called twice with the second + // one planning the "create" part of the replace. We want + // do do our fake synctest sleep only on the first plan call + // (where there's a prior state) because that's the one whose + // execution is supposed to wait until the data resource read + // has completed. + if !req.PriorState.IsNull() { + log.Printf("[TRACE] TestContext2Plan_preventDestroy_dynamicFromManagedResourceDestroyMode: test_instance.foo plan") + } else { + log.Printf("[TRACE] TestContext2Plan_preventDestroy_dynamicFromManagedResourceDestroyMode: test_instance.foo followup plan") + } + return testDiffFn(req) + } + state := states.NewState() + child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_instance.foo").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"i-abc123"}`), + }, + mustProviderConfig(`provider["registry.opentofu.org/hashicorp/test"]`), + addrs.NoKey, + ) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(mainP), + }, + }) + + _, diags := ctx.Plan(context.Background(), m, state, &PlanOpts{ + Mode: plans.DestroyMode, + SkipRefresh: true, + }) + if !diags.HasErrors() { + t.Fatalf("unexpected success; want error about prevent_destroy being set") + } + gotErr := diags.Err().Error() + wantErr := `test_instance.foo has prevent_destroy set` + if strings.Contains(gotErr, "has a prevent_destroy argument but its value will not be known until the apply step") { + // This is just a best-effort hint for one failure mode we've seen before, + // so that if it regresses we'll have a clue about what to investigate. + t.Errorf("unexpected error about unknown values; is there a missing dependency edge between the resource and the input variable in the walkPlanDestroy graph?") + } + if !strings.Contains(gotErr, wantErr) { + t.Fatalf("missing expected error\ngot: %s\nwant error diagnostics with substring %q", gotErr, wantErr) + } +} + func TestContext2Plan_preventDestroy_countBad(t *testing.T) { m := testModule(t, "plan-prevent-destroy-count-bad") p := testProvider("aws") diff --git a/internal/tofu/node_resource_abstract.go b/internal/tofu/node_resource_abstract.go index c778f8a825..251c0bab99 100644 --- a/internal/tofu/node_resource_abstract.go +++ b/internal/tofu/node_resource_abstract.go @@ -251,6 +251,28 @@ func (n *NodeAbstractResource) References() []*addrs.Reference { return result } +// DestroyReferences is a _partial_ implementation of [GraphNodeDestroyer] +// providing a default implementation for any embedding node type that has +// its own implementations of all of the other methods of that interface. +func (n *NodeAbstractResource) DestroyReferences() []*addrs.Reference { + // Config is always optional at destroy time, but if it's present then + // it might influence how we plan and apply the destroy actions. + var result []*addrs.Reference + if c := n.Config; c != nil { + if c.Managed != nil { + // The prevent_destroy setting, if present, forces planning to fail + // if the planned action for any instance of the resource is to + // destroy it, so we need to be able to evaluate the given expression + // for destroy nodes too. + if c.Managed.PreventDestroy != nil { + refs, _ := lang.ReferencesInExpr(addrs.ParseRef, c.Managed.PreventDestroy) + result = append(result, refs...) + } + } + } + return result +} + // referencesInImportAddress find all references relevant to the node in an import target address expression. // The only references we care about here are the references that exist in the keys of hclsyntax.IndexExpr. // For example, if the address is module.my_module1[expression1].aws_s3_bucket.bucket[expression2], then we would only diff --git a/internal/tofu/node_resource_abstract_instance.go b/internal/tofu/node_resource_abstract_instance.go index ae7b7d0b2b..3352bd5593 100644 --- a/internal/tofu/node_resource_abstract_instance.go +++ b/internal/tofu/node_resource_abstract_instance.go @@ -168,6 +168,29 @@ func (n *NodeAbstractResourceInstance) References() []*addrs.Reference { return nil } +// DestroyReferences is a _partial_ implementation of [GraphNodeDestroyer], +// providing a default implementation of this method for any embedder of +// [NodeAbstractResourceInstance] that implements all of the other methods +// of that interface. +func (n *NodeAbstractResourceInstance) DestroyReferences() []*addrs.Reference { + // If we have a configuration attached then we'll delegate to our + // embedded abstract resource, which knows how to extract dependencies + // from configuration. If there is no config, then the dependencies will + // be connected during destroy from those stored in the state. + if n.Config != nil { + if n.Schema == nil { + // We'll produce a log message about this out here so that + // we can include the full instance address, since the equivalent + // message in NodeAbstractResource.References cannot see it. + log.Printf("[WARN] no schema is attached to %s, so destroy-time config references cannot be detected", n.Name()) + return nil + } + return n.NodeAbstractResource.DestroyReferences() + } + + return nil +} + func (n *NodeAbstractResourceInstance) resolveProvider(ctx context.Context, evalCtx EvalContext, hasExpansionData bool, deposedKey states.DeposedKey) tfdiags.Diagnostics { var diags tfdiags.Diagnostics diff --git a/internal/tofu/transform_destroy_edge.go b/internal/tofu/transform_destroy_edge.go index d8a960a87d..1d977c1cf4 100644 --- a/internal/tofu/transform_destroy_edge.go +++ b/internal/tofu/transform_destroy_edge.go @@ -22,6 +22,14 @@ type GraphNodeDestroyer interface { // destroyed by this node. If this returns nil, then this node // is not destroying anything. DestroyAddr() *addrs.AbsResourceInstance + + // DestroyReferences is the equivalent of [GraphNodeReferencer.References] + // for destroy-related nodes, representing the fact that when destroying + // we only care about references in the subset of expressions that relate + // to destroying. Destroy is _mostly_ a state-only operation, but considers + // some metadata from the configuration when available to influence how + // destroying is planned. + DestroyReferences() []*addrs.Reference } // GraphNodeCreator must be implemented by nodes that create OR update resources. diff --git a/internal/tofu/transform_reference.go b/internal/tofu/transform_reference.go index 4c35eaa564..48d335d801 100644 --- a/internal/tofu/transform_reference.go +++ b/internal/tofu/transform_reference.go @@ -124,12 +124,6 @@ func (t *ReferenceTransformer) Transform(_ context.Context, g *Graph) error { // Find the things that reference things and connect them for _, v := range vs { - if _, ok := v.(GraphNodeDestroyer); ok { - // destroy nodes references are not connected, since they can only - // use their own state. - continue - } - parents := m.References(v) parentsDbg := make([]string, len(parents)) for i, v := range parents { @@ -300,20 +294,30 @@ type ReferenceMap map[string][]dag.Vertex // References returns the set of vertices that the given vertex refers to, // and any referenced addresses that do not have corresponding vertices. func (m ReferenceMap) References(v dag.Vertex) []dag.Vertex { - rn, ok := v.(GraphNodeReferencer) - if !ok { + var refsFunc func() []*addrs.Reference + if rn, ok := v.(GraphNodeDestroyer); ok { + // For nodes that represent destroying something we use a separate + // method because destroying typically uses only a subset of the + // references that would be relevant for other operations: destroying + // is _mostly_ a state-only operation that only use the configuration + // opportunistically for some metadata that influences how the destroy + // action is planned. + refsFunc = rn.DestroyReferences + } else if rn, ok := v.(GraphNodeReferencer); ok { + refsFunc = rn.References + } else { return nil } var matches []dag.Vertex - if rrn, ok := rn.(GraphNodeRootReferencer); ok { + if rrn, ok := v.(GraphNodeRootReferencer); ok { for _, ref := range rrn.RootReferences() { matches = append(matches, m.addReference(addrs.RootModule, v, ref)...) } } - for _, ref := range rn.References() { + for _, ref := range refsFunc() { matches = append(matches, m.addReference(vertexReferencePath(v), v, ref)...) }