Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use plan graph builder for destroy #31163

Merged
merged 8 commits into from
Jun 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
138 changes: 138 additions & 0 deletions internal/terraform/context_apply2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package terraform
import (
"errors"
"fmt"
"strings"
"sync"
"testing"
"time"
Expand Down Expand Up @@ -912,3 +913,140 @@ resource "test_resource" "c" {
}
})
}

// pass an input through some expanded values, and back to a provider to make
// sure we can fully evaluate a provider configuration during a destroy plan.
func TestContext2Apply_destroyWithConfiguredProvider(t *testing.T) {
m := testModuleInline(t, map[string]string{
"main.tf": `
variable "in" {
type = map(string)
default = {
"a" = "first"
"b" = "second"
}
}

module "mod" {
source = "./mod"
for_each = var.in
in = each.value
}

locals {
config = [for each in module.mod : each.out]
}

provider "other" {
output = [for each in module.mod : each.out]
local = local.config
var = var.in
}

resource "other_object" "other" {
}
`,
"./mod/main.tf": `
variable "in" {
type = string
}

data "test_object" "d" {
test_string = var.in
}

resource "test_object" "a" {
test_string = var.in
}

output "out" {
value = data.test_object.d.output
}
`})

testProvider := &MockProvider{
GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{
Provider: providers.Schema{Block: simpleTestSchema()},
ResourceTypes: map[string]providers.Schema{
"test_object": providers.Schema{Block: simpleTestSchema()},
},
DataSources: map[string]providers.Schema{
"test_object": providers.Schema{
Block: &configschema.Block{
Attributes: map[string]*configschema.Attribute{
"test_string": {
Type: cty.String,
Optional: true,
},
"output": {
Type: cty.String,
Computed: true,
},
},
},
},
},
},
}

testProvider.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) (resp providers.ReadDataSourceResponse) {
cfg := req.Config.AsValueMap()
s := cfg["test_string"].AsString()
if !strings.Contains("firstsecond", s) {
resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("expected 'first' or 'second', got %s", s))
return resp
}

cfg["output"] = cty.StringVal(s + "-ok")
resp.State = cty.ObjectVal(cfg)
return resp
}

otherProvider := &MockProvider{
GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{
Provider: providers.Schema{
Block: &configschema.Block{
Attributes: map[string]*configschema.Attribute{
"output": {
Type: cty.List(cty.String),
Optional: true,
},
"local": {
Type: cty.List(cty.String),
Optional: true,
},
"var": {
Type: cty.Map(cty.String),
Optional: true,
},
},
},
},
ResourceTypes: map[string]providers.Schema{
"other_object": providers.Schema{Block: simpleTestSchema()},
},
},
}

ctx := testContext2(t, &ContextOpts{
Providers: map[addrs.Provider]providers.Factory{
addrs.NewDefaultProvider("test"): testProviderFuncFixed(testProvider),
addrs.NewDefaultProvider("other"): testProviderFuncFixed(otherProvider),
},
})

opts := SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))
plan, diags := ctx.Plan(m, states.NewState(), opts)
assertNoErrors(t, diags)

state, diags := ctx.Apply(plan, m)
assertNoErrors(t, diags)

// TODO: extend this to ensure the otherProvider is always properly
// configured during the destroy plan
Comment on lines +1045 to +1046
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh... this comment represents exactly what I thought when I first read through this test and saw that it's only testing that the plan succeeds, not that it succeeded for the right reason.

Did you TODO this because there's something blocking us from actually configuring the provider right now? My first instinct here was that it should be possible to check otherProvider.ConfigureProviderResponse after the plan succeeds, but probably there's some extra complexity here I'm not understanding! If so, it would be nice to capture that in this comment too so we can see what needs to be true in order for us TODO what this comment says. 😀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, the provider still won't be configured yet, that will be enabled in a follow up PR. This was intended to be a drop-in replacement of the destroy plan graph with no change in behavior.


