From 92937113bf23d40731fc51d94439ad86b864cc2e Mon Sep 17 00:00:00 2001 From: Christian Mesh Date: Wed, 8 Nov 2023 18:07:09 -0500 Subject: [PATCH] Fix crash during destroy planning due to validation (#830) Signed-off-by: Christian Mesh --- internal/checks/state_report.go | 3 +- internal/tofu/graph_builder_eval.go | 4 +- internal/tofu/graph_builder_plan.go | 4 +- internal/tofu/node_module_variable.go | 10 +- internal/tofu/node_module_variable_test.go | 182 +++++++++++++++++++++ internal/tofu/node_root_variable.go | 14 +- internal/tofu/node_root_variable_test.go | 9 +- internal/tofu/transform_module_variable.go | 11 +- internal/tofu/transform_variable.go | 5 - 9 files changed, 205 insertions(+), 37 deletions(-) diff --git a/internal/checks/state_report.go b/internal/checks/state_report.go index e4bbfd1b87..af5d6c77bf 100644 --- a/internal/checks/state_report.go +++ b/internal/checks/state_report.go @@ -29,7 +29,8 @@ func (c *State) ReportCheckableObjects(configAddr addrs.ConfigCheckable, objectA } if st.objects.Elems != nil { // Can only report checkable objects once per configuration object - panic(fmt.Sprintf("duplicate checkable objects report for %s ", configAddr)) + // This is not a problem as the result is already cached. + return } // At this point we pre-populate all of the check results as StatusUnknown, diff --git a/internal/tofu/graph_builder_eval.go b/internal/tofu/graph_builder_eval.go index ed3d1d9eb3..074df78748 100644 --- a/internal/tofu/graph_builder_eval.go +++ b/internal/tofu/graph_builder_eval.go @@ -67,8 +67,8 @@ func (b *EvalGraphBuilder) Steps() []GraphTransformer { }, // Add dynamic values - &RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues, Planning: true}, - &ModuleVariableTransformer{Config: b.Config, Planning: true}, + &RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues}, + &ModuleVariableTransformer{Config: b.Config}, &LocalTransformer{Config: b.Config}, &OutputTransformer{ Config: b.Config, diff --git a/internal/tofu/graph_builder_plan.go b/internal/tofu/graph_builder_plan.go index c130b53097..620e3356ba 100644 --- a/internal/tofu/graph_builder_plan.go +++ b/internal/tofu/graph_builder_plan.go @@ -128,8 +128,8 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { }, // Add dynamic values - &RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues, Planning: true}, - &ModuleVariableTransformer{Config: b.Config, Planning: true}, + &RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues}, + &ModuleVariableTransformer{Config: b.Config}, &LocalTransformer{Config: b.Config}, &OutputTransformer{ Config: b.Config, diff --git a/internal/tofu/node_module_variable.go b/internal/tofu/node_module_variable.go index f80d6f9adf..134f38be6d 100644 --- a/internal/tofu/node_module_variable.go +++ b/internal/tofu/node_module_variable.go @@ -25,10 +25,6 @@ type nodeExpandModuleVariable struct { Module addrs.Module Config *configs.Variable Expr hcl.Expression - - // Planning must be set to true when building a planning graph, and must be - // false when building an apply graph. - Planning bool } var ( @@ -54,10 +50,8 @@ func (n *nodeExpandModuleVariable) DynamicExpand(ctx EvalContext) (*Graph, error // We should only do this during planning as the apply phase starts with // all the same checkable objects that were registered during the plan. var checkableAddrs addrs.Set[addrs.Checkable] - if n.Planning { - if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(n.Module)) { - checkableAddrs = addrs.MakeSet[addrs.Checkable]() - } + if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(n.Module)) { + checkableAddrs = addrs.MakeSet[addrs.Checkable]() } expander := ctx.InstanceExpander() diff --git a/internal/tofu/node_module_variable_test.go b/internal/tofu/node_module_variable_test.go index d654e75c1b..58b6cdce01 100644 --- a/internal/tofu/node_module_variable_test.go +++ b/internal/tofu/node_module_variable_test.go @@ -4,6 +4,7 @@ package tofu import ( + "errors" "reflect" "testing" @@ -13,7 +14,13 @@ import ( "github.com/zclconf/go-cty/cty" "github.com/opentofu/opentofu/internal/addrs" + "github.com/opentofu/opentofu/internal/checks" "github.com/opentofu/opentofu/internal/configs" + "github.com/opentofu/opentofu/internal/configs/configschema" + "github.com/opentofu/opentofu/internal/plans" + "github.com/opentofu/opentofu/internal/providers" + "github.com/opentofu/opentofu/internal/states" + "github.com/opentofu/opentofu/internal/tfdiags" ) func TestNodeModuleVariablePath(t *testing.T) { @@ -122,3 +129,178 @@ func TestNodeModuleVariableReference_grandchild(t *testing.T) { t.Error(problem) } } + +func TestNodeModuleVariableConstraints(t *testing.T) { + // This is a little extra convoluted to poke at some edge cases that have cropped up in the past around + // evaluating dependent nodes between the plan -> apply and destroy cycle. + m := testModuleInline(t, map[string]string{ + "main.tf": ` + variable "input" { + type = string + validation { + condition = var.input != "" + error_message = "Input must not be empty." + } + } + + module "child" { + source = "./child" + input = var.input + } + + provider "test" { + alias = "secondary" + test_string = module.child.output + } + + resource "test_object" "resource" { + provider = test.secondary + test_string = "test string" + } + + `, + "child/main.tf": ` + variable "input" { + type = string + validation { + condition = var.input != "" + error_message = "Input must not be empty." + } + } + provider "test" { + test_string = "foo" + } + resource "test_object" "resource" { + test_string = var.input + } + output "output" { + value = test_object.resource.id + } + `, + }) + + checkableObjects := []addrs.Checkable{ + addrs.InputVariable{Name: "input"}.Absolute(addrs.RootModuleInstance), + addrs.InputVariable{Name: "input"}.Absolute(addrs.RootModuleInstance.Child("child", addrs.NoKey)), + } + + p := &MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + Provider: providers.Schema{Block: simpleTestSchema()}, + ResourceTypes: map[string]providers.Schema{ + "test_object": providers.Schema{Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + "test_string": { + Type: cty.String, + Required: true, + }, + }, + }}, + }, + }, + } + p.ConfigureProviderFn = func(req providers.ConfigureProviderRequest) (resp providers.ConfigureProviderResponse) { + if req.Config.GetAttr("test_string").IsNull() { + resp.Diagnostics = resp.Diagnostics.Append(errors.New("missing test_string value")) + } + return resp + } + + ctxOpts := &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + } + + t.Run("pass", func(t *testing.T) { + ctx := testContext2(t, ctxOpts) + 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) + } + } + + plan, diags = ctx.Plan(m, state, &PlanOpts{ + Mode: plans.DestroyMode, + SetVariables: InputValues{ + "input": &InputValue{ + Value: cty.StringVal("beep"), + SourceType: ValueFromCLIArg, + }, + }, + }) + assertNoDiagnostics(t, diags) + + 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) { + ctx := testContext2(t, ctxOpts) + _, 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 = "Invalid value for variable" + 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()) + } + }) +} diff --git a/internal/tofu/node_root_variable.go b/internal/tofu/node_root_variable.go index b5559579bc..d9c6f4b788 100644 --- a/internal/tofu/node_root_variable.go +++ b/internal/tofu/node_root_variable.go @@ -25,10 +25,6 @@ type NodeRootVariable struct { // converted or validated, and can be nil for a variable that isn't // set at all. RawValue *InputValue - - // Planning must be set to true when building a planning graph, and must be - // false when building an apply graph. - Planning bool } var ( @@ -87,12 +83,10 @@ func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) tfdiags.Di } } - if n.Planning { - if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(addrs.RootModule)) { - ctx.Checks().ReportCheckableObjects( - n.Addr.InModule(addrs.RootModule), - addrs.MakeSet[addrs.Checkable](n.Addr.Absolute(addrs.RootModuleInstance))) - } + if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(addrs.RootModule)) { + ctx.Checks().ReportCheckableObjects( + n.Addr.InModule(addrs.RootModule), + addrs.MakeSet[addrs.Checkable](n.Addr.Absolute(addrs.RootModuleInstance))) } finalVal, moreDiags := prepareFinalInputVariableValue( diff --git a/internal/tofu/node_root_variable_test.go b/internal/tofu/node_root_variable_test.go index 6dbcfc5a0c..1a679373f9 100644 --- a/internal/tofu/node_root_variable_test.go +++ b/internal/tofu/node_root_variable_test.go @@ -33,6 +33,14 @@ func TestNodeRootVariableExecute(t *testing.T) { }, } + ctx.ChecksState = checks.NewState(&configs.Config{ + Module: &configs.Module{ + Variables: map[string]*configs.Variable{ + "foo": n.Config, + }, + }, + }) + diags := n.Execute(ctx, walkApply) if diags.HasErrors() { t.Fatalf("unexpected error: %s", diags.Err()) @@ -116,7 +124,6 @@ func TestNodeRootVariableExecute(t *testing.T) { Value: cty.StringVal("5"), SourceType: ValueFromUnknown, }, - Planning: true, } ctx.ChecksState = checks.NewState(&configs.Config{ diff --git a/internal/tofu/transform_module_variable.go b/internal/tofu/transform_module_variable.go index eab462939d..71081c91d3 100644 --- a/internal/tofu/transform_module_variable.go +++ b/internal/tofu/transform_module_variable.go @@ -28,10 +28,6 @@ import ( // steps for validating module blocks, separate from this transform. type ModuleVariableTransformer struct { Config *configs.Config - - // Planning must be set to true when building a planning graph, and must be - // false when building an apply graph. - Planning bool } func (t *ModuleVariableTransformer) Transform(g *Graph) error { @@ -110,10 +106,9 @@ func (t *ModuleVariableTransformer) transformSingle(g *Graph, parent, c *configs Addr: addrs.InputVariable{ Name: v.Name, }, - Module: c.Path, - Config: v, - Expr: expr, - Planning: t.Planning, + Module: c.Path, + Config: v, + Expr: expr, } g.Add(node) } diff --git a/internal/tofu/transform_variable.go b/internal/tofu/transform_variable.go index ba012ac426..2da6a4cae3 100644 --- a/internal/tofu/transform_variable.go +++ b/internal/tofu/transform_variable.go @@ -18,10 +18,6 @@ type RootVariableTransformer struct { Config *configs.Config RawValues InputValues - - // Planning must be set to true when building a planning graph, and must be - // false when building an apply graph. - Planning bool } func (t *RootVariableTransformer) Transform(g *Graph) error { @@ -42,7 +38,6 @@ func (t *RootVariableTransformer) Transform(g *Graph) error { }, Config: v, RawValue: t.RawValues[v.Name], - Planning: t.Planning, } g.Add(node) }