stacks: fix errors processing index keys of removed blocks (#36673)

internal/stacks/stackconfig/config_test.go

@@ -37,6 +37,8 @@ func TestLoadConfigDirErrors(t *testing.T) {
 
 	wantDiags := tfdiags.Diagnostics{
 		tfdiags.Sourceless(tfdiags.Error, "Component exists for removed block", "A removed block for component \"a\" was declared without an index, but a component block with the same name was declared at git::https://example.com/errored.git//main.tfstack.hcl:10,1-14.\n\nA removed block without an index indicates that the component and all instances were removed from the configuration, and this is not the case."),
+		tfdiags.Sourceless(tfdiags.Error, "Invalid for_each expression", "A removed block with a for_each expression must reference that expression within the `from` attribute."),
+		tfdiags.Sourceless(tfdiags.Error, "Invalid for_each expression", "A removed block with a for_each expression must reference that expression within the `from` attribute."),
 	}
 
 	count := len(wantDiags)

internal/stacks/stackconfig/removed.go

@@ -127,6 +127,37 @@ func decodeRemovedBlock(block *hcl.Block) (*Removed, tfdiags.Diagnostics) {
 	// reasonable state for careful partial analysis.
 
 	if attr, ok := content.Attributes["for_each"]; ok {
+		if ret.FromIndex == nil {
+			// if we have a for_each expression, then we must have an index
+			// otherwise we'll try and remove the same thing multiple times.
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid for_each expression",
+				Detail:   "A removed block with a for_each expression must reference that expression within the `from` attribute.",
+				Subject:  attr.NameRange.Ptr(),
+			})
+		} else {
+			matches := false
+			for _, variable := range ret.FromIndex.Variables() {
+				if root, ok := variable[0].(hcl.TraverseRoot); ok {
+					if root.Name == "each" {
+						matches = true
+						break
+					}
+				}
+			}
+			if !matches {
+				// You have to refer to the for_each attribute somewhere in the
+				// from attribute.
+				diags = diags.Append(&hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  "Invalid for_each expression",
+					Detail:   "A removed block with a for_each expression must reference that expression within the `from` attribute.",
+					Subject:  attr.NameRange.Ptr(),
+				})
+			}
+		}
+
 		ret.ForEach = attr.Expr
 	}
 	if attr, ok := content.Attributes["providers"]; ok {

internal/stacks/stackconfig/testdata/basics-bundle/errored/main.tfstack.hcl

@@ -30,3 +30,32 @@ removed {
     null = provider.null.a
   }
 }
+
+
+removed {
+  // This is invalid, you must reference the for_each somewhere in the
+  // from attribute if both are present.
+  from = component.b["something"]
+
+  for_each = ["a", "b"]
+
+  source = "./"
+
+  providers = {
+    null = provider.null.a
+  }
+}
+
+removed {
+  // This is invalid, you must reference the for_each somewhere in the
+  // from attribute if both are present.
+  from = component.c
+
+  for_each = ["a", "b"]
+
+  source = "./"
+
+  providers = {
+    null = provider.null.a
+  }
+}

internal/stacks/stackruntime/apply_test.go

@@ -643,6 +643,184 @@ func TestApply(t *testing.T) {
 				},
 			},
 		},
