Merge pull request #24574 from hashicorp/jbardin/module-references

Point module references to the close node

dag/walk.go

@@ -148,7 +148,6 @@ func (w *Walker) Wait() tfdiags.Diagnostics {
 // Multiple Updates can be called in parallel. Update can be called at any
 // time during a walk.
 func (w *Walker) Update(g *AcyclicGraph) {
-	log.Print("[TRACE] dag/walk: updating graph")
 	w.init()
 	v := make(Set)
 	e := make(Set)
@@ -181,7 +180,6 @@ func (w *Walker) Update(g *AcyclicGraph) {
 		w.wait.Add(1)
 
 		// Add to our own set so we know about it already
-		log.Printf("[TRACE] dag/walk: added new vertex: %q", VertexName(v))
 		w.vertices.Add(raw)
 
 		// Initialize the vertex info
@@ -212,8 +210,6 @@ func (w *Walker) Update(g *AcyclicGraph) {
 
 		// Delete it out of the map
 		delete(w.vertexMap, v)
-
-		log.Printf("[TRACE] dag/walk: removed vertex: %q", VertexName(v))
 		w.vertices.Delete(raw)
 	}
 
@@ -242,10 +238,6 @@ func (w *Walker) Update(g *AcyclicGraph) {
 
 		// Record that the deps changed for this waiter
 		changedDeps.Add(waiter)
-
-		log.Printf(
-			"[TRACE] dag/walk: added edge: %q waiting on %q",
-			VertexName(waiter), VertexName(dep))
 		w.edges.Add(raw)
 	}
 
@@ -266,10 +258,6 @@ func (w *Walker) Update(g *AcyclicGraph) {
 
 		// Record that the deps changed for this waiter
 		changedDeps.Add(waiter)
-
-		log.Printf(
-			"[TRACE] dag/walk: removed edge: %q waiting on %q",
-			VertexName(waiter), VertexName(dep))
 		w.edges.Delete(raw)
 	}
 
@@ -310,10 +298,6 @@ func (w *Walker) Update(g *AcyclicGraph) {
 		}
 		info.depsCancelCh = cancelCh
 
-		log.Printf(
-			"[TRACE] dag/walk: dependencies changed for %q, sending new deps",
-			VertexName(v))
-
 		// Start the waiter
 		go w.waitDeps(v, deps, doneCh, cancelCh)
 	}

terraform/node_module_expand.go

@@ -97,6 +97,27 @@ func (n *nodeExpandModule) EvalTree() EvalNode {
 // empty resources and modules from the state.
 type nodeCloseModule struct {
 	Addr addrs.Module
+
+	// orphaned indicates that this module has no expansion, because it no
+	// longer exists in the configuration
+	orphaned bool
+}
+
+var (
+	_ graphNodeModuleCloser  = (*nodeCloseModule)(nil)
+	_ GraphNodeReferenceable = (*nodeCloseModule)(nil)
+)
+
+func (n *nodeCloseModule) ModulePath() addrs.Module {
+	mod, _ := n.Addr.Call()
+	return mod
+}
+
+func (n *nodeCloseModule) ReferenceableAddrs() []addrs.Referenceable {
+	_, call := n.Addr.Call()
+	return []addrs.Referenceable{
+		call,
+	}
 }
 
 func (n *nodeCloseModule) Name() string {
@@ -124,6 +145,7 @@ func (n *nodeCloseModule) EvalTree() EvalNode {
 				Ops: []walkOperation{walkApply, walkDestroy},
 				Node: &evalCloseModule{
 					Addr:     n.Addr,
+					orphaned: n.orphaned,
 				},
 			},
 		},
@@ -132,6 +154,7 @@ func (n *nodeCloseModule) EvalTree() EvalNode {
 
 type evalCloseModule struct {
 	Addr     addrs.Module
+	orphaned bool
 }
 
 func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) {
@@ -141,7 +164,11 @@ func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) {
 	defer ctx.State().Unlock()
 
 	expander := ctx.InstanceExpander()
-	currentModuleInstances := expander.ExpandModule(n.Addr)
+	var currentModuleInstances []addrs.ModuleInstance
+	// we can't expand if we're just removing
+	if !n.orphaned {
+		currentModuleInstances = expander.ExpandModule(n.Addr)
+	}
 
 	for modKey, mod := range state.Modules {
 		if !n.Addr.Equal(mod.Addr.Module()) {
@@ -155,15 +182,20 @@ func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) {
 			}
 		}
 
-		// if this instance is not in the current expansion, remove it from the
-		// state
 		found := false
+		if n.orphaned {
+			// we're removing the entire module, so all instances must go
+			found = true
+		} else {
+			// if this instance is not in the current expansion, remove it from
+			// the state
 			for _, current := range currentModuleInstances {
 				if current.Equal(mod.Addr) {
 					found = true
 					break
 				}
 			}
+		}
 
 		if !found {
 			if len(mod.Resources) > 0 {

terraform/node_module_removed.go

@@ -1,97 +0,0 @@
-package terraform
-
-import (
-	"fmt"
-
-	"github.com/hashicorp/terraform/addrs"
-)
-
-// NodeModuleRemoved represents a module that is no longer in the
-// config.
-type NodeModuleRemoved struct {
-	Addr addrs.ModuleInstance
-}
-
-var (
-	_ GraphNodeModuleInstance   = (*NodeModuleRemoved)(nil)
-	_ RemovableIfNotTargeted    = (*NodeModuleRemoved)(nil)
-	_ GraphNodeEvalable         = (*NodeModuleRemoved)(nil)
-	_ GraphNodeReferencer       = (*NodeModuleRemoved)(nil)
-	_ GraphNodeReferenceOutside = (*NodeModuleRemoved)(nil)
-)
-
-func (n *NodeModuleRemoved) Name() string {
-	return fmt.Sprintf("%s (removed)", n.Addr.String())
-}
-
-// GraphNodeModuleInstance
-func (n *NodeModuleRemoved) Path() addrs.ModuleInstance {
-	return n.Addr
-}
-
-// GraphNodeModulePath implementation
-func (n *NodeModuleRemoved) ModulePath() addrs.Module {
-	// This node represents the module call within a module,
-	// so return the CallerAddr as the path, as the module
-	// call may expand into multiple child instances
-	return n.Addr.Module()
-}
-
-// GraphNodeEvalable
-func (n *NodeModuleRemoved) EvalTree() EvalNode {
-	return &EvalOpFilter{
-		Ops: []walkOperation{walkRefresh, walkApply, walkDestroy},
-		Node: &EvalCheckModuleRemoved{
-			Addr: n.Addr,
-		},
-	}
-}
-
-func (n *NodeModuleRemoved) ReferenceOutside() (selfPath, referencePath addrs.Module) {
-	// Our "References" implementation indicates that this node depends on
-	// the call to the module it represents, which implicitly depends on
-	// everything inside the module. That reference must therefore be
-	// interpreted in terms of our parent module.
-	return n.Addr.Module(), n.Addr.Parent().Module()
-}
-
-func (n *NodeModuleRemoved) References() []*addrs.Reference {
-	// We depend on the call to the module we represent, because that
-	// implicitly then depends on everything inside that module.
-	// Our ReferenceOutside implementation causes this to be interpreted
-	// within the parent module.
-
-	_, call := n.Addr.CallInstance()
-	return []*addrs.Reference{
-		{
-			Subject: call,
-
-			// No source range here, because there's nothing reasonable for
-			// us to return.
-		},
-	}
-}
-
-// RemovableIfNotTargeted
-func (n *NodeModuleRemoved) RemoveIfNotTargeted() bool {
-	// We need to add this so that this node will be removed if
-	// it isn't targeted or a dependency of a target.
-	return true
-}
-
-// EvalCheckModuleRemoved is an EvalNode implementation that verifies that
-// a module has been removed from the state as expected.
-type EvalCheckModuleRemoved struct {
-	Addr addrs.ModuleInstance
-}
-
-func (n *EvalCheckModuleRemoved) Eval(ctx EvalContext) (interface{}, error) {
-	mod := ctx.State().Module(n.Addr)
-	if mod != nil {
-		// If we get here then that indicates a bug either in the states
-		// module or in an earlier step of the graph walk, since we should've
-		// pruned out the module when the last resource was removed from it.
-		return nil, fmt.Errorf("leftover module %s in state that should have been removed; this is a bug in Terraform and should be reported", n.Addr)
-	}
-	return nil, nil
-}

terraform/transform_module_expansion.go

@@ -21,9 +21,12 @@ type ModuleExpansionTransformer struct {
 	// Concrete allows injection of a wrapped module node by the graph builder
 	// to alter the evaluation behavior.
 	Concrete ConcreteModuleNodeFunc
+
+	closers map[string]*nodeCloseModule
 }
 
 func (t *ModuleExpansionTransformer) Transform(g *Graph) error {
+	t.closers = make(map[string]*nodeCloseModule)
 	// The root module is always a singleton and so does not need expansion
 	// processing, but any descendent modules do. We'll process them
 	// recursively using t.transform.
@@ -33,6 +36,26 @@ func (t *ModuleExpansionTransformer) Transform(g *Graph) error {
 			return err
 		}
 	}
+
+	// Now go through and connect all nodes to their respective module closers.
+	// This is done all at once here, because orphaned modules were already
+	// handled by the RemovedModuleTransformer, and those module closers are in
+	// the graph already, and need to be connected to their parent closers.
+	for _, v := range g.Vertices() {
+		// any node that executes within the scope of a module should be a
+		// GraphNodeModulePath
+		pather, ok := v.(GraphNodeModulePath)
+		if !ok {
+			continue
+		}
+		if closer, ok := t.closers[pather.ModulePath().String()]; ok {
+			// The module root depends on each child resource instance, since
+			// during apply the module expansion will complete before the
+			// individual instances are applied.
+			g.Connect(dag.BasicEdge(closer, v))
+		}
+	}
+
 	return nil
 }
 
@@ -58,16 +81,15 @@ func (t *ModuleExpansionTransformer) transform(g *Graph, c *configs.Config, pare
 		g.Connect(dag.BasicEdge(v, parentNode))
 	}
 
-	// Add the root module node to provide a single exit point for the expanded
-	// module.
-	moduleRoot := &nodeCloseModule{
+	// Add the closer (which acts as the root module node) to provide a
+	// single exit point for the expanded module.
+	closer := &nodeCloseModule{
 		Addr: c.Path,
 	}
-	g.Add(moduleRoot)
-	g.Connect(dag.BasicEdge(moduleRoot, v))
+	g.Add(closer)
+	g.Connect(dag.BasicEdge(closer, v))
+	t.closers[c.Path.String()] = closer
 
-	// Connect any node that reports this module as its Path to ensure that
-	// the module expansion will be handled before that node.
 	for _, childV := range g.Vertices() {
 		pather, ok := childV.(GraphNodeModulePath)
 		if !ok {
@@ -76,11 +98,6 @@ func (t *ModuleExpansionTransformer) transform(g *Graph, c *configs.Config, pare
 		if pather.ModulePath().Equal(c.Path) {
 			log.Printf("[TRACE] ModuleExpansionTransformer: %s must wait for expansion of %s", dag.VertexName(childV), c.Path)
 			g.Connect(dag.BasicEdge(childV, v))
-
-			// The module root also depends on each child instance, since
-			// during apply the module expansion will complete before the
-			// individual instances are applied.
-			g.Connect(dag.BasicEdge(moduleRoot, childV))
 		}
 	}
 

terraform/transform_reference.go

@@ -365,17 +365,6 @@ func NewReferenceMap(vs []dag.Vertex) *ReferenceMap {
 			key := m.mapKey(path, addr)
 			vertices[key] = append(vertices[key], v)
 		}
-
-		// Any node can be referenced by the address of the module it belongs
-		// to or any of that module's ancestors.
-		for _, addr := range path.Ancestors()[1:] {
-			// Can be referenced either as the specific call instance (with
-			// an instance key) or as the bare module call itself (the "module"
-			// block in the parent module that created the instance).
-			callPath, call := addr.Call()
-			callKey := m.mapKey(callPath, call)
-			vertices[callKey] = append(vertices[callKey], v)
-		}
 	}
 
 	m.vertices = vertices

terraform/transform_removed_modules.go

@@ -3,6 +3,7 @@ package terraform
 import (
 	"log"
 
+	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/states"
 )
@@ -20,14 +21,25 @@ func (t *RemovedModuleTransformer) Transform(g *Graph) error {
 		return nil
 	}
 
+	removed := map[string]addrs.Module{}
+
 	for _, m := range t.State.Modules {
 		cc := t.Config.DescendentForInstance(m.Addr)
 		if cc != nil {
 			continue
 		}
-
+		removed[m.Addr.Module().String()] = m.Addr.Module()
 		log.Printf("[DEBUG] %s is no longer in configuration\n", m.Addr)
-		g.Add(&NodeModuleRemoved{Addr: m.Addr})
 	}
+
+	// add closers to collect any module instances we're removing
+	for _, modAddr := range removed {
+		closer := &nodeCloseModule{
+			Addr:     modAddr,
+			orphaned: true,
+		}
+		g.Add(closer)
+	}
+
 	return nil
 }