diff --git a/internal/stacks/stackconfig/config_test.go b/internal/stacks/stackconfig/config_test.go index e21a4105e4..6f283b38b5 100644 --- a/internal/stacks/stackconfig/config_test.go +++ b/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) diff --git a/internal/stacks/stackconfig/removed.go b/internal/stacks/stackconfig/removed.go index b65f1f1d46..79d1f445a3 100644 --- a/internal/stacks/stackconfig/removed.go +++ b/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 { diff --git a/internal/stacks/stackconfig/testdata/basics-bundle/errored/main.tfstack.hcl b/internal/stacks/stackconfig/testdata/basics-bundle/errored/main.tfstack.hcl index ea5e907f8a..d753473335 100644 --- a/internal/stacks/stackconfig/testdata/basics-bundle/errored/main.tfstack.hcl +++ b/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 + } +} \ No newline at end of file diff --git a/internal/stacks/stackruntime/apply_test.go b/internal/stacks/stackruntime/apply_test.go index a7e05bce67..d1be66a18a 100644 --- a/internal/stacks/stackruntime/apply_test.go +++ b/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(). diff --git a/internal/stacks/stackruntime/internal/stackeval/main_apply.go b/internal/stacks/stackruntime/internal/stackeval/main_apply.go index 60d70988eb..32532a24e2 100644 --- a/internal/stacks/stackruntime/internal/stackeval/main_apply.go +++ b/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 } } } diff --git a/internal/stacks/stackruntime/internal/stackeval/removed.go b/internal/stacks/stackruntime/internal/stackeval/removed.go index 464af146fa..0ed3c17080 100644 --- a/internal/stacks/stackruntime/internal/stackeval/removed.go +++ b/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)) +} diff --git a/internal/stacks/stackruntime/internal/stackeval/removed_instance.go b/internal/stacks/stackruntime/internal/stackeval/removed_instance.go index c1915b6d9a..bd4b2f2615 100644 --- a/internal/stacks/stackruntime/internal/stackeval/removed_instance.go +++ b/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. diff --git a/internal/stacks/stackruntime/internal/stackeval/stack.go b/internal/stacks/stackruntime/internal/stackeval/stack.go index 188101d15d..cbad6f05dc 100644 --- a/internal/stacks/stackruntime/internal/stackeval/stack.go +++ b/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 + } } } diff --git a/internal/stacks/stackruntime/internal/stackeval/walk_dynamic.go b/internal/stacks/stackruntime/internal/stackeval/walk_dynamic.go index 9a7dfbc7fc..02d46b929b 100644 --- a/internal/stacks/stackruntime/internal/stackeval/walk_dynamic.go +++ b/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) diff --git a/internal/stacks/stackruntime/testdata/mainbundle/test/with-single-input/removed-component-instance-direct/removed-component-instance-direct.tfstack.hcl b/internal/stacks/stackruntime/testdata/mainbundle/test/with-single-input/removed-component-instance-direct/removed-component-instance-direct.tfstack.hcl new file mode 100644 index 0000000000..75f5173858 --- /dev/null +++ b/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 + } +}