evaluate outputs from state

Outputs were being evaluated from changes, even during apply. Make sure
we update the state correctly, and remove the existing change. This
requires adding more Planning fields to the output nodes to
differentiate whether the output is being planned or applied because the
same type handles both cases. We can evaluate separately whether new
types should be introduced to deal with both cases.

The module node cleanup was also prematurely removing module outputs
from the state before evaluation. This was not noticed before because
the evaluation was always falling back to changes. Have the root module
node do the final cleanup for all its children.

It turns out sensitive was also being handled incorrectly, and only
sensitive from configuration was being considered. Make sure to mark the
output as sensitive when storing sensitive values into state, and OR
sensitive marks with the state config when evaluating the output values.

internal/terraform/evaluate.go

@@ -375,6 +375,11 @@ func (d *evaluationStateData) GetModule(addr addrs.ModuleCall, rng tfdiags.Sourc
 	// so we only need to store these by instance key.
 	stateMap := map[addrs.InstanceKey]map[string]cty.Value{}
 	for _, output := range d.Evaluator.State.ModuleOutputs(d.ModulePath, addr) {
+		val := output.Value
+		if output.Sensitive {
+			val = val.Mark(marks.Sensitive)
+		}
+
 		_, callInstance := output.Addr.Module.CallInstance()
 		instance, ok := stateMap[callInstance.Key]
 		if !ok {
@@ -382,7 +387,7 @@ func (d *evaluationStateData) GetModule(addr addrs.ModuleCall, rng tfdiags.Sourc
 			stateMap[callInstance.Key] = instance
 		}
 
-		instance[output.Addr.OutputValue.Name] = output.Value
+		instance[output.Addr.OutputValue.Name] = val
 	}
 
 	// Get all changes that reside for this module call within our path.
@@ -426,10 +431,6 @@ func (d *evaluationStateData) GetModule(addr addrs.ModuleCall, rng tfdiags.Sourc
 			}
 
 			instance[cfg.Name] = outputState
-
-			if cfg.Sensitive {
-				instance[cfg.Name] = outputState.Mark(marks.Sensitive)
-			}
 		}
 
 		// any pending changes override the state state values

internal/terraform/graph_builder_plan.go

@@ -150,7 +150,11 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 		&AttachStateTransformer{State: b.State},
 
 		// Create orphan output nodes
-		&OrphanOutputTransformer{Config: b.Config, State: b.State},
+		&OrphanOutputTransformer{
+			Config:   b.Config,
+			State:    b.State,
+			Planning: true,
+		},
 
 		// Attach the configuration to any resources
 		&AttachResourceConfigTransformer{Config: b.Config},

internal/terraform/node_module_expand.go

@@ -176,22 +176,20 @@ func (n *nodeCloseModule) Name() string {
 }
 
 func (n *nodeCloseModule) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) {
-	if n.Addr.IsRoot() {
-		// If this is the root module, we are cleaning up the walk, so close
-		// any running provisioners
-		diags = diags.Append(ctx.CloseProvisioners())
+	if !n.Addr.IsRoot() {
+		return
 	}
 
+	// If this is the root module, we are cleaning up the walk, so close
+	// any running provisioners
+	diags = diags.Append(ctx.CloseProvisioners())
+
 	switch op {
 	case walkApply, walkDestroy:
 		state := ctx.State().Lock()
 		defer ctx.State().Unlock()
 
 		for modKey, mod := range state.Modules {
-			if !n.Addr.Equal(mod.Addr.Module()) {
-				continue
-			}
-
 			// clean out any empty resources
 			for resKey, res := range mod.Resources {
 				if len(res.Instances) == 0 {

internal/terraform/node_module_expand_test.go

@@ -47,6 +47,18 @@ func TestNodeCloseModuleExecute(t *testing.T) {
 			t.Fatalf("unexpected error: %s", diags.Err())
 		}
 
+		// Since module.child has no resources, it should be removed
+		if _, ok := state.Modules["module.child"]; !ok {
+			t.Fatal("module.child should not be removed from state yet")
+		}
+
+		// the root module should do all the module cleanup
+		node = nodeCloseModule{addrs.RootModule}
+		diags = node.Execute(ctx, walkApply)
+		if diags.HasErrors() {
+			t.Fatalf("unexpected error: %s", diags.Err())
+		}
+
 		// Since module.child has no resources, it should be removed
 		if _, ok := state.Modules["module.child"]; ok {
 			t.Fatal("module.child was not removed from state")

internal/terraform/node_output.go

@@ -102,7 +102,8 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) {
 		switch {
 		case module.IsRoot() && (n.PlanDestroy || n.ApplyDestroy):
 			node = &NodeDestroyableOutput{
-				Addr: absAddr,
+				Addr:     absAddr,
+				Planning: n.Planning,
 			}
 
 		case n.PlanDestroy:
@@ -116,6 +117,7 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) {
 				Change:       change,
 				RefreshOnly:  n.RefreshOnly,
 				DestroyApply: n.ApplyDestroy,
+				Planning:     n.Planning,
 			}
 		}
 
@@ -203,6 +205,8 @@ type NodeApplyableOutput struct {
 	// DestroyApply indicates that we are applying a destroy plan, and do not
 	// need to account for conditional blocks.
 	DestroyApply bool
+
+	Planning bool
 }
 
 var (
@@ -421,7 +425,8 @@ func (n *NodeApplyableOutput) DotNode(name string, opts *dag.DotOpts) *dag.DotNo
 // NodeDestroyableOutput represents an output that is "destroyable":
 // its application will remove the output from the state.
 type NodeDestroyableOutput struct {
-	Addr addrs.AbsOutputValue
+	Addr     addrs.AbsOutputValue
+	Planning bool
 }
 
 var (
@@ -468,7 +473,7 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) tfdia
 	}
 
 	changes := ctx.Changes()
-	if changes != nil {
+	if changes != nil && n.Planning {
 		change := &plans.OutputChange{
 			Addr:      n.Addr,
 			Sensitive: sensitiveBefore,
@@ -485,6 +490,7 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) tfdia
 			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
 	}
@@ -509,7 +515,7 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C
 	// 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 changes != nil && n.Planning {
 		// if this is a root module, try to get a before value from the state for
 		// the diff
 		sensitiveBefore := false
@@ -575,8 +581,13 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C
 			panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err))
 		}
 		log.Printf("[TRACE] setValue: 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
+		changes.AppendOutputChange(cs) // add the new planned change
+	}
+
+	if changes != nil && !n.Planning {
+		// During apply there is no longer any change to track, so we must
+		// ensure the state is updated and not overridden by a change.
+		changes.RemoveOutputChange(n.Addr)
 	}
 
 	if val.IsKnown() && !val.IsNull() {
@@ -584,9 +595,22 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C
 		// out here and then we'll save the real unknown value in the planned
 		// changeset below, if we have one on this graph walk.
 		log.Printf("[TRACE] setValue: Saving value for %s in state", n.Addr)
-		unmarkedVal, _ := val.UnmarkDeep()
+
+		sensitive := n.Config.Sensitive
+		unmarkedVal, valueMarks := val.UnmarkDeep()
+
+		// If the evaluate value contains sensitive marks, the output has no
+		// choice but to declare itself as "sensitive".
+		for mark := range valueMarks {
+			if mark == marks.Sensitive {
+				sensitive = true
+				break
+			}
+		}
+
 		stateVal := cty.UnknownAsNull(unmarkedVal)
-		state.SetOutputValue(n.Addr, stateVal, n.Config.Sensitive)
+		state.SetOutputValue(n.Addr, stateVal, sensitive)
+
 	} else {
 		log.Printf("[TRACE] setValue: Removing %s from state (it is now null)", n.Addr)
 		state.RemoveOutputValue(n.Addr)

internal/terraform/transform_orphan_output.go

@@ -12,8 +12,9 @@ import (
 // in the given config that are in the state and adds them to the graph
 // for deletion.
 type OrphanOutputTransformer struct {
-	Config *configs.Config // Root of config tree
-	State  *states.State   // State is the root state
+	Config   *configs.Config // Root of config tree
+	State    *states.State   // State is the root state
+	Planning bool
 }
 
 func (t *OrphanOutputTransformer) Transform(g *Graph) error {
@@ -52,7 +53,8 @@ func (t *OrphanOutputTransformer) transform(g *Graph, ms *states.Module) error {
 		}
 
 		g.Add(&NodeDestroyableOutput{
-			Addr: addrs.OutputValue{Name: name}.Absolute(moduleAddr),
+			Addr:     addrs.OutputValue{Name: name}.Absolute(moduleAddr),
+			Planning: t.Planning,
 		})
 	}