Merge pull request #33403 from hashicorp/jbardin/deps-with-no-instances

connect references from config nodes during apply

internal/terraform/context_apply_test.go

@@ -6463,7 +6463,7 @@ func TestContext2Apply_errorCreateInvalidNew(t *testing.T) {
 	if got, want := diags.Err().Error(), "forced error"; !strings.Contains(got, want) {
 		t.Errorf("returned error does not contain %q, but it should\n%s", want, diags.Err())
 	}
-	if got, want := len(state.RootModule().Resources), 2; got != want {
+	if got, want := len(state.RootModule().Resources), 1; got != want {
 		t.Errorf("%d resources in state before prune; should have %d\n%s", got, want, spew.Sdump(state))
 	}
 	state.PruneResourceHusks() // aws_instance.bar with no instances gets left behind when we bail out, but that's okay

internal/terraform/graph_builder_apply_test.go

@@ -766,9 +766,8 @@ const testPlanWithCheckGraphBuilderStr = `
   aws_instance.baz
 aws_instance.baz
   aws_instance.baz (expand)
-  aws_instance.foo
 aws_instance.baz (expand)
-  provider["registry.terraform.io/hashicorp/aws"]
+  aws_instance.foo
 aws_instance.foo
   aws_instance.foo (expand)
 aws_instance.foo (expand)
@@ -798,11 +797,9 @@ module.child.test_object.create (expand)
   module.child (expand)
   provider["registry.terraform.io/hashicorp/test"]
 module.child.test_object.other
-  module.child.test_object.create
   module.child.test_object.other (expand)
 module.child.test_object.other (expand)
-  module.child (expand)
-  provider["registry.terraform.io/hashicorp/test"]
+  module.child.test_object.create
 provider["registry.terraform.io/hashicorp/test"]
 provider["registry.terraform.io/hashicorp/test"] (close)
   module.child.test_object.other
@@ -815,10 +812,9 @@ test_object.create
 test_object.create (expand)
   provider["registry.terraform.io/hashicorp/test"]
 test_object.other
-  test_object.create
   test_object.other (expand)
 test_object.other (expand)
-  provider["registry.terraform.io/hashicorp/test"]
+  test_object.create
 `
 
 const testApplyGraphBuilderDestroyCountStr = `
@@ -832,9 +828,8 @@ test_object.A (expand)
 test_object.A[1] (destroy)
   provider["registry.terraform.io/hashicorp/test"]
 test_object.B
-  test_object.A (expand)
   test_object.A[1] (destroy)
   test_object.B (expand)
 test_object.B (expand)
-  provider["registry.terraform.io/hashicorp/test"]
+  test_object.A (expand)
 `

internal/terraform/node_resource_apply.go

@@ -4,11 +4,7 @@
 package terraform
 
 import (
-	"log"
-
 	"github.com/hashicorp/terraform/internal/addrs"
-	"github.com/hashicorp/terraform/internal/dag"
-	"github.com/hashicorp/terraform/internal/lang"
 	"github.com/hashicorp/terraform/internal/tfdiags"
 )
 
@@ -21,7 +17,6 @@ type nodeExpandApplyableResource struct {
 }
 
 var (
-	_ GraphNodeDynamicExpandable    = (*nodeExpandApplyableResource)(nil)
 	_ GraphNodeReferenceable        = (*nodeExpandApplyableResource)(nil)
 	_ GraphNodeReferencer           = (*nodeExpandApplyableResource)(nil)
 	_ GraphNodeConfigResource       = (*nodeExpandApplyableResource)(nil)
@@ -34,82 +29,31 @@ func (n *nodeExpandApplyableResource) expandsInstances() {
 }
 
 func (n *nodeExpandApplyableResource) References() []*addrs.Reference {
-	return (&NodeApplyableResource{NodeAbstractResource: n.NodeAbstractResource}).References()
+	refs := n.NodeAbstractResource.References()
+
+	// The expand node needs to connect to the individual resource instances it
+	// references, but cannot refer to it's own instances without causing
+	// cycles. It would be preferable to entirely disallow self references
+	// without the `self` identifier, but those were allowed in provisioners
+	// for compatibility with legacy configuration. We also can't always just
+	// filter them out for all resource node types, because the only method we
+	// have for catching certain invalid configurations are the cycles that
+	// result from these inter-instance references.
+	return filterSelfRefs(n.Addr.Resource, refs)
 }
 
 func (n *nodeExpandApplyableResource) Name() string {
 	return n.NodeAbstractResource.Name() + " (expand)"
 }
 
-func (n *nodeExpandApplyableResource) DynamicExpand(ctx EvalContext) (*Graph, error) {
-	var g Graph
-
+func (n *nodeExpandApplyableResource) Execute(ctx EvalContext, op walkOperation) tfdiags.Diagnostics {
+	var diags tfdiags.Diagnostics
 	expander := ctx.InstanceExpander()
 	moduleInstances := expander.ExpandModule(n.Addr.Module)
 	for _, module := range moduleInstances {
-		g.Add(&NodeApplyableResource{
-			NodeAbstractResource: n.NodeAbstractResource,
-			Addr:                 n.Addr.Resource.Absolute(module),
-		})
-	}
-	addRootNodeToGraph(&g)
-
-	return &g, nil
-}
-
-// NodeApplyableResource represents a resource that is "applyable":
-// it may need to have its record in the state adjusted to match configuration.
-//
-// Unlike in the plan walk, this resource node does not DynamicExpand. Instead,
-// it should be inserted into the same graph as any instances of the nodes
-// with dependency edges ensuring that the resource is evaluated before any
-// of its instances, which will turn ensure that the whole-resource record
-// in the state is suitably prepared to receive any updates to instances.
-type NodeApplyableResource struct {
-	*NodeAbstractResource
-
-	Addr addrs.AbsResource
-}
-
-var (
-	_ GraphNodeModuleInstance       = (*NodeApplyableResource)(nil)
-	_ GraphNodeConfigResource       = (*NodeApplyableResource)(nil)
-	_ GraphNodeExecutable           = (*NodeApplyableResource)(nil)
-	_ GraphNodeProviderConsumer     = (*NodeApplyableResource)(nil)
-	_ GraphNodeAttachResourceConfig = (*NodeApplyableResource)(nil)
-	_ GraphNodeReferencer           = (*NodeApplyableResource)(nil)
-)
-
-func (n *NodeApplyableResource) Path() addrs.ModuleInstance {
-	return n.Addr.Module
-}
-
-func (n *NodeApplyableResource) References() []*addrs.Reference {
-	if n.Config == nil {
-		log.Printf("[WARN] NodeApplyableResource %q: no configuration, so can't determine References", dag.VertexName(n))
-		return nil
+		ctx = ctx.WithPath(module)
+		diags = diags.Append(n.writeResourceState(ctx, n.Addr.Resource.Absolute(module)))
 	}
 
-	var result []*addrs.Reference
-
-	// Since this node type only updates resource-level metadata, we only
-	// need to worry about the parts of the configuration that affect
-	// our "each mode": the count and for_each meta-arguments.
-	refs, _ := lang.ReferencesInExpr(addrs.ParseRef, n.Config.Count)
-	result = append(result, refs...)
-	refs, _ = lang.ReferencesInExpr(addrs.ParseRef, n.Config.ForEach)
-	result = append(result, refs...)
-
-	return result
-}
-
-// GraphNodeExecutable
-func (n *NodeApplyableResource) Execute(ctx EvalContext, op walkOperation) tfdiags.Diagnostics {
-	if n.Config == nil {
-		// Nothing to do, then.
-		log.Printf("[TRACE] NodeApplyableResource: no configuration present for %s", n.Name())
-		return nil
-	}
-
-	return n.writeResourceState(ctx, n.Addr)
+	return diags
 }

internal/terraform/node_resource_apply_test.go

@@ -12,33 +12,40 @@ import (
 	"github.com/hashicorp/terraform/internal/states"
 )
 
-func TestNodeApplyableResourceExecute(t *testing.T) {
+func TestNodeExpandApplyableResourceExecute(t *testing.T) {
 	state := states.NewState()
-	ctx := &MockEvalContext{
-		StateState:               state.SyncWrapper(),
-		InstanceExpanderExpander: instances.NewExpander(),
-	}
-
 	t.Run("no config", func(t *testing.T) {
-		node := NodeApplyableResource{
+		ctx := &MockEvalContext{
+			StateState:               state.SyncWrapper(),
+			InstanceExpanderExpander: instances.NewExpander(),
+		}
+
+		node := &nodeExpandApplyableResource{
 			NodeAbstractResource: &NodeAbstractResource{
+				Addr:   mustConfigResourceAddr("test_instance.foo"),
 				Config: nil,
 			},
-			Addr: mustAbsResourceAddr("test_instance.foo"),
 		}
 		diags := node.Execute(ctx, walkApply)
 		if diags.HasErrors() {
 			t.Fatalf("unexpected error: %s", diags.Err())
 		}
+
+		state.PruneResourceHusks()
 		if !state.Empty() {
 			t.Fatalf("expected no state, got:\n %s", state.String())
 		}
 	})
 
 	t.Run("simple", func(t *testing.T) {
+		ctx := &MockEvalContext{
+			StateState:               state.SyncWrapper(),
+			InstanceExpanderExpander: instances.NewExpander(),
+		}
 
-		node := NodeApplyableResource{
+		node := &nodeExpandApplyableResource{
 			NodeAbstractResource: &NodeAbstractResource{
+				Addr: mustConfigResourceAddr("test_instance.foo"),
 				Config: &configs.Resource{
 					Mode: addrs.ManagedResourceMode,
 					Type: "test_instance",
@@ -49,7 +56,6 @@ func TestNodeApplyableResourceExecute(t *testing.T) {
 					Module:   addrs.RootModule,
 				},
 			},
-			Addr: mustAbsResourceAddr("test_instance.foo"),
 		}
 		diags := node.Execute(ctx, walkApply)
 		if diags.HasErrors() {

internal/terraform/transform_destroy_cbd_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"github.com/hashicorp/terraform/internal/addrs"
+	"github.com/hashicorp/terraform/internal/dag"
 	"github.com/hashicorp/terraform/internal/plans"
 	"github.com/hashicorp/terraform/internal/states"
 )
@@ -61,6 +62,13 @@ func cbdTestSteps(steps []GraphTransformer) []GraphTransformer {
 func filterInstances(g *Graph) *Graph {
 	for _, v := range g.Vertices() {
 		if _, ok := v.(GraphNodeResourceInstance); !ok {
+			// connect around the node to remove it without breaking deps
+			for _, down := range g.DownEdges(v) {
+				for _, up := range g.UpEdges(v) {
+					g.Connect(dag.BasicEdge(up, down))
+				}
+			}
+
 			g.Remove(v)
 		}
 

internal/terraform/validate_selfref.go

@@ -61,3 +61,30 @@ func validateSelfRef(addr addrs.Referenceable, config hcl.Body, providerSchema *
 
 	return diags
 }
+
+// Legacy provisioner configurations may refer to single instances using the
+// resource address. We need to filter these out from the reported references
+// to prevent cycles.
+func filterSelfRefs(self addrs.Resource, refs []*addrs.Reference) []*addrs.Reference {
+	for i := 0; i < len(refs); i++ {
+		ref := refs[i]
+
+		var subject addrs.Resource
+		switch subj := ref.Subject.(type) {
+		case addrs.Resource:
+			subject = subj
+		case addrs.ResourceInstance:
+			subject = subj.ContainingResource()
+		default:
+			continue
+		}
+
+		if self.Equal(subject) {
+			tail := len(refs) - 1
+
+			refs[i], refs[tail] = refs[tail], refs[i]
+			refs = refs[:tail]
+		}
+	}
+	return refs
+}