Merge #19237: Handle unknown values properly in module outputs

Since the state models can't preserve unknown values, we need to rely on the plan to persist these until the effective configuration can be fully resolved during the apply phase.

plans/changes.go

@@ -254,6 +254,7 @@ func (oc *OutputChange) Encode() (*OutputChangeSrc, error) {
 		return nil, err
 	}
 	return &OutputChangeSrc{
+		Addr:      oc.Addr,
 		ChangeSrc: *cs,
 		Sensitive: oc.Sensitive,
 	}, err

plans/changes_sync.go

@@ -133,8 +133,8 @@ func (cs *ChangesSync) RemoveOutputChange(addr addrs.AbsOutputValue) {
 	defer cs.lock.Unlock()
 
 	addrStr := addr.String()
-	for i, r := range cs.changes.Resources {
-		if r.Addr.String() != addrStr {
+	for i, o := range cs.changes.Outputs {
+		if o.Addr.String() != addrStr {
 			continue
 		}
 		copy(cs.changes.Outputs[i:], cs.changes.Outputs[i+1:])

terraform/context_plan_test.go

@@ -5513,3 +5513,137 @@ func objectVal(t *testing.T, schema *configschema.Block, m map[string]cty.Value)
 	}
 	return v
 }
+
+func TestContext2Plan_requiredModuleOutput(t *testing.T) {
+	m := testModule(t, "plan-required-output")
+	p := testProvider("test")
+	p.GetSchemaReturn = &ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_resource": {
+				Attributes: map[string]*configschema.Attribute{
+					"id":       {Type: cty.String, Computed: true},
+					"required": {Type: cty.String, Required: true},
+				},
+			},
+		},
+	}
+	p.DiffFn = testDiffFn
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		ProviderResolver: providers.ResolverFixed(
+			map[string]providers.Factory{
+				"test": testProviderFuncFixed(p),
+			},
+		),
+	})
+
+	plan, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatalf("unexpected errors: %s", diags.Err())
+	}
+
+	schema := p.GetSchemaReturn.ResourceTypes["test_resource"]
+	ty := schema.ImpliedType()
+
+	if len(plan.Changes.Resources) != 2 {
+		t.Fatal("expected 2 changes, got", len(plan.Changes.Resources))
+	}
+
+	for _, res := range plan.Changes.Resources {
+		t.Run(fmt.Sprintf("%s %s", res.Action, res.Addr), func(t *testing.T) {
+			if res.Action != plans.Create {
+				t.Fatalf("expected resource creation, got %s", res.Action)
+			}
+			ric, err := res.Decode(ty)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			var expected cty.Value
+			switch i := ric.Addr.String(); i {
+			case "test_resource.root":
+				expected = objectVal(t, schema, map[string]cty.Value{
+					"id":       cty.UnknownVal(cty.String),
+					"required": cty.UnknownVal(cty.String),
+				})
+			case "module.mod.test_resource.for_output":
+				expected = objectVal(t, schema, map[string]cty.Value{
+					"id":       cty.UnknownVal(cty.String),
+					"required": cty.StringVal("val"),
+				})
+			default:
+				t.Fatal("unknown instance:", i)
+			}
+
+			checkVals(t, expected, ric.After)
+		})
+	}
+}
+
+func TestContext2Plan_requiredModuleObject(t *testing.T) {
+	m := testModule(t, "plan-required-whole-mod")
+	p := testProvider("test")
+	p.GetSchemaReturn = &ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_resource": {
+				Attributes: map[string]*configschema.Attribute{
+					"id":       {Type: cty.String, Computed: true},
+					"required": {Type: cty.String, Required: true},
+				},
+			},
+		},
+	}
+	p.DiffFn = testDiffFn
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		ProviderResolver: providers.ResolverFixed(
+			map[string]providers.Factory{
+				"test": testProviderFuncFixed(p),
+			},
+		),
+	})
+
+	plan, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatalf("unexpected errors: %s", diags.Err())
+	}
+
+	schema := p.GetSchemaReturn.ResourceTypes["test_resource"]
+	ty := schema.ImpliedType()
+
+	if len(plan.Changes.Resources) != 2 {
+		t.Fatal("expected 2 changes, got", len(plan.Changes.Resources))
+	}
+
+	for _, res := range plan.Changes.Resources {
+		t.Run(fmt.Sprintf("%s %s", res.Action, res.Addr), func(t *testing.T) {
+			if res.Action != plans.Create {
+				t.Fatalf("expected resource creation, got %s", res.Action)
+			}
+			ric, err := res.Decode(ty)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			var expected cty.Value
+			switch i := ric.Addr.String(); i {
+			case "test_resource.root":
+				expected = objectVal(t, schema, map[string]cty.Value{
+					"id":       cty.UnknownVal(cty.String),
+					"required": cty.UnknownVal(cty.String),
+				})
+			case "module.mod.test_resource.for_output":
+				expected = objectVal(t, schema, map[string]cty.Value{
+					"id":       cty.UnknownVal(cty.String),
+					"required": cty.StringVal("val"),
+				})
+			default:
+				t.Fatal("unknown instance:", i)
+			}
+
+			checkVals(t, expected, ric.After)
+		})
+	}
+}

