backend/local: treat output changes as side-effects to be applied

This is a baby-step towards an intended future where all Terraform actions
which have side-effects in either remote objects or the Terraform state
can go through the plan+apply workflow.

This initial change is focused only on allowing plan+apply for changes to
root module output values, so that these can be written into a new state
snapshot (for consumption by terraform_remote_state elsewhere) without
having to go outside of the primary workflow by running
"terraform refresh".

This is also better than "terraform refresh" because it gives an
opportunity to review the proposed changes before applying them, as we're
accustomed to with resource changes.

The downside here is that Terraform Core was not designed to produce
accurate changesets for root module outputs. Although we added a place for
it in the plan model in Terraform 0.12, Terraform Core currently produces
inaccurate changesets there which don't properly track the prior values.

We're planning to rework Terraform Core's evaluation approach in a
forthcoming release so it would itself be able to distinguish between the
prior state and the planned new state to produce an accurate changeset,
but this commit introduces a temporary stop-gap solution of implementing
the logic up in the local backend code, where we can freeze a snapshot of
the prior state before we take any other actions and then use that to
produce an accurate output changeset to decide whether the plan has
externally-visible side-effects and render any changes to output values.

This temporary approach should be replaced by a more appropriately-placed
solution in Terraform Core in a release, which should then allow further
behaviors in similar vein, such as user-visible drift detection for
resource instances.

backend/local/backend_apply.go

@@ -57,7 +57,16 @@ func (b *Local) opApply(
 		return
 	}
 
-	// Setup the state
+	// Before we do anything else we'll take a snapshot of the prior state
+	// so we can use it for some fixups to our detection of whether the plan
+	// includes externally-visible side-effects that need to be applied.
+	// (We should be able to remove this once we complete the planned work
+	// described in the comment for func planHasSideEffects in backend_plan.go .)
+	// We go directly to the state manager here because the state inside
+	// tfCtx was already implicitly changed by a validation walk inside
+	// the b.context method.
+	priorState := opState.State().DeepCopy()
+
 	runningOp.State = tfCtx.State()
 
 	// If we weren't given a plan, then we refresh/plan
@@ -83,7 +92,7 @@ func (b *Local) opApply(
 			return
 		}
 
