From e2c64bc255b8d11d2dc0b6b2fdb172fa55733942 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 28 Oct 2020 17:52:03 -0700 Subject: [PATCH] core: Annotate for_each errors with expression info Our diagnostics model allows for optionally annotating an error or warning with information about the expression and eval context it was generated from, which the diagnostic renderer for the UI will then use to give the user some additional hints about what values may have contributed to the error. We previously didn't have those annotations on the results of evaluating for_each expressions though, because in that case we were using the helper function to evaluate an expression in one shot and thus we didn't ever have a reference to the EvalContext in order to include it in the diagnostic values. Now, at the expense of having to handle the evaluation at a slightly lower level of abstraction, we'll annotate all of the for_each error messages with source expression information. This is valuable because we see users often confused as to how their complex for_each expressions ended up being invalid, and hopefully giving some information about what the inputs were will allow more users to self-solve. --- lang/eval.go | 10 +-- terraform/eval_for_each.go | 106 ++++++++++++++++++++++---------- terraform/eval_for_each_test.go | 12 +++- terraform/eval_validate.go | 2 +- terraform/node_module_expand.go | 2 +- 5 files changed, 93 insertions(+), 39 deletions(-) diff --git a/lang/eval.go b/lang/eval.go index 381ec4288..f3369069d 100644 --- a/lang/eval.go +++ b/lang/eval.go @@ -129,10 +129,12 @@ func (s *Scope) EvalExpr(expr hcl.Expression, wantType cty.Type) (cty.Value, tfd if convErr != nil { val = cty.UnknownVal(wantType) diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Incorrect value type", - Detail: fmt.Sprintf("Invalid expression value: %s.", tfdiags.FormatError(convErr)), - Subject: expr.Range().Ptr(), + Severity: hcl.DiagError, + Summary: "Incorrect value type", + Detail: fmt.Sprintf("Invalid expression value: %s.", tfdiags.FormatError(convErr)), + Subject: expr.Range().Ptr(), + Expression: expr, + EvalContext: ctx, }) } } diff --git a/terraform/eval_for_each.go b/terraform/eval_for_each.go index 75e6eabf1..7164a3650 100644 --- a/terraform/eval_for_each.go +++ b/terraform/eval_for_each.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/terraform/lang" "github.com/hashicorp/terraform/tfdiags" "github.com/zclconf/go-cty/cty" ) @@ -17,16 +18,9 @@ import ( // returning an error if the count value is not known, and converting the // cty.Value to a map[string]cty.Value for compatibility with other calls. func evaluateForEachExpression(expr hcl.Expression, ctx EvalContext) (forEach map[string]cty.Value, diags tfdiags.Diagnostics) { - forEachVal, diags := evaluateForEachExpressionValue(expr, ctx) - if !forEachVal.IsKnown() { - // Attach a diag as we do with count, with the same downsides - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid for_each argument", - Detail: `The "for_each" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target argument to first apply only the resources that the for_each depends on.`, - Subject: expr.Range().Ptr(), - }) - } + forEachVal, diags := evaluateForEachExpressionValue(expr, ctx, false) + // forEachVal might be unknown, but if it is then there should already + // be an error about it in diags, which we'll return below. if forEachVal.IsNull() || !forEachVal.IsKnown() || forEachVal.LengthInt() == 0 { // we check length, because an empty set return a nil map @@ -38,7 +32,7 @@ func evaluateForEachExpression(expr hcl.Expression, ctx EvalContext) (forEach ma // evaluateForEachExpressionValue is like evaluateForEachExpression // except that it returns a cty.Value map or set which can be unknown. -func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.Value, tfdiags.Diagnostics) { +func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowUnknown bool) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics nullMap := cty.NullVal(cty.Map(cty.DynamicPseudoType)) @@ -46,14 +40,32 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V return nullMap, diags } - forEachVal, forEachDiags := ctx.EvaluateExpr(expr, cty.DynamicPseudoType, nil) + refs, moreDiags := lang.ReferencesInExpr(expr) + diags = diags.Append(moreDiags) + scope := ctx.EvaluationScope(nil, EvalDataForNoInstanceKey) + var hclCtx *hcl.EvalContext + if scope != nil { + hclCtx, moreDiags = scope.EvalContext(refs) + } else { + // This shouldn't happen in real code, but it can unfortunately arise + // in unit tests due to incompletely-implemented mocks. :( + hclCtx = &hcl.EvalContext{} + } + diags = diags.Append(moreDiags) + if diags.HasErrors() { // Can't continue if we don't even have a valid scope + return nullMap, diags + } + + forEachVal, forEachDiags := expr.Value(hclCtx) diags = diags.Append(forEachDiags) if forEachVal.ContainsMarked() { diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid for_each argument", - Detail: "Sensitive variable, or values derived from sensitive variables, cannot be used as for_each arguments. If used, the sensitive value could be exposed as a resource instance key.", - Subject: expr.Range().Ptr(), + Severity: hcl.DiagError, + Summary: "Invalid for_each argument", + Detail: "Sensitive variable, or values derived from sensitive variables, cannot be used as for_each arguments. If used, the sensitive value could be exposed as a resource instance key.", + Subject: expr.Range().Ptr(), + Expression: expr, + EvalContext: hclCtx, }) } if diags.HasErrors() { @@ -64,22 +76,36 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V switch { case forEachVal.IsNull(): diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid for_each argument", - Detail: `The given "for_each" argument value is unsuitable: the given "for_each" argument value is null. A map, or set of strings is allowed.`, - Subject: expr.Range().Ptr(), + Severity: hcl.DiagError, + Summary: "Invalid for_each argument", + Detail: `The given "for_each" argument value is unsuitable: the given "for_each" argument value is null. A map, or set of strings is allowed.`, + Subject: expr.Range().Ptr(), + Expression: expr, + EvalContext: hclCtx, }) return nullMap, diags case !forEachVal.IsKnown(): + if !allowUnknown { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid for_each argument", + Detail: errInvalidForEachUnknownDetail, + Subject: expr.Range().Ptr(), + Expression: expr, + EvalContext: hclCtx, + }) + } // ensure that we have a map, and not a DynamicValue return cty.UnknownVal(cty.Map(cty.DynamicPseudoType)), diags case !(ty.IsMapType() || ty.IsSetType() || ty.IsObjectType()): diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid for_each argument", - Detail: fmt.Sprintf(`The given "for_each" argument value is unsuitable: the "for_each" argument must be a map, or set of strings, and you have provided a value of type %s.`, ty.FriendlyName()), - Subject: expr.Range().Ptr(), + Severity: hcl.DiagError, + Summary: "Invalid for_each argument", + Detail: fmt.Sprintf(`The given "for_each" argument value is unsuitable: the "for_each" argument must be a map, or set of strings, and you have provided a value of type %s.`, ty.FriendlyName()), + Subject: expr.Range().Ptr(), + Expression: expr, + EvalContext: hclCtx, }) return nullMap, diags @@ -94,15 +120,27 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V // since we can't use a set values that are unknown, we treat the // entire set as unknown if !forEachVal.IsWhollyKnown() { + if !allowUnknown { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid for_each argument", + Detail: errInvalidForEachUnknownDetail, + Subject: expr.Range().Ptr(), + Expression: expr, + EvalContext: hclCtx, + }) + } return cty.UnknownVal(ty), diags } if ty.ElementType() != cty.String { diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid for_each set argument", - Detail: fmt.Sprintf(`The given "for_each" argument value is unsuitable: "for_each" supports maps and sets of strings, but you have provided a set containing type %s.`, forEachVal.Type().ElementType().FriendlyName()), - Subject: expr.Range().Ptr(), + Severity: hcl.DiagError, + Summary: "Invalid for_each set argument", + Detail: fmt.Sprintf(`The given "for_each" argument value is unsuitable: "for_each" supports maps and sets of strings, but you have provided a set containing type %s.`, forEachVal.Type().ElementType().FriendlyName()), + Subject: expr.Range().Ptr(), + Expression: expr, + EvalContext: hclCtx, }) return cty.NullVal(ty), diags } @@ -114,10 +152,12 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V item, _ := it.Element() if item.IsNull() { diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid for_each set argument", - Detail: fmt.Sprintf(`The given "for_each" argument value is unsuitable: "for_each" sets must not contain null values.`), - Subject: expr.Range().Ptr(), + Severity: hcl.DiagError, + Summary: "Invalid for_each set argument", + Detail: fmt.Sprintf(`The given "for_each" argument value is unsuitable: "for_each" sets must not contain null values.`), + Subject: expr.Range().Ptr(), + Expression: expr, + EvalContext: hclCtx, }) return cty.NullVal(ty), diags } @@ -126,3 +166,5 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V return forEachVal, nil } + +const errInvalidForEachUnknownDetail = `The "for_each" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target argument to first apply only the resources that the for_each depends on.` diff --git a/terraform/eval_for_each_test.go b/terraform/eval_for_each_test.go index c7920a694..cc0c9fded 100644 --- a/terraform/eval_for_each_test.go +++ b/terraform/eval_for_each_test.go @@ -150,6 +150,16 @@ func TestEvaluateForEachExpression_errors(t *testing.T) { if got, want := diags[0].Description().Detail, test.DetailSubstring; !strings.Contains(got, want) { t.Errorf("wrong diagnostic detail %#v; want %#v", got, want) } + if fromExpr := diags[0].FromExpr(); fromExpr != nil { + if fromExpr.Expression == nil { + t.Errorf("diagnostic does not refer to an expression") + } + if fromExpr.EvalContext == nil { + t.Errorf("diagnostic does not refer to an EvalContext") + } + } else { + t.Errorf("diagnostic does not support FromExpr\ngot: %s", spew.Sdump(diags[0])) + } }) } } @@ -164,7 +174,7 @@ func TestEvaluateForEachExpressionKnown(t *testing.T) { t.Run(name, func(t *testing.T) { ctx := &MockEvalContext{} ctx.installSimpleEval() - forEachVal, diags := evaluateForEachExpressionValue(expr, ctx) + forEachVal, diags := evaluateForEachExpressionValue(expr, ctx, true) if len(diags) != 0 { t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) diff --git a/terraform/eval_validate.go b/terraform/eval_validate.go index c5daf01ef..288cc62a2 100644 --- a/terraform/eval_validate.go +++ b/terraform/eval_validate.go @@ -519,7 +519,7 @@ func (n *EvalValidateResource) validateCount(ctx EvalContext, expr hcl.Expressio } func (n *EvalValidateResource) validateForEach(ctx EvalContext, expr hcl.Expression) (diags tfdiags.Diagnostics) { - val, forEachDiags := evaluateForEachExpressionValue(expr, ctx) + val, forEachDiags := evaluateForEachExpressionValue(expr, ctx, true) // If the value isn't known then that's the best we can do for now, but // we'll check more thoroughly during the plan walk if !val.IsKnown() { diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index 6f41232de..2fbd77426 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -232,7 +232,7 @@ func (n *nodeValidateModule) Execute(ctx EvalContext, op walkOperation) (diags t diags = diags.Append(countDiags) case n.ModuleCall.ForEach != nil: - _, forEachDiags := evaluateForEachExpressionValue(n.ModuleCall.ForEach, ctx) + _, forEachDiags := evaluateForEachExpressionValue(n.ModuleCall.ForEach, ctx, true) diags = diags.Append(forEachDiags) }