Merge pull request #32051 from hashicorp/jbardin/destroy-checkable-ou…

…tputs

Clean up handling of check-related graph nodes

internal/plans/changes_sync.go

@@ -21,24 +21,6 @@ type ChangesSync struct {
 	changes *Changes
 }
 
-// IsFullDestroy returns true if the set of changes indicates we are doing a
-// destroy of all resources.
-func (cs *ChangesSync) IsFullDestroy() bool {
-	if cs == nil {
-		panic("FullDestroy on nil ChangesSync")
-	}
-	cs.lock.Lock()
-	defer cs.lock.Unlock()
-
-	for _, c := range cs.changes.Resources {
-		if c.Action != Delete {
-			return false
-		}
-	}
-
-	return true
-}
-
 // AppendResourceInstanceChange records the given resource instance change in
 // the set of planned resource changes.
 //

internal/plans/plan.go

@@ -28,6 +28,11 @@ type Plan struct {
 	// to the end-user, and so it must not be used to influence apply-time
 	// behavior. The actions during apply must be described entirely by
 	// the Changes field, regardless of how the plan was created.
+	//
+	// FIXME: destroy operations still rely on DestroyMode being set, because
+	// there is no other source of this information in the plan. New behavior
+	// should not be added based on this flag, and changing the flag should be
+	// checked carefully against existing destroy behaviors.
 	UIMode Mode
 
 	VariableValues    map[string]DynamicValue

internal/terraform/context.go

@@ -15,8 +15,6 @@ import (
 	"github.com/hashicorp/terraform/internal/states"
 	"github.com/hashicorp/terraform/internal/tfdiags"
 	"github.com/zclconf/go-cty/cty"
-
-	_ "github.com/hashicorp/terraform/internal/logging"
 )
 
 // InputMode defines what sort of input will be asked for when Input

internal/terraform/context_apply.go

@@ -111,6 +111,18 @@ func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, validate
 		}
 	}
 
+	operation := walkApply
+	if plan.UIMode == plans.DestroyMode {
+		// FIXME: Due to differences in how objects must be handled in the
+		// graph and evaluated during a complete destroy, we must continue to
+		// use plans.DestroyMode to switch on this behavior. If all objects
+		// which require special destroy handling can be tracked in the plan,
+		// then this switch will no longer be needed and we can remove the
+		// walkDestroy operation mode.
+		// TODO: Audit that and remove walkDestroy as an operation mode.
+		operation = walkDestroy
+	}
+
 	graph, moreDiags := (&ApplyGraphBuilder{
 		Config:             config,
 		Changes:            plan.Changes,
@@ -119,22 +131,13 @@ func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, validate
 		Plugins:            c.plugins,
 		Targets:            plan.TargetAddrs,
 		ForceReplace:       plan.ForceReplaceAddrs,
+		Operation:          operation,
 	}).Build(addrs.RootModuleInstance)
 	diags = diags.Append(moreDiags)
 	if moreDiags.HasErrors() {
 		return nil, walkApply, diags
 	}
 
-	operation := walkApply
-	if plan.UIMode == plans.DestroyMode {
-		// NOTE: This is a vestigial violation of the rule that we mustn't
-		// use plan.UIMode to affect apply-time behavior. It's a design error
-		// if anything downstream switches behavior when operation is set
-		// to walkDestroy, but we've not yet fully audited that.
-		// TODO: Audit that and remove walkDestroy as an operation mode.
-		operation = walkDestroy
-	}
-
 	return graph, operation, diags
 }
 

internal/terraform/context_apply2_test.go

@@ -1485,3 +1485,159 @@ resource "test_object" "y" {
 	_, diags = ctx.Apply(plan, m)
 	assertNoErrors(t, diags)
 }