terraform/eval_output.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/plans"
+	"github.com/hashicorp/terraform/states"
 )
 
 // EvalDeleteOutput is an EvalNode implementation that deletes an output
@@ -61,22 +62,33 @@ func (n *EvalWriteOutput) Eval(ctx EvalContext) (interface{}, error) {
 			// if we're continuing, make sure the output is included, and
 			// marked as unknown. If the evaluator was able to find a type
 			// for the value in spite of the error then we'll use it.
-			state.SetOutputValue(addr, cty.UnknownVal(val.Type()), n.Sensitive)
+			n.setValue(addr, state, changes, cty.UnknownVal(val.Type()))
 			return nil, EvalEarlyExitError{}
 		}
 		return nil, diags.Err()
 	}
 
+	n.setValue(addr, state, changes, val)
+
+	return nil, nil
+}
+
+func (n *EvalWriteOutput) setValue(addr addrs.AbsOutputValue, state *states.SyncState, changes *plans.ChangesSync, val cty.Value) {
 	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] EvalWriteOutput: Saving value for %s in state", addr)
 		stateVal := cty.UnknownAsNull(val)
 		state.SetOutputValue(addr, stateVal, n.Sensitive)
 	} else {
+		log.Printf("[TRACE] EvalWriteOutput: Removing %s from state (it is now null)", addr)
 		state.RemoveOutputValue(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"
@@ -116,8 +128,8 @@ func (n *EvalWriteOutput) Eval(ctx EvalContext) (interface{}, error) {
 			// Should never happen, since we just constructed this right above
 			panic(fmt.Sprintf("planned change for %s could not be encoded: %s", addr, err))
 		}
-		changes.AppendOutputChange(cs)
+		log.Printf("[TRACE] EvalWriteOutput: Saving %s change for %s in changeset", change.Action, addr)
+		changes.RemoveOutputChange(addr) // remove any existing planned change, if present
+		changes.AppendOutputChange(cs)   // add the new planned change
 	}
-
-	return nil, nil
 }

terraform/evaluate.go

@@ -305,14 +305,31 @@ func (d *evaluationStateData) GetModuleInstance(addr addrs.ModuleCallInstance, r
 	vals := map[string]cty.Value{}
 	for n := range outputConfigs {
 		addr := addrs.OutputValue{Name: n}.Absolute(moduleAddr)
-		os := d.Evaluator.State.OutputValue(addr)
-		if os == nil {
-			// Not evaluated yet?
-			vals[n] = cty.DynamicVal
-			continue
-		}
 
-		vals[n] = os.Value
+		// If a pending change is present in our current changeset then its value
+		// takes priority over what's in state. (It will usually be the same but
+		// will differ if the new value is unknown during planning.)
+		if changeSrc := d.Evaluator.Changes.GetOutputChange(addr); changeSrc != nil {
+			change, err := changeSrc.Decode()
+			if err != nil {
+				// This should happen only if someone has tampered with a plan
+				// file, so we won't bother with a pretty error for it.
+				diags = diags.Append(fmt.Errorf("planned change for %s could not be decoded: %s", addr, err))
+				vals[n] = cty.DynamicVal
+				continue
+			}
+			// We care only about the "after" value, which is the value this output
+			// will take on after the plan is applied.
+			vals[n] = change.After
+		} else {
+			os := d.Evaluator.State.OutputValue(addr)
+			if os == nil {
+				// Not evaluated yet?
+				vals[n] = cty.DynamicVal
+				continue
+			}
+			vals[n] = os.Value
+		}
 	}
 	return cty.ObjectVal(vals), diags
 }

terraform/test-fixtures/plan-required-output/main.tf

@@ -0,0 +1,7 @@
+resource "test_resource" "root" {
+  required = module.mod.object.id
+}
+
+module "mod" {
+  source = "./mod"
+}

terraform/test-fixtures/plan-required-output/mod/main.tf

@@ -0,0 +1,7 @@
+resource "test_resource" "for_output" {
+  required = "val"
+}
+
+output "object" {
+  value = test_resource.for_output
+}

terraform/test-fixtures/plan-required-whole-mod/main.tf

@@ -0,0 +1,17 @@
+resource "test_resource" "root" {
+  required = local.object.id
+}
+
+locals {
+  # This indirection is here to force the evaluator to produce the whole
+  # module object here rather than just fetching the single "object" output.
+  # This makes this fixture different than plan-required-output, which just
+  # accesses module.mod.object.id directly and thus visits a different
+  # codepath in the evaluator.
+  mod    = module.mod
+  object = local.mod.object
+}
+
+module "mod" {
+  source = "./mod"
+}

terraform/test-fixtures/plan-required-whole-mod/mod/main.tf

@@ -0,0 +1,7 @@
+resource "test_resource" "for_output" {
+  required = "val"
+}
+
+output "object" {
+  value = test_resource.for_output
+}