From 6ec0f97fff9f50ff2b147aa20a8729d34c8297ec Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Tue, 7 Oct 2025 14:11:00 +0200 Subject: [PATCH] refactor: make action diff transformer only responsible for attaching action invocations --- internal/plans/changes_src.go | 2 + internal/terraform/graph_builder_apply.go | 23 +++++ internal/terraform/graph_builder_plan.go | 7 +- .../terraform/node_action_trigger_abstract.go | 9 +- .../terraform/node_action_trigger_apply.go | 82 +++++++++++++++++ internal/terraform/transform_action_diff.go | 89 +++++-------------- .../transform_action_invoke_apply.go | 36 ++++++++ ...oke.go => transform_action_invoke_plan.go} | 6 +- .../transform_action_trigger_config.go | 54 ++++++++--- 9 files changed, 225 insertions(+), 83 deletions(-) create mode 100644 internal/terraform/node_action_trigger_apply.go create mode 100644 internal/terraform/transform_action_invoke_apply.go rename internal/terraform/{transform_action_invoke.go => transform_action_invoke_plan.go} (86%) diff --git a/internal/plans/changes_src.go b/internal/plans/changes_src.go index 426b062f43..37acd2dd36 100644 --- a/internal/plans/changes_src.go +++ b/internal/plans/changes_src.go @@ -617,6 +617,8 @@ func (acs *ActionInvocationInstanceSrc) Less(other *ActionInvocationInstanceSrc) return acs.ActionTrigger.Less(other.ActionTrigger) } +// TODO: Do we still need this? + // FilterLaterActionInvocations returns the list of action invocations that // should be triggered after this one. This function assumes the supplied list // of action invocations has already been filtered to invocations against the diff --git a/internal/terraform/graph_builder_apply.go b/internal/terraform/graph_builder_apply.go index 326a05a2dc..f835b82f41 100644 --- a/internal/terraform/graph_builder_apply.go +++ b/internal/terraform/graph_builder_apply.go @@ -152,6 +152,29 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { Config: b.Config, }, + &ActionTriggerConfigTransformer{ + Config: b.Config, + Operation: b.Operation, + ActionTargets: b.ActionTargets, + + ConcreteActionTriggerNodeFunc: func(node *nodeAbstractActionTriggerExpand, timing RelativeActionTiming) dag.Vertex { + return &nodeActionTriggerApplyExpand{ + nodeAbstractActionTriggerExpand: node, + + relativeTiming: timing, + } + }, + // we want before_* actions to run before and after_* actions to run after the resource + CreateNodesAsAfter: false, + }, + + &ActionInvokeApplyTransformer{ + Config: b.Config, + Operation: b.Operation, + ActionTargets: b.ActionTargets, + Changes: b.Changes, + }, + &ActionDiffTransformer{ Changes: b.Changes, Config: b.Config, diff --git a/internal/terraform/graph_builder_plan.go b/internal/terraform/graph_builder_plan.go index bae28db5bd..4a395ed7e0 100644 --- a/internal/terraform/graph_builder_plan.go +++ b/internal/terraform/graph_builder_plan.go @@ -171,14 +171,17 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { ActionTargets: b.ActionTargets, queryPlanMode: b.queryPlan, - ConcreteActionTriggerNodeFunc: func(node *nodeAbstractActionTriggerExpand) dag.Vertex { + ConcreteActionTriggerNodeFunc: func(node *nodeAbstractActionTriggerExpand, _ RelativeActionTiming) dag.Vertex { return &nodeActionTriggerPlanExpand{ nodeAbstractActionTriggerExpand: node, } }, + + // We plan all actions after the resource is handled + CreateNodesAsAfter: true, }, - &ActionInvokeTransformer{ + &ActionInvokePlanTransformer{ Config: b.Config, Operation: b.Operation, ActionTargets: b.ActionTargets, diff --git a/internal/terraform/node_action_trigger_abstract.go b/internal/terraform/node_action_trigger_abstract.go index 387151ba2d..40cf0e6d59 100644 --- a/internal/terraform/node_action_trigger_abstract.go +++ b/internal/terraform/node_action_trigger_abstract.go @@ -14,9 +14,16 @@ import ( "github.com/hashicorp/terraform/internal/lang/langrefs" ) +type RelativeActionTiming = string + +const ( + RelativeActionTimingBefore = "before" + RelativeActionTimingAfter = "after" +) + // ConcreteActionTriggerNodeFunc is a callback type used to convert an // abstract action trigger to a concrete one of some type. -type ConcreteActionTriggerNodeFunc func(*nodeAbstractActionTriggerExpand) dag.Vertex +type ConcreteActionTriggerNodeFunc func(*nodeAbstractActionTriggerExpand, RelativeActionTiming) dag.Vertex type nodeAbstractActionTriggerExpand struct { Addr addrs.ConfigAction diff --git a/internal/terraform/node_action_trigger_apply.go b/internal/terraform/node_action_trigger_apply.go new file mode 100644 index 0000000000..7c61bb0254 --- /dev/null +++ b/internal/terraform/node_action_trigger_apply.go @@ -0,0 +1,82 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package terraform + +import ( + "fmt" + + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/dag" + "github.com/hashicorp/terraform/internal/lang/langrefs" + "github.com/hashicorp/terraform/internal/plans" + "github.com/hashicorp/terraform/internal/tfdiags" +) + +type nodeActionTriggerApplyExpand struct { + *nodeAbstractActionTriggerExpand + + actionInvocationInstances []*plans.ActionInvocationInstanceSrc + relativeTiming RelativeActionTiming +} + +var ( + _ GraphNodeDynamicExpandable = (*nodeActionTriggerApplyExpand)(nil) + _ GraphNodeReferencer = (*nodeActionTriggerApplyExpand)(nil) +) + +func (n *nodeActionTriggerApplyExpand) Name() string { + return fmt.Sprintf("%s (apply)", n.nodeAbstractActionTriggerExpand.Name()) +} + +func (n *nodeActionTriggerApplyExpand) DynamicExpand(ctx EvalContext) (*Graph, tfdiags.Diagnostics) { + var g Graph + var diags tfdiags.Diagnostics + + if n.lifecycleActionTrigger == nil { + panic("Only actions triggered by plan and apply are supported") + } + + invocationMap := map[*plans.ActionInvocationInstanceSrc]*nodeActionTriggerApplyInstance{} + // We already planned the action invocations, so we can just add them to the graph + for _, ai := range n.actionInvocationInstances { + node := &nodeActionTriggerApplyInstance{ + ActionInvocation: ai, + resolvedProvider: n.resolvedProvider, + ActionTriggerRange: n.lifecycleActionTrigger.invokingSubject.Ptr(), + ConditionExpr: n.lifecycleActionTrigger.conditionExpr, + } + g.Add(node) + invocationMap[ai] = node + } + + for _, ai := range n.actionInvocationInstances { + node := invocationMap[ai] + others := ai.FilterLaterActionInvocations(n.actionInvocationInstances) + for _, other := range others { + otherNode := invocationMap[other] + g.Connect(dag.BasicEdge(otherNode, node)) + } + } + + addRootNodeToGraph(&g) + return &g, diags +} + +func (n *nodeActionTriggerApplyExpand) SetActionInvocationInstances(instances []*plans.ActionInvocationInstanceSrc) { + n.actionInvocationInstances = instances +} + +func (n *nodeActionTriggerApplyExpand) References() []*addrs.Reference { + var refs []*addrs.Reference + refs = append(refs, &addrs.Reference{ + Subject: n.Addr.Action, + }) + + if n.lifecycleActionTrigger != nil { + conditionRefs, _ := langrefs.ReferencesInExpr(addrs.ParseRef, n.lifecycleActionTrigger.conditionExpr) + refs = append(refs, conditionRefs...) + } + + return refs +} diff --git a/internal/terraform/transform_action_diff.go b/internal/terraform/transform_action_diff.go index fbb70a46a8..40b69b99bc 100644 --- a/internal/terraform/transform_action_diff.go +++ b/internal/terraform/transform_action_diff.go @@ -8,7 +8,6 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" - "github.com/hashicorp/terraform/internal/dag" "github.com/hashicorp/terraform/internal/plans" ) @@ -21,85 +20,41 @@ type ActionDiffTransformer struct { func (t *ActionDiffTransformer) Transform(g *Graph) error { applyNodes := addrs.MakeMap[addrs.AbsResourceInstance, *NodeApplyableResourceInstance]() + actionTriggerNodes := addrs.MakeMap[addrs.ConfigResource, []*nodeActionTriggerApplyExpand]() for _, vs := range g.Vertices() { - applyableResource, ok := vs.(*NodeApplyableResourceInstance) - if !ok { - continue + if applyableResource, ok := vs.(*NodeApplyableResourceInstance); ok { + applyNodes.Put(applyableResource.Addr, applyableResource) } - applyNodes.Put(applyableResource.Addr, applyableResource) + if atn, ok := vs.(*nodeActionTriggerApplyExpand); ok { + configResource := actionTriggerNodes.Get(atn.lifecycleActionTrigger.resourceAddress) + actionTriggerNodes.Put(atn.lifecycleActionTrigger.resourceAddress, append(configResource, atn)) + } } - invocationMap := map[*plans.ActionInvocationInstanceSrc]*nodeActionTriggerApplyInstance{} - triggerMap := addrs.MakeMap[addrs.AbsResourceInstance, []*plans.ActionInvocationInstanceSrc]() - for _, action := range t.Changes.ActionInvocations { - // Add nodes for each action invocation - node := &nodeActionTriggerApplyInstance{ - ActionInvocation: action, + for _, ai := range t.Changes.ActionInvocations { + lat, ok := ai.ActionTrigger.(*plans.LifecycleActionTrigger) + if !ok { + continue } + isBefore := lat.ActionTriggerEvent == configs.BeforeCreate || lat.ActionTriggerEvent == configs.BeforeUpdate + isAfter := lat.ActionTriggerEvent == configs.AfterCreate || lat.ActionTriggerEvent == configs.AfterUpdate - // If the action invocations is triggered within the lifecycle of a resource - // we want to add information about the source location to the apply node - if at, ok := action.ActionTrigger.(*plans.LifecycleActionTrigger); ok { - moduleInstance := t.Config.DescendantForInstance(at.TriggeringResourceAddr.Module) - if moduleInstance == nil { - panic(fmt.Sprintf("Could not find module instance for resource %s in config", at.TriggeringResourceAddr.String())) - } - - resourceInstance := moduleInstance.Module.ResourceByAddr(at.TriggeringResourceAddr.Resource.Resource) - if resourceInstance == nil { - panic(fmt.Sprintf("Could not find resource instance for resource %s in config", at.TriggeringResourceAddr.String())) - } - - triggerBlock := resourceInstance.Managed.ActionTriggers[at.ActionTriggerBlockIndex] - if triggerBlock == nil { - panic(fmt.Sprintf("Could not find action trigger block %d for resource %s in config", at.ActionTriggerBlockIndex, at.TriggeringResourceAddr.String())) - } - - triggerMap.Put(at.TriggeringResourceAddr, append(triggerMap.Get(at.TriggeringResourceAddr), action)) - - act := triggerBlock.Actions[at.ActionsListIndex] - node.ActionTriggerRange = &act.Range - node.ConditionExpr = triggerBlock.Condition + atns, ok := actionTriggerNodes.GetOk(lat.TriggeringResourceAddr.ConfigResource()) + if !ok { + return fmt.Errorf("no action trigger nodes found for resource %s", lat.TriggeringResourceAddr) } + // We add the action invocations one by one + for _, atn := range atns { + beforeMatches := atn.relativeTiming == RelativeActionTimingBefore && isBefore + afterMatches := atn.relativeTiming == RelativeActionTimingAfter && isAfter - g.Add(node) - invocationMap[action] = node - - // Add edge to triggering resource - if lat, ok := action.ActionTrigger.(*plans.LifecycleActionTrigger); ok { - // Add edges for lifecycle action triggers - resourceNode, ok := applyNodes.GetOk(lat.TriggeringResourceAddr) - if !ok { - panic("Could not find resource node for lifecycle action trigger") + if (beforeMatches || afterMatches) && atn.lifecycleActionTrigger.actionTriggerBlockIndex == lat.ActionTriggerBlockIndex && atn.lifecycleActionTrigger.actionListIndex == lat.ActionsListIndex { + atn.actionInvocationInstances = append(atn.actionInvocationInstances, ai) } - switch lat.ActionTriggerEvent { - case configs.BeforeCreate, configs.BeforeUpdate, configs.BeforeDestroy: - g.Connect(dag.BasicEdge(resourceNode, node)) - case configs.AfterCreate, configs.AfterUpdate, configs.AfterDestroy: - g.Connect(dag.BasicEdge(node, resourceNode)) - default: - panic("Unknown event") - } } } - // Find all dependencies between action invocations - for _, action := range t.Changes.ActionInvocations { - at, ok := action.ActionTrigger.(*plans.LifecycleActionTrigger) - if !ok { - // only add dependencies between lifecycle actions. invoke actions - // all act independently. - continue - } - - node := invocationMap[action] - others := action.FilterLaterActionInvocations(triggerMap.Get(at.TriggeringResourceAddr)) - for _, other := range others { - otherNode := invocationMap[other] - g.Connect(dag.BasicEdge(otherNode, node)) - } - } return nil } diff --git a/internal/terraform/transform_action_invoke_apply.go b/internal/terraform/transform_action_invoke_apply.go new file mode 100644 index 0000000000..4163d01bef --- /dev/null +++ b/internal/terraform/transform_action_invoke_apply.go @@ -0,0 +1,36 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package terraform + +import ( + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/configs" + "github.com/hashicorp/terraform/internal/plans" +) + +type ActionInvokeApplyTransformer struct { + Config *configs.Config + ActionTargets []addrs.Targetable + Operation walkOperation + Changes *plans.ChangesSrc + + queryPlanMode bool +} + +func (t *ActionInvokeApplyTransformer) Transform(g *Graph) error { + if t.Operation != walkApply || t.queryPlanMode || len(t.ActionTargets) == 0 { + return nil + } + + // We just want to add all invoke triggered action invocations + for _, action := range t.Changes.ActionInvocations { + // Add nodes for each action invocation + node := &nodeActionTriggerApplyInstance{ + ActionInvocation: action, + } + g.Add(node) + } + + return nil +} diff --git a/internal/terraform/transform_action_invoke.go b/internal/terraform/transform_action_invoke_plan.go similarity index 86% rename from internal/terraform/transform_action_invoke.go rename to internal/terraform/transform_action_invoke_plan.go index 5ae491118e..66eabcb7f5 100644 --- a/internal/terraform/transform_action_invoke.go +++ b/internal/terraform/transform_action_invoke_plan.go @@ -10,7 +10,7 @@ import ( "github.com/hashicorp/terraform/internal/configs" ) -type ActionInvokeTransformer struct { +type ActionInvokePlanTransformer struct { Config *configs.Config ActionTargets []addrs.Targetable Operation walkOperation @@ -18,7 +18,7 @@ type ActionInvokeTransformer struct { queryPlanMode bool } -func (t *ActionInvokeTransformer) Transform(g *Graph) error { +func (t *ActionInvokePlanTransformer) Transform(g *Graph) error { if t.Operation != walkPlan || t.queryPlanMode || len(t.ActionTargets) == 0 { return nil } @@ -38,6 +38,8 @@ func (t *ActionInvokeTransformer) Transform(g *Graph) error { if module != nil { config = module.Module.Actions[target.Action.Action.String()] } + default: + return fmt.Errorf("Targeted unknown action type %T", target) } if config == nil { diff --git a/internal/terraform/transform_action_trigger_config.go b/internal/terraform/transform_action_trigger_config.go index 8e035350a2..e76da39bca 100644 --- a/internal/terraform/transform_action_trigger_config.go +++ b/internal/terraform/transform_action_trigger_config.go @@ -20,11 +20,12 @@ type ActionTriggerConfigTransformer struct { queryPlanMode bool ConcreteActionTriggerNodeFunc ConcreteActionTriggerNodeFunc + CreateNodesAsAfter bool } func (t *ActionTriggerConfigTransformer) Transform(g *Graph) error { // We don't want to run if we are using the query plan mode or have targets in place - if t.Operation != walkPlan || t.queryPlanMode || len(t.ActionTargets) > 0 { + if (t.Operation != walkPlan && t.Operation != walkApply) || t.queryPlanMode || len(t.ActionTargets) > 0 { return nil } @@ -71,8 +72,19 @@ func (t *ActionTriggerConfigTransformer) transformSingle(g *Graph, config *confi } for _, r := range config.Module.ManagedResources { - priorNodes := []dag.Vertex{} + priorBeforeNodes := []dag.Vertex{} + priorAfterNodes := []dag.Vertex{} for i, at := range r.Managed.ActionTriggers { + containsBeforeEvent := false + containsAfterEvent := false + for _, event := range at.Events { + if event == configs.BeforeCreate || event == configs.BeforeUpdate { + containsBeforeEvent = true + } else if event == configs.AfterCreate || event == configs.AfterUpdate { + containsAfterEvent = true + } + } + for j, action := range at.Actions { refs, parseRefDiags := langrefs.ReferencesInExpr(addrs.ParseRef, action.Expr) if parseRefDiags != nil { @@ -121,19 +133,39 @@ func (t *ActionTriggerConfigTransformer) transformSingle(g *Graph, config *confi }, } - nat := t.ConcreteActionTriggerNodeFunc(abstract) - g.Add(nat) + // If CreateNodesAsAfter is set we want all nodes to run after the resource + // If not we want expansion nodes only to exist if they are being used + if !t.CreateNodesAsAfter && containsBeforeEvent { + nat := t.ConcreteActionTriggerNodeFunc(abstract, RelativeActionTimingBefore) + g.Add(nat) + + // We want to run before the resource nodes + for _, node := range resourceNode { + g.Connect(dag.BasicEdge(node, nat)) + } - // We always want to plan after the resource is done planning - for _, node := range resourceNode { - g.Connect(dag.BasicEdge(nat, node)) + // We want to run after all prior nodes + for _, priorNode := range priorBeforeNodes { + g.Connect(dag.BasicEdge(nat, priorNode)) + } + priorBeforeNodes = append(priorBeforeNodes, nat) } - // We want to plan after all prior nodes - for _, priorNode := range priorNodes { - g.Connect(dag.BasicEdge(nat, priorNode)) + if t.CreateNodesAsAfter || containsAfterEvent { + nat := t.ConcreteActionTriggerNodeFunc(abstract, RelativeActionTimingAfter) + g.Add(nat) + + // We want to run after the resource nodes + for _, node := range resourceNode { + g.Connect(dag.BasicEdge(nat, node)) + } + + // We want to run after all prior nodes + for _, priorNode := range priorAfterNodes { + g.Connect(dag.BasicEdge(nat, priorNode)) + } + priorAfterNodes = append(priorAfterNodes, nat) } - priorNodes = append(priorNodes, nat) } } }