+
+// Outputs should not cause evaluation errors during destroy
+// Check eval from both root level outputs and module outputs, which are
+// handled differently during apply.
+func TestContext2Apply_outputsNotToEvaluate(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+module "mod" {
+  source = "./mod"
+  cond = false
+}
+
+output "from_resource" {
+  value = module.mod.from_resource
+}
+
+output "from_data" {
+  value = module.mod.from_data
+}
+`,
+
+		"./mod/main.tf": `
+variable "cond" {
+  type = bool
+}
+
+module "mod" {
+  source = "../mod2/"
+  cond = var.cond
+}
+
+output "from_resource" {
+  value = module.mod.resource
+}
+
+output "from_data" {
+  value = module.mod.data
+}
+`,
+
+		"./mod2/main.tf": `
+variable "cond" {
+  type = bool
+}
+
+resource "test_object" "x" {
+  count = var.cond ? 0:1
+}
+
+data "test_object" "d" {
+  count = var.cond ? 0:1
+}
+
+output "resource" {
+  value = var.cond ? null : test_object.x.*.test_string[0]
+}
+
+output "data" {
+  value = one(data.test_object.d[*].test_string)
+}
+`})
+
+	p := simpleMockProvider()
+	p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) (resp providers.ReadDataSourceResponse) {
+		resp.State = req.Config
+		return resp
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	// apply the state
+	opts := SimplePlanOpts(plans.NormalMode, nil)
+	plan, diags := ctx.Plan(m, states.NewState(), opts)
+	assertNoErrors(t, diags)
+
+	state, diags := ctx.Apply(plan, m)
+	assertNoErrors(t, diags)
+
+	// and destroy
+	opts = SimplePlanOpts(plans.DestroyMode, nil)
+	plan, diags = ctx.Plan(m, state, opts)
+	assertNoErrors(t, diags)
+
+	state, diags = ctx.Apply(plan, m)
+	assertNoErrors(t, diags)
+
+	// and destroy again with no state
+	if !state.Empty() {
+		t.Fatal("expected empty state, got", state)
+	}
+
+	opts = SimplePlanOpts(plans.DestroyMode, nil)
+	plan, diags = ctx.Plan(m, state, opts)
+	assertNoErrors(t, diags)
+
+	_, diags = ctx.Apply(plan, m)
+	assertNoErrors(t, diags)
+}
+
+// don't evaluate conditions on outputs when destroying
+func TestContext2Apply_noOutputChecksOnDestroy(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+module "mod" {
+  source = "./mod"
+}
+
+output "from_resource" {
+  value = module.mod.from_resource
+}
+`,
+
+		"./mod/main.tf": `
+resource "test_object" "x" {
+  test_string = "wrong val"
+}
+
+output "from_resource" {
+  value = test_object.x.test_string
+  precondition {
+    condition     = test_object.x.test_string == "ok"
+    error_message = "resource error"
+  }
+}
+`})
+
+	p := simpleMockProvider()
+
+	state := states.NewState()
+	mod := state.EnsureModule(addrs.RootModuleInstance.Child("mod", addrs.NoKey))
+	mod.SetResourceInstanceCurrent(
+		mustResourceInstanceAddr("test_object.x").Resource,
+		&states.ResourceInstanceObjectSrc{
+			Status:    states.ObjectReady,
+			AttrsJSON: []byte(`{"test_string":"wrong_val"}`),
+		},
+		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+	)
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	opts := SimplePlanOpts(plans.DestroyMode, nil)
+	plan, diags := ctx.Plan(m, state, opts)
+	assertNoErrors(t, diags)
+
+	_, diags = ctx.Apply(plan, m)
+	assertNoErrors(t, diags)
+}

internal/terraform/context_eval_test.go

@@ -85,3 +85,46 @@ func TestContextEval(t *testing.T) {
 		})
 	}
 }
