From 3c6b2a8780ad090670f36754cf468817f701b25a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Mar 2021 13:46:28 -0500 Subject: [PATCH 1/4] rename attachDataResourceDependsOnTransformer Clarify the use of this transformer, interface and method which now does not apply to anything but `depends_on` for data sources, --- terraform/graph_builder_import.go | 2 +- terraform/graph_builder_plan.go | 2 +- terraform/node_resource_abstract.go | 30 ++++++++++++++--------------- terraform/transform_reference.go | 20 +++++++++---------- 4 files changed, 27 insertions(+), 27 deletions(-) diff --git a/terraform/graph_builder_import.go b/terraform/graph_builder_import.go index 8754f3ebc..e2bd1788f 100644 --- a/terraform/graph_builder_import.go +++ b/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{}, diff --git a/terraform/graph_builder_plan.go b/terraform/graph_builder_plan.go index 2e3a35ccb..d9e92606b 100644 --- a/terraform/graph_builder_plan.go +++ b/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}, diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index e152a6cb6..85e9dfeb7 100644 --- a/terraform/node_resource_abstract.go +++ b/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 } diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 3760b6c3a..37937d05e 100644 --- a/terraform/transform_reference.go +++ b/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 From 7674e19d4e63382844952d071472b5836a9781a6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Mar 2021 14:51:44 -0500 Subject: [PATCH 2/4] data source destroy nodes do not need a provider Removing a data source is a state-only operation, and the node itself does not require a provider. --- terraform/node_resource_destroy.go | 52 +++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index fa5ba3dc8..09d2dc1d6 100644 --- a/terraform/node_resource_destroy.go +++ b/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 + // 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) + 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)) +} From 58b085f0dc6e5f65f3ed52e464e44f2205965a4b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Mar 2021 14:52:43 -0500 Subject: [PATCH 3/4] prune unused providers from the graph, again The provider transformers remove extra provider nodes when they are initially setup, but it may turn out that they are not used later on. The pruneUnusedNodesTransformer takes care of removing unused expansion nodes, which originally required a provider, and hence may cause some provider nodes to no longer be needed. We can also detect these and remove them during the pruneUnusedNodesTransformer process. --- terraform/transform_destroy_edge.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index c6ec449fc..c3f15871b 100644 --- a/terraform/transform_destroy_edge.go +++ b/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 } From e6ab48addf32ff8d9d20467e50973cc4eaa9ec43 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 8 Mar 2021 15:33:07 -0500 Subject: [PATCH 4/4] test data source destroy with no provider --- terraform/context_apply2_test.go | 108 +++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/terraform/context_apply2_test.go b/terraform/context_apply2_test.go index a9e816fc4..a739db2ad 100644 --- a/terraform/context_apply2_test.go +++ b/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()) + } +}