opts.Mode = plans.DestroyMode
// destroy only a single instance not included in the moved statements
_, diags = ctx.Plan(m, state, opts)
assertNoErrors(t, diags)
}
2 changes: 1 addition & 1 deletion internal/terraform/context_plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State,
}).Build(addrs.RootModuleInstance)
return graph, walkPlan, diags
case plans.DestroyMode:
graph, diags := (&DestroyPlanGraphBuilder{
graph, diags := DestroyPlanGraphBuilder(&PlanGraphBuilder{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be reasonable to make this be consistent with how we deal with RefreshOnlyMode above and have this instead "just" set a flag in the PlanGraphBuilder struct and let the Build method worry about the details?

I think I ended up leaving it in this odd shape where NormalMode and RefreshOnlyMode are largely the same just because the DestroyMode case was a drastically different shape, but if we normalize these to all just be conditional flags on the struct then we could potentially rework this to avoid duplicating the common fields for each case and only set skipPlanChanges / destroy in the non-normal modes.

Of course fine to leave this for another day if it seems too risky, since it's just some further cleanup after all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was following the pattern used by the validate graph, because I thought I would end up injecting more concrete functions that I ended using. I think as part of the follow up here we should get rid of both the validate and destroy builder entry points, and just make everything a plan ;)

Config: config,
State: prevRunState,
RootVariableValues: opts.SetVariables,
Expand Down
5 changes: 1 addition & 4 deletions internal/terraform/graph_builder_apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
&ForcedCBDTransformer{},

// Destruction ordering
&DestroyEdgeTransformer{
Config: b.Config,
State: b.State,
},
&DestroyEdgeTransformer{},
&CBDEdgeTransformer{
Config: b.Config,
State: b.State,
Expand Down
108 changes: 5 additions & 103 deletions internal/terraform/graph_builder_destroy_plan.go
Original file line number Diff line number Diff line change
@@ -1,115 +1,17 @@
package terraform

import (
"github.com/hashicorp/terraform/internal/addrs"
"github.com/hashicorp/terraform/internal/configs"
"github.com/hashicorp/terraform/internal/dag"
"github.com/hashicorp/terraform/internal/states"
"github.com/hashicorp/terraform/internal/tfdiags"
)

// DestroyPlanGraphBuilder implements GraphBuilder and is responsible for
// planning a pure-destroy.
//
// Planning a pure destroy operation is simple because we can ignore most
// ordering configuration and simply reverse the state. This graph mainly
// exists for targeting, because we need to walk the destroy dependencies to
// ensure we plan the required resources. Without the requirement for
// targeting, the plan could theoretically be created directly from the state.
type DestroyPlanGraphBuilder struct {
// Config is the configuration tree to build the plan from.
Config *configs.Config

// State is the current state
State *states.State

// RootVariableValues are the raw input values for root input variables
// given by the caller, which we'll resolve into final values as part
// of the plan walk.
RootVariableValues InputValues

// Plugins is a library of plug-in components (providers and
// provisioners) available for use.
Plugins *contextPlugins

// Targets are resources to target
Targets []addrs.Targetable

// If set, skipRefresh will cause us stop skip refreshing any existing
// resource instances as part of our planning. This will cause us to fail
// to detect if an object has already been deleted outside of Terraform.
skipRefresh bool
}

// See GraphBuilder
func (b *DestroyPlanGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Diagnostics) {
return (&BasicGraphBuilder{
Steps: b.Steps(),
Name: "DestroyPlanGraphBuilder",
}).Build(path)
}

// See GraphBuilder
func (b *DestroyPlanGraphBuilder) Steps() []GraphTransformer {
concreteResourceInstance := func(a *NodeAbstractResourceInstance) dag.Vertex {
func DestroyPlanGraphBuilder(p *PlanGraphBuilder) GraphBuilder {
p.ConcreteResourceInstance = func(a *NodeAbstractResourceInstance) dag.Vertex {
return &NodePlanDestroyableResourceInstance{
NodeAbstractResourceInstance: a,
skipRefresh: b.skipRefresh,
}
}
concreteResourceInstanceDeposed := func(a *NodeAbstractResourceInstance, key states.DeposedKey) dag.Vertex {
return &NodePlanDeposedResourceInstanceObject{
NodeAbstractResourceInstance: a,
DeposedKey: key,
skipRefresh: b.skipRefresh,
}
}

concreteProvider := func(a *NodeAbstractProvider) dag.Vertex {
return &NodeApplyableProvider{
NodeAbstractProvider: a,
skipRefresh: p.skipRefresh,
}
}
p.destroy = true

steps := []GraphTransformer{
// Creates nodes for the resource instances tracked in the state.
&StateTransformer{
ConcreteCurrent: concreteResourceInstance,
ConcreteDeposed: concreteResourceInstanceDeposed,
State: b.State,
},

// Create the delete changes for root module outputs.
&OutputTransformer{
Config: b.Config,
Destroy: true,
},

// Attach the state
&AttachStateTransformer{State: b.State},

// Attach the configuration to any resources
&AttachResourceConfigTransformer{Config: b.Config},

transformProviders(concreteProvider, b.Config),

// Destruction ordering. We require this only so that
// targeting below will prune the correct things.
&DestroyEdgeTransformer{
Config: b.Config,
State: b.State,
},

&TargetsTransformer{Targets: b.Targets},

// Close opened plugin connections
&CloseProviderTransformer{},

// Close the root module
&CloseRootModuleTransformer{},

&TransitiveReductionTransformer{},
}

return steps
return p
}