Merge pull request #32308 from hashicorp/jbardin/output-eval-fix

always evaluate outputs from state during apply

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,11 +176,13 @@ func (n *nodeCloseModule) Name() string {
 }
 
 func (n *nodeCloseModule) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) {
-	if n.Addr.IsRoot() {
+	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:
@@ -188,10 +190,6 @@ func (n *nodeCloseModule) Execute(ctx EvalContext, op walkOperation) (diags tfdi
 		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

@@ -103,6 +103,7 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) {
 		case module.IsRoot() && (n.PlanDestroy || n.ApplyDestroy):
 			node = &NodeDestroyableOutput{
 				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 (
@@ -422,6 +426,7 @@ func (n *NodeApplyableOutput) DotNode(name string, opts *dag.DotOpts) *dag.DotNo
 // its application will remove the output from the state.
 type NodeDestroyableOutput struct {
 	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,18 +581,36 @@ 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
 	}
 
+	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() {
 		// 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
 		// 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

@@ -14,6 +14,7 @@ import (
 type OrphanOutputTransformer struct {
 	Config   *configs.Config // Root of config tree
 	State    *states.State   // State is the root state
+	Planning bool
 }
 
 func (t *OrphanOutputTransformer) Transform(g *Graph) error {
@@ -53,6 +54,7 @@ func (t *OrphanOutputTransformer) transform(g *Graph, ms *states.Module) error {
 
 		g.Add(&NodeDestroyableOutput{
 			Addr:     addrs.OutputValue{Name: name}.Absolute(moduleAddr),
+			Planning: t.Planning,
 		})
 	}