-		trivialPlan := plan.Changes.Empty()
+		trivialPlan := !planHasSideEffects(priorState, plan.Changes)
 		hasUI := op.UIOut != nil && op.UIIn != nil
 		mustConfirm := hasUI && ((op.Destroy && (!op.DestroyForce && !op.AutoApprove)) || (!op.Destroy && !op.AutoApprove && !trivialPlan))
 		if mustConfirm {
@@ -108,7 +117,7 @@ func (b *Local) opApply(
 
 			if !trivialPlan {
 				// Display the plan of what we are going to apply/destroy.
-				b.renderPlan(plan, runningOp.State, tfCtx.Schemas())
+				b.renderPlan(plan, runningOp.State, priorState, tfCtx.Schemas())
 				b.CLI.Output("")
 			}
 

backend/local/backend_plan.go

@@ -10,11 +10,13 @@ import (
 
 	"github.com/mitchellh/cli"
 	"github.com/mitchellh/colorstring"
+	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/backend"
 	"github.com/hashicorp/terraform/command/format"
 	"github.com/hashicorp/terraform/plans"
+	"github.com/hashicorp/terraform/plans/objchange"
 	"github.com/hashicorp/terraform/plans/planfile"
 	"github.com/hashicorp/terraform/states"
 	"github.com/hashicorp/terraform/states/statemgr"
@@ -74,7 +76,16 @@ func (b *Local) opPlan(
 		return
 	}
 
-	// Setup the state
+	// Before we do anything else we'll take a snapshot of the prior state
+	// so we can use it for some fixups to our detection of whether the plan
+	// includes externally-visible side-effects that need to be applied.
+	// (We should be able to remove this once we complete the planned work
+	// described in the comment for func planHasSideEffects below.)
+	// We go directly to the state manager here because the state inside
+	// tfCtx was already implicitly changed by a validation walk inside
+	// the b.context method.
+	priorState := opState.State().DeepCopy()
+
 	runningOp.State = tfCtx.State()
 
 	// If we're refreshing before plan, perform that
@@ -122,8 +133,8 @@ func (b *Local) opPlan(
 		b.ReportResult(runningOp, diags)
 		return
 	}
-	// Record state
-	runningOp.PlanEmpty = plan.Changes.Empty()
+	// Record whether this plan includes any side-effects that could be applied.
+	runningOp.PlanEmpty = !planHasSideEffects(priorState, plan.Changes)
 
 	// Save the plan to disk
 	if path := op.PlanOutPath; path != "" {
@@ -160,14 +171,14 @@ func (b *Local) opPlan(
 	if b.CLI != nil {
 		schemas := tfCtx.Schemas()
 
-		if plan.Changes.Empty() {
+		if runningOp.PlanEmpty {
 			b.CLI.Output("\n" + b.Colorize().Color(strings.TrimSpace(planNoChanges)))
 			// Even if there are no changes, there still could be some warnings
 			b.ShowDiagnostics(diags)
 			return
 		}
 
-		b.renderPlan(plan, baseState, schemas)
+		b.renderPlan(plan, baseState, priorState, schemas)
 
 		// If we've accumulated any warnings along the way then we'll show them
 		// here just before we show the summary and next steps. If we encountered
@@ -194,8 +205,8 @@ func (b *Local) opPlan(
 	}
 }
 
-func (b *Local) renderPlan(plan *plans.Plan, state *states.State, schemas *terraform.Schemas) {
-	RenderPlan(plan, state, schemas, b.CLI, b.Colorize())
+func (b *Local) renderPlan(plan *plans.Plan, baseState *states.State, priorState *states.State, schemas *terraform.Schemas) {
+	RenderPlan(plan, baseState, priorState, schemas, b.CLI, b.Colorize())
 }
 
 // RenderPlan renders the given plan to the given UI.
@@ -209,7 +220,16 @@ func (b *Local) renderPlan(plan *plans.Plan, state *states.State, schemas *terra
 // please consider whether it's time to do the more disruptive refactoring
 // so that something other than the local backend package is offering this
 // functionality.
-func RenderPlan(plan *plans.Plan, state *states.State, schemas *terraform.Schemas, ui cli.Ui, colorize *colorstring.Colorize) {
+//
+// The difference between baseState and priorState is that baseState is the
+// result of implicitly running refresh (unless that was disabled) while
+// priorState is a snapshot of the state as it was before we took any actions
+// at all. priorState can optionally be nil if the caller has only a saved
+// plan and not the prior state it was built from. In that case, changes to
+// output values will not currently be rendered because their prior values
+// are currently stored only in the prior state. (see the docstring for
+// func planHasSideEffects for why this is and when that might change)
+func RenderPlan(plan *plans.Plan, baseState *states.State, priorState *states.State, schemas *terraform.Schemas, ui cli.Ui, colorize *colorstring.Colorize) {
 	counts := map[plans.Action]int{}
 	var rChanges []*plans.ResourceInstanceChangeSrc
 	for _, change := range plan.Changes.Resources {
@@ -280,8 +300,8 @@ func RenderPlan(plan *plans.Plan, state *states.State, schemas *terraform.Schema
 
 		// check if the change is due to a tainted resource
 		tainted := false
-		if !state.Empty() {
-			if is := state.ResourceInstance(rcs.Addr); is != nil {
+		if !baseState.Empty() {
+			if is := baseState.ResourceInstance(rcs.Addr); is != nil {
 				if obj := is.GetGeneration(rcs.DeposedKey.Generation()); obj != nil {
 					tainted = obj.Status == states.ObjectTainted
 				}
@@ -314,6 +334,159 @@ func RenderPlan(plan *plans.Plan, state *states.State, schemas *terraform.Schema
 			"%d to add, %d to change, %d to destroy.",
 		stats[plans.Create], stats[plans.Update], stats[plans.Delete],
 	)))
+
+	// If there is at least one planned change to the root module outputs
+	// then we'll render a summary of those too. This is easier said than done
+	// because currently output changes are not accurately recorded in
+	// plan.Changes.Outputs (see the func planHasSideEffects docstring for why)
+	// and so we must use priorState to produce an actually-accurate changeset
+	// to display.
+	//
+	// Some callers (i.e. "terraform show") only have the plan and therefore
+	// can't provide the prior state. In that case we'll skip showing the
+	// outputs for now, until we can make plan.Changes.Outputs itself be
+	// accurate and self-contained.
+	if priorState != nil {
+		var synthOutputChanges []*plans.OutputChangeSrc
+		outputChangeCount := 0
+		for _, addr := range allRootModuleOutputs(priorState, plan.Changes) {
+			before := cty.NullVal(cty.DynamicPseudoType)
+			after := cty.NullVal(cty.DynamicPseudoType)
+			sensitive := false
+			if changeSrc := plan.Changes.OutputValue(addr); changeSrc != nil {
+				sensitive = sensitive || changeSrc.Sensitive
+				change, err := changeSrc.Decode()
+				if err != nil {
+					// It would be very strange to get here because changeSrc was
+					// presumably just created by Terraform Core and so should never
+					// be invalid.
+					panic(fmt.Sprintf("failed to decode change for %s: %s", addr, err))
+				}
+				after = change.After
+			}
+			if priorOutputState := priorState.OutputValue(addr); priorOutputState != nil {
+				sensitive = sensitive || priorOutputState.Sensitive
+				before = priorOutputState.Value
+			}
+
+			// We'll now construct ourselves a new, accurate change.
+			change := &plans.OutputChange{
+				Addr:      addr,
+				Sensitive: sensitive,
+				Change: plans.Change{
+					Action: objchange.ActionForChange(before, after),
+					Before: before,
+					After:  after,
+				},
+			}
+			if change.Action == plans.NoOp {
+				continue // ignore non-changes
+			}
+			outputChangeCount++
+			newChangeSrc, err := change.Encode()
+			if err != nil {
+				// Again, it would be very strange to see an error here because
+				// we've literally just created this value in memory above.
+				panic(fmt.Sprintf("failed to encode change for %s: %s", addr, err))
+			}
+			synthOutputChanges = append(synthOutputChanges, newChangeSrc)
+		}
+		if outputChangeCount > 0 {
+			ui.Output(colorize.Color("[reset]\n[bold]Changes to Outputs:[reset]" + format.OutputChanges(synthOutputChanges, colorize)))
+		}
+	}
+}
+
+// planHasSideEffects determines whether the given planned changeset has
+// externally-visible side-effects that warrant giving the user an opportunity
+// to apply the plan. If planHasSideEffects returns false, the caller should
+// return a "No changes" message and not offer to apply the plan.
+//
+// This is currently implemented here, rather than in the "terraform" package,
+// because with the current separation of the refresh vs. plan walks there is
+// never any single point in the "terraform" package where both the prior and
+// planned new values for outputs are available at once. We have this out here
+// as a temporary workaround for that design problem, with the intent of moving
+// this down into the "terraform" package once we've completed some work to
+// combine the refresh and plan walks together into a single walk and thus
+// that walk will be able to see both the prior and new values for outputs.
+func planHasSideEffects(priorState *states.State, changes *plans.Changes) bool {
+	if !changes.Empty() {
+		// At the time of writing, changes.Empty considers only resource
+		// changes because the planned changes for outputs are inaccurate.
+		// If we have at least one resource change then we know we have
+		// side-effects though, regardless of outputs.
+		return true
+	}
+
+	// If we get here then there are definitely no resource changes in the plan
+	// but we may have some changes to outputs that "changes" hasn't properly
+	// captured, because it treats all outputs as being either created or
+	// deleted regardless of their prior values. To work around that for now,
+	// we'll use priorState to see if those planned changes really are changes.
+	for _, addr := range allRootModuleOutputs(priorState, changes) {
+		before := cty.NullVal(cty.DynamicPseudoType)
+		after := cty.NullVal(cty.DynamicPseudoType)
+		if changeSrc := changes.OutputValue(addr); changeSrc != nil {
+			change, err := changeSrc.Decode()
+			if err != nil {
+				// It would be very strange to get here because changeSrc was
+				// presumably just created by Terraform Core and so should never
+				// be invalid. In this unlikely event, we'll just conservatively
+				// assume there is a change.
+				return true
+			}
+			after = change.After
+		}
+		if priorState != nil {
+			if priorOutputState := priorState.OutputValue(addr); priorOutputState != nil {
+				before = priorOutputState.Value
+			}
+		}
+		if objchange.ActionForChange(before, after) != plans.NoOp {
+			return true
+		}
+	}
+
+	// If we fall out here then we didn't find any effective changes in the
+	// outputs, and we already showed that there were no resource changes, so
+	// this plan has no side-effects.
+	return false
+}
+
+// allRootModuleOutputs is a helper function to produce the union of all
+// root module output values across both the given prior state and the given
+// changeset. This is to compensate for the fact that the outputs portion of
+// a plans.Changes is currently incomplete and inaccurate due to limitations of
+// Terraform Core's design; we need to use information from the prior state
+// to compensate for those limitations when making decisions based on the
+// effective output changes.
+func allRootModuleOutputs(priorState *states.State, changes *plans.Changes) []addrs.AbsOutputValue {
+	m := make(map[string]addrs.AbsOutputValue)
+	if priorState != nil {
+		for _, os := range priorState.RootModule().OutputValues {
+			m[os.Addr.String()] = os.Addr
+		}
+	}
+	if changes != nil {
+		for _, oc := range changes.Outputs {
+			if !oc.Addr.Module.IsRoot() {
+				continue
+			}
+			m[oc.Addr.String()] = oc.Addr
+		}
+	}
+	if len(m) == 0 {
+		return nil
+	}
+	ret := make([]addrs.AbsOutputValue, 0, len(m))
+	for _, addr := range m {
+		ret = append(ret, addr)
+	}
+	sort.Slice(ret, func(i, j int) bool {
+		return ret[i].OutputValue.Name < ret[j].OutputValue.Name
+	})
+	return ret
 }
 
 const planHeaderIntro = `

backend/local/backend_plan_test.go

@@ -130,6 +130,87 @@ func TestLocal_planNoConfig(t *testing.T) {
 	}
 }
 
+func TestLocal_planOutputsChanged(t *testing.T) {
+	b, cleanup := TestLocal(t)
+	defer cleanup()
+	testStateFile(t, b.StatePath, states.BuildState(func(ss *states.SyncState) {
+		ss.SetOutputValue(addrs.AbsOutputValue{
+			Module:      addrs.RootModuleInstance,
+			OutputValue: addrs.OutputValue{Name: "changed"},
+		}, cty.StringVal("before"), false)
+		ss.SetOutputValue(addrs.AbsOutputValue{
+			Module:      addrs.RootModuleInstance,
+			OutputValue: addrs.OutputValue{Name: "sensitive_before"},
+		}, cty.StringVal("before"), true)
+		ss.SetOutputValue(addrs.AbsOutputValue{
+			Module:      addrs.RootModuleInstance,
+			OutputValue: addrs.OutputValue{Name: "sensitive_after"},
+		}, cty.StringVal("before"), false)
+		ss.SetOutputValue(addrs.AbsOutputValue{
+			Module:      addrs.RootModuleInstance,
+			OutputValue: addrs.OutputValue{Name: "removed"}, // not present in the config fixture
+		}, cty.StringVal("before"), false)
+		ss.SetOutputValue(addrs.AbsOutputValue{
+			Module:      addrs.RootModuleInstance,
+			OutputValue: addrs.OutputValue{Name: "unchanged"},
+		}, cty.StringVal("before"), false)
+		// NOTE: This isn't currently testing the situation where the new
+		// value of an output is unknown, because to do that requires there to
+		// be at least one managed resource Create action in the plan and that
+		// would defeat the point of this test, which is to ensure that a
+		// plan containing only output changes is considered "non-empty".
+		// For now we're not too worried about testing the "new value is
+		// unknown" situation because that's already common for printing out
+		// resource changes and we already have many tests for that.
+	}))
+	b.CLI = cli.NewMockUi()
+	outDir := testTempDir(t)
+	defer os.RemoveAll(outDir)
+	planPath := filepath.Join(outDir, "plan.tfplan")
+	op, configCleanup := testOperationPlan(t, "./testdata/plan-outputs-changed")
+	defer configCleanup()
+	op.PlanRefresh = true
+	op.PlanOutPath = planPath
+	cfg := cty.ObjectVal(map[string]cty.Value{
+		"path": cty.StringVal(b.StatePath),
+	})
+	cfgRaw, err := plans.NewDynamicValue(cfg, cfg.Type())
+	if err != nil {
+		t.Fatal(err)
+	}
+	op.PlanOutBackend = &plans.Backend{
+		// Just a placeholder so that we can generate a valid plan file.
+		Type:   "local",
+		Config: cfgRaw,
+	}
+	run, err := b.Operation(context.Background(), op)
+	if err != nil {
+		t.Fatalf("bad: %s", err)
+	}
+	<-run.Done()
+	if run.Result != backend.OperationSuccess {
+		t.Fatalf("plan operation failed")
+	}
+	if run.PlanEmpty {
+		t.Fatal("plan should not be empty")
+	}
+
+	expectedOutput := strings.TrimSpace(`
+Plan: 0 to add, 0 to change, 0 to destroy.
+
+Changes to Outputs:
+  + added            = "after"
+  ~ changed          = "before" -> "after"
+  - removed          = "before" -> null
+  ~ sensitive_after  = (sensitive value)
+  ~ sensitive_before = (sensitive value)
+`)
+	output := b.CLI.(*cli.MockUi).OutputWriter.String()
+	if !strings.Contains(output, expectedOutput) {
+		t.Fatalf("Unexpected output:\n%s\n\nwant output containing:\n%s", output, expectedOutput)
+	}
+}
+
 func TestLocal_planTainted(t *testing.T) {
 	b, cleanup := TestLocal(t)
 	defer cleanup()