refactor: make action diff transformer only responsible for attaching action invocations

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

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,

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,

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

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
+}

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
 }

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
+}

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 {

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)
 			}
 		}
 	}