+
+// ensure that we can execute a console when outputs have preconditions
+func TestContextEval_outputsWithPreconditions(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+module "mod" {
+  source = "./mod"
+  input  = "ok"
+}
+
+output "out" {
+  value = module.mod.out
+}
+`,
+
+		"./mod/main.tf": `
+variable "input" {
+  type = string
+}
+
+output "out" {
+  value = var.input
+
+  precondition {
+    condition     = var.input != ""
+    error_message = "error"
+  }
+}
+`,
+	})
+
+	p := simpleMockProvider()
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	_, diags := ctx.Eval(m, states.NewState(), addrs.RootModuleInstance, &EvalOpts{
+		SetVariables: testInputValuesUnset(m.Module.Variables),
+	})
+	assertNoErrors(t, diags)
+}

internal/terraform/context_plan.go

@@ -334,11 +334,11 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State
 	// must coordinate with this by taking that action only when c.skipRefresh
 	// _is_ set. This coupling between the two is unfortunate but necessary
 	// to work within our current structure.
-	if !opts.SkipRefresh {
+	if !opts.SkipRefresh && !prevRunState.Empty() {
 		log.Printf("[TRACE] Context.destroyPlan: calling Context.plan to get the effect of refreshing the prior state")
-		normalOpts := *opts
-		normalOpts.Mode = plans.NormalMode
-		refreshPlan, refreshDiags := c.plan(config, prevRunState, &normalOpts)
+		refreshOpts := *opts
+		refreshOpts.Mode = plans.RefreshOnlyMode
+		refreshPlan, refreshDiags := c.refreshOnlyPlan(config, prevRunState, &refreshOpts)
 		if refreshDiags.HasErrors() {
 			// NOTE: Normally we'd append diagnostics regardless of whether
 			// there are errors, just in case there are warnings we'd want to

internal/terraform/evaluate.go

@@ -728,7 +728,7 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc
 
 	// Decode all instances in the current state
 	instances := map[addrs.InstanceKey]cty.Value{}
-	pendingDestroy := d.Evaluator.Changes.IsFullDestroy()
+	pendingDestroy := d.Operation == walkDestroy
 	for key, is := range rs.Instances {
 		if is == nil || is.Current == nil {
 			// Assume we're dealing with an instance that hasn't been created yet.

internal/terraform/graph.go

@@ -106,7 +106,7 @@ func (g *Graph) walk(walker GraphWalker) tfdiags.Diagnostics {
 					diags = diags.Append(tfdiags.Sourceless(
 						tfdiags.Error,
 						"Graph node has invalid dynamic subgraph",
-						fmt.Sprintf("The internal logic for %q generated an invalid dynamic subgraph: the root node is %T, which is not a suitable root node type.\n\nThis is a bug in Terraform. Please report it!", dag.VertexName(v), v),
+						fmt.Sprintf("The internal logic for %q generated an invalid dynamic subgraph: the root node is %T, which is not a suitable root node type.\n\nThis is a bug in Terraform. Please report it!", dag.VertexName(v), n),
 					))
 					return
 				}

internal/terraform/graph_builder_apply.go

@@ -46,6 +46,9 @@ type ApplyGraphBuilder struct {
 	// The apply step refers to these as part of verifying that the planned
 	// actions remain consistent between plan and apply.
 	ForceReplace []addrs.AbsResourceInstance
+
+	// Plan Operation this graph will be used for.
+	Operation walkOperation
 }
 
 // See GraphBuilder
@@ -92,7 +95,10 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues},
 		&ModuleVariableTransformer{Config: b.Config},
 		&LocalTransformer{Config: b.Config},
-		&OutputTransformer{Config: b.Config, Changes: b.Changes},
+		&OutputTransformer{
+			Config:       b.Config,
+			ApplyDestroy: b.Operation == walkDestroy,
+		},
 
 		// Creates all the resource instances represented in the diff, along
 		// with dependency edges against the whole-resource nodes added by
@@ -136,7 +142,9 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 		&ForcedCBDTransformer{},
 
 		// Destruction ordering
-		&DestroyEdgeTransformer{},
+		&DestroyEdgeTransformer{
+			Changes: b.Changes,
+		},
 		&CBDEdgeTransformer{
 			Config: b.Config,
 			State:  b.State,

internal/terraform/graph_builder_eval.go

@@ -67,7 +67,10 @@ func (b *EvalGraphBuilder) Steps() []GraphTransformer {
 		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues},
 		&ModuleVariableTransformer{Config: b.Config},
 		&LocalTransformer{Config: b.Config},
-		&OutputTransformer{Config: b.Config},
+		&OutputTransformer{
+			Config:   b.Config,
+			Planning: true,
+		},
 
 		// Attach the configuration to any resources
 		&AttachResourceConfigTransformer{Config: b.Config},

internal/terraform/graph_builder_plan.go

@@ -110,9 +110,9 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 		&ModuleVariableTransformer{Config: b.Config},
 		&LocalTransformer{Config: b.Config},
 		&OutputTransformer{
-			Config:            b.Config,
-			RefreshOnly:       b.skipPlanChanges,
-			removeRootOutputs: b.Operation == walkPlanDestroy,
+			Config:      b.Config,
+			RefreshOnly: b.skipPlanChanges,
+			PlanDestroy: b.Operation == walkPlanDestroy,
 
 			// NOTE: We currently treat anything built with the plan graph
 			// builder as "planning" for our purposes here, because we share