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.
This commit is contained in:
Martin Atkins 2020-10-28 17:52:03 -07:00
parent 1e601dc7c2
commit e2c64bc255
5 changed files with 93 additions and 39 deletions

View File

@ -133,6 +133,8 @@ func (s *Scope) EvalExpr(expr hcl.Expression, wantType cty.Type) (cty.Value, tfd
Summary: "Incorrect value type", Summary: "Incorrect value type",
Detail: fmt.Sprintf("Invalid expression value: %s.", tfdiags.FormatError(convErr)), Detail: fmt.Sprintf("Invalid expression value: %s.", tfdiags.FormatError(convErr)),
Subject: expr.Range().Ptr(), Subject: expr.Range().Ptr(),
Expression: expr,
EvalContext: ctx,
}) })
} }
} }

View File

@ -4,6 +4,7 @@ import (
"fmt" "fmt"
"github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2"
"github.com/hashicorp/terraform/lang"
"github.com/hashicorp/terraform/tfdiags" "github.com/hashicorp/terraform/tfdiags"
"github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty"
) )
@ -17,16 +18,9 @@ import (
// returning an error if the count value is not known, and converting the // 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. // 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) { func evaluateForEachExpression(expr hcl.Expression, ctx EvalContext) (forEach map[string]cty.Value, diags tfdiags.Diagnostics) {
forEachVal, diags := evaluateForEachExpressionValue(expr, ctx) forEachVal, diags := evaluateForEachExpressionValue(expr, ctx, false)
if !forEachVal.IsKnown() { // forEachVal might be unknown, but if it is then there should already
// Attach a diag as we do with count, with the same downsides // be an error about it in diags, which we'll return below.
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(),
})
}
if forEachVal.IsNull() || !forEachVal.IsKnown() || forEachVal.LengthInt() == 0 { if forEachVal.IsNull() || !forEachVal.IsKnown() || forEachVal.LengthInt() == 0 {
// we check length, because an empty set return a nil map // 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 // evaluateForEachExpressionValue is like evaluateForEachExpression
// except that it returns a cty.Value map or set which can be unknown. // 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 var diags tfdiags.Diagnostics
nullMap := cty.NullVal(cty.Map(cty.DynamicPseudoType)) nullMap := cty.NullVal(cty.Map(cty.DynamicPseudoType))
@ -46,7 +40,23 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V
return nullMap, diags 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) diags = diags.Append(forEachDiags)
if forEachVal.ContainsMarked() { if forEachVal.ContainsMarked() {
diags = diags.Append(&hcl.Diagnostic{ diags = diags.Append(&hcl.Diagnostic{
@ -54,6 +64,8 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V
Summary: "Invalid for_each argument", 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.", 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(), Subject: expr.Range().Ptr(),
Expression: expr,
EvalContext: hclCtx,
}) })
} }
if diags.HasErrors() { if diags.HasErrors() {
@ -68,9 +80,21 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V
Summary: "Invalid for_each argument", 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.`, 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(), Subject: expr.Range().Ptr(),
Expression: expr,
EvalContext: hclCtx,
}) })
return nullMap, diags return nullMap, diags
case !forEachVal.IsKnown(): 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 // ensure that we have a map, and not a DynamicValue
return cty.UnknownVal(cty.Map(cty.DynamicPseudoType)), diags return cty.UnknownVal(cty.Map(cty.DynamicPseudoType)), diags
@ -80,6 +104,8 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V
Summary: "Invalid for_each argument", 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()), 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(), Subject: expr.Range().Ptr(),
Expression: expr,
EvalContext: hclCtx,
}) })
return nullMap, diags return nullMap, diags
@ -94,6 +120,16 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V
// since we can't use a set values that are unknown, we treat the // since we can't use a set values that are unknown, we treat the
// entire set as unknown // entire set as unknown
if !forEachVal.IsWhollyKnown() { 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 return cty.UnknownVal(ty), diags
} }
@ -103,6 +139,8 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V
Summary: "Invalid for_each set argument", 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()), 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(), Subject: expr.Range().Ptr(),
Expression: expr,
EvalContext: hclCtx,
}) })
return cty.NullVal(ty), diags return cty.NullVal(ty), diags
} }
@ -118,6 +156,8 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V
Summary: "Invalid for_each set argument", 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.`), Detail: fmt.Sprintf(`The given "for_each" argument value is unsuitable: "for_each" sets must not contain null values.`),
Subject: expr.Range().Ptr(), Subject: expr.Range().Ptr(),
Expression: expr,
EvalContext: hclCtx,
}) })
return cty.NullVal(ty), diags return cty.NullVal(ty), diags
} }
@ -126,3 +166,5 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.V
return forEachVal, nil 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.`

View File

@ -150,6 +150,16 @@ func TestEvaluateForEachExpression_errors(t *testing.T) {
if got, want := diags[0].Description().Detail, test.DetailSubstring; !strings.Contains(got, want) { if got, want := diags[0].Description().Detail, test.DetailSubstring; !strings.Contains(got, want) {
t.Errorf("wrong diagnostic detail %#v; want %#v", 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) { t.Run(name, func(t *testing.T) {
ctx := &MockEvalContext{} ctx := &MockEvalContext{}
ctx.installSimpleEval() ctx.installSimpleEval()
forEachVal, diags := evaluateForEachExpressionValue(expr, ctx) forEachVal, diags := evaluateForEachExpressionValue(expr, ctx, true)
if len(diags) != 0 { if len(diags) != 0 {
t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) t.Errorf("unexpected diagnostics %s", spew.Sdump(diags))

View File

@ -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) { 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 // 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 // we'll check more thoroughly during the plan walk
if !val.IsKnown() { if !val.IsKnown() {

View File

@ -232,7 +232,7 @@ func (n *nodeValidateModule) Execute(ctx EvalContext, op walkOperation) (diags t
diags = diags.Append(countDiags) diags = diags.Append(countDiags)
case n.ModuleCall.ForEach != nil: case n.ModuleCall.ForEach != nil:
_, forEachDiags := evaluateForEachExpressionValue(n.ModuleCall.ForEach, ctx) _, forEachDiags := evaluateForEachExpressionValue(n.ModuleCall.ForEach, ctx, true)
diags = diags.Append(forEachDiags) diags = diags.Append(forEachDiags)
} }