From 90ea7b0bc51b4beb4018449473b8905c67969883 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 22 Jun 2022 11:26:56 -0700 Subject: [PATCH] tfdiags: Treat unknown-related or sensitive-related messages differently By observing the sorts of questions people ask in the community, and the ways they ask them, we've inferred that various different people have been confused by Terraform reporting that a value won't be known until apply or that a value is sensitive as part of an error message when that message doesn't actually relate to the known-ness and sensitivity of any value. Quite reasonably, someone who sees Terraform discussing an unfamiliar concept like unknown values can assume that it must be somehow relevant to the problem being discussed, and so in that sense Terraform's current error messages are giving "too much information": information that isn't actually helpful in understanding the problem being described, and in the worst case is a distraction from understanding the problem being described. With that in mind then, here we introduce an explicit annotation on diagnostic objects that are directly talking about unknown values or sensitive values, and then the diagnostic renderer will react to that to avoid using the terminology "known only after apply" or "sensitive" in the generated diagnostic annotations unless we're rendering a message that is explicitly related to one of those topics. This ends up being a bit of a cross-cutting concern because the code that generates these diagnostics and the code that renders them are in separate packages and are not directly aware of each other. With that in mind, the logic for actually deciding for a particular diagnostic whether it's flagged in one of these special ways lives inside the tfdiags package as an intermediation point, which both the diagnostic generator (in the core package) and the diagnostic renderer can both depend on. --- internal/command/format/diagnostic_test.go | 123 ++++++++++++++++ internal/command/views/json/diagnostic.go | 31 +++- .../command/views/json/diagnostic_test.go | 136 ++++++++++++++++++ ...e-when-not-caused-by-sensitive-values.json | 26 ++++ ...ype-when-not-caused-by-unknown-values.json | 26 ++++ internal/terraform/context_plan_test.go | 9 ++ internal/terraform/diagnostics.go | 42 ++++++ internal/terraform/eval_count.go | 8 +- internal/terraform/eval_for_each.go | 3 + internal/terraform/eval_for_each_test.go | 24 +++- internal/tfdiags/diagnostic_extra.go | 68 +++++++++ 11 files changed, 489 insertions(+), 7 deletions(-) create mode 100644 internal/command/views/json/testdata/diagnostic/error-with-source-code-subject-and-expression-referring-to-sensitive-value-when-not-caused-by-sensitive-values.json create mode 100644 internal/command/views/json/testdata/diagnostic/error-with-source-code-subject-and-unknown-expression-of-unknown-type-when-not-caused-by-unknown-values.json create mode 100644 internal/terraform/diagnostics.go diff --git a/internal/command/format/diagnostic_test.go b/internal/command/format/diagnostic_test.go index 31efacdeb757..95f2ed6aa1ba 100644 --- a/internal/command/format/diagnostic_test.go +++ b/internal/command/format/diagnostic_test.go @@ -130,6 +130,7 @@ func TestDiagnostic(t *testing.T) { }), }, }, + Extra: diagnosticCausedBySensitive(true), }, `[red]╷[reset] [red]│[reset] [bold][red]Error: [reset][bold]Bad bad bad[reset] @@ -164,6 +165,7 @@ func TestDiagnostic(t *testing.T) { }), }, }, + Extra: diagnosticCausedByUnknown(true), }, `[red]╷[reset] [red]│[reset] [bold][red]Error: [reset][bold]Bad bad bad[reset] @@ -198,6 +200,7 @@ func TestDiagnostic(t *testing.T) { }), }, }, + Extra: diagnosticCausedByUnknown(true), }, `[red]╷[reset] [red]│[reset] [bold][red]Error: [reset][bold]Bad bad bad[reset] @@ -391,6 +394,7 @@ Whatever shall we do? }), }, }, + Extra: diagnosticCausedBySensitive(true), }, ` Error: Bad bad bad @@ -400,6 +404,37 @@ Error: Bad bad bad ├──────────────── │ boop.beep has a sensitive value +Whatever shall we do? +`, + }, + "error with source code subject and expression referring to sensitive value when not related to sensitivity": { + &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Bad bad bad", + Detail: "Whatever shall we do?", + Subject: &hcl.Range{ + Filename: "test.tf", + Start: hcl.Pos{Line: 1, Column: 6, Byte: 5}, + End: hcl.Pos{Line: 1, Column: 12, Byte: 11}, + }, + Expression: hcltest.MockExprTraversal(hcl.Traversal{ + hcl.TraverseRoot{Name: "boop"}, + hcl.TraverseAttr{Name: "beep"}, + }), + EvalContext: &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "boop": cty.ObjectVal(map[string]cty.Value{ + "beep": cty.StringVal("blah").Mark(marks.Sensitive), + }), + }, + }, + }, + ` +Error: Bad bad bad + + on test.tf line 1: + 1: test source code + Whatever shall we do? `, }, @@ -424,6 +459,7 @@ Whatever shall we do? }), }, }, + Extra: diagnosticCausedByUnknown(true), }, ` Error: Bad bad bad @@ -433,6 +469,39 @@ Error: Bad bad bad ├──────────────── │ boop.beep is a string, known only after apply +Whatever shall we do? +`, + }, + "error with source code subject and unknown string expression when problem isn't unknown-related": { + &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Bad bad bad", + Detail: "Whatever shall we do?", + Subject: &hcl.Range{ + Filename: "test.tf", + Start: hcl.Pos{Line: 1, Column: 6, Byte: 5}, + End: hcl.Pos{Line: 1, Column: 12, Byte: 11}, + }, + Expression: hcltest.MockExprTraversal(hcl.Traversal{ + hcl.TraverseRoot{Name: "boop"}, + hcl.TraverseAttr{Name: "beep"}, + }), + EvalContext: &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "boop": cty.ObjectVal(map[string]cty.Value{ + "beep": cty.UnknownVal(cty.String), + }), + }, + }, + }, + ` +Error: Bad bad bad + + on test.tf line 1: + 1: test source code + ├──────────────── + │ boop.beep is a string + Whatever shall we do? `, }, @@ -457,6 +526,7 @@ Whatever shall we do? }), }, }, + Extra: diagnosticCausedByUnknown(true), }, ` Error: Bad bad bad @@ -466,6 +536,37 @@ Error: Bad bad bad ├──────────────── │ boop.beep will be known only after apply +Whatever shall we do? +`, + }, + "error with source code subject and unknown expression of unknown type when problem isn't unknown-related": { + &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Bad bad bad", + Detail: "Whatever shall we do?", + Subject: &hcl.Range{ + Filename: "test.tf", + Start: hcl.Pos{Line: 1, Column: 6, Byte: 5}, + End: hcl.Pos{Line: 1, Column: 12, Byte: 11}, + }, + Expression: hcltest.MockExprTraversal(hcl.Traversal{ + hcl.TraverseRoot{Name: "boop"}, + hcl.TraverseAttr{Name: "beep"}, + }), + EvalContext: &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "boop": cty.ObjectVal(map[string]cty.Value{ + "beep": cty.UnknownVal(cty.DynamicPseudoType), + }), + }, + }, + }, + ` +Error: Bad bad bad + + on test.tf line 1: + 1: test source code + Whatever shall we do? `, }, @@ -820,3 +921,25 @@ func (e fakeDiagFunctionCallExtra) CalledFunctionName() string { func (e fakeDiagFunctionCallExtra) FunctionCallError() error { return nil } + +// diagnosticCausedByUnknown is a testing helper for exercising our logic +// for selectively showing unknown values alongside our source snippets for +// diagnostics that are explicitly marked as being caused by unknown values. +type diagnosticCausedByUnknown bool + +var _ tfdiags.DiagnosticExtraBecauseUnknown = diagnosticCausedByUnknown(true) + +func (e diagnosticCausedByUnknown) DiagnosticCausedByUnknown() bool { + return bool(e) +} + +// diagnosticCausedBySensitive is a testing helper for exercising our logic +// for selectively showing sensitive values alongside our source snippets for +// diagnostics that are explicitly marked as being caused by sensitive values. +type diagnosticCausedBySensitive bool + +var _ tfdiags.DiagnosticExtraBecauseSensitive = diagnosticCausedBySensitive(true) + +func (e diagnosticCausedBySensitive) DiagnosticCausedBySensitive() bool { + return bool(e) +} diff --git a/internal/command/views/json/diagnostic.go b/internal/command/views/json/diagnostic.go index bfd5ddd97ee0..1175792c72a2 100644 --- a/internal/command/views/json/diagnostic.go +++ b/internal/command/views/json/diagnostic.go @@ -271,6 +271,8 @@ func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string][]byte) *Diagnost vars := expr.Variables() values := make([]DiagnosticExpressionValue, 0, len(vars)) seen := make(map[string]struct{}, len(vars)) + includeUnknown := tfdiags.DiagnosticCausedByUnknown(diag) + includeSensitive := tfdiags.DiagnosticCausedBySensitive(diag) Traversals: for _, traversal := range vars { for len(traversal) > 1 { @@ -292,14 +294,35 @@ func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string][]byte) *Diagnost } switch { case val.HasMark(marks.Sensitive): - // We won't say anything at all about sensitive values, - // because we might give away something that was - // sensitive about them. + // We only mention a sensitive value if the diagnostic + // we're rendering is explicitly marked as being + // caused by sensitive values, because otherwise + // readers tend to be misled into thinking the error + // is caused by the sensitive value even when it isn't. + if !includeSensitive { + continue Traversals + } + // Even when we do mention one, we keep it vague + // in order to minimize the chance of giving away + // whatever was sensitive about it. value.Statement = "has a sensitive value" case !val.IsKnown(): + // We'll avoid saying anything about unknown or + // "known after apply" unless the diagnostic is + // explicitly marked as being caused by unknown + // values, because otherwise readers tend to be + // misled into thinking the error is caused by the + // unknown value even when it isn't. if ty := val.Type(); ty != cty.DynamicPseudoType { - value.Statement = fmt.Sprintf("is a %s, known only after apply", ty.FriendlyName()) + if includeUnknown { + value.Statement = fmt.Sprintf("is a %s, known only after apply", ty.FriendlyName()) + } else { + value.Statement = fmt.Sprintf("is a %s", ty.FriendlyName()) + } } else { + if !includeUnknown { + continue Traversals + } value.Statement = "will be known only after apply" } default: diff --git a/internal/command/views/json/diagnostic_test.go b/internal/command/views/json/diagnostic_test.go index 640b82bc60cb..422dade9b3cb 100644 --- a/internal/command/views/json/diagnostic_test.go +++ b/internal/command/views/json/diagnostic_test.go @@ -412,6 +412,7 @@ func TestNewDiagnostic(t *testing.T) { }), }, }, + Extra: diagnosticCausedBySensitive(true), }, &Diagnostic{ Severity: "error", @@ -445,6 +446,61 @@ func TestNewDiagnostic(t *testing.T) { }, }, }, + "error with source code subject and expression referring to sensitive value when not caused by sensitive values": { + &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Wrong noises", + Detail: "Biological sounds are not allowed", + Subject: &hcl.Range{ + Filename: "test.tf", + Start: hcl.Pos{Line: 2, Column: 9, Byte: 42}, + End: hcl.Pos{Line: 2, Column: 26, Byte: 59}, + }, + Expression: hcltest.MockExprTraversal(hcl.Traversal{ + hcl.TraverseRoot{Name: "var"}, + hcl.TraverseAttr{Name: "boop"}, + hcl.TraverseIndex{Key: cty.StringVal("hello!")}, + }), + EvalContext: &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "var": cty.ObjectVal(map[string]cty.Value{ + "boop": cty.MapVal(map[string]cty.Value{ + "hello!": cty.StringVal("bleurgh").Mark(marks.Sensitive), + }), + }), + }, + }, + }, + &Diagnostic{ + Severity: "error", + Summary: "Wrong noises", + Detail: "Biological sounds are not allowed", + Range: &DiagnosticRange{ + Filename: "test.tf", + Start: Pos{ + Line: 2, + Column: 9, + Byte: 42, + }, + End: Pos{ + Line: 2, + Column: 26, + Byte: 59, + }, + }, + Snippet: &DiagnosticSnippet{ + Context: strPtr(`resource "test_resource" "test"`), + Code: (` foo = var.boop["hello!"]`), + StartLine: (2), + HighlightStartOffset: (8), + HighlightEndOffset: (25), + Values: []DiagnosticExpressionValue{ + // The sensitive value is filtered out because this is + // not a sensitive-value-related diagnostic message. + }, + }, + }, + }, "error with source code subject and expression referring to a collection containing a sensitive value": { &hcl.Diagnostic{ Severity: hcl.DiagError, @@ -525,6 +581,7 @@ func TestNewDiagnostic(t *testing.T) { }), }, }, + Extra: diagnosticCausedByUnknown(true), }, &Diagnostic{ Severity: "error", @@ -582,6 +639,7 @@ func TestNewDiagnostic(t *testing.T) { }), }, }, + Extra: diagnosticCausedByUnknown(true), }, &Diagnostic{ Severity: "error", @@ -615,6 +673,61 @@ func TestNewDiagnostic(t *testing.T) { }, }, }, + "error with source code subject and unknown expression of unknown type when not caused by unknown values": { + &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Wrong noises", + Detail: "Biological sounds are not allowed", + Subject: &hcl.Range{ + Filename: "test.tf", + Start: hcl.Pos{Line: 2, Column: 9, Byte: 42}, + End: hcl.Pos{Line: 2, Column: 26, Byte: 59}, + }, + Expression: hcltest.MockExprTraversal(hcl.Traversal{ + hcl.TraverseRoot{Name: "var"}, + hcl.TraverseAttr{Name: "boop"}, + hcl.TraverseIndex{Key: cty.StringVal("hello!")}, + }), + EvalContext: &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "var": cty.ObjectVal(map[string]cty.Value{ + "boop": cty.MapVal(map[string]cty.Value{ + "hello!": cty.UnknownVal(cty.DynamicPseudoType), + }), + }), + }, + }, + }, + &Diagnostic{ + Severity: "error", + Summary: "Wrong noises", + Detail: "Biological sounds are not allowed", + Range: &DiagnosticRange{ + Filename: "test.tf", + Start: Pos{ + Line: 2, + Column: 9, + Byte: 42, + }, + End: Pos{ + Line: 2, + Column: 26, + Byte: 59, + }, + }, + Snippet: &DiagnosticSnippet{ + Context: strPtr(`resource "test_resource" "test"`), + Code: (` foo = var.boop["hello!"]`), + StartLine: (2), + HighlightStartOffset: (8), + HighlightEndOffset: (25), + Values: []DiagnosticExpressionValue{ + // The unknown value is filtered out because this is + // not an unknown-value-related diagnostic message. + }, + }, + }, + }, "error with source code subject with multiple expression values": { &hcl.Diagnostic{ Severity: hcl.DiagError, @@ -666,6 +779,7 @@ func TestNewDiagnostic(t *testing.T) { }), }, }, + Extra: diagnosticCausedBySensitive(true), }, &Diagnostic{ Severity: "error", @@ -813,3 +927,25 @@ func TestNewDiagnostic(t *testing.T) { // are fields which are pointer-to-string to ensure that the rendered JSON // results in `null` for an empty value, rather than `""`. func strPtr(s string) *string { return &s } + +// diagnosticCausedByUnknown is a testing helper for exercising our logic +// for selectively showing unknown values alongside our source snippets for +// diagnostics that are explicitly marked as being caused by unknown values. +type diagnosticCausedByUnknown bool + +var _ tfdiags.DiagnosticExtraBecauseUnknown = diagnosticCausedByUnknown(true) + +func (e diagnosticCausedByUnknown) DiagnosticCausedByUnknown() bool { + return bool(e) +} + +// diagnosticCausedBySensitive is a testing helper for exercising our logic +// for selectively showing sensitive values alongside our source snippets for +// diagnostics that are explicitly marked as being caused by sensitive values. +type diagnosticCausedBySensitive bool + +var _ tfdiags.DiagnosticExtraBecauseSensitive = diagnosticCausedBySensitive(true) + +func (e diagnosticCausedBySensitive) DiagnosticCausedBySensitive() bool { + return bool(e) +} diff --git a/internal/command/views/json/testdata/diagnostic/error-with-source-code-subject-and-expression-referring-to-sensitive-value-when-not-caused-by-sensitive-values.json b/internal/command/views/json/testdata/diagnostic/error-with-source-code-subject-and-expression-referring-to-sensitive-value-when-not-caused-by-sensitive-values.json new file mode 100644 index 000000000000..8d3625dbe78d --- /dev/null +++ b/internal/command/views/json/testdata/diagnostic/error-with-source-code-subject-and-expression-referring-to-sensitive-value-when-not-caused-by-sensitive-values.json @@ -0,0 +1,26 @@ +{ + "severity": "error", + "summary": "Wrong noises", + "detail": "Biological sounds are not allowed", + "range": { + "filename": "test.tf", + "start": { + "line": 2, + "column": 9, + "byte": 42 + }, + "end": { + "line": 2, + "column": 26, + "byte": 59 + } + }, + "snippet": { + "context": "resource \"test_resource\" \"test\"", + "code": " foo = var.boop[\"hello!\"]", + "start_line": 2, + "highlight_start_offset": 8, + "highlight_end_offset": 25, + "values": [] + } +} diff --git a/internal/command/views/json/testdata/diagnostic/error-with-source-code-subject-and-unknown-expression-of-unknown-type-when-not-caused-by-unknown-values.json b/internal/command/views/json/testdata/diagnostic/error-with-source-code-subject-and-unknown-expression-of-unknown-type-when-not-caused-by-unknown-values.json new file mode 100644 index 000000000000..8d3625dbe78d --- /dev/null +++ b/internal/command/views/json/testdata/diagnostic/error-with-source-code-subject-and-unknown-expression-of-unknown-type-when-not-caused-by-unknown-values.json @@ -0,0 +1,26 @@ +{ + "severity": "error", + "summary": "Wrong noises", + "detail": "Biological sounds are not allowed", + "range": { + "filename": "test.tf", + "start": { + "line": 2, + "column": 9, + "byte": 42 + }, + "end": { + "line": 2, + "column": 26, + "byte": 59 + } + }, + "snippet": { + "context": "resource \"test_resource\" \"test\"", + "code": " foo = var.boop[\"hello!\"]", + "start_line": 2, + "highlight_start_offset": 8, + "highlight_end_offset": 25, + "values": [] + } +} diff --git a/internal/terraform/context_plan_test.go b/internal/terraform/context_plan_test.go index d05c2eb75c8d..2323dcc3b445 100644 --- a/internal/terraform/context_plan_test.go +++ b/internal/terraform/context_plan_test.go @@ -3093,6 +3093,15 @@ func TestContext2Plan_forEachUnknownValue(t *testing.T) { if !strings.Contains(gotErrStr, wantErrStr) { t.Fatalf("missing expected error\ngot: %s\n\nwant: error containing %q", gotErrStr, wantErrStr) } + + // We should have a diagnostic that is marked as being caused by unknown + // values. + for _, diag := range diags { + if tfdiags.DiagnosticCausedByUnknown(diag) { + return // don't fall through to the error below + } + } + t.Fatalf("no diagnostic is marked as being caused by unknown\n%s", diags.Err().Error()) } func TestContext2Plan_destroy(t *testing.T) { diff --git a/internal/terraform/diagnostics.go b/internal/terraform/diagnostics.go new file mode 100644 index 000000000000..26f22f06ce6c --- /dev/null +++ b/internal/terraform/diagnostics.go @@ -0,0 +1,42 @@ +package terraform + +import ( + "github.com/hashicorp/terraform/internal/tfdiags" +) + +// This file contains some package-local helpers for working with diagnostics. +// For the main diagnostics API, see the separate "tfdiags" package. + +// diagnosticCausedByUnknown is an implementation of +// tfdiags.DiagnosticExtraBecauseUnknown which we can use in the "Extra" field +// of a diagnostic to indicate that the problem was caused by unknown values +// being involved in an expression evaluation. +// +// When using this, set the Extra to diagnosticCausedByUnknown(true) and also +// populate the EvalContext and Expression fields of the diagnostic so that +// the diagnostic renderer can use all of that information together to assist +// the user in understanding what was unknown. +type diagnosticCausedByUnknown bool + +var _ tfdiags.DiagnosticExtraBecauseUnknown = diagnosticCausedByUnknown(true) + +func (e diagnosticCausedByUnknown) DiagnosticCausedByUnknown() bool { + return bool(e) +} + +// diagnosticCausedBySensitive is an implementation of +// tfdiags.DiagnosticExtraBecauseSensitive which we can use in the "Extra" field +// of a diagnostic to indicate that the problem was caused by sensitive values +// being involved in an expression evaluation. +// +// When using this, set the Extra to diagnosticCausedBySensitive(true) and also +// populate the EvalContext and Expression fields of the diagnostic so that +// the diagnostic renderer can use all of that information together to assist +// the user in understanding what was sensitive. +type diagnosticCausedBySensitive bool + +var _ tfdiags.DiagnosticExtraBecauseSensitive = diagnosticCausedBySensitive(true) + +func (e diagnosticCausedBySensitive) DiagnosticCausedBySensitive() bool { + return bool(e) +} diff --git a/internal/terraform/eval_count.go b/internal/terraform/eval_count.go index c74a051b301f..d4ab1a998f60 100644 --- a/internal/terraform/eval_count.go +++ b/internal/terraform/eval_count.go @@ -31,6 +31,12 @@ func evaluateCountExpression(expr hcl.Expression, ctx EvalContext) (int, tfdiags Summary: "Invalid count argument", Detail: `The "count" 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 count depends on.`, Subject: expr.Range().Ptr(), + + // TODO: Also populate Expression and EvalContext in here, but + // we can't easily do that right now because the hcl.EvalContext + // (which is not the same as the ctx we have in scope here) is + // hidden away inside evaluateCountExpressionValue. + Extra: diagnosticCausedByUnknown(true), }) } @@ -91,7 +97,7 @@ func evaluateCountExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.Val diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid count argument", - Detail: `The given "count" argument value is unsuitable: negative numbers are not supported.`, + Detail: `The given "count" argument value is unsuitable: must be greater than or equal to zero.`, Subject: expr.Range().Ptr(), }) return nullCount, diags diff --git a/internal/terraform/eval_for_each.go b/internal/terraform/eval_for_each.go index ba7a8c9bca6c..3c80ebff01d3 100644 --- a/internal/terraform/eval_for_each.go +++ b/internal/terraform/eval_for_each.go @@ -70,6 +70,7 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowU Subject: expr.Range().Ptr(), Expression: expr, EvalContext: hclCtx, + Extra: diagnosticCausedBySensitive(true), }) } @@ -109,6 +110,7 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowU Subject: expr.Range().Ptr(), Expression: expr, EvalContext: hclCtx, + Extra: diagnosticCausedByUnknown(true), }) } // ensure that we have a map, and not a DynamicValue @@ -144,6 +146,7 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowU Subject: expr.Range().Ptr(), Expression: expr, EvalContext: hclCtx, + Extra: diagnosticCausedByUnknown(true), }) } return cty.UnknownVal(ty), diags diff --git a/internal/terraform/eval_for_each_test.go b/internal/terraform/eval_for_each_test.go index be4b551cf039..05dba9cabc46 100644 --- a/internal/terraform/eval_for_each_test.go +++ b/internal/terraform/eval_for_each_test.go @@ -88,38 +88,45 @@ func TestEvaluateForEachExpression_valid(t *testing.T) { func TestEvaluateForEachExpression_errors(t *testing.T) { tests := map[string]struct { - Expr hcl.Expression - Summary, DetailSubstring string + Expr hcl.Expression + Summary, DetailSubstring string + CausedByUnknown, CausedBySensitive bool }{ "null set": { hcltest.MockExprLiteral(cty.NullVal(cty.Set(cty.String))), "Invalid for_each argument", `the given "for_each" argument value is null`, + false, false, }, "string": { hcltest.MockExprLiteral(cty.StringVal("i am definitely a set")), "Invalid for_each argument", "must be a map, or set of strings, and you have provided a value of type string", + false, false, }, "list": { hcltest.MockExprLiteral(cty.ListVal([]cty.Value{cty.StringVal("a"), cty.StringVal("a")})), "Invalid for_each argument", "must be a map, or set of strings, and you have provided a value of type list", + false, false, }, "tuple": { hcltest.MockExprLiteral(cty.TupleVal([]cty.Value{cty.StringVal("a"), cty.StringVal("b")})), "Invalid for_each argument", "must be a map, or set of strings, and you have provided a value of type tuple", + false, false, }, "unknown string set": { hcltest.MockExprLiteral(cty.UnknownVal(cty.Set(cty.String))), "Invalid for_each argument", "set includes values derived from resource attributes that cannot be determined until apply", + true, false, }, "unknown map": { hcltest.MockExprLiteral(cty.UnknownVal(cty.Map(cty.Bool))), "Invalid for_each argument", "map includes keys derived from resource attributes that cannot be determined until apply", + true, false, }, "marked map": { hcltest.MockExprLiteral(cty.MapVal(map[string]cty.Value{ @@ -128,31 +135,37 @@ func TestEvaluateForEachExpression_errors(t *testing.T) { }).Mark(marks.Sensitive)), "Invalid for_each argument", "Sensitive values, or values derived from sensitive values, cannot be used as for_each arguments. If used, the sensitive value could be exposed as a resource instance key.", + false, true, }, "set containing booleans": { hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.BoolVal(true)})), "Invalid for_each set argument", "supports maps and sets of strings, but you have provided a set containing type bool", + false, false, }, "set containing null": { hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.NullVal(cty.String)})), "Invalid for_each set argument", "must not contain null values", + false, false, }, "set containing unknown value": { hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.UnknownVal(cty.String)})), "Invalid for_each argument", "set includes values derived from resource attributes that cannot be determined until apply", + true, false, }, "set containing dynamic unknown value": { hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.UnknownVal(cty.DynamicPseudoType)})), "Invalid for_each argument", "set includes values derived from resource attributes that cannot be determined until apply", + true, false, }, "set containing marked values": { hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.StringVal("beep").Mark(marks.Sensitive), cty.StringVal("boop")})), "Invalid for_each argument", "Sensitive values, or values derived from sensitive values, cannot be used as for_each arguments. If used, the sensitive value could be exposed as a resource instance key.", + false, true, }, } @@ -184,6 +197,13 @@ func TestEvaluateForEachExpression_errors(t *testing.T) { } else { t.Errorf("diagnostic does not support FromExpr\ngot: %s", spew.Sdump(diags[0])) } + + if got, want := tfdiags.DiagnosticCausedByUnknown(diags[0]), test.CausedByUnknown; got != want { + t.Errorf("wrong result from tfdiags.DiagnosticCausedByUnknown\ngot: %#v\nwant: %#v", got, want) + } + if got, want := tfdiags.DiagnosticCausedBySensitive(diags[0]), test.CausedBySensitive; got != want { + t.Errorf("wrong result from tfdiags.DiagnosticCausedBySensitive\ngot: %#v\nwant: %#v", got, want) + } }) } } diff --git a/internal/tfdiags/diagnostic_extra.go b/internal/tfdiags/diagnostic_extra.go index dfa746ec03e0..97a14746c462 100644 --- a/internal/tfdiags/diagnostic_extra.go +++ b/internal/tfdiags/diagnostic_extra.go @@ -101,3 +101,71 @@ type DiagnosticExtraUnwrapper interface { // value returns a value that was also wrapping it. UnwrapDiagnosticExtra() interface{} } + +// DiagnosticExtraBecauseUnknown is an interface implemented by values in +// the Extra field of Diagnostic when the diagnostic is potentially caused by +// the presence of unknown values in an expression evaluation. +// +// Just implementing this interface is not sufficient signal, though. Callers +// must also call the DiagnosticCausedByUnknown method in order to confirm +// the result, or use the package-level function DiagnosticCausedByUnknown +// as a convenient wrapper. +type DiagnosticExtraBecauseUnknown interface { + // DiagnosticCausedByUnknown returns true if the associated diagnostic + // was caused by the presence of unknown values during an expression + // evaluation, or false otherwise. + // + // Callers might use this to tailor what contextual information they show + // alongside an error report in the UI, to avoid potential confusion + // caused by talking about the presence of unknown values if that was + // immaterial to the error. + DiagnosticCausedByUnknown() bool +} + +// DiagnosticCausedByUnknown returns true if the given diagnostic has an +// indication that it was caused by the presence of unknown values during +// an expression evaluation. +// +// This is a wrapper around checking if the diagnostic's extra info implements +// interface DiagnosticExtraBecauseUnknown and then calling its method if so. +func DiagnosticCausedByUnknown(diag Diagnostic) bool { + maybe := ExtraInfo[DiagnosticExtraBecauseUnknown](diag) + if maybe == nil { + return false + } + return maybe.DiagnosticCausedByUnknown() +} + +// DiagnosticExtraBecauseSensitive is an interface implemented by values in +// the Extra field of Diagnostic when the diagnostic is potentially caused by +// the presence of sensitive values in an expression evaluation. +// +// Just implementing this interface is not sufficient signal, though. Callers +// must also call the DiagnosticCausedBySensitive method in order to confirm +// the result, or use the package-level function DiagnosticCausedBySensitive +// as a convenient wrapper. +type DiagnosticExtraBecauseSensitive interface { + // DiagnosticCausedBySensitive returns true if the associated diagnostic + // was caused by the presence of sensitive values during an expression + // evaluation, or false otherwise. + // + // Callers might use this to tailor what contextual information they show + // alongside an error report in the UI, to avoid potential confusion + // caused by talking about the presence of sensitive values if that was + // immaterial to the error. + DiagnosticCausedBySensitive() bool +} + +// DiagnosticCausedBySensitive returns true if the given diagnostic has an +// indication that it was caused by the presence of sensitive values during +// an expression evaluation. +// +// This is a wrapper around checking if the diagnostic's extra info implements +// interface DiagnosticExtraBecauseSensitive and then calling its method if so. +func DiagnosticCausedBySensitive(diag Diagnostic) bool { + maybe := ExtraInfo[DiagnosticExtraBecauseSensitive](diag) + if maybe == nil { + return false + } + return maybe.DiagnosticCausedBySensitive() +}