Skip to content

Commit 45d0c04

Browse files
alisdairalisdair
committed
core: Add fallback for JSON syntax error messages
Custom variable validations specified using JSON syntax would always parse error messages as string literals, even if they included template expressions. We need to be as backwards compatible with this behaviour as possible, which results in this complex fallback logic. More detail about this in the extensive code comments.
1 parent b59bffa commit 45d0c04

File tree

2 files changed

+207
-2
lines changed

2 files changed

+207
-2
lines changed

‎internal/terraform/eval_variable.go‎

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"strings"
77

88
"github.com/hashicorp/hcl/v2"
9+
"github.com/hashicorp/hcl/v2/gohcl"
910
"github.com/hashicorp/terraform/internal/addrs"
1011
"github.com/hashicorp/terraform/internal/configs"
1112
"github.com/hashicorp/terraform/internal/tfdiags"
@@ -211,12 +212,62 @@ func evalVariableValidations(addr addrs.AbsInputVariableInstance, config *config
211212
result, moreDiags := validation.Condition.Value(hclCtx)
212213
ruleDiags = ruleDiags.Append(moreDiags)
213214
errorValue, errorDiags := validation.ErrorMessage.Value(hclCtx)
214-
ruleDiags = ruleDiags.Append(errorDiags)
215+
216+
// The following error handling is a workaround to preserve backwards
217+
// compatibility. Due to an implementation quirk, all prior versions of
218+
// Terraform would treat error messages specified using JSON
219+
// configuration syntax (.tf.json) as string literals, even if they
220+
// contained the "${" template expression operator. This behaviour did
221+
// not match that of HCL configuration syntax, where a template
222+
// expression would result in a validation error.
223+
//
224+
// As a result, users writing or generating JSON configuration syntax
225+
// may have specified error messages which are invalid template
226+
// expressions. As we add support for error message expressions, we are
227+
// unable to perfectly distinguish between these two cases.
228+
//
229+
// To ensure that we don't break backwards compatibility, we have the
230+
// below fallback logic if the error message fails to evaluate. This
231+
// should only have any effect for JSON configurations. The gohcl
232+
// DecodeExpression function behaves differently when the source of the
233+
// expression is a JSON configuration file and a nil context is passed.
234+
if errorDiags.HasErrors() {
235+
// Attempt to decode the expression as a string literal. Passing
236+
// nil as the context forces a JSON syntax string value to be
237+
// interpreted as a string literal.
238+
var errorString string
239+
moreErrorDiags := gohcl.DecodeExpression(validation.ErrorMessage, nil, &errorString)
240+
if !moreErrorDiags.HasErrors() {
241+
// Decoding succeeded, meaning that this is a JSON syntax
242+
// string value. We rewrap that as a cty value to allow later
243+
// decoding to succeed.
244+
errorValue = cty.StringVal(errorString)
245+
246+
// This warning diagnostic explains this odd behaviour, while
247+
// giving us an escape hatch to change this to a hard failure
248+
// in some future Terraform 1.x version.
249+
errorDiags = hcl.Diagnostics{
250+
&hcl.Diagnostic{
251+
Severity: hcl.DiagWarning,
252+
Summary: "Validation error message expression is invalid",
253+
Detail: fmt.Sprintf("The error message provided could not be evaluated as an expression, so Terraform is interpreting it as a string literal.\n\nIn future versions of Terraform, this will be considered an error. Please file a GitHub issue if this would break your workflow.\n\n%s", errorDiags.Error()),
254+
Subject: validation.ErrorMessage.Range().Ptr(),
255+
Context: validation.DeclRange.Ptr(),
256+
Expression: validation.ErrorMessage,
257+
EvalContext: hclCtx,
258+
},
259+
}
260+
}
261+
262+
// We want to either report the original diagnostics if the
263+
// fallback failed, or the warning generated above if it succeeded.
264+
ruleDiags = ruleDiags.Append(errorDiags)
265+
}
215266

216267
diags = diags.Append(ruleDiags)
217268

218269
if ruleDiags.HasErrors() {
219-
log.Printf("[TRACE] evalVariableValidations: %s rule %s condition expression failed: %s", addr, validation.DeclRange, ruleDiags.Err().Error())
270+
log.Printf("[TRACE] evalVariableValidations: %s rule %s check rule evaluation failed: %s", addr, validation.DeclRange, ruleDiags.Err().Error())
220271
}
221272
if !result.IsKnown() {
222273
log.Printf("[TRACE] evalVariableValidations: %s rule %s condition value is unknown, so skipping validation for now", addr, validation.DeclRange)

‎internal/terraform/eval_variable_test.go‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,14 @@ package terraform
22

33
import (
44
"fmt"
5+
"strings"
56
"testing"
67

78
"github.com/hashicorp/hcl/v2"
89
"github.com/zclconf/go-cty/cty"
910

1011
"github.com/hashicorp/terraform/internal/addrs"
12+
"github.com/hashicorp/terraform/internal/lang"
1113
"github.com/hashicorp/terraform/internal/tfdiags"
1214
)
1315

@@ -561,3 +563,155 @@ func TestPrepareFinalInputVariableValue(t *testing.T) {
561563
}
562564
})
563565
}
566+
567+
// These tests cover the JSON syntax configuration edge case handling,
568+
// the background of which is described in detail in comments in the
569+
// evalVariableValidations function. Future versions of Terraform may
570+
// be able to remove this behaviour altogether.
571+
func TestEvalVariableValidations_jsonErrorMessageEdgeCase(t *testing.T) {
572+
cfgSrc := `{
573+
"variable": {
574+
"valid": {
575+
"type": "string",
576+
"validation": {
577+
"condition": "${var.valid != \"bar\"}",
578+
"error_message": "Valid template string ${var.valid}"
579+
}
580+
},
581+
"invalid": {
582+
"type": "string",
583+
"validation": {
584+
"condition": "${var.invalid != \"bar\"}",
585+
"error_message": "Invalid template string ${"
586+
}
587+
}
588+
}
589+
}
590+
`
591+
cfg := testModuleInline(t, map[string]string{
592+
"main.tf.json": cfgSrc,
593+
})
594+
variableConfigs := cfg.Module.Variables
595+
596+
// Because we loaded our pseudo-module from a temporary file, the
597+
// declaration source ranges will have unpredictable filenames. We'll
598+
// fix that here just to make things easier below.
599+
for _, vc := range variableConfigs {
600+
vc.DeclRange.Filename = "main.tf.json"
601+
for _, v := range vc.Validations {
602+
v.DeclRange.Filename = "main.tf.json"
603+
}
604+
}
605+
606+
tests := []struct {
607+
varName string
608+
given cty.Value
609+
wantErr []string
610+
wantWarn []string
611+
}{
612+
// Valid variable validation declaration, assigned value which passes
613+
// the condition generates no diagnostics.
614+
{
615+
varName: "valid",
616+
given: cty.StringVal("foo"),
617+
},
618+
// Assigning a value which fails the condition generates an error
619+
// message with the expression successfully evaluated.
620+
{
621+
varName: "valid",
622+
given: cty.StringVal("bar"),
623+
wantErr: []string{
624+
"Invalid value for variable",
625+
"Valid template string bar",
626+
},
627+
},
628+
// Invalid variable validation declaration due to an unparseable
629+
// template string. Assigning a value which passes the condition
630+
// results in a warning about the error message.
631+
{
632+
varName: "invalid",
633+
given: cty.StringVal("foo"),
634+
wantWarn: []string{
635+
"Validation error message expression is invalid",
636+
"Missing expression; Expected the start of an expression, but found the end of the file.",
637+
},
638+
},
639+
// Assigning a value which fails the condition generates an error
640+
// message including the configured string interpreted as a literal
641+
// value, and the same warning diagnostic as above.
642+
{
643+
varName: "invalid",
644+
given: cty.StringVal("bar"),
645+
wantErr: []string{
646+
"Invalid value for variable",
647+
"Invalid template string ${",
648+
},
649+
wantWarn: []string{
650+
"Validation error message expression is invalid",
651+
"Missing expression; Expected the start of an expression, but found the end of the file.",
652+
},
653+
},
654+
}
655+
656+
for _, test := range tests {
657+
t.Run(fmt.Sprintf("%s %#v", test.varName, test.given), func(t *testing.T) {
658+
varAddr := addrs.InputVariable{Name: test.varName}.Absolute(addrs.RootModuleInstance)
659+
varCfg := variableConfigs[test.varName]
660+
if varCfg == nil {
661+
t.Fatalf("invalid variable name %q", test.varName)
662+
}
663+
664+
// Build a mock context to allow the function under test to
665+
// retrieve the variable value and evaluate the expressions
666+
ctx := &MockEvalContext{}
667+
668+
// We need a minimal scope to allow basic functions to be passed to
669+
// the HCL scope
670+
ctx.EvaluationScopeScope = &lang.Scope{}
671+
ctx.GetVariableValueFunc = func(addr addrs.AbsInputVariableInstance) cty.Value {
672+
if got, want := addr.String(), varAddr.String(); got != want {
673+
t.Errorf("incorrect argument to GetVariableValue: got %s, want %s", got, want)
674+
}
675+
return test.given
676+
}
677+
678+
gotDiags := evalVariableValidations(
679+
varAddr, varCfg, nil, ctx,
680+
)
681+
682+
if len(test.wantErr) == 0 && len(test.wantWarn) == 0 {
683+
if len(gotDiags) > 0 {
684+
t.Errorf("no diags expected, got %s", gotDiags.Err().Error())
685+
}
686+
} else {
687+
wantErrs:
688+
for _, want := range test.wantErr {
689+
for _, diag := range gotDiags {
690+
if diag.Severity() != tfdiags.Error {
691+
continue
692+
}
693+
desc := diag.Description()
694+
if strings.Contains(desc.Summary, want) || strings.Contains(desc.Detail, want) {
695+
continue wantErrs
696+
}
697+
}
698+
t.Errorf("no error diagnostics found containing %q\ngot: %s", want, gotDiags.Err().Error())
699+
}
700+
701+
wantWarns:
702+
for _, want := range test.wantWarn {
703+
for _, diag := range gotDiags {
704+
if diag.Severity() != tfdiags.Warning {
705+
continue
706+
}
707+
desc := diag.Description()
708+
if strings.Contains(desc.Summary, want) || strings.Contains(desc.Detail, want) {
709+
continue wantWarns
710+
}
711+
}
712+
t.Errorf("no warning diagnostics found containing %q\ngot: %s", want, gotDiags.Err().Error())
713+
}
714+
}
715+
})
716+
}
717+
}

0 commit comments

Comments
 (0)