Merge pull request #30151 from hashicorp/f-non-existing-module-instance-crash

core: Fix crash with orphaned module instance

internal/command/format/diff.go

@@ -107,6 +107,11 @@ func ResourceChange(
 		case plans.ResourceInstanceDeleteBecauseNoResourceConfig:
 			buf.WriteString(fmt.Sprintf("\n  # (because %s is not in configuration)", addr.Resource.Resource))
 		case plans.ResourceInstanceDeleteBecauseNoModule:
+			// FIXME: Ideally we'd truncate addr.Module to reflect the earliest
+			// step that doesn't exist, so it's clearer which call this refers
+			// to, but we don't have enough information out here in the UI layer
+			// to decide that; only the "expander" in Terraform Core knows
+			// which module instance keys are actually declared.
 			buf.WriteString(fmt.Sprintf("\n  # (because %s is not in configuration)", addr.Module))
 		case plans.ResourceInstanceDeleteBecauseWrongRepetition:
 			// We have some different variations of this one

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 {