diff --git a/internal/terraform/context_apply_test.go b/internal/terraform/context_apply_test.go index a4076eedce..545cf2e133 100644 --- a/internal/terraform/context_apply_test.go +++ b/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 diff --git a/internal/terraform/graph_builder_apply_test.go b/internal/terraform/graph_builder_apply_test.go index 5041749480..c217be2b70 100644 --- a/internal/terraform/graph_builder_apply_test.go +++ b/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) ` diff --git a/internal/terraform/node_resource_apply.go b/internal/terraform/node_resource_apply.go index 3754196924..fa0de372bb 100644 --- a/internal/terraform/node_resource_apply.go +++ b/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 } diff --git a/internal/terraform/transform_destroy_cbd_test.go b/internal/terraform/transform_destroy_cbd_test.go index 240b29bf32..29eaba5a94 100644 --- a/internal/terraform/transform_destroy_cbd_test.go +++ b/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) } diff --git a/internal/terraform/validate_selfref.go b/internal/terraform/validate_selfref.go index 6f38d3e2a4..77cded4524 100644 --- a/internal/terraform/validate_selfref.go +++ b/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 +}