+		"removed component instance direct": {
+			path: filepath.Join("with-single-input", "removed-component-instance-direct"),
+			state: 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",
+						}),
+					})).
+				Build(),
+			store: stacks_testing_provider.NewResourceStoreBuilder().
+				AddResource("removed", cty.ObjectVal(map[string]cty.Value{
+					"id":    cty.StringVal("removed"),
+					"value": cty.StringVal("removed"),
+				})).
+				Build(),
+			cycles: []TestCycle{
+				{
+					planInputs: map[string]cty.Value{
+						"input": cty.SetVal([]cty.Value{
+							cty.StringVal("added"),
+						}),
+					},
+					wantPlannedChanges: []stackplan.PlannedChange{
+						&stackplan.PlannedChangeApplyable{
+							Applyable: true,
+						},
+						// 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"},
+							Action: plans.Create,
+							Before: cty.NullVal(cty.DynamicPseudoType),
+							After: cty.SetVal([]cty.Value{
+								cty.StringVal("added"),
+							}),
+						},
+					},
+					wantAppliedChanges: []stackstate.AppliedChange{
+						&stackstate.AppliedChangeComponentInstance{
+							ComponentAddr:         mustAbsComponent("component.self"),
+							ComponentInstanceAddr: mustAbsComponentInstance("component.self[\"added\"]"),
+							OutputValues:          make(map[addrs.OutputValue]cty.Value),
+							InputVariables: map[addrs.InputVariable]cty.Value{
+								mustInputVariable("id"):    cty.StringVal("added"),
+								mustInputVariable("input"): cty.StringVal("added"),
+							},
+						},
+						&stackstate.AppliedChangeResourceInstanceObject{
+							ResourceInstanceObjectAddr: mustAbsResourceInstanceObject("component.self[\"added\"].testing_resource.data"),
+							NewStateSrc: &states.ResourceInstanceObjectSrc{
+								AttrsJSON: mustMarshalJSONAttrs(map[string]any{
+									"id":    "added",
+									"value": "added",
+								}),
+								Status:       states.ObjectReady,
+								Dependencies: make([]addrs.ConfigResource, 0),
+							},
+							ProviderConfigAddr: mustDefaultRootProvider("testing"),
+							Schema:             stacks_testing_provider.TestingResourceSchema,
+						},
+						&stackstate.AppliedChangeComponentInstanceRemoved{
+							ComponentAddr:         mustAbsComponent("component.self"),
+							ComponentInstanceAddr: mustAbsComponentInstance("component.self[\"removed\"]"),
+						},
+						&stackstate.AppliedChangeResourceInstanceObject{
+							ResourceInstanceObjectAddr: mustAbsResourceInstanceObject("component.self[\"removed\"].testing_resource.data"),
+							ProviderConfigAddr:         mustDefaultRootProvider("testing"),
+							NewStateSrc:                nil,
+							Schema:                     nil,
+						},
+						&stackstate.AppliedChangeInputVariable{
+							Addr: mustStackInputVariable("input"),
+							Value: cty.SetVal([]cty.Value{
+								cty.StringVal("added"),
+							}),
+						},
+					},
+				},
+			},
+		},
 		"removed embedded component": {
 			path: filepath.Join("with-single-input", "removed-embedded-component"),
 			state: stackstate.NewStateBuilder().

internal/stacks/stackruntime/internal/stackeval/main_apply.go

@@ -108,15 +108,18 @@ func ApplyPlan(ctx context.Context, config *stackconfig.Config, plan *stackplan.
 								// it.
 								log.Printf("[TRACE]: %s has planned changes, but was unknown. Check further messages to find out if this was an error.", addr)
 							} else {
-								i, ok := insts[addr.Item.Key]
-								if !ok {
+								for _, i := range insts {
+									if i.from.Item.Key == addr.Item.Key {
+										inst = i
+										break
+									}
+								}
+								if inst == nil {
 									// Again, this might be okay if the component
 									// block was deferred but the removed block had
 									// proper changes (or vice versa). We'll note
 									// this in the logs but just skip processing it.
 									log.Printf("[TRACE]: %s has planned changes, but does not seem to be declared. Check further messages to find out if this was an error.", addr)
-								} else {
-									inst = i
 								}
 							}
 						}

internal/stacks/stackruntime/internal/stackeval/removed.go

@@ -5,11 +5,15 @@ package stackeval
 
 import (
 	"context"
+	"fmt"
+	"time"
 
+	"github.com/hashicorp/hcl/v2"
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/instances"
+	"github.com/hashicorp/terraform/internal/lang"
 	"github.com/hashicorp/terraform/internal/promising"
 	"github.com/hashicorp/terraform/internal/stacks/stackaddrs"
 	"github.com/hashicorp/terraform/internal/stacks/stackplan"
@@ -107,7 +111,56 @@ func (r *Removed) Instances(ctx context.Context, phase EvalPhase) (map[addrs.Ins
 		// First, evaluate the for_each value to get the set of instances the
 		// user has asked to be removed.
 		result := instancesMap(forEachValue, func(ik addrs.InstanceKey, rd instances.RepetitionData) *RemovedInstance {
-			return newRemovedInstance(r, ik, rd, false)
+			expr := r.Config(ctx).config.FromIndex
+			if expr == nil {
+				if ik != addrs.NoKey {
+					// error, but this shouldn't happen as we validate there is
+					// no for each if the expression is null when parsing the
+					// configuration.
+					panic("has FromIndex expression, but no ForEach attribute")
+				}
+
+				from := stackaddrs.AbsComponentInstance{
+					Stack: r.addr.Stack,
+					Item: stackaddrs.ComponentInstance{
+						Component: r.addr.Item,
+						Key:       addrs.NoKey,
+					},
+				}
+
+				return newRemovedInstance(r, from, rd, false)
+			}
+
+			// Otherwise, we're going to parse the FromIndex expression now.
+
+			result, moreDiags := EvalExprAndEvalContext(ctx, expr, phase, &removedInstanceExpressionScope{r, rd})
+			diags = diags.Append(moreDiags)
+			if moreDiags.HasErrors() {
+				return nil
+			}
+
+			key, err := addrs.ParseInstanceKey(result.Value)
+			if err != nil {
+				diags = diags.Append(&hcl.Diagnostic{
+					Severity:    hcl.DiagError,
+					Summary:     "Failed to parse instance key",
+					Detail:      fmt.Sprintf("The `from` attribute contains an invalid instance key for the given address: %s.", err),
+					Subject:     result.Expression.Range().Ptr(),
+					Expression:  result.Expression,
+					EvalContext: result.EvalContext,
+				})
+				return nil
+			}
+
+			from := stackaddrs.AbsComponentInstance{
+				Stack: r.addr.Stack,
+				Item: stackaddrs.ComponentInstance{
+					Component: r.addr.Item,
+					Key:       key,
+				},
+			}
+
+			return newRemovedInstance(r, from, rd, false)
 		})
 
 		// Now, filter out any instances that are not known to the previous
@@ -119,6 +172,12 @@ func (r *Removed) Instances(ctx context.Context, phase EvalPhase) (map[addrs.Ins
 		knownAddrs := make([]stackaddrs.AbsComponentInstance, 0, len(result.insts))
 		knownInstances := make(map[addrs.InstanceKey]*RemovedInstance, len(result.insts))
 		for key, ci := range result.insts {
+			if ci == nil {
+				// if ci is nil, then it means we couldn't process the address
+				// for this instance above
+				continue
+			}
+
 			switch phase {
 			case PlanPhase:
 				if r.main.PlanPrevState().HasComponentInstance(ci.Addr()) {
@@ -153,17 +212,6 @@ func (r *Removed) Instances(ctx context.Context, phase EvalPhase) (map[addrs.Ins
 	return result.insts, result.unknown, diags
 }
 
-func (r *Removed) UnknownInstance(ctx context.Context, phase EvalPhase) *RemovedInstance {
-	inst, err := r.unknownInstance.For(phase).Do(ctx, func(ctx context.Context) (*RemovedInstance, error) {
-		forEachValue, _ := r.ForEachValue(ctx, phase)
-		return newRemovedInstance(r, addrs.WildcardKey, instances.UnknownForEachRepetitionData(forEachValue.Type()), true), nil
-	})
-	if err != nil {
-		panic(err)
-	}
-	return inst
-}
-
 func (r *Removed) PlanIsComplete(ctx context.Context) bool {
 	if !r.main.Planning() {
 		panic("PlanIsComplete used when not in the planning phase")
@@ -234,3 +282,26 @@ func (r *Removed) CheckApply(ctx context.Context) ([]stackstate.AppliedChange, t
 	_, _, diags := r.Instances(ctx, ApplyPhase)
 	return nil, diags
 }
+
+var _ ExpressionScope = (*removedInstanceExpressionScope)(nil)
+
+// removedInstanceExpressionScope is wrapper around the Removed expression
+// scope that also includes repetition data for a specific instance of this
+// removed block.
+type removedInstanceExpressionScope struct {
+	call *Removed
+	rd   instances.RepetitionData
+}
+
+func (r *removedInstanceExpressionScope) ResolveExpressionReference(ctx context.Context, ref stackaddrs.Reference) (Referenceable, tfdiags.Diagnostics) {
+	stack := r.call.Stack(ctx)
+	return stack.resolveExpressionReference(ctx, ref, nil, r.rd)
+}
+
+func (r *removedInstanceExpressionScope) PlanTimestamp() time.Time {
+	return r.call.main.PlanTimestamp()
+}
+
+func (r *removedInstanceExpressionScope) ExternalFunctions(ctx context.Context) (lang.ExternalFuncs, tfdiags.Diagnostics) {
+	return r.call.main.ProviderFunctions(ctx, r.call.Config(ctx).StackConfig(ctx))
+}

internal/stacks/stackruntime/internal/stackeval/removed_instance.go

@@ -37,7 +37,7 @@ var (
 
 type RemovedInstance struct {
 	call     *Removed
-	key      addrs.InstanceKey
+	from     stackaddrs.AbsComponentInstance
 	deferred bool
 
 	main *Main
@@ -47,10 +47,10 @@ type RemovedInstance struct {
 	moduleTreePlan promising.Once[withDiagnostics[*plans.Plan]]
 }
 
-func newRemovedInstance(call *Removed, key addrs.InstanceKey, repetition instances.RepetitionData, deferred bool) *RemovedInstance {
+func newRemovedInstance(call *Removed, from stackaddrs.AbsComponentInstance, repetition instances.RepetitionData, deferred bool) *RemovedInstance {
 	return &RemovedInstance{
 		call:       call,
-		key:        key,
+		from:       from,
 		deferred:   deferred,
 		main:       call.main,
 		repetition: repetition,
@@ -63,15 +63,7 @@ func (r *RemovedInstance) reportNamedPromises(cb func(id promising.PromiseID, na
 }
 
 func (r *RemovedInstance) Addr() stackaddrs.AbsComponentInstance {
-	callAddr := r.call.Addr()
-	stackAddr := callAddr.Stack
-	return stackaddrs.AbsComponentInstance{
-		Stack: stackAddr,
-		Item: stackaddrs.ComponentInstance{
-			Component: callAddr.Item,
-			Key:       r.key,
-		},
-	}
+	return r.from
 }
 
 func (r *RemovedInstance) ModuleTreePlan(ctx context.Context) (*plans.Plan, tfdiags.Diagnostics) {
@@ -82,7 +74,7 @@ func (r *RemovedInstance) ModuleTreePlan(ctx context.Context) (*plans.Plan, tfdi
 		if component != nil {
 			insts, unknown := component.Instances(ctx, PlanPhase)
 			if !unknown {
-				if _, exists := insts[r.key]; exists {
+				if _, exists := insts[r.Addr().Item.Key]; exists {
 					// The instance we're planning to remove is also targeted
 					// by a component block. We won't remove it, and we'll
 					// report a diagnostic to that effect.

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

@@ -692,6 +692,7 @@ func (s *Stack) PlanChanges(ctx context.Context) ([]stackplan.PlannedChange, tfd
 	// if a component is targeted.
 
 	var changes []stackplan.PlannedChange
+Instance:
 	for inst := range s.main.PlanPrevState().AllComponentInstances().All() {
 
 		// We track here whether this component instance has any associated
@@ -747,10 +748,13 @@ func (s *Stack) PlanChanges(ctx context.Context) ([]stackplan.PlannedChange, tfd
 				continue
 			}
 
-			if _, exists := insts[inst.Item.Key]; exists {
-				// This component is targeted by a removed block, so we won't
-				// add an error.
-				continue
+			for _, i := range insts {
+				// the instance key for a removed block doesn't always translate
+				// directly into the instance key in the address, so we have
+				// to check for the correct one.
+				if i.from.Item.Key == inst.Item.Key {
+					continue Instance
+				}
 			}
 		}
 

internal/stacks/stackruntime/internal/stackeval/walk_dynamic.go

@@ -239,7 +239,10 @@ func walkDynamicObjectsInStack[Output any](
 
 					// This instance is not claimed by the component block, so
 					// we'll mark it as being removed by the removed block.
-					inst := newRemovedInstance(removed, inst.Key, instances.RepetitionData{
+					inst := newRemovedInstance(removed, stackaddrs.AbsComponentInstance{
+						Stack: stack.addr,
+						Item:  inst,
+					}, instances.RepetitionData{
 						EachKey:   inst.Key.Value(),
 						EachValue: cty.UnknownVal(cty.DynamicPseudoType),
 					}, true)

internal/stacks/stackruntime/testdata/mainbundle/test/with-single-input/removed-component-instance-direct/removed-component-instance-direct.tfstack.hcl

@@ -0,0 +1,37 @@
+required_providers {
+  testing = {
+    source  = "hashicorp/testing"
+    version = "0.1.0"
+  }
+}
+
+provider "testing" "default" {}
+
+variable "input" {
+  type = set(string)
+}
+
+component "self" {
+  source = "../"
+
+  for_each = var.input
+
+  providers = {
+    testing = provider.testing.default
+  }
+
+  inputs = {
+    id   = each.key
+    input = each.key
+  }
+}
+
+removed {
+  from = component.self["removed"]
+
+  source = "../"
+
+  providers = {
+    testing = provider.testing.default
+  }
+}