From f8d4664bcd53f55ebf2db1767e6b8fc745d83d69 Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Thu, 14 Sep 2023 09:44:21 +0200 Subject: [PATCH] Add additional validation around unknown and null values in test variables (#33861) * Add additional validation around unknown values in test variables * address comments --- internal/backend/local/test.go | 173 +++++++++++------- internal/command/test_test.go | 161 ++++++++++++++++ .../test/nested_unknown_values/main.tf | 19 ++ .../nested_unknown_values/main.tftest.hcl | 33 ++++ .../test/null_value_in_assert/main.tf | 17 ++ .../test/null_value_in_assert/main.tftest.hcl | 16 ++ .../test/null_value_in_vars/fail.tftest.hcl | 13 ++ .../testdata/test/null_value_in_vars/main.tf | 18 ++ .../test/null_value_in_vars/pass.tftest.hcl | 18 ++ .../test/unknown_value_in_assert/main.tf | 14 ++ .../unknown_value_in_assert/main.tftest.hcl | 11 ++ .../test/unknown_value_in_vars/main.tf | 14 ++ .../unknown_value_in_vars/main.tftest.hcl | 10 + internal/terraform/evaluate.go | 2 +- 14 files changed, 450 insertions(+), 69 deletions(-) create mode 100644 internal/command/testdata/test/nested_unknown_values/main.tf create mode 100644 internal/command/testdata/test/nested_unknown_values/main.tftest.hcl create mode 100644 internal/command/testdata/test/null_value_in_assert/main.tf create mode 100644 internal/command/testdata/test/null_value_in_assert/main.tftest.hcl create mode 100644 internal/command/testdata/test/null_value_in_vars/fail.tftest.hcl create mode 100644 internal/command/testdata/test/null_value_in_vars/main.tf create mode 100644 internal/command/testdata/test/null_value_in_vars/pass.tftest.hcl create mode 100644 internal/command/testdata/test/unknown_value_in_assert/main.tf create mode 100644 internal/command/testdata/test/unknown_value_in_assert/main.tftest.hcl create mode 100644 internal/command/testdata/test/unknown_value_in_vars/main.tf create mode 100644 internal/command/testdata/test/unknown_value_in_vars/main.tftest.hcl diff --git a/internal/backend/local/test.go b/internal/backend/local/test.go index e453070656..0d40fba870 100644 --- a/internal/backend/local/test.go +++ b/internal/backend/local/test.go @@ -1269,22 +1269,75 @@ func (runner *TestFileRunner) prepareInputVariablesForAssertions(config *configs func (runner *TestFileRunner) ctx(run *moduletest.Run, file *moduletest.File, availableVariables terraform.InputValues) (*hcl.EvalContext, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - availableRunBlocks := make(map[string]bool) + // First, let's build the set of available run blocks. + + availableRunBlocks := make(map[string]*terraform.TestContext) + runs := make(map[string]cty.Value) for _, run := range file.Runs { name := run.Name - if _, exists := runner.PriorStates[name]; exists { + attrs := make(map[string]cty.Value) + if ctx, exists := runner.PriorStates[name]; exists { // We have executed this run block previously, therefore it is // available as a reference at this point in time. - availableRunBlocks[name] = true + availableRunBlocks[name] = ctx + + for name, config := range ctx.Config.Module.Outputs { + output := ctx.State.OutputValue(addrs.AbsOutputValue{ + OutputValue: addrs.OutputValue{ + Name: name, + }, + Module: addrs.RootModuleInstance, + }) + + var value cty.Value + switch { + case output == nil: + // This means the run block returned null for this output. + // It is likely this will produce an error later if it is + // referenced, but users can actually specify that null + // is an acceptable value for an input variable so we won't + // actually raise a fuss about this at all. + value = cty.NullVal(cty.DynamicPseudoType) + case output.Value.IsNull() || output.Value == cty.NilVal: + // This means the output value was returned as (known after + // apply). If this is referenced it always an error, we + // can't handle this in an appropriate way at all. For now, + // we just mark it as unknown and then later we check and + // resolve all the references. We'll raise an error at that + // point if the user actually attempts to reference a value + // that is unknown. + value = cty.DynamicVal + default: + value = output.Value + } + + if config.Sensitive || (output != nil && output.Sensitive) { + value = value.Mark(marks.Sensitive) + } + + attrs[name] = value + } + + runs[name] = cty.ObjectVal(attrs) + continue } // We haven't executed this run block yet, therefore it is not available // as a reference at this point in time. - availableRunBlocks[name] = false + availableRunBlocks[name] = nil } + // Second, let's build the set of available variables. + + vars := make(map[string]cty.Value) + for name, variable := range availableVariables { + vars[name] = variable.Value + } + + // Third, let's do some basic validation over the references. + for _, value := range run.Config.Variables { refs, refDiags := lang.ReferencesInExpr(addrs.ParseRefFromTestingScope, value) diags = diags.Append(refDiags) @@ -1294,7 +1347,7 @@ func (runner *TestFileRunner) ctx(run *moduletest.Run, file *moduletest.File, av for _, ref := range refs { if addr, ok := ref.Subject.(addrs.Run); ok { - available, exists := availableRunBlocks[addr.Name] + ctx, exists := availableRunBlocks[addr.Name] if !exists { // Then this is a made up run block. @@ -1308,7 +1361,7 @@ func (runner *TestFileRunner) ctx(run *moduletest.Run, file *moduletest.File, av continue } - if !available { + if ctx == nil { // This run block exists, but it is after the current run block. diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, @@ -1320,12 +1373,50 @@ func (runner *TestFileRunner) ctx(run *moduletest.Run, file *moduletest.File, av continue } - // Otherwise, we're good. This is an acceptable reference. + value, valueDiags := ref.Remaining.TraverseRel(runs[addr.Name]) + diags = diags.Append(valueDiags) + if valueDiags.HasErrors() { + // This means the reference was invalid somehow, we've + // already added the errors to our diagnostics though so + // we'll just carry on. + continue + } + + if !value.IsWhollyKnown() { + // This is not valid, we cannot allow users to pass unknown + // values into run blocks. There's just going to be + // difficult and confusing errors later if this happens. + + if ctx.Run.Config.Command == configs.PlanTestCommand { + // Then the user has likely attempted to use an output + // that is (known after apply) due to the referenced + // run block only being a plan command. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reference to unknown value", + Detail: fmt.Sprintf("The value for %s is unknown. Run block %q is executing a \"plan\" operation, and the specified output value is only known after apply.", ref.DisplayString(), addr.Name), + Subject: ref.SourceRange.ToHCL().Ptr(), + }) + + continue + } + + // Otherwise, this is a bug in Terraform. We shouldn't be + // producing (known after apply) values during apply + // operations. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reference to unknown value", + Detail: fmt.Sprintf("The value for %s is unknown; This is a bug in Terraform, please report it.", ref.DisplayString()), + Subject: ref.SourceRange.ToHCL().Ptr(), + }) + } + continue } if addr, ok := ref.Subject.(addrs.InputVariable); ok { - if _, exists := availableVariables[addr.Name]; !exists { + if _, exists := vars[addr.Name]; !exists { // This variable reference doesn't exist. diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, @@ -1352,66 +1443,12 @@ func (runner *TestFileRunner) ctx(run *moduletest.Run, file *moduletest.File, av } } - return &hcl.EvalContext{ - Variables: func() map[string]cty.Value { - blocks := make(map[string]cty.Value) - for run, ctx := range runner.PriorStates { - - outputs := make(map[string]cty.Value) - for _, output := range ctx.Config.Module.Outputs { - value := ctx.State.OutputValue(addrs.AbsOutputValue{ - Module: addrs.RootModuleInstance, - OutputValue: addrs.OutputValue{ - Name: output.Name, - }, - }) - - if value == nil { - // Then this output returned null when the configuration - // executed. For now, we'll just skip this output. - // - // There are several things we could try to do, like - // figure out the type based on the variable that it - // is referencing and wrap it up as cty.Val(...) or we - // could not try and work anything out and return it as - // a cty.NilVal. - // - // Both of these mean the error would be raised later - // as non-optional variables would say they don't have - // a value. By just ignoring it here, we get an error - // quicker that says this output doesn't exist. I think - // that would prompt users to go look at the output and - // realise it might be returning null and make the - // connection. With the other approaches they'd look at - // their variable definitions and think they are - // assigning it a value since we would be telling them - // the output does exist. - // - // Let's do the simple thing now, and see what the - // future holds. - continue - } + // Finally, we can just populate our hcl.EvalContext. - if value.Sensitive || output.Sensitive { - outputs[output.Name] = value.Value.Mark(marks.Sensitive) - continue - } - - outputs[output.Name] = value.Value - } - - blocks[run] = cty.ObjectVal(outputs) - } - - variables := make(map[string]cty.Value) - for name, variable := range availableVariables { - variables[name] = variable.Value - } - - return map[string]cty.Value{ - "run": cty.ObjectVal(blocks), - "var": cty.ObjectVal(variables), - } - }(), + return &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "run": cty.ObjectVal(runs), + "var": cty.ObjectVal(vars), + }, }, diags } diff --git a/internal/command/test_test.go b/internal/command/test_test.go index 514ff367b2..e7c8898e2d 100644 --- a/internal/command/test_test.go +++ b/internal/command/test_test.go @@ -4,6 +4,7 @@ package command import ( + "fmt" "path" "strings" "testing" @@ -1265,3 +1266,163 @@ input must contain the character 'b' t.Errorf("should have deleted all resources on completion but left %v", provider.ResourceString()) } } + +func TestTest_UnknownAndNulls(t *testing.T) { + + tcs := map[string]struct { + code int + stdout string + stderr string + }{ + "null_value_in_assert": { + code: 1, + stdout: `main.tftest.hcl... in progress + run "first"... fail +main.tftest.hcl... tearing down +main.tftest.hcl... fail + +Failure! 0 passed, 1 failed. +`, + stderr: ` +Error: Test assertion failed + + on main.tftest.hcl line 8, in run "first": + 8: condition = test_resource.resource.value == output.null_output + ├──────────────── + │ output.null_output is null + │ test_resource.resource.value is "bar" + +this is always going to fail +`, + }, + "null_value_in_vars": { + code: 1, + stdout: `fail.tftest.hcl... in progress + run "first"... pass + run "second"... fail +fail.tftest.hcl... tearing down +fail.tftest.hcl... fail +pass.tftest.hcl... in progress + run "first"... pass + run "second"... pass +pass.tftest.hcl... tearing down +pass.tftest.hcl... pass + +Failure! 3 passed, 1 failed. +`, + stderr: ` +Error: Required variable not set + + on fail.tftest.hcl line 11, in run "second": + 11: interesting_input = run.first.null_output + +The given value is not suitable for var.interesting_input defined at +main.tf:7,1-29: required variable may not be set to null. +`, + }, + "unknown_value_in_assert": { + code: 1, + stdout: `main.tftest.hcl... in progress + run "one"... pass + run "two"... fail +main.tftest.hcl... tearing down +main.tftest.hcl... fail + +Failure! 1 passed, 1 failed. +`, + stderr: fmt.Sprintf(` +Error: Unknown condition value + + on main.tftest.hcl line 8, in run "two": + 8: condition = output.destroy_fail == run.one.destroy_fail + ├──────────────── + │ output.destroy_fail is false + +Condition expression could not be evaluated at this time. This means you have +executed a %s block with %s and one of the values your +condition depended on is not known until after the plan has been applied. +Either remove this value from your condition, or execute an %s command +from this %s block. +`, "`run`", "`command = plan`", "`apply`", "`run`"), + }, + "unknown_value_in_vars": { + code: 1, + stdout: `main.tftest.hcl... in progress + run "one"... pass + run "two"... fail +main.tftest.hcl... tearing down +main.tftest.hcl... fail + +Failure! 1 passed, 1 failed. +`, + stderr: ` +Error: Reference to unknown value + + on main.tftest.hcl line 8, in run "two": + 8: destroy_fail = run.one.destroy_fail + +The value for run.one.destroy_fail is unknown. Run block "one" is executing a +"plan" operation, and the specified output value is only known after apply. +`, + }, + "nested_unknown_values": { + code: 1, + stdout: `main.tftest.hcl... in progress + run "first"... pass + run "second"... pass + run "third"... fail +main.tftest.hcl... tearing down +main.tftest.hcl... fail + +Failure! 2 passed, 1 failed. +`, + stderr: ` +Error: Reference to unknown value + + on main.tftest.hcl line 31, in run "third": + 31: input = run.second + +The value for run.second is unknown. Run block "second" is executing a "plan" +operation, and the specified output value is only known after apply. +`, + }, + } + + for name, tc := range tcs { + t.Run(name, func(t *testing.T) { + td := t.TempDir() + testCopyDir(t, testFixturePath(path.Join("test", name)), td) + defer testChdir(t, td)() + + provider := testing_command.NewProvider(nil) + view, done := testView(t) + + c := &TestCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(provider.Provider), + View: view, + }, + } + + code := c.Run([]string{"-no-color"}) + output := done(t) + + if code != tc.code { + t.Errorf("expected return code %d but got %d", tc.code, code) + } + + expectedOut := tc.stdout + actualOut := output.Stdout() + if diff := cmp.Diff(expectedOut, actualOut); len(diff) > 0 { + t.Errorf("unexpected output\n\nexpected:\n%s\nactual:\n%s\ndiff:\n%s", expectedOut, actualOut, diff) + } + + expectedErr := tc.stderr + actualErr := output.Stderr() + if diff := cmp.Diff(expectedErr, actualErr); len(diff) > 0 { + t.Errorf("unexpected output\n\nexpected:\n%s\nactual:\n%s\ndiff:\n%s", expectedErr, actualErr, diff) + } + }) + } + +} diff --git a/internal/command/testdata/test/nested_unknown_values/main.tf b/internal/command/testdata/test/nested_unknown_values/main.tf new file mode 100644 index 0000000000..6c7977cff7 --- /dev/null +++ b/internal/command/testdata/test/nested_unknown_values/main.tf @@ -0,0 +1,19 @@ + +variable "input" { + type = object({ + one = string, + two = string, + }) +} + +resource "test_resource" "resource" { + value = var.input.two +} + +output "one" { + value = test_resource.resource.id +} + +output "two" { + value = test_resource.resource.value +} diff --git a/internal/command/testdata/test/nested_unknown_values/main.tftest.hcl b/internal/command/testdata/test/nested_unknown_values/main.tftest.hcl new file mode 100644 index 0000000000..1a63ad93a6 --- /dev/null +++ b/internal/command/testdata/test/nested_unknown_values/main.tftest.hcl @@ -0,0 +1,33 @@ + +run "first" { + + command = plan + + variables { + input = { + one = "one" + two = "two" + } + } +} + +run "second" { + + command = plan + + variables { + input = { + # This should be okay, as run.first.one is unknown but we're not + # referencing it directly. + one = "one" + two = run.first.two + } + } +} + +run "third" { + variables { + # This should fail as one of the values in run.second is unknown. + input = run.second + } +} \ No newline at end of file diff --git a/internal/command/testdata/test/null_value_in_assert/main.tf b/internal/command/testdata/test/null_value_in_assert/main.tf new file mode 100644 index 0000000000..aac3d54110 --- /dev/null +++ b/internal/command/testdata/test/null_value_in_assert/main.tf @@ -0,0 +1,17 @@ + +variable "null_input" { + type = string + default = null +} + +variable "interesting_input" { + type = string +} + +resource "test_resource" "resource" { + value = var.interesting_input +} + +output "null_output" { + value = var.null_input +} diff --git a/internal/command/testdata/test/null_value_in_assert/main.tftest.hcl b/internal/command/testdata/test/null_value_in_assert/main.tftest.hcl new file mode 100644 index 0000000000..83b26d3814 --- /dev/null +++ b/internal/command/testdata/test/null_value_in_assert/main.tftest.hcl @@ -0,0 +1,16 @@ + +run "first" { + variables { + interesting_input = "bar" + } + + assert { + condition = test_resource.resource.value == output.null_output + error_message = "this is always going to fail" + } + + assert { + condition = var.null_input == output.null_output + error_message = "this should pass" + } +} diff --git a/internal/command/testdata/test/null_value_in_vars/fail.tftest.hcl b/internal/command/testdata/test/null_value_in_vars/fail.tftest.hcl new file mode 100644 index 0000000000..24b7bef410 --- /dev/null +++ b/internal/command/testdata/test/null_value_in_vars/fail.tftest.hcl @@ -0,0 +1,13 @@ + +run "first" { + variables { + interesting_input = "bar" + } +} + +run "second" { + variables { + // It shouldn't let this happen. + interesting_input = run.first.null_output + } +} diff --git a/internal/command/testdata/test/null_value_in_vars/main.tf b/internal/command/testdata/test/null_value_in_vars/main.tf new file mode 100644 index 0000000000..8d80485053 --- /dev/null +++ b/internal/command/testdata/test/null_value_in_vars/main.tf @@ -0,0 +1,18 @@ + +variable "null_input" { + type = string + default = null +} + +variable "interesting_input" { + type = string + nullable = false +} + +resource "test_resource" "resource" { + value = var.interesting_input +} + +output "null_output" { + value = var.null_input +} diff --git a/internal/command/testdata/test/null_value_in_vars/pass.tftest.hcl b/internal/command/testdata/test/null_value_in_vars/pass.tftest.hcl new file mode 100644 index 0000000000..3568b2c2f4 --- /dev/null +++ b/internal/command/testdata/test/null_value_in_vars/pass.tftest.hcl @@ -0,0 +1,18 @@ + +run "first" { + variables { + interesting_input = "bar" + } +} + +run "second" { + variables { + interesting_input = "bar" + null_input = run.first.null_output + } + + assert { + condition = output.null_output == run.first.null_output + error_message = "should have passed" + } +} diff --git a/internal/command/testdata/test/unknown_value_in_assert/main.tf b/internal/command/testdata/test/unknown_value_in_assert/main.tf new file mode 100644 index 0000000000..f269501272 --- /dev/null +++ b/internal/command/testdata/test/unknown_value_in_assert/main.tf @@ -0,0 +1,14 @@ + +variable "destroy_fail" { + type = bool + default = null + nullable = true +} + +resource "test_resource" "resource" { + destroy_fail = var.destroy_fail +} + +output "destroy_fail" { + value = test_resource.resource.destroy_fail +} diff --git a/internal/command/testdata/test/unknown_value_in_assert/main.tftest.hcl b/internal/command/testdata/test/unknown_value_in_assert/main.tftest.hcl new file mode 100644 index 0000000000..5bf5dcfed2 --- /dev/null +++ b/internal/command/testdata/test/unknown_value_in_assert/main.tftest.hcl @@ -0,0 +1,11 @@ + +run "one" { + command = plan +} + +run "two" { + assert { + condition = output.destroy_fail == run.one.destroy_fail + error_message = "should fail" + } +} diff --git a/internal/command/testdata/test/unknown_value_in_vars/main.tf b/internal/command/testdata/test/unknown_value_in_vars/main.tf new file mode 100644 index 0000000000..f269501272 --- /dev/null +++ b/internal/command/testdata/test/unknown_value_in_vars/main.tf @@ -0,0 +1,14 @@ + +variable "destroy_fail" { + type = bool + default = null + nullable = true +} + +resource "test_resource" "resource" { + destroy_fail = var.destroy_fail +} + +output "destroy_fail" { + value = test_resource.resource.destroy_fail +} diff --git a/internal/command/testdata/test/unknown_value_in_vars/main.tftest.hcl b/internal/command/testdata/test/unknown_value_in_vars/main.tftest.hcl new file mode 100644 index 0000000000..2103aaebe1 --- /dev/null +++ b/internal/command/testdata/test/unknown_value_in_vars/main.tftest.hcl @@ -0,0 +1,10 @@ + +run "one" { + command = plan +} + +run "two" { + variables { + destroy_fail = run.one.destroy_fail + } +} diff --git a/internal/terraform/evaluate.go b/internal/terraform/evaluate.go index b9548bd728..622083263e 100644 --- a/internal/terraform/evaluate.go +++ b/internal/terraform/evaluate.go @@ -989,7 +989,7 @@ func (d *evaluationStateData) GetOutput(addr addrs.OutputValue, rng tfdiags.Sour Value: cty.NilVal, Sensitive: config.Sensitive, } - } else if output.Value == cty.NilVal { + } else if output.Value == cty.NilVal || output.Value.IsNull() { // Then we did get a value but Terraform itself thought it was NilVal // so we treat this as if the value isn't yet known. output.Value = cty.DynamicVal