From cac484da16c6e8a4cbd0c1c585736e94ef859f50 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 28 Oct 2025 15:37:24 -0400 Subject: [PATCH 1/2] don't carry all replace addresses through instances Once we have a resource instance we no longer need to have all possible forceReplace addresses available within that instance. We can determine immediately if the instance address is in the set if replacement addresses. This avoids bugs caused by sharing the `-replace` flag addresses across all resource nodes. --- internal/terraform/graph_builder_apply.go | 4 ++- .../node_resource_abstract_instance.go | 29 ++++--------------- .../terraform/node_resource_apply_instance.go | 8 ++--- internal/terraform/node_resource_plan.go | 3 +- .../terraform/node_resource_plan_instance.go | 10 +++---- 5 files changed, 17 insertions(+), 37 deletions(-) diff --git a/internal/terraform/graph_builder_apply.go b/internal/terraform/graph_builder_apply.go index f835b82f41..8835a434e5 100644 --- a/internal/terraform/graph_builder_apply.go +++ b/internal/terraform/graph_builder_apply.go @@ -4,6 +4,8 @@ package terraform import ( + "slices" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/dag" @@ -110,7 +112,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { concreteResourceInstance := func(a *NodeAbstractResourceInstance) dag.Vertex { return &NodeApplyableResourceInstance{ NodeAbstractResourceInstance: a, - forceReplace: b.ForceReplace, + forceReplace: slices.ContainsFunc(b.ForceReplace, a.Addr.Equal), } } diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index e15a5593c3..a1f951c34b 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -795,7 +795,7 @@ func (n *NodeAbstractResourceInstance) plan( plannedChange *plans.ResourceInstanceChange, currentState *states.ResourceInstanceObject, createBeforeDestroy bool, - forceReplace []addrs.AbsResourceInstance, + forceReplace bool, ) (*plans.ResourceInstanceChange, *states.ResourceInstanceObject, *providers.Deferred, instances.RepetitionData, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics var keyData instances.RepetitionData @@ -2975,26 +2975,7 @@ func resourceInstancePrevRunAddr(ctx EvalContext, currentAddr addrs.AbsResourceI return table.OldAddr(currentAddr) } -func getAction(addr addrs.AbsResourceInstance, priorVal, plannedNewVal cty.Value, createBeforeDestroy bool, writeOnly cty.PathSet, forceReplace []addrs.AbsResourceInstance, reqRep cty.PathSet) (action plans.Action, actionReason plans.ResourceInstanceChangeActionReason) { - // The user might also ask us to force replacing a particular resource - // instance, regardless of whether the provider thinks it needs replacing. - // For example, users typically do this if they learn a particular object - // has become degraded in an immutable infrastructure scenario and so - // replacing it with a new object is a viable repair path. - matchedForceReplace := false - for _, candidateAddr := range forceReplace { - if candidateAddr.Equal(addr) { - matchedForceReplace = true - break - } - - // For "force replace" purposes we require an exact resource instance - // address to match. If a user forgets to include the instance key - // for a multi-instance resource then it won't match here, but we - // have an earlier check in NodePlannableResource.Execute that should - // prevent us from getting here in that case. - } - +func getAction(addr addrs.AbsResourceInstance, priorVal, plannedNewVal cty.Value, createBeforeDestroy bool, writeOnly cty.PathSet, forceReplace bool, reqRep cty.PathSet) (action plans.Action, actionReason plans.ResourceInstanceChangeActionReason) { // Unmark for this test for value equality. eqV := plannedNewVal.Equals(priorVal) eq := eqV.IsKnown() && eqV.True() @@ -3002,7 +2983,7 @@ func getAction(addr addrs.AbsResourceInstance, priorVal, plannedNewVal cty.Value switch { case priorVal.IsNull(): action = plans.Create - case matchedForceReplace || !reqRep.Empty() || !writeOnly.Intersection(reqRep).Empty(): + case forceReplace || !reqRep.Empty() || !writeOnly.Intersection(reqRep).Empty(): // If the user "forced replace" of this instance of if there are any // "requires replace" paths left _after our filtering above_ then this // is a replace action. @@ -3012,12 +2993,12 @@ func getAction(addr addrs.AbsResourceInstance, priorVal, plannedNewVal cty.Value action = plans.DeleteThenCreate } switch { - case matchedForceReplace: + case forceReplace: actionReason = plans.ResourceInstanceReplaceByRequest case !reqRep.Empty(): actionReason = plans.ResourceInstanceReplaceBecauseCannotUpdate } - case eq && !matchedForceReplace: + case eq && !forceReplace: action = plans.NoOp default: action = plans.Update diff --git a/internal/terraform/node_resource_apply_instance.go b/internal/terraform/node_resource_apply_instance.go index fbaecc29bc..ee551a1e49 100644 --- a/internal/terraform/node_resource_apply_instance.go +++ b/internal/terraform/node_resource_apply_instance.go @@ -31,11 +31,9 @@ type NodeApplyableResourceInstance struct { graphNodeDeposer // implementation of GraphNodeDeposerConfig - // forceReplace are resource instance addresses where the user wants to - // force generating a replace action. This set isn't pre-filtered, so - // it might contain addresses that have nothing to do with the resource - // that this node represents, which the node itself must therefore ignore. - forceReplace []addrs.AbsResourceInstance + // forceReplace indicates that this resource is being replaced for external + // reasons, like a -replace flag or via replace_triggered_by. + forceReplace bool } var ( diff --git a/internal/terraform/node_resource_plan.go b/internal/terraform/node_resource_plan.go index 2a6e3590bd..1b8763c2af 100644 --- a/internal/terraform/node_resource_plan.go +++ b/internal/terraform/node_resource_plan.go @@ -6,6 +6,7 @@ package terraform import ( "fmt" "log" + "slices" "strings" "github.com/hashicorp/hcl/v2" @@ -586,7 +587,7 @@ func (n *nodeExpandPlannableResource) concreteResource(ctx EvalContext, knownImp ForceCreateBeforeDestroy: n.CreateBeforeDestroy(), skipRefresh: n.skipRefresh, skipPlanChanges: skipPlanChanges, - forceReplace: n.forceReplace, + forceReplace: slices.ContainsFunc(n.forceReplace, a.Addr.Equal), } if importID, ok := knownImports.GetOk(a.Addr); ok { diff --git a/internal/terraform/node_resource_plan_instance.go b/internal/terraform/node_resource_plan_instance.go index 4dec19172f..74f7140a37 100644 --- a/internal/terraform/node_resource_plan_instance.go +++ b/internal/terraform/node_resource_plan_instance.go @@ -40,11 +40,9 @@ type NodePlannableResourceInstance struct { // for any instances. skipPlanChanges bool - // forceReplace are resource instance addresses where the user wants to - // force generating a replace action. This set isn't pre-filtered, so - // it might contain addresses that have nothing to do with the resource - // that this node represents, which the node itself must therefore ignore. - forceReplace []addrs.AbsResourceInstance + // forceReplace indicates that this resource is being replaced for external + // reasons, like a -replace flag or via replace_triggered_by. + forceReplace bool // replaceTriggeredBy stores references from replace_triggered_by which // triggered this instance to be replaced. @@ -568,7 +566,7 @@ func (n *NodePlannableResourceInstance) replaceTriggered(ctx EvalContext, repDat // triggered the replacement in the plan. // Rather than further complicating the plan method with more // options, we can refactor both of these features later. - n.forceReplace = append(n.forceReplace, n.Addr) + n.forceReplace = true log.Printf("[DEBUG] ReplaceTriggeredBy forcing replacement of %s due to change in %s", n.Addr, ref.DisplayString()) n.replaceTriggeredBy = append(n.replaceTriggeredBy, ref) From cfbde8e5fa9f2557d8c47573f73f970e26571566 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 29 Oct 2025 18:00:55 -0400 Subject: [PATCH 2/2] CHANGELOG --- .changes/v1.14/BUG FIXES-20251029-175958.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/v1.14/BUG FIXES-20251029-175958.yaml diff --git a/.changes/v1.14/BUG FIXES-20251029-175958.yaml b/.changes/v1.14/BUG FIXES-20251029-175958.yaml new file mode 100644 index 0000000000..e9f7d9d2ae --- /dev/null +++ b/.changes/v1.14/BUG FIXES-20251029-175958.yaml @@ -0,0 +1,5 @@ +kind: BUG FIXES +body: Combinations of replace_triggered_by and -replace could result in some instances not being replaced +time: 2025-10-29T17:59:58.326396-04:00 +custom: + Issue: "37833"