Merge pull request #28017 from hashicorp/jbardin/data-destroy-deps-2

destroying data source does not require a provider

terraform/context_apply2_test.go

@@ -1,11 +1,14 @@
 package terraform
 
 import (
+	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/providers"
 	"github.com/hashicorp/terraform/states"
+	"github.com/zclconf/go-cty/cty"
 )
 
 // Test that the PreApply hook is called with the correct deposed key
@@ -69,3 +72,108 @@ func TestContext2Apply_createBeforeDestroy_deposedKeyPreApply(t *testing.T) {
 		t.Errorf("expected gen to be %q, but was %q", deposedKey, gen)
 	}
 }
+
+func TestContext2Apply_destroyWithDataSourceExpansion(t *testing.T) {
+	// While managed resources store their destroy-time dependencies, data
+	// sources do not. This means that if a provider were only included in a
+	// destroy graph because of data sources, it could have dependencies which
+	// are not correctly ordered. Here we verify that the provider is not
+	// included in the destroy operation, and all dependency evaluations
+	// succeed.
+
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+module "mod" {
+  source = "./mod"
+}
+
+provider "other" {
+  foo = module.mod.data
+}
+
+# this should not require the provider be present during destroy
+data "other_data_source" "a" {
+}
+`,
+
+		"mod/main.tf": `
+data "test_data_source" "a" {
+  count = 1
+}
+
+data "test_data_source" "b" {
+  count = data.test_data_source.a[0].foo == "ok" ? 1 : 0
+}
+
+output "data" {
+  value = data.test_data_source.a[0].foo == "ok" ? data.test_data_source.b[0].foo : "nope"
+}
+`,
+	})
+
+	testP := testProvider("test")
+	otherP := testProvider("other")
+
+	readData := func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse {
+		return providers.ReadDataSourceResponse{
+			State: cty.ObjectVal(map[string]cty.Value{
+				"id":  cty.StringVal("data_source"),
+				"foo": cty.StringVal("ok"),
+			}),
+		}
+	}
+
+	testP.ReadDataSourceFn = readData
+	otherP.ReadDataSourceFn = readData
+
+	ps := map[addrs.Provider]providers.Factory{
+		addrs.NewDefaultProvider("test"):  testProviderFuncFixed(testP),
+		addrs.NewDefaultProvider("other"): testProviderFuncFixed(otherP),
+	}
+
+	otherP.ConfigureProviderFn = func(req providers.ConfigureProviderRequest) (resp providers.ConfigureProviderResponse) {
+		foo := req.Config.GetAttr("foo")
+		if foo.IsNull() || foo.AsString() != "ok" {
+			resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("incorrect config val: %#v\n", foo))
+		}
+		return resp
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Config:    m,
+		Providers: ps,
+	})
+
+	_, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+
+	_, diags = ctx.Apply()
+	if diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+
+	// now destroy the whole thing
+	ctx = testContext2(t, &ContextOpts{
+		Config:    m,
+		Providers: ps,
+		Destroy:   true,
+	})
+
+	_, diags = ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+
+	otherP.ConfigureProviderFn = func(req providers.ConfigureProviderRequest) (resp providers.ConfigureProviderResponse) {
+		// should not be used to destroy data sources
+		resp.Diagnostics = resp.Diagnostics.Append(errors.New("provider should not be used"))
+		return resp
+	}
+
+	_, diags = ctx.Apply()
+	if diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+}

terraform/graph_builder_import.go

@@ -84,7 +84,7 @@ func (b *ImportGraphBuilder) Steps() []GraphTransformer {
 
 		// Make sure data sources are aware of any depends_on from the
 		// configuration
-		&attachDataResourceDependenciesTransformer{},
+		&attachDataResourceDependsOnTransformer{},
 
 		// Close opened plugin connections
 		&CloseProviderTransformer{},

terraform/graph_builder_plan.go

@@ -137,7 +137,7 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 
 		// Make sure data sources are aware of any depends_on from the
 		// configuration
-		&attachDataResourceDependenciesTransformer{},
+		&attachDataResourceDependsOnTransformer{},
 
 		// Target
 		&TargetsTransformer{Targets: b.Targets},

terraform/node_resource_abstract.go

@@ -62,7 +62,7 @@ type NodeAbstractResource struct {
 	// Set from GraphNodeTargetable
 	Targets []addrs.Targetable
 
-	// Set from AttachResourceDependencies
+	// Set from AttachDataResourceDependsOn
 	dependsOn      []addrs.ConfigResource
 	forceDependsOn bool
 
@@ -71,18 +71,18 @@ type NodeAbstractResource struct {
 }
 
 var (
-	_ GraphNodeReferenceable              = (*NodeAbstractResource)(nil)
-	_ GraphNodeReferencer                 = (*NodeAbstractResource)(nil)
-	_ GraphNodeProviderConsumer           = (*NodeAbstractResource)(nil)
-	_ GraphNodeProvisionerConsumer        = (*NodeAbstractResource)(nil)
-	_ GraphNodeConfigResource             = (*NodeAbstractResource)(nil)
-	_ GraphNodeAttachResourceConfig       = (*NodeAbstractResource)(nil)
-	_ GraphNodeAttachResourceSchema       = (*NodeAbstractResource)(nil)
-	_ GraphNodeAttachProvisionerSchema    = (*NodeAbstractResource)(nil)
-	_ GraphNodeAttachProviderMetaConfigs  = (*NodeAbstractResource)(nil)
-	_ GraphNodeTargetable                 = (*NodeAbstractResource)(nil)
-	_ graphNodeAttachResourceDependencies = (*NodeAbstractResource)(nil)
-	_ dag.GraphNodeDotter                 = (*NodeAbstractResource)(nil)
+	_ GraphNodeReferenceable               = (*NodeAbstractResource)(nil)
+	_ GraphNodeReferencer                  = (*NodeAbstractResource)(nil)
+	_ GraphNodeProviderConsumer            = (*NodeAbstractResource)(nil)
+	_ GraphNodeProvisionerConsumer         = (*NodeAbstractResource)(nil)
+	_ GraphNodeConfigResource              = (*NodeAbstractResource)(nil)
+	_ GraphNodeAttachResourceConfig        = (*NodeAbstractResource)(nil)
+	_ GraphNodeAttachResourceSchema        = (*NodeAbstractResource)(nil)
+	_ GraphNodeAttachProvisionerSchema     = (*NodeAbstractResource)(nil)
+	_ GraphNodeAttachProviderMetaConfigs   = (*NodeAbstractResource)(nil)
+	_ GraphNodeTargetable                  = (*NodeAbstractResource)(nil)
+	_ graphNodeAttachDataResourceDependsOn = (*NodeAbstractResource)(nil)
+	_ dag.GraphNodeDotter                  = (*NodeAbstractResource)(nil)
 )
 
 // NewNodeAbstractResource creates an abstract resource graph node for
@@ -264,8 +264,8 @@ func (n *NodeAbstractResource) SetTargets(targets []addrs.Targetable) {
 	n.Targets = targets
 }
 
-// graphNodeAttachResourceDependencies
-func (n *NodeAbstractResource) AttachResourceDependencies(deps []addrs.ConfigResource, force bool) {
+// graphNodeAttachDataResourceDependsOn
+func (n *NodeAbstractResource) AttachDataResourceDependsOn(deps []addrs.ConfigResource, force bool) {
 	n.dependsOn = deps
 	n.forceDependsOn = force
 }

terraform/node_resource_destroy.go

@@ -43,6 +43,14 @@ func (n *NodeDestroyResourceInstance) Name() string {
 	return n.ResourceInstanceAddr().String() + " (destroy)"
 }
 
+func (n *NodeDestroyResourceInstance) ProvidedBy() (addr addrs.ProviderConfig, exact bool) {
+	if n.Addr.Resource.Resource.Mode == addrs.DataResourceMode {
+		// indicate that this node does not require a configured provider
+		return nil, true
+	}
+	return n.NodeAbstractResourceInstance.ProvidedBy()
+}
+
 // GraphNodeDestroyer
 func (n *NodeDestroyResourceInstance) DestroyAddr() *addrs.AbsResourceInstance {
 	addr := n.ResourceInstanceAddr()
@@ -126,6 +134,20 @@ func (n *NodeDestroyResourceInstance) References() []*addrs.Reference {
 func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) {
 	addr := n.ResourceInstanceAddr()
 
+	// Eval info is different depending on what kind of resource this is
+	switch addr.Resource.Resource.Mode {
+	case addrs.ManagedResourceMode:
+		return n.managedResourceExecute(ctx)
+	case addrs.DataResourceMode:
+		return n.dataResourceExecute(ctx)
+	default:
+		panic(fmt.Errorf("unsupported resource mode %s", n.Config.Mode))
+	}
+}
+
+func (n *NodeDestroyResourceInstance) managedResourceExecute(ctx EvalContext) (diags tfdiags.Diagnostics) {
+	addr := n.ResourceInstanceAddr()
+
 	// Get our state
 	is := n.instanceState
 	if is == nil {
@@ -172,7 +194,7 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation)
 	}
 
 	// Run destroy provisioners if not tainted
-	if state != nil && state.Status != states.ObjectTainted {
+	if state.Status != states.ObjectTainted {
 		applyProvisionersDiags := n.evalApplyProvisioners(ctx, state, false, configs.ProvisionerWhenDestroy)
 		diags = diags.Append(applyProvisionersDiags)
 		// keep the diags separate from the main set until we handle the cleanup
@@ -187,21 +209,15 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation)
 
 	// Managed resources need to be destroyed, while data sources
 	// are only removed from state.
-	if addr.Resource.Resource.Mode == addrs.ManagedResourceMode {
-		// we pass a nil configuration to apply because we are destroying
-		s, d := n.apply(ctx, state, changeApply, nil, false)
-		state, diags = s, diags.Append(d)
-		// we don't return immediately here on error, so that the state can be
-		// finalized
-
-		err := n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState)
-		if err != nil {
-			return diags.Append(err)
-		}
-	} else {
-		log.Printf("[TRACE] NodeDestroyResourceInstance: removing state object for %s", n.Addr)
-		state := ctx.State()
-		state.SetResourceInstanceCurrent(n.Addr, nil, n.ResolvedProvider)
+	// we pass a nil configuration to apply because we are destroying
+	s, d := n.apply(ctx, state, changeApply, nil, false)
+	state, diags = s, diags.Append(d)
+	// we don't return immediately here on error, so that the state can be
+	// finalized
+
+	err = n.writeResourceInstanceState(ctx, state, n.Dependencies, workingState)
+	if err != nil {
+		return diags.Append(err)
 	}
 
 	// create the err value for postApplyHook
@@ -209,3 +225,9 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation)
 	diags = diags.Append(updateStateHook(ctx))
 	return diags
 }
+
+func (n *NodeDestroyResourceInstance) dataResourceExecute(ctx EvalContext) (diags tfdiags.Diagnostics) {
+	log.Printf("[TRACE] NodeDestroyResourceInstance: removing state object for %s", n.Addr)
+	ctx.State().SetResourceInstanceCurrent(n.Addr, nil, n.ResolvedProvider)
+	return diags.Append(updateStateHook(ctx))
+}

terraform/transform_destroy_edge.go

@@ -221,6 +221,13 @@ func (t *pruneUnusedNodesTransformer) Transform(g *Graph) error {
 						}
 					}
 
+				case GraphNodeProvider:
+					// Providers that may have been required by expansion nodes
+					// that we no longer need can also be removed.
+					if g.UpEdges(n).Len() > 0 {
+						return
+					}
+
 				default:
 					return
 				}

terraform/transform_reference.go

@@ -50,7 +50,7 @@ type graphNodeDependsOn interface {
 	DependsOn() []*addrs.Reference
 }
 
-// graphNodeAttachResourceDependencies records all resources that are transitively
+// graphNodeAttachDataResourceDependsOn records all resources that are transitively
 // referenced through depends_on in the configuration. This is used by data
 // resources to determine if they can be read during the plan, or if they need
 // to be further delayed until apply.
@@ -60,18 +60,18 @@ type graphNodeDependsOn interface {
 // unrelated module instances, the fact that it only happens when there are any
 // resource updates pending means we can still avoid the problem of the
 // "perpetual diff"
-type graphNodeAttachResourceDependencies interface {
+type graphNodeAttachDataResourceDependsOn interface {
 	GraphNodeConfigResource
 	graphNodeDependsOn
 
-	// AttachResourceDependencies stored the discovered dependencies in the
+	// AttachDataResourceDependsOn stores the discovered dependencies in the
 	// resource node for evaluation later.
 	//
 	// The force parameter indicates that even if there are no dependencies,
 	// force the data source to act as though there are for refresh purposes.
 	// This is needed because yet-to-be-created resources won't be in the
 	// initial refresh graph, but may still be referenced through depends_on.
-	AttachResourceDependencies(deps []addrs.ConfigResource, force bool)
+	AttachDataResourceDependsOn(deps []addrs.ConfigResource, force bool)
 }
 
 // GraphNodeReferenceOutside is an interface that can optionally be implemented.
@@ -157,12 +157,12 @@ func (m depMap) add(v dag.Vertex) {
 	}
 }
 
-// attachDataResourceDependenciesTransformer records all resources transitively referenced
-// through a configuration depends_on.
-type attachDataResourceDependenciesTransformer struct {
+// attachDataResourceDependsOnTransformer records all resources transitively
+// referenced through a configuration depends_on.
+type attachDataResourceDependsOnTransformer struct {
 }
 
-func (t attachDataResourceDependenciesTransformer) Transform(g *Graph) error {
+func (t attachDataResourceDependsOnTransformer) Transform(g *Graph) error {
 	// First we need to make a map of referenceable addresses to their vertices.
 	// This is very similar to what's done in ReferenceTransformer, but we keep
 	// implementation separate as they may need to change independently.
@@ -170,7 +170,7 @@ func (t attachDataResourceDependenciesTransformer) Transform(g *Graph) error {
 	refMap := NewReferenceMap(vertices)
 
 	for _, v := range vertices {
-		depender, ok := v.(graphNodeAttachResourceDependencies)
+		depender, ok := v.(graphNodeAttachDataResourceDependsOn)
 		if !ok {
 			continue
 		}
@@ -195,7 +195,7 @@ func (t attachDataResourceDependenciesTransformer) Transform(g *Graph) error {
 		}
 
 		log.Printf("[TRACE] attachDataDependenciesTransformer: %s depends on %s", depender.ResourceAddr(), res)
-		depender.AttachResourceDependencies(res, fromModule)
+		depender.AttachDataResourceDependsOn(res, fromModule)
 	}
 
 	return nil