diff --git a/internal/instances/expander.go b/internal/instances/expander.go index 3e3ac9675..4a0bd936f 100644 --- a/internal/instances/expander.go +++ b/internal/instances/expander.go @@ -120,6 +120,41 @@ func (e *Expander) ExpandModule(addr addrs.Module) []addrs.ModuleInstance { return ret } +// GetDeepestExistingModuleInstance is a funny specialized function for +// determining how many steps we can traverse through the given module instance +// address before encountering an undeclared instance of a declared module. +// +// The result is the longest prefix of the given address which steps only +// through module instances that exist. +// +// All of the modules on the given path must already have had their +// expansion registered using one of the SetModule* methods before calling, +// or this method will panic. +func (e *Expander) GetDeepestExistingModuleInstance(given addrs.ModuleInstance) addrs.ModuleInstance { + exps := e.exps // start with the root module expansions + for i := 0; i < len(given); i++ { + step := given[i] + callName := step.Name + if _, ok := exps.moduleCalls[addrs.ModuleCall{Name: callName}]; !ok { + // This is a bug in the caller, because it should always register + // expansions for an object and all of its ancestors before requesting + // expansion of it. + panic(fmt.Sprintf("no expansion has been registered for %s", given[:i].Child(callName, addrs.NoKey))) + } + + var ok bool + exps, ok = exps.childInstances[step] + if !ok { + // We've found a non-existing instance, so we're done. + return given[:i] + } + } + + // If we complete the loop above without returning early then the entire + // given address refers to a declared module instance. + return given +} + // ExpandModuleResource finds the exhaustive set of resource instances resulting from // the expansion of the given resource and all of its containing modules. // @@ -149,6 +184,14 @@ func (e *Expander) ExpandModuleResource(moduleAddr addrs.Module, resourceAddr ad // All of the modules on the path to the identified resource and the resource // itself must already have had their expansion registered using one of the // SetModule*/SetResource* methods before calling, or this method will panic. +// +// ExpandModuleResource returns all instances of a resource across all +// instances of its containing module, whereas this ExpandResource function +// is more specific and only expands within a single module instance. If +// any of the module instances selected in the module path of the given address +// aren't valid for that module's expansion then ExpandResource returns an +// empty result, reflecting that a non-existing module instance can never +// contain any existing resource instances. func (e *Expander) ExpandResource(resourceAddr addrs.AbsResource) []addrs.AbsResourceInstance { e.mu.RLock() defer e.mu.RUnlock() @@ -380,9 +423,23 @@ func (m *expanderModule) resourceInstances(moduleAddr addrs.ModuleInstance, reso panic(fmt.Sprintf("no expansion has been registered for %s", parentAddr.Child(callName, addrs.NoKey))) } - inst := m.childInstances[step] - moduleInstAddr := append(parentAddr, step) - return inst.resourceInstances(moduleAddr[1:], resourceAddr, moduleInstAddr) + if inst, ok := m.childInstances[step]; ok { + moduleInstAddr := append(parentAddr, step) + return inst.resourceInstances(moduleAddr[1:], resourceAddr, moduleInstAddr) + } else { + // If we have the module _call_ registered (as we checked above) + // but we don't have the given module _instance_ registered, that + // suggests that the module instance key in "step" is not declared + // by the current definition of this module call. That means the + // module instance doesn't exist at all, and therefore it can't + // possibly declare any resource instances either. + // + // For example, if we were asked about module.foo[0].aws_instance.bar + // but module.foo doesn't currently have count set, then there is no + // module.foo[0] at all, and therefore no aws_instance.bar + // instances inside it. + return nil + } } return m.onlyResourceInstances(resourceAddr, parentAddr) } diff --git a/internal/instances/expander_test.go b/internal/instances/expander_test.go index 8e4d0f8f0..2e9832889 100644 --- a/internal/instances/expander_test.go +++ b/internal/instances/expander_test.go @@ -199,14 +199,47 @@ func TestExpander(t *testing.T) { } }) t.Run("module single resource count2", func(t *testing.T) { - got := ex.ExpandModuleResource( + // Two different ways of asking the same question, which should + // both produce the same result. + // First: nested expansion of all instances of the resource across + // all instances of the module, but it's a single-instance module + // so the first level is a singleton. + got1 := ex.ExpandModuleResource( mustModuleAddr(`single`), count2ResourceAddr, ) + // Second: expansion of only instances belonging to a specific + // instance of the module, but again it's a single-instance module + // so there's only one to ask about. + got2 := ex.ExpandResource( + count2ResourceAddr.Absolute( + addrs.RootModuleInstance.Child("single", addrs.NoKey), + ), + ) want := []addrs.AbsResourceInstance{ mustAbsResourceInstanceAddr(`module.single.test.count2[0]`), mustAbsResourceInstanceAddr(`module.single.test.count2[1]`), } + if diff := cmp.Diff(want, got1); diff != "" { + t.Errorf("wrong ExpandModuleResource result\n%s", diff) + } + if diff := cmp.Diff(want, got2); diff != "" { + t.Errorf("wrong ExpandResource result\n%s", diff) + } + }) + t.Run("module single resource count2 with non-existing module instance", func(t *testing.T) { + got := ex.ExpandResource( + count2ResourceAddr.Absolute( + // Note: This is intentionally an invalid instance key, + // so we're asking about module.single[1].test.count2 + // even though module.single doesn't have count set and + // therefore there is no module.single[1]. + addrs.RootModuleInstance.Child("single", addrs.IntKey(1)), + ), + ) + // If the containing module instance doesn't exist then it can't + // possibly have any resource instances inside it. + want := ([]addrs.AbsResourceInstance)(nil) if diff := cmp.Diff(want, got); diff != "" { t.Errorf("wrong result\n%s", diff) } @@ -261,6 +294,36 @@ func TestExpander(t *testing.T) { t.Errorf("wrong result\n%s", diff) } }) + t.Run("module count2 module count2 GetDeepestExistingModuleInstance", func(t *testing.T) { + t.Run("first step invalid", func(t *testing.T) { + got := ex.GetDeepestExistingModuleInstance(mustModuleInstanceAddr(`module.count2["nope"].module.count2[0]`)) + want := addrs.RootModuleInstance + if !want.Equal(got) { + t.Errorf("wrong result\ngot: %s\nwant: %s", got, want) + } + }) + t.Run("second step invalid", func(t *testing.T) { + got := ex.GetDeepestExistingModuleInstance(mustModuleInstanceAddr(`module.count2[1].module.count2`)) + want := mustModuleInstanceAddr(`module.count2[1]`) + if !want.Equal(got) { + t.Errorf("wrong result\ngot: %s\nwant: %s", got, want) + } + }) + t.Run("neither step valid", func(t *testing.T) { + got := ex.GetDeepestExistingModuleInstance(mustModuleInstanceAddr(`module.count2.module.count2["nope"]`)) + want := addrs.RootModuleInstance + if !want.Equal(got) { + t.Errorf("wrong result\ngot: %s\nwant: %s", got, want) + } + }) + t.Run("both steps valid", func(t *testing.T) { + got := ex.GetDeepestExistingModuleInstance(mustModuleInstanceAddr(`module.count2[1].module.count2[0]`)) + want := mustModuleInstanceAddr(`module.count2[1].module.count2[0]`) + if !want.Equal(got) { + t.Errorf("wrong result\ngot: %s\nwant: %s", got, want) + } + }) + }) t.Run("module count2 resource count2 resource count2", func(t *testing.T) { got := ex.ExpandModuleResource( mustModuleAddr(`count2.count2`), diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 85872ac53..006e9e932 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -2076,3 +2076,105 @@ output "output" { } } } + +func TestContext2Plan_moduleExpandOrphansResourceInstance(t *testing.T) { + // This test deals with the situation where a user has changed the + // repetition/expansion mode for a module call while there are already + // resource instances from the previous declaration in the state. + // + // This is conceptually just the same as removing the resources + // from the module configuration only for that instance, but the + // implementation of it ends up a little different because it's + // an entry in the resource address's _module path_ that we'll find + // missing, rather than the resource's own instance key, and so + // our analyses need to handle that situation by indicating that all + // of the resources under the missing module instance have zero + // instances, regardless of which resource in that module we might + // be asking about, and do so without tripping over any missing + // registrations in the instance expander that might lead to panics + // if we aren't careful. + // + // (For some history here, see https://github.com/hashicorp/terraform/issues/30110 ) + + addrNoKey := mustResourceInstanceAddr("module.child.test_object.a[0]") + addrZeroKey := mustResourceInstanceAddr("module.child[0].test_object.a[0]") + m := testModuleInline(t, map[string]string{ + "main.tf": ` + module "child" { + source = "./child" + count = 1 + } + `, + "child/main.tf": ` + resource "test_object" "a" { + count = 1 + } + `, + }) + + state := states.BuildState(func(s *states.SyncState) { + // Notice that addrNoKey is the address which lacks any instance key + // for module.child, and so that module instance doesn't match the + // call declared above with count = 1, and therefore the resource + // inside is "orphaned" even though the resource block actually + // still exists there. + s.SetResourceInstanceCurrent(addrNoKey, &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`)) + }) + + p := simpleMockProvider() + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan(m, state, &PlanOpts{ + Mode: plans.NormalMode, + }) + if diags.HasErrors() { + t.Fatalf("unexpected errors\n%s", diags.Err().Error()) + } + + t.Run(addrNoKey.String(), func(t *testing.T) { + instPlan := plan.Changes.ResourceInstance(addrNoKey) + if instPlan == nil { + t.Fatalf("no plan for %s at all", addrNoKey) + } + + if got, want := instPlan.Addr, addrNoKey; !got.Equal(want) { + t.Errorf("wrong current address\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.PrevRunAddr, addrNoKey; !got.Equal(want) { + t.Errorf("wrong previous run address\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.Action, plans.Delete; got != want { + t.Errorf("wrong planned action\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.ActionReason, plans.ResourceInstanceDeleteBecauseNoModule; got != want { + t.Errorf("wrong action reason\ngot: %s\nwant: %s", got, want) + } + }) + + t.Run(addrZeroKey.String(), func(t *testing.T) { + instPlan := plan.Changes.ResourceInstance(addrZeroKey) + if instPlan == nil { + t.Fatalf("no plan for %s at all", addrZeroKey) + } + + if got, want := instPlan.Addr, addrZeroKey; !got.Equal(want) { + t.Errorf("wrong current address\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.PrevRunAddr, addrZeroKey; !got.Equal(want) { + t.Errorf("wrong previous run address\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.Action, plans.Create; got != want { + t.Errorf("wrong planned action\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.ActionReason, plans.ResourceInstanceChangeNoReason; got != want { + t.Errorf("wrong action reason\ngot: %s\nwant: %s", got, want) + } + }) +} diff --git a/internal/terraform/node_resource_plan_orphan.go b/internal/terraform/node_resource_plan_orphan.go index 7f9a683f1..94aa18f98 100644 --- a/internal/terraform/node_resource_plan_orphan.go +++ b/internal/terraform/node_resource_plan_orphan.go @@ -157,13 +157,29 @@ func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalCon func (n *NodePlannableResourceInstanceOrphan) deleteActionReason(ctx EvalContext) plans.ResourceInstanceChangeActionReason { cfg := n.Config if cfg == nil { - // NOTE: We'd ideally detect if the containing module is what's missing - // and then use ResourceInstanceDeleteBecauseNoModule for that case, - // but we don't currently have access to the full configuration here, - // so we need to be less specific. return plans.ResourceInstanceDeleteBecauseNoResourceConfig } + // If this is a resource instance inside a module instance that's no + // longer declared then we will have a config (because config isn't + // instance-specific) but the expander will know that our resource + // address's module path refers to an undeclared module instance. + if expander := ctx.InstanceExpander(); expander != nil { // (sometimes nil in MockEvalContext in tests) + validModuleAddr := expander.GetDeepestExistingModuleInstance(n.Addr.Module) + if len(validModuleAddr) != len(n.Addr.Module) { + // If we get here then at least one step in the resource's module + // path is to a module instance that doesn't exist at all, and + // so a missing module instance is the delete reason regardless + // of whether there might _also_ be a change to the resource + // configuration inside the module. (Conceptually the configurations + // inside the non-existing module instance don't exist at all, + // but they end up existing just as an artifact of the + // implementation detail that we detect module instance orphans + // only dynamically.) + return plans.ResourceInstanceDeleteBecauseNoModule + } + } + switch n.Addr.Resource.Key.(type) { case nil: // no instance key at all if cfg.Count != nil || cfg.ForEach != nil {