diff --git a/.changes/v1.12/BUG FIXES-20250226-171815.yaml b/.changes/v1.12/BUG FIXES-20250226-171815.yaml new file mode 100644 index 0000000000..603db1d022 --- /dev/null +++ b/.changes/v1.12/BUG FIXES-20250226-171815.yaml @@ -0,0 +1,5 @@ +kind: BUG FIXES +body: 'Avoid reporting duplicate attribute-associated diagnostics, such as "Available Write-only Attribute Alternative"' +time: 2025-02-26T17:18:15.521208Z +custom: + Issue: "36579" diff --git a/internal/backend/local/backend_plan.go b/internal/backend/local/backend_plan.go index 61fb931737..3937a4948b 100644 --- a/internal/backend/local/backend_plan.go +++ b/internal/backend/local/backend_plan.go @@ -76,7 +76,7 @@ func (b *Local) opPlan( b.ContextOpts = new(terraform.ContextOpts) } - // Get our context + // Set up backend and get our context lr, configSnap, opState, ctxDiags := b.localRun(op) diags = diags.Append(ctxDiags) if ctxDiags.HasErrors() { @@ -120,7 +120,9 @@ func (b *Local) opPlan( // NOTE: We intentionally don't stop here on errors because we always want // to try to present a partial plan report and, if the user chose to, // generate a partial saved plan file for external analysis. - diags = diags.Append(planDiags) + // Plan() may produce some diagnostic warnings which were already + // produced when setting up context above, so we deduplicate them here. + diags = diags.AppendWithoutDuplicates(planDiags...) // Even if there are errors we need to handle anything that may be // contained within the plan, so only exit if there is no data at all. diff --git a/internal/backend/remote-state/s3/backend_test.go b/internal/backend/remote-state/s3/backend_test.go index 7ea4d3dea2..10e00c0e3e 100644 --- a/internal/backend/remote-state/s3/backend_test.go +++ b/internal/backend/remote-state/s3/backend_test.go @@ -1935,9 +1935,7 @@ func TestBackendConfig_Proxy(t *testing.T) { raw, diags := testBackendConfigDiags(t, New(), backend.TestWrapConfig(config)) b := raw.(*Backend) - if diff := cmp.Diff(diags, tc.expectedDiags, tfdiags.DiagnosticComparer); diff != "" { - t.Errorf("unexpected diagnostics difference: %s", diff) - } + tfdiags.AssertDiagnosticsMatch(t, diags, tc.expectedDiags) client := b.awsConfig.HTTPClient bClient, ok := client.(*awshttp.BuildableClient) diff --git a/internal/terraform/context_plan_ephemeral_test.go b/internal/terraform/context_plan_ephemeral_test.go index 92ebf0b3c6..045547670e 100644 --- a/internal/terraform/context_plan_ephemeral_test.go +++ b/internal/terraform/context_plan_ephemeral_test.go @@ -5,6 +5,7 @@ package terraform import ( "fmt" + "path/filepath" "testing" "github.com/google/go-cmp/cmp" @@ -63,6 +64,11 @@ module "child" { Severity: hcl.DiagError, Summary: "Ephemeral value not allowed", Detail: "This output value is not declared as returning an ephemeral value, so it cannot be set to a result derived from an ephemeral value.", + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "child", "main.tf"), + Start: hcl.Pos{Line: 3, Column: 13, Byte: 30}, + End: hcl.Pos{Line: 3, Column: 31, Byte: 48}, + }, }) }, }, @@ -109,6 +115,11 @@ resource "test_object" "test" { Severity: hcl.DiagError, Summary: "Invalid use of ephemeral value", Detail: `Ephemeral values are not valid for "test_string", because it is not a write-only attribute and must be persisted to state.`, + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 6, Column: 17, Byte: 88}, + End: hcl.Pos{Line: 6, Column: 52, Byte: 123}, + }, }) }, }, @@ -164,13 +175,17 @@ resource "test_object" "test" { Severity: hcl.DiagError, Summary: "Invalid for_each argument", Detail: `The given "for_each" value is derived from an ephemeral value, which means that Terraform cannot persist it between plan/apply rounds. Use only non-ephemeral values to specify a resource's instance keys.`, + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 4, Column: 14, Byte: 83}, + End: hcl.Pos{Line: 4, Column: 55, Byte: 124}, + }, }) }, }, "resource expansion - count": { module: map[string]string{ - "main.tf": ` ephemeral "ephem_resource" "data" {} resource "test_object" "test" { @@ -184,6 +199,11 @@ resource "test_object" "test" { Severity: hcl.DiagError, Summary: "Invalid count argument", Detail: `The given "count" is derived from an ephemeral value, which means that Terraform cannot persist it between plan/apply rounds. Use only non-ephemeral values to specify the number of resource instances.`, + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 4, Column: 11, Byte: 80}, + End: hcl.Pos{Line: 4, Column: 53, Byte: 122}, + }, }) }, }, @@ -210,6 +230,11 @@ module "child" { Severity: hcl.DiagError, Summary: "Invalid for_each argument", Detail: `The given "for_each" value is derived from an ephemeral value, which means that Terraform cannot persist it between plan/apply rounds. Use only non-ephemeral values to specify a resource's instance keys.`, + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 5, Column: 16, Byte: 71}, + End: hcl.Pos{Line: 5, Column: 57, Byte: 112}, + }, }) }, }, @@ -234,6 +259,11 @@ module "child" { Severity: hcl.DiagError, Summary: "Invalid count argument", Detail: `The given "count" is derived from an ephemeral value, which means that Terraform cannot persist it between plan/apply rounds. Use only non-ephemeral values to specify the number of resource instances.`, + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 4, Column: 13, Byte: 67}, + End: hcl.Pos{Line: 4, Column: 55, Byte: 109}, + }, }) }, }, @@ -261,6 +291,11 @@ resource "test_object" "test" { Severity: hcl.DiagError, Summary: "Invalid for_each argument", Detail: `The given "for_each" value is derived from an ephemeral value, which means that Terraform cannot persist it between plan/apply rounds. Use only non-ephemeral values to specify a resource's instance keys.`, + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 11, Column: 16, Byte: 207}, + End: hcl.Pos{Line: 11, Column: 57, Byte: 248}, + }, }, ) }, @@ -288,6 +323,11 @@ module "child" { Severity: hcl.DiagError, Summary: "Ephemeral value not allowed", Detail: "This output value is not declared as returning an ephemeral value, so it cannot be set to a result derived from an ephemeral value.", + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "child", "main.tf"), + Start: hcl.Pos{Line: 6, Column: 13, Byte: 132}, + End: hcl.Pos{Line: 6, Column: 64, Byte: 183}, + }, }) }, }, @@ -322,6 +362,11 @@ module "child" { Severity: hcl.DiagError, Summary: "Ephemeral value not allowed", Detail: "This output value is not declared as returning an ephemeral value, so it cannot be set to a result derived from an ephemeral value.", + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "child", "main.tf"), + Start: hcl.Pos{Line: 14, Column: 13, Byte: 245}, + End: hcl.Pos{Line: 14, Column: 78, Byte: 310}, + }, }) }, }, @@ -378,17 +423,32 @@ check "check_using_ephemeral_value" { Severity: hcl.DiagWarning, Summary: "Check block assertion failed", Detail: "Fine to persist", + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 6, Column: 17, Byte: 104}, + End: hcl.Pos{Line: 6, Column: 60, Byte: 147}, + }, }) diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagWarning, Summary: "Check block assertion failed", Detail: "This check failed, but has an invalid error message as described in the other accompanying messages.", + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 10, Column: 17, Byte: 217}, + End: hcl.Pos{Line: 10, Column: 60, Byte: 260}, + }, }) diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagWarning, Summary: "Error message refers to ephemeral values", Detail: "The error expression used to explain this condition refers to ephemeral values, so Terraform will not display the resulting message." + "\n\nYou can correct this by removing references to ephemeral values, or by using the ephemeralasnull() function on the references to not reveal ephemeral data.", + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 11, Column: 21, Byte: 281}, + End: hcl.Pos{Line: 11, Column: 83, Byte: 343}, + }, }) return diags }, @@ -451,14 +511,29 @@ module "child" { Severity: hcl.DiagError, Summary: "Ephemeral value not allowed", Detail: "This output value is not declared as returning an ephemeral value, so it cannot be set to a result derived from an ephemeral value.", + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "child", "main.tf"), + Start: hcl.Pos{Line: 15, Column: 13, Byte: 376}, + End: hcl.Pos{Line: 15, Column: 33, Byte: 396}, + }, }, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Ephemeral value not allowed", Detail: "This output value is not declared as returning an ephemeral value, so it cannot be set to a result derived from an ephemeral value.", + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "child", "main.tf"), + Start: hcl.Pos{Line: 18, Column: 13, Byte: 435}, + End: hcl.Pos{Line: 18, Column: 31, Byte: 453}, + }, }, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Ephemeral value not allowed", Detail: "This output value is not declared as returning an ephemeral value, so it cannot be set to a result derived from an ephemeral value.", + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "child", "main.tf"), + Start: hcl.Pos{Line: 21, Column: 13, Byte: 491}, + End: hcl.Pos{Line: 21, Column: 30, Byte: 508}, + }, }) }, }, @@ -483,6 +558,11 @@ ephemeral "ephem_resource" "data" { Severity: hcl.DiagError, Summary: "Resource precondition failed", Detail: "value should not be 2", + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 8, Column: 19, Byte: 116}, + End: hcl.Pos{Line: 8, Column: 40, Byte: 137}, + }, }) }, }, @@ -507,6 +587,11 @@ ephemeral "ephem_resource" "data" { Severity: hcl.DiagError, Summary: "Resource postcondition failed", Detail: `value should be "pass"`, + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 8, Column: 19, Byte: 117}, + End: hcl.Pos{Line: 8, Column: 39, Byte: 137}, + }, }) }, }, @@ -541,12 +626,22 @@ output "out" { Detail: fmt.Sprintf(`The error message included a sensitive value, so it will not be displayed. This was checked by the validation rule at %s.`, m.Module.Variables["ephem"].Validations[0].DeclRange.String()), + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 2, Column: 1, Byte: 1}, + End: hcl.Pos{Line: 2, Column: 17, Byte: 17}, + }, }).Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Error message refers to ephemeral values", Detail: `The error expression used to explain this condition refers to ephemeral values. Terraform will not display the resulting message. You can correct this by removing references to ephemeral values, or by carefully using the ephemeralasnull() function if the expression will not reveal the ephemeral data.`, + Subject: &hcl.Range{ + Filename: filepath.Join(m.Module.SourceDir, "main.tf"), + Start: hcl.Pos{Line: 8, Column: 21, Byte: 142}, + End: hcl.Pos{Line: 8, Column: 76, Byte: 197}, + }, }) }, }, diff --git a/internal/terraform/node_resource_plan_partialexp.go b/internal/terraform/node_resource_plan_partialexp.go index 719dfb98ac..8a85908826 100644 --- a/internal/terraform/node_resource_plan_partialexp.go +++ b/internal/terraform/node_resource_plan_partialexp.go @@ -201,6 +201,7 @@ func (n *nodePlannablePartialExpandedResource) managedResourceExecute(ctx EvalCo } unmarkedConfigVal, _ := configVal.UnmarkDeep() + log.Printf("[TRACE] Validating partially expanded config for %q", n.addr) validateResp := provider.ValidateResourceConfig( providers.ValidateResourceConfigRequest{ TypeName: n.addr.Resource().Type, diff --git a/internal/terraform/node_resource_validate.go b/internal/terraform/node_resource_validate.go index 9cf671f454..728ef89250 100644 --- a/internal/terraform/node_resource_validate.go +++ b/internal/terraform/node_resource_validate.go @@ -5,6 +5,7 @@ package terraform import ( "fmt" + "log" "strings" "github.com/hashicorp/hcl/v2" @@ -390,6 +391,7 @@ func (n *NodeValidatableResource) validateResource(ctx EvalContext) tfdiags.Diag // Use unmarked value for validate request unmarkedConfigVal, _ := configVal.UnmarkDeep() + log.Printf("[TRACE] Validating config for %q", n.Addr) req := providers.ValidateResourceConfigRequest{ TypeName: n.Config.Type, Config: unmarkedConfigVal, diff --git a/internal/terraform/terraform_test.go b/internal/terraform/terraform_test.go index 92bb8c6cd5..9ca5ef48e5 100644 --- a/internal/terraform/terraform_test.go +++ b/internal/terraform/terraform_test.go @@ -92,7 +92,10 @@ func testModuleWithSnapshot(t *testing.T, name string) (*configs.Config, *config func testModuleInline(t testing.TB, sources map[string]string) *configs.Config { t.Helper() - cfgPath := t.TempDir() + cfgPath, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } for path, configStr := range sources { dir := filepath.Dir(path) diff --git a/internal/tfdiags/compare.go b/internal/tfdiags/compare.go index dfb6f54144..8a7cc297d0 100644 --- a/internal/tfdiags/compare.go +++ b/internal/tfdiags/compare.go @@ -7,10 +7,8 @@ import "github.com/google/go-cmp/cmp" // DiagnosticComparer returns a cmp.Option that can be used with // the package github.com/google/go-cmp/cmp. // -// The comparer checks these match between the diagnostics: -// 1) Severity -// 2) Description -// 3) Attribute cty.Path, if present +// The comparer relies on the underlying Diagnostic implementing +// [ComparableDiagnostic]. // // Example usage: // @@ -20,18 +18,15 @@ var DiagnosticComparer cmp.Option = cmp.Comparer(diagnosticComparerSimple) // diagnosticComparerSimple returns false when a difference is identified between // the two Diagnostic arguments. func diagnosticComparerSimple(l, r Diagnostic) bool { - if l.Severity() != r.Severity() { - return false - } - if l.Description() != r.Description() { + ld, ok := l.(ComparableDiagnostic) + if !ok { return false } - // Do the diagnostics originate from the same attribute name, if any? - lp := GetAttribute(l) - rp := GetAttribute(r) - if len(lp) != len(rp) { + rd, ok := r.(ComparableDiagnostic) + if !ok { return false } - return lp.Equals(rp) + + return ld.Equals(rd) } diff --git a/internal/tfdiags/compare_test.go b/internal/tfdiags/compare_test.go index f050387ebf..69a08efc03 100644 --- a/internal/tfdiags/compare_test.go +++ b/internal/tfdiags/compare_test.go @@ -81,27 +81,6 @@ func TestDiagnosticComparer(t *testing.T) { }(), expectDiff: true, }, - // Scenarios where diagnostics will be considered equavalent, even if they aren't fully the same - "reports that diagnostics match even if sources (Subject) are different; ignored in simple comparison": { - diag1: hclDiagnostic{&baseError}, - diag2: func() Diagnostic { - d := baseError - d.Subject = &hcl.Range{ - Filename: "foobar.tf", - Start: hcl.Pos{ - Line: 0, - Column: 0, - Byte: 0, - }, - End: hcl.Pos{ - Line: 1, - Column: 1, - Byte: 1, - }, - } - return hclDiagnostic{&d} - }(), - }, "reports that diagnostics match even if sources (Context) are different; ignored in simple comparison": { diag1: hclDiagnostic{&baseError}, diag2: func() Diagnostic { diff --git a/internal/tfdiags/contextual.go b/internal/tfdiags/contextual.go index 3d0d86d0fb..7f2bd5e5df 100644 --- a/internal/tfdiags/contextual.go +++ b/internal/tfdiags/contextual.go @@ -205,6 +205,31 @@ func (d *attributeDiagnostic) ElaborateFromConfigBody(body hcl.Body, addr string return &ret } +func (d *attributeDiagnostic) Equals(otherDiag ComparableDiagnostic) bool { + od, ok := otherDiag.(*attributeDiagnostic) + if !ok { + return false + } + if d.severity != od.severity { + return false + } + if d.summary != od.summary { + return false + } + if d.detail != od.detail { + return false + } + if !d.attrPath.Equals(od.attrPath) { + return false + } + + // address can differ between and after expansion + // even though it represents the same attribute + // so we avoid comparing it here + + return sourceRangeEquals(d.subject, od.subject) +} + func traversePathSteps(traverse []cty.PathStep, body hcl.Body) hcl.Body { for i := 0; i < len(traverse); i++ { step := traverse[i] @@ -386,3 +411,41 @@ func (d *wholeBodyDiagnostic) Source() Source { Subject: d.subject, } } + +func (d *wholeBodyDiagnostic) Equals(otherDiag ComparableDiagnostic) bool { + od, ok := otherDiag.(*wholeBodyDiagnostic) + if !ok { + return false + } + if d.severity != od.severity { + return false + } + if d.summary != od.summary { + return false + } + if d.detail != od.detail { + return false + } + + // address can differ between and after expansion + // even though it represents the same attribute + // so we avoid comparing it here + + return sourceRangeEquals(d.subject, od.subject) +} + +func sourceRangeEquals(l, r *SourceRange) bool { + if l == nil || r == nil { + return l == r + } + if l.Filename != r.Filename { + return false + } + if l.Start.Byte != r.Start.Byte { + return false + } + if l.End.Byte != r.End.Byte { + return false + } + return true +} diff --git a/internal/tfdiags/diagnostic.go b/internal/tfdiags/diagnostic.go index 7977c7ca86..fbead195e1 100644 --- a/internal/tfdiags/diagnostic.go +++ b/internal/tfdiags/diagnostic.go @@ -27,6 +27,10 @@ type Diagnostic interface { ExtraInfo() interface{} } +type ComparableDiagnostic interface { + Equals(otherDiag ComparableDiagnostic) bool +} + type Severity rune //go:generate go tool golang.org/x/tools/cmd/stringer -type=Severity diff --git a/internal/tfdiags/diagnostics.go b/internal/tfdiags/diagnostics.go index 1fd33904a2..fcb2e47cd0 100644 --- a/internal/tfdiags/diagnostics.go +++ b/internal/tfdiags/diagnostics.go @@ -83,6 +83,46 @@ func (diags Diagnostics) Append(new ...interface{}) Diagnostics { return diags } +// ContainsDiagnostic returns true of a given diagnostic is contained +// within the Diagnostics slice. +// Comparisons are done via [ComparableDiagnostic]. +func (diags Diagnostics) ContainsDiagnostic(diag ComparableDiagnostic) bool { + for _, d := range diags { + if cd, ok := d.(ComparableDiagnostic); ok && diag.Equals(cd) { + return true + } + } + return false +} + +// AppendWithoutDuplicates appends a Diagnostic unless one is already contained +// according to [ContainsDiagnostic], i.e. based on [ComparableDiagnostic]. +func (diags Diagnostics) AppendWithoutDuplicates(newDiags ...Diagnostic) Diagnostics { + for _, newItem := range newDiags { + if newItem == nil { + continue + } + + cd, ok := newItem.(ComparableDiagnostic) + if !ok { + // append what we cannot compare + diags = diags.Append(newItem) + } + + if diags.ContainsDiagnostic(cd) { + continue + } + + diags = diags.Append(newItem) + } + + if len(diags) == 0 { + return nil + } + + return diags +} + func diagnosticsForError(err error) []Diagnostic { if err == nil { return nil diff --git a/internal/tfdiags/hcl.go b/internal/tfdiags/hcl.go index 7f0a84d9ea..edf93342e9 100644 --- a/internal/tfdiags/hcl.go +++ b/internal/tfdiags/hcl.go @@ -57,6 +57,43 @@ func (d hclDiagnostic) ExtraInfo() interface{} { return d.diag.Extra } +func (d hclDiagnostic) Equals(otherDiag ComparableDiagnostic) bool { + od, ok := otherDiag.(hclDiagnostic) + if !ok { + return false + } + if d.diag.Severity != od.diag.Severity { + return false + } + if d.diag.Summary != od.diag.Summary { + return false + } + if d.diag.Detail != od.diag.Detail { + return false + } + if !hclRangeEquals(d.diag.Subject, od.diag.Subject) { + return false + } + + return true +} + +func hclRangeEquals(l, r *hcl.Range) bool { + if l == nil || r == nil { + return l == r + } + if l.Filename != r.Filename { + return false + } + if l.Start.Byte != r.Start.Byte { + return false + } + if l.End.Byte != r.End.Byte { + return false + } + return true +} + // SourceRangeFromHCL constructs a SourceRange from the corresponding range // type within the HCL package. func SourceRangeFromHCL(hclRange hcl.Range) SourceRange { diff --git a/internal/tfdiags/rpc_friendly.go b/internal/tfdiags/rpc_friendly.go index 9a160a2c74..223a21aa60 100644 --- a/internal/tfdiags/rpc_friendly.go +++ b/internal/tfdiags/rpc_friendly.go @@ -51,6 +51,27 @@ func (d *rpcFriendlyDiag) Source() Source { } } +func (d *rpcFriendlyDiag) Equals(otherDiag ComparableDiagnostic) bool { + od, ok := otherDiag.(*rpcFriendlyDiag) + if !ok { + return false + } + if d.Severity_ != od.Severity_ { + return false + } + if d.Summary_ != od.Summary_ { + return false + } + if d.Detail_ != od.Detail_ { + return false + } + if !sourceRangeEquals(d.Subject_, od.Subject_) { + return false + } + + return true +} + func (d rpcFriendlyDiag) FromExpr() *FromExpr { // RPC-friendly diagnostics cannot preserve expression information because // expressions themselves are not RPC-friendly.