instances: Non-existing module instance has no resource instances

Previously we were treating it as a programming error to ask for the
instances of a resource inside an instance of a module that is declared
but whose declaration doesn't include the given instance key.

However, that's actually a valid situation which can arise if, for
example, the user has changed the repetition/expansion mode for an
existing module call and so now all of the resource instances addresses it
previously contained are "orphaned".

To represent that, we'll instead say that an invalid instance key of a
declared module behaves as if it contains no resource instances at all,
regardless of the configurations of any resources nested inside. This
then gives the result needed to successfully detect all of the former
resource instances as "orphaned" and plan to destroy them.

However, this then introduces a new case for
NodePlannableResourceInstanceOrphan.deleteActionReason to deal with: the
resource configuration still exists (because configuration isn't aware of
individual module/resource instances) but the module instance does not.
This actually allows us to resolve, at least partially, a previous missing
piece of explaining to the user why the resource instances are planned
for deletion in that case, finally allowing us to be explicit to the user
that it's because of the module instance being removed, which
internally we call plans.ResourceInstanceDeleteBecauseNoModule.

Co-authored-by: Alisdair McDiarmid <alisdair@users.noreply.github.com>

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]
+		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)
 }

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`),

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)
+		}
+	})
+}

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 {