Merge pull request #26533 from hashicorp/jbardin/plan-output-changes

Use recorded changes for outputs and plan root output removals

command/testdata/show-json/basic-update/output.json

@@ -55,9 +55,9 @@
     "output_changes": {
         "test": {
             "actions": [
-                "create"
+                "no-op"
             ],
-            "before": null,
+            "before": "bar",
             "after": "bar",
             "after_unknown": false
         }

command/testdata/show-json/basic-update/terraform.tfstate

@@ -3,7 +3,12 @@
     "terraform_version": "0.12.0",
     "serial": 7,
     "lineage": "configuredUnchanged",
-    "outputs": {},
+    "outputs": {
+        "test": {
+            "value": "bar",
+            "type": "string"
+        }
+    },
     "resources": [
         {
             "mode": "managed",

command/testdata/show-json/multi-resource-update/output.json

@@ -90,9 +90,9 @@
     "output_changes": {
         "test": {
             "actions": [
-                "create"
+                "no-op"
             ],
-            "before": null,
+            "before": "bar",
             "after": "bar",
             "after_unknown": false
         }

command/testdata/show-json/multi-resource-update/terraform.tfstate

@@ -3,7 +3,12 @@
     "terraform_version": "0.12.0",
     "serial": 7,
     "lineage": "configuredUnchanged",
-    "outputs": {},
+    "outputs": {
+        "test": {
+            "value": "bar",
+            "type": "string"
+        }
+    },
     "resources": [
         {
             "mode": "managed",

terraform/context.go

@@ -277,7 +277,6 @@ func (c *Context) Graph(typ GraphType, opts *ContextGraphOpts) (*Graph, tfdiags.
 			Components: c.components,
 			Schemas:    c.schemas,
 			Targets:    c.targets,
-			Destroy:    c.destroy,
 			Validate:   opts.Validate,
 		}).Build(addrs.RootModuleInstance)
 

terraform/context_apply_test.go

@@ -9142,10 +9142,13 @@ func TestContext2Apply_plannedDestroyInterpolatedCount(t *testing.T) {
 	}
 
 	// Applying the plan should now succeed
-	_, diags = ctx.Apply()
+	state, diags = ctx.Apply()
 	if diags.HasErrors() {
 		t.Fatalf("apply failed: %s", diags.Err())
 	}
+	if !state.Empty() {
+		t.Fatalf("state not empty: %s\n", state)
+	}
 }
 
 func TestContext2Apply_scaleInMultivarRef(t *testing.T) {

terraform/graph_builder_apply.go

@@ -40,9 +40,6 @@ type ApplyGraphBuilder struct {
 	// outputs should go into the diff so that this is unnecessary.
 	Targets []addrs.Targetable
 
-	// Destroy, if true, represents a pure destroy operation
-	Destroy bool
-
 	// Validate will do structural validation of the graph.
 	Validate bool
 }
@@ -91,7 +88,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 		&RootVariableTransformer{Config: b.Config},
 		&ModuleVariableTransformer{Config: b.Config},
 		&LocalTransformer{Config: b.Config},
-		&OutputTransformer{Config: b.Config},
+		&OutputTransformer{Config: b.Config, Changes: b.Changes},
 
 		// Creates all the resource instances represented in the diff, along
 		// with dependency edges against the whole-resource nodes added by
@@ -150,17 +147,6 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 			Schemas: b.Schemas,
 		},
 
-		// Create a destroy node for root outputs to remove them from the
-		// state.  This does nothing unless invoked via the destroy command
-		// directly.  A destroy is identical to a normal apply, except for the
-		// fact that we also have configuration to evaluate. While the rest of
-		// the unused nodes can be programmatically pruned (via
-		// pruneUnusedNodesTransformer), root module outputs always have an
-		// implied dependency on remote state. This means that if they exist in
-		// the configuration, the only signal to remove them is via the destroy
-		// command itself.
-		&destroyRootOutputTransformer{Destroy: b.Destroy},
-
 		// We need to remove configuration nodes that are not used at all, as
 		// they may not be able to evaluate, especially during destroy.
 		// These include variables, locals, and instance expanders.

terraform/graph_builder_apply_test.go

@@ -468,7 +468,6 @@ func TestApplyGraphBuilder_provisionerDestroy(t *testing.T) {
 	}
 
 	b := &ApplyGraphBuilder{
-		Destroy:    true,
 		Config:     testModule(t, "graph-builder-apply-provisioner"),
 		Changes:    changes,
 		Components: simpleMockComponentFactory(),

terraform/graph_builder_destroy_plan.go

@@ -72,6 +72,11 @@ func (b *DestroyPlanGraphBuilder) Steps() []GraphTransformer {
 			State:           b.State,
 		},
 
+		&OutputTransformer{
+			Config:  b.Config,
+			Destroy: true,
+		},
+
 		// Attach the state
 		&AttachStateTransformer{State: b.State},
 

terraform/graph_builder_plan_test.go

@@ -288,10 +288,10 @@ local.instance_id (expand)
   aws_instance.web (expand)
 meta.count-boundary (EachMode fixup)
   aws_load_balancer.weblb (expand)
-  output.instance_id (expand)
+  output.instance_id
 openstack_floating_ip.random (expand)
   provider["registry.terraform.io/hashicorp/openstack"]
-output.instance_id (expand)
+output.instance_id
   local.instance_id (expand)
 provider["registry.terraform.io/hashicorp/aws"]
   openstack_floating_ip.random (expand)

terraform/node_output.go

@@ -11,15 +11,18 @@ import (
 	"github.com/hashicorp/terraform/lang"
 	"github.com/hashicorp/terraform/plans"
 	"github.com/hashicorp/terraform/states"
+	"github.com/hashicorp/terraform/tfdiags"
 	"github.com/zclconf/go-cty/cty"
 )
 
-// nodeExpandOutput is the placeholder for an output that has not yet had
-// its module path expanded.
+// nodeExpandOutput is the placeholder for a non-root module output that has
+// not yet had its module path expanded.
 type nodeExpandOutput struct {
-	Addr   addrs.OutputValue
-	Module addrs.Module
-	Config *configs.Output
+	Addr    addrs.OutputValue
+	Module  addrs.Module
+	Config  *configs.Output
+	Changes []*plans.OutputChangeSrc
+	Destroy bool
 }
 
 var (
@@ -34,17 +37,37 @@ var (
 func (n *nodeExpandOutput) expandsInstances() {}
 
 func (n *nodeExpandOutput) temporaryValue() bool {
-	// this must always be evaluated if it is a root module output
+	// non root outputs are temporary
 	return !n.Module.IsRoot()
 }
 
 func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) {
-	var g Graph
+	if n.Destroy {
+		// if we're planning a destroy, we only need to handle the root outputs.
+		// The destroy plan doesn't evaluate any other config, so we can skip
+		// the rest of the outputs.
+		return n.planDestroyRootOutput(ctx)
+	}
+
 	expander := ctx.InstanceExpander()
+
+	var g Graph
 	for _, module := range expander.ExpandModule(n.Module) {
+		absAddr := n.Addr.Absolute(module)
+
+		// Find any recorded change for this output
+		var change *plans.OutputChangeSrc
+		for _, c := range n.Changes {
+			if c.Addr.String() == absAddr.String() {
+				change = c
+				break
+			}
+		}
+
 		o := &NodeApplyableOutput{
-			Addr:   n.Addr.Absolute(module),
+			Addr:   absAddr,
 			Config: n.Config,
+			Change: change,
 		}
 		log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o)
 		g.Add(o)
@@ -52,6 +75,27 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) {
 	return &g, nil
 }
 
+// if we're planing a destroy operation, add a destroy node for any root output
+func (n *nodeExpandOutput) planDestroyRootOutput(ctx EvalContext) (*Graph, error) {
+	if !n.Module.IsRoot() {
+		return nil, nil
+	}
+	state := ctx.State()
+	if state == nil {
+		return nil, nil
+	}
+
+	var g Graph
+	o := &NodeDestroyableOutput{
+		Addr:   n.Addr.Absolute(addrs.RootModuleInstance),
+		Config: n.Config,
+	}
+	log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o)
+	g.Add(o)
+
+	return &g, nil
+}
+
 func (n *nodeExpandOutput) Name() string {
 	path := n.Module.String()
 	addr := n.Addr.String() + " (expand)"
@@ -100,6 +144,8 @@ func (n *nodeExpandOutput) ReferenceOutside() (selfPath, referencePath addrs.Mod
 
 // GraphNodeReferencer
 func (n *nodeExpandOutput) References() []*addrs.Reference {
+	// root outputs might be destroyable, and may not reference anything in
+	// that case
 	return referencesForOutput(n.Config)
 }
 
@@ -108,6 +154,8 @@ func (n *nodeExpandOutput) References() []*addrs.Reference {
 type NodeApplyableOutput struct {
 	Addr   addrs.AbsOutputValue
 	Config *configs.Output // Config is the output in the config
+	// If this is being evaluated during apply, we may have a change recorded already
+	Change *plans.OutputChangeSrc
 }
 
 var (
@@ -199,27 +247,7 @@ func (n *NodeApplyableOutput) References() []*addrs.Reference {
 
 // GraphNodeExecutable
 func (n *NodeApplyableOutput) Execute(ctx EvalContext, op walkOperation) error {
-	// This has to run before we have a state lock, since evaluation also
-	// reads the state
-	val, diags := ctx.EvaluateExpr(n.Config.Expr, cty.DynamicPseudoType, nil)
-	// We'll handle errors below, after we have loaded the module.
-
-	// Outputs don't have a separate mode for validation, so validate
-	// depends_on expressions here too
-	diags = diags.Append(validateDependsOn(ctx, n.Config.DependsOn))
-
-	// Ensure that non-sensitive outputs don't include sensitive values
-	_, marks := val.UnmarkDeep()
-	_, hasSensitive := marks["sensitive"]
-	if !n.Config.Sensitive && hasSensitive {
-		diags = diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  "Output refers to sensitive values",
-			Detail:   "Expressions used in outputs can only refer to sensitive values if the sensitive attribute is true.",
-			Subject:  n.Config.DeclRange.Ptr(),
-		})
-	}
-
+	var diags tfdiags.Diagnostics
 	state := ctx.State()
 	if state == nil {
 		return nil
@@ -227,6 +255,40 @@ func (n *NodeApplyableOutput) Execute(ctx EvalContext, op walkOperation) error {
 
 	changes := ctx.Changes() // may be nil, if we're not working on a changeset
 
+	val := cty.UnknownVal(cty.DynamicPseudoType)
+	changeRecorded := n.Change != nil
+	// we we have a change recorded, we don't need to re-evaluate if the value
+	// was known
+	if changeRecorded {
+		var err error
+		val, err = n.Change.After.Decode(cty.DynamicPseudoType)
+		diags = diags.Append(err)
+	}
+
+	// If there was no change recorded, or the recorded change was not wholly
+	// known, then we need to re-evaluate the output
+	if !changeRecorded || !val.IsWhollyKnown() {
+		// This has to run before we have a state lock, since evaluation also
+		// reads the state
+		val, diags = ctx.EvaluateExpr(n.Config.Expr, cty.DynamicPseudoType, nil)
+		// We'll handle errors below, after we have loaded the module.
+		// Outputs don't have a separate mode for validation, so validate
+		// depends_on expressions here too
+		diags = diags.Append(validateDependsOn(ctx, n.Config.DependsOn))
+
+		// Ensure that non-sensitive outputs don't include sensitive values
+		_, marks := val.UnmarkDeep()
+		_, hasSensitive := marks["sensitive"]
+		if !n.Config.Sensitive && hasSensitive {
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Output refers to sensitive values",
+				Detail:   "Expressions used in outputs can only refer to sensitive values if the sensitive attribute is true.",
+				Subject:  n.Config.DeclRange.Ptr(),
+			})
+		}
+	}
+
 	// handling the interpolation error
 	if diags.HasErrors() {
 		if flagWarnOutputErrors {
@@ -261,7 +323,7 @@ func (n *NodeApplyableOutput) DotNode(name string, opts *dag.DotOpts) *dag.DotNo
 	}
 }
 
-// NodeDestroyableOutput represents an output that is "destroybale":
+// NodeDestroyableOutput represents an output that is "destroyable":
 // its application will remove the output from the state.
 type NodeDestroyableOutput struct {
 	Addr   addrs.AbsOutputValue
@@ -293,6 +355,44 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error
 	if state == nil {
 		return nil
 	}
+
+	// if this is a root module, try to get a before value from the state for
+	// the diff
+	sensitiveBefore := false
+	before := cty.NullVal(cty.DynamicPseudoType)
+	mod := state.Module(n.Addr.Module)
+	if n.Addr.Module.IsRoot() && mod != nil {
+		for name, o := range mod.OutputValues {
+			if name == n.Addr.OutputValue.Name {
+				sensitiveBefore = o.Sensitive
+				before = o.Value
+				break
+			}
+		}
+	}
+
+	changes := ctx.Changes()
+	if changes != nil {
+		change := &plans.OutputChange{
+			Addr:      n.Addr,
+			Sensitive: sensitiveBefore,
+			Change: plans.Change{
+				Action: plans.Delete,
+				Before: before,
+				After:  cty.NullVal(cty.DynamicPseudoType),
+			},
+		}
+
+		cs, err := change.Encode()
+		if err != nil {
+			// Should never happen, since we just constructed this right above
+			panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err))
+		}
+		log.Printf("[TRACE] NodeDestroyableOutput: Saving %s change for %s in changeset", change.Action, n.Addr)
+		changes.RemoveOutputChange(n.Addr) // remove any existing planned change, if present
+		changes.AppendOutputChange(cs)     // add the new planned change
+	}
+
 	state.RemoveOutputValue(n.Addr)
 	return nil
 }
@@ -309,6 +409,75 @@ func (n *NodeDestroyableOutput) DotNode(name string, opts *dag.DotOpts) *dag.Dot
 }
 
 func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.ChangesSync, val cty.Value) {
+	// If we have an active changeset then we'll first replicate the value in
+	// there and lookup the prior value in the state. This is used in
+	// preference to the state where present, since it *is* able to represent
+	// unknowns, while the state cannot.
+	if changes != nil {
+		// if this is a root module, try to get a before value from the state for
+		// the diff
+		sensitiveBefore := false
+		before := cty.NullVal(cty.DynamicPseudoType)
+		mod := state.Module(n.Addr.Module)
+		if n.Addr.Module.IsRoot() && mod != nil {
+			for name, o := range mod.OutputValues {
+				if name == n.Addr.OutputValue.Name {
+					before = o.Value
+					sensitiveBefore = o.Sensitive
+					break
+				}
+			}
+		}
+
+		// We will not show the value is either the before or after are marked
+		// as sensitivity. We can show the value again once sensitivity is
+		// removed from both the config and the state.
+		sensitiveChange := sensitiveBefore || n.Config.Sensitive
+
+		// strip any marks here just to be sure we don't panic on the True comparison
+		val, _ = val.UnmarkDeep()
+
+		var action plans.Action
+		switch {
+		case val.IsNull():
+			action = plans.Delete
+
+		case before.IsNull():
+			action = plans.Create
+
+		case val.IsWhollyKnown() &&
+			val.Equals(before).True() &&
+			n.Config.Sensitive == sensitiveBefore:
+			// Sensitivity must also match to be a NoOp.
+			// Theoretically marks may not match here, but sensitivity is the
+			// only one we can act on, and the state will have been loaded
+			// without any marks to consider.
+			action = plans.NoOp
+
+		default:
+			action = plans.Update
+		}
+
+		change := &plans.OutputChange{
+			Addr:      n.Addr,
+			Sensitive: sensitiveChange,
+			Change: plans.Change{
+				Action: action,
+				Before: before,
+				After:  val,
+			},
+		}
+
+		cs, err := change.Encode()
+		if err != nil {
+			// Should never happen, since we just constructed this right above
+			panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err))
+		}
+		log.Printf("[TRACE] ExecuteWriteOutput: Saving %s change for %s in changeset", change.Action, n.Addr)
+		changes.RemoveOutputChange(n.Addr) // remove any existing planned change, if present
+		changes.AppendOutputChange(cs)     // add the new planned change
+	}
+
 	if val.IsKnown() && !val.IsNull() {
 		// The state itself doesn't represent unknown values, so we null them
 		// out here and then we'll save the real unknown value in the planned
@@ -322,50 +491,4 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C
 		state.RemoveOutputValue(n.Addr)
 	}
 
-	// If we also have an active changeset then we'll replicate the value in
-	// there. This is used in preference to the state where present, since it
-	// *is* able to represent unknowns, while the state cannot.
-	if changes != nil {
-		// For the moment we are not properly tracking changes to output
-		// values, and just marking them always as "Create" or "Destroy"
-		// actions. A future release will rework the output lifecycle so we
-		// can track their changes properly, in a similar way to how we work
-		// with resource instances.
-
-		var change *plans.OutputChange
-		if !val.IsNull() {
-			change = &plans.OutputChange{
-				Addr:      n.Addr,
-				Sensitive: n.Config.Sensitive,
-				Change: plans.Change{
-					Action: plans.Create,
-					Before: cty.NullVal(cty.DynamicPseudoType),
-					After:  val,
-				},
-			}
-		} else {
-			change = &plans.OutputChange{
-				Addr:      n.Addr,
-				Sensitive: n.Config.Sensitive,
-				Change: plans.Change{
-					// This is just a weird placeholder delete action since
-					// we don't have an actual prior value to indicate.
-					// FIXME: Generate real planned changes for output values
-					// that include the old values.
-					Action: plans.Delete,
-					Before: cty.NullVal(cty.DynamicPseudoType),
-					After:  cty.NullVal(cty.DynamicPseudoType),
-				},
-			}
-		}
-
-		cs, err := change.Encode()
-		if err != nil {
-			// Should never happen, since we just constructed this right above
-			panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err))
-		}
-		log.Printf("[TRACE] ExecuteWriteOutput: Saving %s change for %s in changeset", change.Action, n.Addr)
-		changes.RemoveOutputChange(n.Addr) // remove any existing planned change, if present
-		changes.AppendOutputChange(cs)     // add the new planned change
-	}
 }

terraform/node_resource_apply.go

@@ -26,7 +26,8 @@ var (
 	_ GraphNodeTargetable           = (*nodeExpandApplyableResource)(nil)
 )
 
-func (n *nodeExpandApplyableResource) expandsInstances() {}
+func (n *nodeExpandApplyableResource) expandsInstances() {
+}
 
 func (n *nodeExpandApplyableResource) References() []*addrs.Reference {
 	return (&NodeApplyableResource{NodeAbstractResource: n.NodeAbstractResource}).References()

terraform/transform_output.go

@@ -6,6 +6,7 @@ import (
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/dag"
+	"github.com/hashicorp/terraform/plans"
 )
 
 // OutputTransformer is a GraphTransformer that adds all the outputs
@@ -15,7 +16,12 @@ import (
 // aren't changing since there is no downside: the state will be available
 // even if the dependent items aren't changing.
 type OutputTransformer struct {
-	Config *configs.Config
+	Config  *configs.Config
+	Changes *plans.Changes
+
+	// if this is a planed destroy, root outputs are still in the configuration
+	// so we need to record that we wish to remove them
+	Destroy bool
 }
 
 func (t *OutputTransformer) Transform(g *Graph) error {
@@ -37,65 +43,61 @@ func (t *OutputTransformer) transform(g *Graph, c *configs.Config) error {
 		}
 	}
 
-	// Add plannable outputs to the graph, which will be dynamically expanded
+	// Add outputs to the graph, which will be dynamically expanded
 	// into NodeApplyableOutputs to reflect possible expansion
 	// through the presence of "count" or "for_each" on the modules.
+
+	var changes []*plans.OutputChangeSrc
+	if t.Changes != nil {
+		changes = t.Changes.Outputs
+	}
+
 	for _, o := range c.Module.Outputs {
-		node := &nodeExpandOutput{
-			Addr:   addrs.OutputValue{Name: o.Name},
-			Module: c.Path,
-			Config: o,
+		addr := addrs.OutputValue{Name: o.Name}
+
+		var rootChange *plans.OutputChangeSrc
+		for _, c := range changes {
+			if c.Addr.Module.IsRoot() && c.Addr.OutputValue.Name == o.Name {
+				rootChange = c
+			}
 		}
+
+		destroy := t.Destroy
+		if rootChange != nil {
+			destroy = rootChange.Action == plans.Delete
+		}
+
+		// If this is a root output, we add the apply or destroy node directly,
+		// as the root modules does not expand.
+
+		var node dag.Vertex
+		switch {
+		case c.Path.IsRoot() && destroy:
+			node = &NodeDestroyableOutput{
+				Addr:   addr.Absolute(addrs.RootModuleInstance),
+				Config: o,
+			}
+
+		case c.Path.IsRoot():
+			node = &NodeApplyableOutput{
+				Addr:   addr.Absolute(addrs.RootModuleInstance),
+				Config: o,
+				Change: rootChange,
+			}
+
+		default:
+			node = &nodeExpandOutput{
+				Addr:    addr,
+				Module:  c.Path,
+				Config:  o,
+				Changes: changes,
+				Destroy: t.Destroy,
+			}
+		}
+
 		log.Printf("[TRACE] OutputTransformer: adding %s as %T", o.Name, node)
 		g.Add(node)
 	}
 
 	return nil
 }
-
-// destroyRootOutputTransformer is a GraphTransformer that adds nodes to delete
-// outputs during destroy. We need to do this to ensure that no stale outputs
-// are ever left in the state.
-type destroyRootOutputTransformer struct {
-	Destroy bool
-}
-
-func (t *destroyRootOutputTransformer) Transform(g *Graph) error {
-	// Only clean root outputs on a full destroy
-	if !t.Destroy {
-		return nil
-	}
-
-	for _, v := range g.Vertices() {
-		output, ok := v.(*nodeExpandOutput)
-		if !ok {
-			continue
-		}
-
-		// We only destroy root outputs
-		if !output.Module.Equal(addrs.RootModule) {
-			continue
-		}
-
-		// create the destroy node for this output
-		node := &NodeDestroyableOutput{
-			Addr:   output.Addr.Absolute(addrs.RootModuleInstance),
-			Config: output.Config,
-		}
-
-		log.Printf("[TRACE] creating %s", node.Name())
-		g.Add(node)
-
-		deps := g.UpEdges(v)
-
-		for _, d := range deps {
-			log.Printf("[TRACE] %s depends on %s", node.Name(), dag.VertexName(d))
-			g.Connect(dag.BasicEdge(node, d))
-		}
-
-		// We no longer need the expand node, since we intend to remove this
-		// output from the state.
-		g.Remove(v)
-	}
-	return nil
-}

terraform/transform_targets_test.go

@@ -122,7 +122,7 @@ module.child.module.grandchild.output.id (expand)
   module.child.module.grandchild.aws_instance.foo
 module.child.output.grandchild_id (expand)
   module.child.module.grandchild.output.id (expand)
-output.grandchild_id (expand)
+output.grandchild_id
   module.child.output.grandchild_id (expand)
 	`)
 	if actual != expected {
@@ -193,7 +193,7 @@ module.child.module.grandchild.output.id (expand)
   module.child.module.grandchild.aws_instance.foo
 module.child.output.grandchild_id (expand)
   module.child.module.grandchild.output.id (expand)
-output.grandchild_id (expand)
+output.grandchild_id
   module.child.output.grandchild_id (expand)
 	`)
 	if actual != expected {