stacks: ensure that all components in state are referenced in configuration (#35677)

* stacks: ensure that all components in state are referenced by a component or resource block

* fix compile error

internal/stacks/stackruntime/apply_test.go

@@ -3149,8 +3149,6 @@ func TestApply_RemovedBlocks(t *testing.T) {
 	// TODO: Add tests for and implement the following cases:
 	//   - Removed and component blocks that target the same instance.
 	//   - Edge cases around missing providers and type mismatches.
-	//   - Make sure all instances in state are targeted by a component or a
-	//     removed block.
 	//   - Validate what happens when a removed block foreach evaluates to
 	//     unknown.
 	//   - Add a test for a removed block in an embedded stack.

internal/stacks/stackruntime/helper_test.go

@@ -113,12 +113,12 @@ func expectDiagnosticsForTest(t *testing.T, actual tfdiags.Diagnostics, expected
 			t.Errorf("diagnostic [%d] has wrong severity: %s (expected %s)", ix, actual[ix].Severity(), expected[ix].severity)
 		}
 
-		if actual[ix].Description().Summary != expected[ix].summary {
-			t.Errorf("diagnostic [%d] has wrong summary: %s (expected %s)", ix, actual[ix].Description().Summary, expected[ix].summary)
+		if diff := cmp.Diff(actual[ix].Description().Summary, expected[ix].summary); len(diff) > 0 {
+			t.Errorf("diagnostic [%d] has wrong summary: %s", ix, diff)
 		}
 
-		if actual[ix].Description().Detail != expected[ix].detail {
-			t.Errorf("diagnostic [%d] has wrong detail: %s (expected %s)", ix, actual[ix].Description().Detail, expected[ix].detail)
+		if diff := cmp.Diff(actual[ix].Description().Detail, expected[ix].detail); len(diff) > 0 {
+			t.Errorf("diagnostic [%d] has wrong detail: %s", ix, diff)
 		}
 	}
 }

internal/stacks/stackruntime/internal/stackeval/stack.go

@@ -654,6 +654,8 @@ func (s *Stack) PlanChanges(ctx context.Context) ([]stackplan.PlannedChange, tfd
 		return nil, nil
 	}
 
+	var diags tfdiags.Diagnostics
+
 	// For a root stack we'll return a PlannedChange for each of the output
 	// values, so the caller can see how these would change if this plan is
 	// applied.
@@ -664,6 +666,81 @@ func (s *Stack) PlanChanges(ctx context.Context) ([]stackplan.PlannedChange, tfd
 		panic(fmt.Sprintf("invalid result from Stack.ResultValue: %#v", resultVal))
 	}
 
+	// We want to check that all of the components we have in state are
+	// targeted by something (either a component or a removed block) in
+	// the configuration.
+	//
+	// The root stack analysis is the best place to do this. We must do this
+	// during the plan (and not during the analysis) because we may have
+	// for-each attributes that need to be expanded before we can determine
+	// if a component is targeted.
+
+	for _, inst := range s.main.PlanPrevState().AllComponentInstances().Elems() {
+
+		if s.main.PlanPrevState().ComponentInstanceResourceInstanceObjects(inst).Len() == 0 {
+			// Then this component instance doesn't have any resource instances
+			// associated with it, so it doesn't matter if it is or isn't
+			// targeted by anything in the configuration.
+			//
+			// Perhaps we should modify the applied state to remove empty
+			// components instead of keeping them around?
+			continue
+		}
+
+		stack := s.main.Stack(ctx, inst.Stack, PlanPhase)
+		if stack == nil {
+			diags = diags.Append(tfdiags.Sourceless(
+				tfdiags.Error,
+				"Unclaimed component instance",
+				fmt.Sprintf("The component instance %s is not claimed by any component or removed block in the configuration. Make sure it is instantiated by a component block, or targeted for removal by a removed block.", inst.String()),
+			))
+			continue
+		}
+
+		component, removed := stack.ApplyableComponents(ctx, inst.Item.Component)
+		if component != nil {
+			insts, unknown := component.Instances(ctx, PlanPhase)
+			if unknown {
+				// We can't determine if the component is targeted or not. This
+				// is okay, as any changes to this component will be deferred
+				// anyway and a follow up plan will then detect the missing
+				// component if it exists.
+				continue
+			}
+
+			if _, exists := insts[inst.Item.Key]; exists {
+				// This component is targeted by a component block, so we won't
+				// add an error.
+				continue
+			}
+		}
+
+		if removed != nil {
+			insts, unknown, _ := removed.Instances(ctx, PlanPhase)
+			if unknown {
+				// We can't determine if the component is targeted or not. This
+				// is okay, as any changes to this component will be deferred
+				// anyway and a follow up plan will then detect the missing
+				// component if it exists.
+				continue
+			}
+
+			if _, exists := insts[inst.Item.Key]; exists {
+				// This component is targeted by a removed block, so we won't
+				// add an error.
+				continue
+			}
+		}
+
+		// Otherwise, we have a component that is not targeted by anything in
+		// the configuration. We should add an error.
+		diags = diags.Append(tfdiags.Sourceless(
+			tfdiags.Error,
+			"Unclaimed component instance",
+			fmt.Sprintf("The component instance %s is not claimed by any component or removed block in the configuration. Make sure it is instantiated by a component block, or targeted for removal by a removed block.", inst.String()),
+		))
+	}
+
 	var changes []stackplan.PlannedChange
 	for it := resultVal.ElementIterator(); it.Next(); {
 		k, v := it.Element()
@@ -681,7 +758,6 @@ func (s *Stack) PlanChanges(ctx context.Context) ([]stackplan.PlannedChange, tfd
 			// Any other marks should've been dealt with by our caller before
 			// getting here, since we only know how to preserve the sensitive
 			// marking.
-			var diags tfdiags.Diagnostics
 			diags = diags.Append(fmt.Errorf(
 				"%s%s: unhandled value marks %#v (this is a bug in Terraform)",
 				outputAddr,
@@ -714,7 +790,7 @@ func (s *Stack) PlanChanges(ctx context.Context) ([]stackplan.PlannedChange, tfd
 			NewValueSensitivePaths: sensitivePaths,
 		})
 	}
-	return changes, nil
+	return changes, diags
 }
 
 // CheckApply implements Applyable.

internal/stacks/stackruntime/plan_test.go

@@ -8,12 +8,14 @@ import (
 	"encoding/json"
 	"fmt"
 	"path"
+	"path/filepath"
 	"sort"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/hashicorp/hcl/v2"
 	"github.com/zclconf/go-cty-debug/ctydebug"
 	"github.com/zclconf/go-cty/cty"
@@ -4248,6 +4250,243 @@ func TestPlan_DependsOnUpdatesRequirements(t *testing.T) {
 	}
 }
 
+func TestPlan_RemovedBlocks(t *testing.T) {
+	fakePlanTimestamp, err := time.Parse(time.RFC3339, "1991-08-25T20:57:08Z")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	lock := depsfile.NewLocks()
+	lock.SetProvider(
+		addrs.NewDefaultProvider("testing"),
+		providerreqs.MustParseVersion("0.0.0"),
+		providerreqs.MustParseVersionConstraints("=0.0.0"),
+		providerreqs.PreferredHashes([]providerreqs.Hash{}),
+	)
+
+	tcs := map[string]struct {
+		source          string
+		initialState    *stackstate.State
+		store           *stacks_testing_provider.ResourceStore
+		inputs          map[string]cty.Value
+		wantPlanChanges []stackplan.PlannedChange
+		wantPlanDiags   []expectedDiagnostic
+	}{
+		"orphaned component": {
+			source: filepath.Join("with-single-input", "removed-component-instance"),
+			initialState: stackstate.NewStateBuilder().
+				AddComponentInstance(stackstate.NewComponentInstanceBuilder(mustAbsComponentInstance("component.self[\"removed\"]")).
+					AddInputVariable("id", cty.StringVal("removed")).
+					AddInputVariable("input", cty.StringVal("removed"))).
+				AddResourceInstance(stackstate.NewResourceInstanceBuilder().
+					SetAddr(mustAbsResourceInstanceObject("component.self[\"removed\"].testing_resource.data")).
+					SetProviderAddr(mustDefaultRootProvider("testing")).
+					SetResourceInstanceObjectSrc(states.ResourceInstanceObjectSrc{
+						Status: states.ObjectReady,
+						AttrsJSON: mustMarshalJSONAttrs(map[string]any{
+							"id":    "removed",
+							"value": "removed",
+						}),
+					})).
+				AddComponentInstance(stackstate.NewComponentInstanceBuilder(mustAbsComponentInstance("component.self[\"orphaned\"]")).
+					AddInputVariable("id", cty.StringVal("orphaned")).
+					AddInputVariable("input", cty.StringVal("orphaned"))).
+				AddResourceInstance(stackstate.NewResourceInstanceBuilder().
+					SetAddr(mustAbsResourceInstanceObject("component.self[\"orphaned\"].testing_resource.data")).
+					SetProviderAddr(mustDefaultRootProvider("testing")).
+					SetResourceInstanceObjectSrc(states.ResourceInstanceObjectSrc{
+						Status: states.ObjectReady,
+						AttrsJSON: mustMarshalJSONAttrs(map[string]any{
+							"id":    "orphaned",
+							"value": "orphaned",
+						}),
+					})).
+				Build(),
+			store: stacks_testing_provider.NewResourceStoreBuilder().
+				AddResource("removed", cty.ObjectVal(map[string]cty.Value{
+					"id":    cty.StringVal("removed"),
+					"value": cty.StringVal("removed"),
+				})).
+				AddResource("orphaned", cty.ObjectVal(map[string]cty.Value{
+					"id":    cty.StringVal("orphaned"),
+					"value": cty.StringVal("orphaned"),
+				})).
+				Build(),
+			inputs: map[string]cty.Value{
+				"input": cty.SetVal([]cty.Value{
+					cty.StringVal("added"),
+				}),
+				"removed": cty.SetVal([]cty.Value{
+					cty.StringVal("removed"),
+				}),
+			},
+			wantPlanChanges: []stackplan.PlannedChange{
+				&stackplan.PlannedChangeApplyable{
+					Applyable: false, // No! We have an unclaimed instance!
+				},
+				// we're expecting the new component to be created
+				&stackplan.PlannedChangeComponentInstance{
+					Addr:          mustAbsComponentInstance("component.self[\"added\"]"),
+					PlanComplete:  true,
+					PlanApplyable: true,
+					Action:        plans.Create,
+					PlannedInputValues: map[string]plans.DynamicValue{
+						"id":    mustPlanDynamicValueDynamicType(cty.StringVal("added")),
+						"input": mustPlanDynamicValueDynamicType(cty.StringVal("added")),
+					},
+					PlannedInputValueMarks: map[string][]cty.PathValueMarks{
+						"input": nil,
+						"id":    nil,
+					},
+					PlannedOutputValues: make(map[string]cty.Value),
+					PlannedCheckResults: &states.CheckResults{},
+					PlanTimestamp:       fakePlanTimestamp,
+				},
+				&stackplan.PlannedChangeResourceInstancePlanned{
+					ResourceInstanceObjectAddr: mustAbsResourceInstanceObject("component.self[\"added\"].testing_resource.data"),
+					ChangeSrc: &plans.ResourceInstanceChangeSrc{
+						Addr:        mustAbsResourceInstance("testing_resource.data"),
+						PrevRunAddr: mustAbsResourceInstance("testing_resource.data"),
+						ChangeSrc: plans.ChangeSrc{
+							Action: plans.Create,
+							Before: mustPlanDynamicValue(cty.NullVal(cty.Object(map[string]cty.Type{
+								"id":    cty.String,
+								"value": cty.String,
+							}))),
+							After: mustPlanDynamicValue(cty.ObjectVal(map[string]cty.Value{
+								"id":    cty.StringVal("added"),
+								"value": cty.StringVal("added"),
+							})),
+						},
+						ProviderAddr: mustDefaultRootProvider("testing"),
+					},
+					ProviderConfigAddr: mustDefaultRootProvider("testing"),
+					Schema:             stacks_testing_provider.TestingResourceSchema,
+				},
+				&stackplan.PlannedChangeComponentInstance{
+					Addr:          mustAbsComponentInstance("component.self[\"removed\"]"),
+					PlanComplete:  true,
+					PlanApplyable: true,
+					Mode:          plans.DestroyMode,
+					Action:        plans.Delete,
+					PlannedInputValues: map[string]plans.DynamicValue{
+						"id":    mustPlanDynamicValueDynamicType(cty.StringVal("removed")),
+						"input": mustPlanDynamicValueDynamicType(cty.StringVal("removed")),
+					},
+					PlannedInputValueMarks: map[string][]cty.PathValueMarks{
+						"input": nil,
+						"id":    nil,
+					},
+					PlannedOutputValues: make(map[string]cty.Value),
+					PlannedCheckResults: &states.CheckResults{},
+					PlanTimestamp:       fakePlanTimestamp,
+				},
+				&stackplan.PlannedChangeResourceInstancePlanned{
+					ResourceInstanceObjectAddr: mustAbsResourceInstanceObject("component.self[\"removed\"].testing_resource.data"),
+					ChangeSrc: &plans.ResourceInstanceChangeSrc{
+						Addr:        mustAbsResourceInstance("testing_resource.data"),
+						PrevRunAddr: mustAbsResourceInstance("testing_resource.data"),
+						ChangeSrc: plans.ChangeSrc{
+							Action: plans.Delete,
+							Before: mustPlanDynamicValue(cty.ObjectVal(map[string]cty.Value{
+								"id":    cty.StringVal("removed"),
+								"value": cty.StringVal("removed"),
+							})),
+							After: mustPlanDynamicValue(cty.NullVal(cty.Object(map[string]cty.Type{
+								"id":    cty.String,
+								"value": cty.String,
+							}))),
+						},
+						ProviderAddr: mustDefaultRootProvider("testing"),
+					},
+					PriorStateSrc: &states.ResourceInstanceObjectSrc{
+						AttrsJSON: mustMarshalJSONAttrs(map[string]any{
+							"id":    "removed",
+							"value": "removed",
+						}),
+						Dependencies: make([]addrs.ConfigResource, 0),
+						Status:       states.ObjectReady,
+					},
+					ProviderConfigAddr: mustDefaultRootProvider("testing"),
+					Schema:             stacks_testing_provider.TestingResourceSchema,
+				},
+				&stackplan.PlannedChangeHeader{
+					TerraformVersion: version.SemVer,
+				},
+				&stackplan.PlannedChangePlannedTimestamp{
+					PlannedTimestamp: fakePlanTimestamp,
+				},
+				&stackplan.PlannedChangeRootInputValue{
+					Addr: stackaddrs.InputVariable{Name: "input"},
+					Value: cty.SetVal([]cty.Value{
+						cty.StringVal("added"),
+					}),
+				},
+				&stackplan.PlannedChangeRootInputValue{
+					Addr: stackaddrs.InputVariable{Name: "removed"},
+					Value: cty.SetVal([]cty.Value{
+						cty.StringVal("removed"),
+					}),
+				},
+			},
+			wantPlanDiags: []expectedDiagnostic{
+				{
+					severity: tfdiags.Error,
+					summary:  "Unclaimed component instance",
+					detail:   "The component instance component.self[\"orphaned\"] is not claimed by any component or removed block in the configuration. Make sure it is instantiated by a component block, or targeted for removal by a removed block.",
+				},
+			},
+		},
+	}
+
+	for name, tc := range tcs {
+		t.Run(name, func(t *testing.T) {
+			ctx := context.Background()
+			cfg := loadMainBundleConfigForTest(t, tc.source)
+
+			inputs := make(map[stackaddrs.InputVariable]ExternalInputValue, len(tc.inputs))
+			for name, input := range tc.inputs {
+				inputs[stackaddrs.InputVariable{Name: name}] = ExternalInputValue{
+					Value: input,
+				}
+			}
+
+			providers := map[addrs.Provider]providers.Factory{
+				addrs.NewDefaultProvider("testing"): func() (providers.Interface, error) {
+					return stacks_testing_provider.NewProviderWithData(t, tc.store), nil
+				},
+			}
+
+			planChangesCh := make(chan stackplan.PlannedChange)
+			planDiagsCh := make(chan tfdiags.Diagnostic)
+			planReq := PlanRequest{
+				Config:             cfg,
+				ProviderFactories:  providers,
+				InputValues:        inputs,
+				ForcePlanTimestamp: &fakePlanTimestamp,
+				PrevState:          tc.initialState,
+				DependencyLocks:    *lock,
+			}
+			planResp := PlanResponse{
+				PlannedChanges: planChangesCh,
+				Diagnostics:    planDiagsCh,
+			}
+			go Plan(ctx, &planReq, &planResp)
+			gotPlanChanges, gotPlanDiags := collectPlanOutput(planChangesCh, planDiagsCh)
+
+			sort.SliceStable(gotPlanChanges, func(i, j int) bool {
+				return plannedChangeSortKey(gotPlanChanges[i]) < plannedChangeSortKey(gotPlanChanges[j])
+			})
+			sort.SliceStable(gotPlanDiags, diagnosticSortFunc(gotPlanDiags))
+
+			expectDiagnosticsForTest(t, gotPlanDiags, tc.wantPlanDiags...)
+			if diff := cmp.Diff(tc.wantPlanChanges, gotPlanChanges, ctydebug.CmpOptions, cmpCollectionsSet, cmpopts.IgnoreUnexported(states.ResourceInstanceObjectSrc{})); diff != "" {
+				t.Errorf("wrong changes\n%s", diff)
+			}
+		})
+	}
+}
+
 // collectPlanOutput consumes the two output channels emitting results from
 // a call to [Plan], and collects all of the data written to them before
 // returning once changesCh has been closed by the sender to indicate that