From 2f78b5c86a2ba9234b8d000e3de7bc6981bcf75e Mon Sep 17 00:00:00 2001 From: Thomas Nelson Date: Fri, 12 Jul 2024 16:11:57 -0500 Subject: [PATCH 1/4] Add rule for finding duplicate keys in map expressions --- rules/terraform_map_duplicate_keys.go | 106 +++++++++++++++ rules/terraform_map_duplicate_keys_test.go | 151 +++++++++++++++++++++ 2 files changed, 257 insertions(+) create mode 100644 rules/terraform_map_duplicate_keys.go create mode 100644 rules/terraform_map_duplicate_keys_test.go diff --git a/rules/terraform_map_duplicate_keys.go b/rules/terraform_map_duplicate_keys.go new file mode 100644 index 0000000..cef5a30 --- /dev/null +++ b/rules/terraform_map_duplicate_keys.go @@ -0,0 +1,106 @@ +package rules + +import ( + "fmt" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/terraform-linters/tflint-plugin-sdk/tflint" + "github.com/terraform-linters/tflint-ruleset-terraform/project" + "github.com/zclconf/go-cty/cty" +) + +// This rule checks for map literals with duplicate keys +type TerraformMapDuplicateKeysRule struct { + tflint.DefaultRule +} + +func NewTerraformMapDuplicateKeysRule() *TerraformMapDuplicateKeysRule { + return &TerraformMapDuplicateKeysRule{} +} + +func (r *TerraformMapDuplicateKeysRule) Name() string { + return "terraform_map_duplicate_keys" +} + +func (r *TerraformMapDuplicateKeysRule) Enabled() bool { + return true +} + +func (r *TerraformMapDuplicateKeysRule) Severity() tflint.Severity { + return tflint.WARNING +} + +func (r *TerraformMapDuplicateKeysRule) Link() string { + return project.ReferenceLink(r.Name()) +} + +func (r *TerraformMapDuplicateKeysRule) Check(runner tflint.Runner) error { + path, err := runner.GetModulePath() + if err != nil { + return err + } + if !path.IsRoot() { + // This rule does not evaluate child modules + return nil + } + + diags := runner.WalkExpressions(tflint.ExprWalkFunc(func(e hcl.Expression) hcl.Diagnostics { + return r.checkObjectConsExpr(e, runner) + })) + if diags.HasErrors() { + return diags + } + + return nil +} + +func (r *TerraformMapDuplicateKeysRule) checkObjectConsExpr(e hcl.Expression, runner tflint.Runner) hcl.Diagnostics { + exprMap, ok := e.(*hclsyntax.ObjectConsExpr) + if !ok { + // Ignore everything that isn't an ObjectConsExpr + return nil + } + diags := hcl.Diagnostics{} + foundKeys := make(map[string]hcl.Range) + for _, item := range exprMap.Items { + expr := item.KeyExpr.(*hclsyntax.ObjectConsKeyExpr) + val := cty.Value{} + err := runner.EvaluateExpr(expr, &val, &tflint.EvaluateExprOption{}) + if err != nil { + fmt.Printf("Failed to evaluate an expression, continuing\n") + diags.Append( + &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "failed to evaluate expression", + Detail: err.Error(), + }, + ) + continue + } + if !val.IsKnown() || val.IsNull() { + // When trying to evaluate an expression + // with a variable without a default, + // runner.evaluateExpr() returns a null value. + // Ignore this case since there's nothing we can do. + fmt.Printf("Unknown key, continuing\n") + continue + } + + if previousRange, exists := foundKeys[val.AsString()]; exists { + msg := fmt.Sprintf("Duplicate key: '%s'\nThe previous definition was at %s", val.AsString(), previousRange) + if err := runner.EmitIssue(r, msg, expr.Range()); err != nil { + diags.Append( + &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "failed to call EmitIssue()", + Detail: err.Error(), + }, + ) + } + } else { + foundKeys[val.AsString()] = expr.Range() + } + } + return diags +} diff --git a/rules/terraform_map_duplicate_keys_test.go b/rules/terraform_map_duplicate_keys_test.go new file mode 100644 index 0000000..89f7673 --- /dev/null +++ b/rules/terraform_map_duplicate_keys_test.go @@ -0,0 +1,151 @@ +package rules + +import ( + "testing" + + "github.com/hashicorp/hcl/v2" + "github.com/terraform-linters/tflint-plugin-sdk/helper" +) + +func Test_TerraformMapDuplicateKeys(t *testing.T) { + cases := []struct { + Name string + Content string + Expected helper.Issues + Fixed string + }{ + { + Name: "No duplicates", + Content: ` +resource "null_resource" "test" { + test = { + a = 1 + b = 2 + c = 3 + } +}`, + Expected: helper.Issues{}, + }, + { + Name: "duplicate keys in map literal", + Content: ` +resource "null_resource" "test" { + triggers = { + a = "b" + a = "c" + } +}`, + Expected: helper.Issues{ + { + Rule: NewTerraformMapDuplicateKeysRule(), + Message: "Duplicate key: 'a'\nThe previous definition was at module.tf:4,9-10", + Range: hcl.Range{ + Filename: "module.tf", + Start: hcl.Pos{Line: 5, Column: 9}, + End: hcl.Pos{Line: 5, Column: 10}, + }, + }, + }, + }, + { + Name: "Using variables as keys", + Content: ` +variable "a" { + type = string + default = "b" +} + +resource "null_resource" "test" { + map = { + (var.a) = 5 + b = 8 + } +}`, + Expected: helper.Issues{ + { + Rule: NewTerraformMapDuplicateKeysRule(), + Message: "Duplicate key: 'b'\nThe previous definition was at module.tf:9,4-11", + Range: hcl.Range{ + Filename: "module.tf", + Start: hcl.Pos{Line: 10, Column: 4}, + End: hcl.Pos{Line: 10, Column: 5}, + }, + }, + }, + }, + { + Name: "Using a variable as a key without a default", + Content: ` +variable "unknown" { + type = string +} + +resource "null_resource" "test" { + map = { + x = 8 + (var.unknown) = 5 + } +}`, + Expected: helper.Issues{}, + }, + { + Name: "Multiple duplicates in same map", + Content: ` +resource "null_resource" "test" { + map = { + a = 7 + a = 8 + a = 9 + } +}`, + Expected: helper.Issues{ + { + Rule: NewTerraformMapDuplicateKeysRule(), + Message: "Duplicate key: 'a'\nThe previous definition was at module.tf:4,4-5", + Range: hcl.Range{ + Filename: "module.tf", + Start: hcl.Pos{Line: 5, Column: 4}, + End: hcl.Pos{Line: 5, Column: 5}, + }, + }, + { + Rule: NewTerraformMapDuplicateKeysRule(), + Message: "Duplicate key: 'a'\nThe previous definition was at module.tf:4,4-5", + Range: hcl.Range{ + Filename: "module.tf", + Start: hcl.Pos{Line: 6, Column: 4}, + End: hcl.Pos{Line: 6, Column: 5}, + }, + }, + }, + }, + { + Name: "Using same key in different maps is okay", + Content: ` + +resource "null_resource" "test" { + map = { + x = 1 + } + map2 = { + x = 2 + } +}`, + Expected: helper.Issues{}, + }, + } + + rule := NewTerraformMapDuplicateKeysRule() + + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { + runner := testRunner(t, map[string]string{"module.tf": tc.Content}) + + if err := rule.Check(runner); err != nil { + t.Fatalf("Unexpected error occurred: %s", err) + } + + helper.AssertIssues(t, tc.Expected, runner.Runner.(*helper.Runner).Issues) + }) + } +} From 775aaca597fdd2ab79ee3ccd13f8922ca9f4c0cd Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Thu, 25 Jul 2024 10:30:27 -0700 Subject: [PATCH 2/4] clean up code, issue messages --- rules/terraform_map_duplicate_keys.go | 67 ++++++++++++---------- rules/terraform_map_duplicate_keys_test.go | 8 +-- 2 files changed, 42 insertions(+), 33 deletions(-) diff --git a/rules/terraform_map_duplicate_keys.go b/rules/terraform_map_duplicate_keys.go index cef5a30..d50bdfd 100644 --- a/rules/terraform_map_duplicate_keys.go +++ b/rules/terraform_map_duplicate_keys.go @@ -5,6 +5,7 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/terraform-linters/tflint-plugin-sdk/logger" "github.com/terraform-linters/tflint-plugin-sdk/tflint" "github.com/terraform-linters/tflint-ruleset-terraform/project" "github.com/zclconf/go-cty/cty" @@ -56,51 +57,59 @@ func (r *TerraformMapDuplicateKeysRule) Check(runner tflint.Runner) error { } func (r *TerraformMapDuplicateKeysRule) checkObjectConsExpr(e hcl.Expression, runner tflint.Runner) hcl.Diagnostics { - exprMap, ok := e.(*hclsyntax.ObjectConsExpr) + objExpr, ok := e.(*hclsyntax.ObjectConsExpr) if !ok { - // Ignore everything that isn't an ObjectConsExpr return nil } - diags := hcl.Diagnostics{} - foundKeys := make(map[string]hcl.Range) - for _, item := range exprMap.Items { + + var diags hcl.Diagnostics + keys := make(map[string]hcl.Range) + + for _, item := range objExpr.Items { expr := item.KeyExpr.(*hclsyntax.ObjectConsKeyExpr) - val := cty.Value{} - err := runner.EvaluateExpr(expr, &val, &tflint.EvaluateExprOption{}) + var val cty.Value + + err := runner.EvaluateExpr(expr, &val, nil) if err != nil { - fmt.Printf("Failed to evaluate an expression, continuing\n") - diags.Append( - &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "failed to evaluate expression", - Detail: err.Error(), - }, - ) + logger.Debug("Failed to evaluate an expression, continuing", "error", err) + + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "failed to evaluate expression", + Detail: err.Error(), + }) continue } + if !val.IsKnown() || val.IsNull() { // When trying to evaluate an expression - // with a variable without a default, + // with a variable without a value, // runner.evaluateExpr() returns a null value. // Ignore this case since there's nothing we can do. - fmt.Printf("Unknown key, continuing\n") + logger.Debug("Unknown key, continuing", "range", expr.Range()) continue } - if previousRange, exists := foundKeys[val.AsString()]; exists { - msg := fmt.Sprintf("Duplicate key: '%s'\nThe previous definition was at %s", val.AsString(), previousRange) - if err := runner.EmitIssue(r, msg, expr.Range()); err != nil { - diags.Append( - &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "failed to call EmitIssue()", - Detail: err.Error(), - }, - ) + if previous, exists := keys[val.AsString()]; exists { + if err := runner.EmitIssue( + r, + fmt.Sprintf("Duplicate key: %q, previously defined at %s", val.AsString(), previous), + expr.Range(), + ); err != nil { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "failed to call EmitIssue()", + Detail: err.Error(), + }) + + return diags } - } else { - foundKeys[val.AsString()] = expr.Range() + + continue } + + keys[val.AsString()] = expr.Range() } + return diags } diff --git a/rules/terraform_map_duplicate_keys_test.go b/rules/terraform_map_duplicate_keys_test.go index 89f7673..f15f4a5 100644 --- a/rules/terraform_map_duplicate_keys_test.go +++ b/rules/terraform_map_duplicate_keys_test.go @@ -38,7 +38,7 @@ resource "null_resource" "test" { Expected: helper.Issues{ { Rule: NewTerraformMapDuplicateKeysRule(), - Message: "Duplicate key: 'a'\nThe previous definition was at module.tf:4,9-10", + Message: `Duplicate key: "a", previously defined at module.tf:4,9-10`, Range: hcl.Range{ Filename: "module.tf", Start: hcl.Pos{Line: 5, Column: 9}, @@ -64,7 +64,7 @@ resource "null_resource" "test" { Expected: helper.Issues{ { Rule: NewTerraformMapDuplicateKeysRule(), - Message: "Duplicate key: 'b'\nThe previous definition was at module.tf:9,4-11", + Message: `Duplicate key: "b", previously defined at module.tf:9,4-11`, Range: hcl.Range{ Filename: "module.tf", Start: hcl.Pos{Line: 10, Column: 4}, @@ -101,7 +101,7 @@ resource "null_resource" "test" { Expected: helper.Issues{ { Rule: NewTerraformMapDuplicateKeysRule(), - Message: "Duplicate key: 'a'\nThe previous definition was at module.tf:4,4-5", + Message: `Duplicate key: "a", previously defined at module.tf:4,4-5`, Range: hcl.Range{ Filename: "module.tf", Start: hcl.Pos{Line: 5, Column: 4}, @@ -110,7 +110,7 @@ resource "null_resource" "test" { }, { Rule: NewTerraformMapDuplicateKeysRule(), - Message: "Duplicate key: 'a'\nThe previous definition was at module.tf:4,4-5", + Message: `Duplicate key: "a", previously defined at module.tf:4,4-5`, Range: hcl.Range{ Filename: "module.tf", Start: hcl.Pos{Line: 6, Column: 4}, From a2e8d25841488f3d589c1b36f51611159249c82b Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Thu, 25 Jul 2024 10:35:27 -0700 Subject: [PATCH 3/4] previously defined -> first defined --- rules/terraform_map_duplicate_keys.go | 4 ++-- rules/terraform_map_duplicate_keys_test.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/rules/terraform_map_duplicate_keys.go b/rules/terraform_map_duplicate_keys.go index d50bdfd..b216955 100644 --- a/rules/terraform_map_duplicate_keys.go +++ b/rules/terraform_map_duplicate_keys.go @@ -90,10 +90,10 @@ func (r *TerraformMapDuplicateKeysRule) checkObjectConsExpr(e hcl.Expression, ru continue } - if previous, exists := keys[val.AsString()]; exists { + if declRange, exists := keys[val.AsString()]; exists { if err := runner.EmitIssue( r, - fmt.Sprintf("Duplicate key: %q, previously defined at %s", val.AsString(), previous), + fmt.Sprintf("Duplicate key: %q, first defined at %s", val.AsString(), declRange), expr.Range(), ); err != nil { diags = append(diags, &hcl.Diagnostic{ diff --git a/rules/terraform_map_duplicate_keys_test.go b/rules/terraform_map_duplicate_keys_test.go index f15f4a5..8d0ee6c 100644 --- a/rules/terraform_map_duplicate_keys_test.go +++ b/rules/terraform_map_duplicate_keys_test.go @@ -38,7 +38,7 @@ resource "null_resource" "test" { Expected: helper.Issues{ { Rule: NewTerraformMapDuplicateKeysRule(), - Message: `Duplicate key: "a", previously defined at module.tf:4,9-10`, + Message: `Duplicate key: "a", first defined at module.tf:4,9-10`, Range: hcl.Range{ Filename: "module.tf", Start: hcl.Pos{Line: 5, Column: 9}, @@ -64,7 +64,7 @@ resource "null_resource" "test" { Expected: helper.Issues{ { Rule: NewTerraformMapDuplicateKeysRule(), - Message: `Duplicate key: "b", previously defined at module.tf:9,4-11`, + Message: `Duplicate key: "b", first defined at module.tf:9,4-11`, Range: hcl.Range{ Filename: "module.tf", Start: hcl.Pos{Line: 10, Column: 4}, @@ -101,7 +101,7 @@ resource "null_resource" "test" { Expected: helper.Issues{ { Rule: NewTerraformMapDuplicateKeysRule(), - Message: `Duplicate key: "a", previously defined at module.tf:4,4-5`, + Message: `Duplicate key: "a", first defined at module.tf:4,4-5`, Range: hcl.Range{ Filename: "module.tf", Start: hcl.Pos{Line: 5, Column: 4}, @@ -110,7 +110,7 @@ resource "null_resource" "test" { }, { Rule: NewTerraformMapDuplicateKeysRule(), - Message: `Duplicate key: "a", previously defined at module.tf:4,4-5`, + Message: `Duplicate key: "a", first defined at module.tf:4,4-5`, Range: hcl.Range{ Filename: "module.tf", Start: hcl.Pos{Line: 6, Column: 4}, From 81d1f7938cf979d0ce5c5dff6e64f48b17529211 Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Thu, 25 Jul 2024 10:39:51 -0700 Subject: [PATCH 4/4] add test for quoted keys --- rules/terraform_map_duplicate_keys_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/rules/terraform_map_duplicate_keys_test.go b/rules/terraform_map_duplicate_keys_test.go index 8d0ee6c..732958d 100644 --- a/rules/terraform_map_duplicate_keys_test.go +++ b/rules/terraform_map_duplicate_keys_test.go @@ -47,6 +47,27 @@ resource "null_resource" "test" { }, }, }, + { + Name: "duplicate keys with quoting", + Content: ` +resource "null_resource" "test" { + triggers = { + a = "b" + "a" = "c" + } +}`, + Expected: helper.Issues{ + { + Rule: NewTerraformMapDuplicateKeysRule(), + Message: `Duplicate key: "a", first defined at module.tf:4,9-10`, + Range: hcl.Range{ + Filename: "module.tf", + Start: hcl.Pos{Line: 5, Column: 9}, + End: hcl.Pos{Line: 5, Column: 12}, + }, + }, + }, + }, { Name: "Using variables as keys", Content: `