From e28b1c8579151361be6f85725d79bf841cfd86f8 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 28 Sep 2022 10:49:57 -0700 Subject: [PATCH] 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. --- internal/terraform/context_apply2_test.go | 115 ++++++++++++++++++++++ internal/terraform/graph_builder_plan.go | 7 ++ internal/terraform/node_output.go | 21 +++- internal/terraform/transform_output.go | 5 + 4 files changed, 146 insertions(+), 2 deletions(-) diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 93837626ff7b..73ef7b3d0cf5 100644 --- a/internal/terraform/context_apply2_test.go +++ b/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,119 @@ 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 + + precondition { + condition = var.input != "" + error_message = "Input must not be empty." + } + } + `, + "child/main.tf": ` + variable "input" { + type = string + } + + output "result" { + value = var.input + + precondition { + condition = var.input != "" + error_message = "Input must not be empty." + } + } + `, + }) + + checkableObjects := []addrs.Checkable{ + addrs.OutputValue{Name: "result"}.Absolute(addrs.RootModuleInstance), + addrs.OutputValue{Name: "result"}.Absolute(addrs.RootModuleInstance.Child("child", addrs.NoKey)), + } + + 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) + + for _, addr := range checkableObjects { + 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 for %s during planning\ngot: %s\nwant: %s", addr, got, want) + } + } + + state, diags := ctx.Apply(plan, m) + assertNoDiagnostics(t, diags) + for _, addr := range checkableObjects { + 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 for %s after apply\ngot: %s\nwant: %s", addr, 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, diff --git a/internal/terraform/graph_builder_plan.go b/internal/terraform/graph_builder_plan.go index d65bab26def0..3a4a457e45fb 100644 --- a/internal/terraform/graph_builder_plan.go +++ b/internal/terraform/graph_builder_plan.go @@ -113,6 +113,13 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { Config: b.Config, RefreshOnly: b.skipPlanChanges, removeRootOutputs: b.Operation == walkPlanDestroy, + + // NOTE: We currently treat anything built with the plan graph + // builder as "planning" for our purposes here, because we share + // the same graph node implementation between all of the walk + // types and so the pre-planning walks still think they are + // producing a plan even though we immediately discard it. + Planning: true, }, // Add orphan resources diff --git a/internal/terraform/node_output.go b/internal/terraform/node_output.go index 5ce32c244582..efd9c1ee1723 100644 --- a/internal/terraform/node_output.go +++ b/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 diff --git a/internal/terraform/transform_output.go b/internal/terraform/transform_output.go index afc9ab09ecee..ffed7df19681 100644 --- a/internal/terraform/transform_output.go +++ b/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, } }