core: Don't re-register checkable outputs during the apply step

Once again we're caught out by sharing the same output value node type
between the plan phase and the apply phase. To allow for some slight
variation between plan and apply without drastic refactoring here we just
add a new flag to nodeExpandOutput which is true only during the planning
phase.

This then allows us to register the checkable objects only during the
planning phase and not incorrectly re-register them during the apply phase.
It's incorrect to re-register during apply because we carry over the
planned checkable objects from the plan phase into the apply phase so we
can guarantee that the final state will have all of the same checkable
objects that the plan did.

This avoids a panic during the apply phase from the incorrect duplicate
registration.

Author: apparentlymart

internal/terraform/context_apply2_test.go

@@ -14,11 +14,13 @@ import (
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/terraform/internal/addrs"
+	"github.com/hashicorp/terraform/internal/checks"
 	"github.com/hashicorp/terraform/internal/configs/configschema"
 	"github.com/hashicorp/terraform/internal/lang/marks"
 	"github.com/hashicorp/terraform/internal/plans"
 	"github.com/hashicorp/terraform/internal/providers"
 	"github.com/hashicorp/terraform/internal/states"
+	"github.com/hashicorp/terraform/internal/tfdiags"
 )
 
 // Test that the PreApply hook is called with the correct deposed key
@@ -917,6 +919,106 @@ resource "test_resource" "c" {
 	})
 }
 
+func TestContext2Apply_outputValuePrecondition(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+			variable "input" {
+				type = string
+			}
+
+			module "child" {
+				source = "./child"
+
+				input = var.input
+			}
+
+			output "result" {
+				value = module.child.result
+			}
+		`,
+		"child/main.tf": `
+			variable "input" {
+				type = string
+			}
+
+			output "result" {
+				value = var.input
+
+				precondition {
+					condition     = var.input != ""
+					error_message = "Input must not be empty."
+				}
+			}
+		`,
+	})
+
+	t.Run("pass", func(t *testing.T) {
+		ctx := testContext2(t, &ContextOpts{})
+		plan, diags := ctx.Plan(m, states.NewState(), &PlanOpts{
+			Mode: plans.NormalMode,
+			SetVariables: InputValues{
+				"input": &InputValue{
+					Value:      cty.StringVal("beep"),
+					SourceType: ValueFromCLIArg,
+				},
+			},
+		})
+		assertNoDiagnostics(t, diags)
+
+		addr := addrs.OutputValue{Name: "result"}.Absolute(addrs.RootModuleInstance.Child("child", addrs.NoKey))
+		result := plan.Checks.GetObjectResult(addr)
+		if result == nil {
+			t.Fatalf("no check result for %s in the plan", addr)
+		}
+		if got, want := result.Status, checks.StatusPass; got != want {
+			t.Fatalf("wrong check status during planning\ngot:  %s\nwant: %s", got, want)
+		}
+
+		state, diags := ctx.Apply(plan, m)
+		assertNoDiagnostics(t, diags)
+		result = state.CheckResults.GetObjectResult(addr)
+		if result == nil {
+			t.Fatalf("no check result for %s in the final state", addr)
+		}
+		if got, want := result.Status, checks.StatusPass; got != want {
+			t.Errorf("wrong check status after apply\ngot:  %s\nwant: %s", got, want)
+		}
+	})
+
+	t.Run("fail", func(t *testing.T) {
+		// NOTE: This test actually catches a failure during planning and so
+		// cannot proceed to apply, so it's really more of a plan test
+		// than an apply test but better to keep all of these
+		// thematically-related test cases together.
+		ctx := testContext2(t, &ContextOpts{})
+		_, diags := ctx.Plan(m, states.NewState(), &PlanOpts{
+			Mode: plans.NormalMode,
+			SetVariables: InputValues{
+				"input": &InputValue{
+					Value:      cty.StringVal(""),
+					SourceType: ValueFromCLIArg,
+				},
+			},
+		})
+		if !diags.HasErrors() {
+			t.Fatalf("succeeded; want error")
+		}
+
+		const wantSummary = "Module output value precondition failed"
+		found := false
+		for _, diag := range diags {
+			if diag.Severity() == tfdiags.Error && diag.Description().Summary == wantSummary {
+				found = true
+				break
+			}
+		}
+
+		if !found {
+			t.Fatalf("missing expected error\nwant summary: %s\ngot: %s", wantSummary, diags.Err().Error())
+		}
+	})
+}
+
 func TestContext2Apply_resourceConditionApplyTimeFail(t *testing.T) {
 	// This tests the less common situation where a condition fails due to
 	// a change in a resource other than the one the condition is attached to,

internal/terraform/graph_builder_plan.go

@@ -113,6 +113,7 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 			Config:            b.Config,
 			RefreshOnly:       b.skipPlanChanges,
 			removeRootOutputs: b.Operation == walkPlanDestroy,
+			Planning:          b.Operation == walkPlan || b.Operation == walkPlanDestroy,
 		},
 
 		// Add orphan resources

internal/terraform/node_output.go

@@ -25,6 +25,14 @@ type nodeExpandOutput struct {
 	Config      *configs.Output
 	Destroy     bool
 	RefreshOnly bool
+
+	// Planning is set to true when this node is in a graph that was produced
+	// by the plan graph builder, as opposed to the apply graph builder.
+	// This quirk is just because we share the same node type between both
+	// phases but in practice there are a few small differences in the actions
+	// we need to take between plan and apply. See method DynamicExpand for
+	// details.
+	Planning bool
 }
 
 var (
@@ -59,9 +67,18 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) {
 	// wants to know the addresses of the checkable objects so that it can
 	// treat them as unknown status if we encounter an error before actually
 	// visiting the checks.
+	//
+	// We must do this only during planning, because the apply phase will start
+	// with all of the same checkable objects that were registered during the
+	// planning phase. Consumers of our JSON plan and state formats expect
+	// that the set of checkable objects will be consistent between the plan
+	// and any state snapshots created during apply, and that only the statuses
+	// of those objects will have changed.
 	var checkableAddrs addrs.Set[addrs.Checkable]
-	if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(n.Module)) {
-		checkableAddrs = addrs.MakeSet[addrs.Checkable]()
+	if n.Planning {
+		if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(n.Module)) {
+			checkableAddrs = addrs.MakeSet[addrs.Checkable]()
+		}
 	}
 
 	var g Graph

internal/terraform/transform_output.go

@@ -26,6 +26,10 @@ type OutputTransformer struct {
 	// Refresh-only mode means that any failing output preconditions are
 	// reported as warnings rather than errors
 	RefreshOnly bool
+
+	// Planning must be set to true only when we're building a planning graph.
+	// It must be set to false whenever we're building an apply graph.
+	Planning bool
 }
 
 func (t *OutputTransformer) Transform(g *Graph) error {
@@ -89,6 +93,7 @@ func (t *OutputTransformer) transform(g *Graph, c *configs.Config) error {
 				Config:      o,
 				Destroy:     t.removeRootOutputs,
 				RefreshOnly: t.RefreshOnly,
+				Planning:    t.Planning,
 			}
 		}