From 5d5c1a4bfe768ce9adddb31777bc4e65c5127dc9 Mon Sep 17 00:00:00 2001 From: Kazuma Watanabe Date: Wed, 16 Oct 2024 16:38:33 +0000 Subject: [PATCH] Fix issues caused by multiple terraform blocks See also https://github.com/terraform-linters/tflint-ruleset-terraform/issues/205 Some rules that reference terraform blocks do not assume that blocks can be defined multiple times and can lead to inconsistent results in that case. --- rules/terraform_required_providers.go | 4 +- rules/terraform_required_providers_test.go | 135 +++++++++++++++++- rules/terraform_unused_required_providers.go | 6 +- ...erraform_unused_required_providers_test.go | 37 +++++ rules/terraform_workspace_remote.go | 19 +-- rules/terraform_workspace_remote_test.go | 17 +++ 6 files changed, 206 insertions(+), 12 deletions(-) diff --git a/rules/terraform_required_providers.go b/rules/terraform_required_providers.go index a5c3773..c5cccf9 100644 --- a/rules/terraform_required_providers.go +++ b/rules/terraform_required_providers.go @@ -151,7 +151,9 @@ func (r *TerraformRequiredProvidersRule) Check(rr tflint.Runner) error { requiredProviders := hclext.Attributes{} for _, terraform := range body.Blocks { for _, requiredProvidersBlock := range terraform.Body.Blocks { - requiredProviders = requiredProvidersBlock.Body.Attributes + for name, attr := range requiredProvidersBlock.Body.Attributes { + requiredProviders[name] = attr + } } } diff --git a/rules/terraform_required_providers_test.go b/rules/terraform_required_providers_test.go index e49db60..d461914 100644 --- a/rules/terraform_required_providers_test.go +++ b/rules/terraform_required_providers_test.go @@ -1,10 +1,14 @@ package rules import ( + "reflect" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/hashicorp/hcl/v2" "github.com/terraform-linters/tflint-plugin-sdk/helper" + "github.com/terraform-linters/tflint-plugin-sdk/tflint" ) func Test_TerraformRequiredProvidersRule(t *testing.T) { @@ -570,6 +574,107 @@ output "foo" { }, }, }, + { + Name: "multiple required providers", + Content: ` +terraform { + required_providers { + template = "~> 2" + } + + required_providers { + aws = "~> 5.0" + } +} + +provider "template" {} +provider "aws" {} +provider "google" {} + +terraform { + required_providers { + google = "~> 6.0" + } +} +`, + Expected: helper.Issues{ + { + Rule: NewTerraformRequiredProvidersRule(), + Message: "Legacy version constraint for provider \"template\" in `required_providers`", + Range: hcl.Range{ + Filename: "module.tf", + Start: hcl.Pos{ + Line: 4, + Column: 16, + }, + End: hcl.Pos{ + Line: 4, + Column: 22, + }, + }, + }, + { + Rule: NewTerraformRequiredProvidersRule(), + Message: "Legacy version constraint for provider \"aws\" in `required_providers`", + Range: hcl.Range{ + Filename: "module.tf", + Start: hcl.Pos{ + Line: 8, + Column: 11, + }, + End: hcl.Pos{ + Line: 8, + Column: 19, + }, + }, + }, + { + Rule: NewTerraformRequiredProvidersRule(), + Message: "Legacy version constraint for provider \"google\" in `required_providers`", + Range: hcl.Range{ + Filename: "module.tf", + Start: hcl.Pos{ + Line: 18, + Column: 14, + }, + End: hcl.Pos{ + Line: 18, + Column: 22, + }, + }, + }, + }, + Fixed: ` +terraform { + required_providers { + template = { + source = "hashicorp/template" + version = "~> 2" + } + } + + required_providers { + aws = { + source = "hashicorp/aws" + version = "~> 5.0" + } + } +} + +provider "template" {} +provider "aws" {} +provider "google" {} + +terraform { + required_providers { + google = { + source = "hashicorp/google" + version = "~> 6.0" + } + } +} +`, + }, } rule := NewTerraformRequiredProvidersRule() @@ -590,7 +695,35 @@ output "foo" { t.Fatalf("Unexpected error occurred: %s", err) } - helper.AssertIssues(t, tc.Expected, runner.Runner.(*helper.Runner).Issues) + // TODO: replace the following assertions without ordering by AssertIssues + // helper.AssertIssues(t, tc.Expected, runner.Runner.(*helper.Runner).Issues) + opts := []cmp.Option{ + cmpopts.IgnoreFields(hcl.Pos{}, "Byte"), + cmp.Comparer(func(x, y tflint.Rule) bool { + return reflect.TypeOf(x) == reflect.TypeOf(y) + }), + cmpopts.SortSlices(func(i, j *helper.Issue) bool { + if i.Range.Filename != j.Range.Filename { + return i.Range.Filename < j.Range.Filename + } + if i.Range.Start.Line != j.Range.Start.Line { + return i.Range.Start.Line < j.Range.Start.Line + } + if i.Range.Start.Column != j.Range.Start.Column { + return i.Range.Start.Column < j.Range.Start.Column + } + if i.Range.End.Line != j.Range.End.Line { + return i.Range.End.Line > j.Range.End.Line + } + if i.Range.End.Column != j.Range.End.Column { + return i.Range.End.Column > j.Range.End.Column + } + return i.Message < j.Message + }), + } + if diff := cmp.Diff(tc.Expected, runner.Runner.(*helper.Runner).Issues, opts...); diff != "" { + t.Fatalf("Expected issues are not matched:\n %s\n", diff) + } want := map[string]string{} if tc.Fixed != "" { want[filename] = tc.Fixed diff --git a/rules/terraform_unused_required_providers.go b/rules/terraform_unused_required_providers.go index 5b83c6c..b75b484 100644 --- a/rules/terraform_unused_required_providers.go +++ b/rules/terraform_unused_required_providers.go @@ -82,8 +82,10 @@ func (r *TerraformUnusedRequiredProvidersRule) Check(rr tflint.Runner) error { } for _, block := range content.Blocks { - var attrDiags hcl.Diagnostics - requiredProviders, attrDiags = block.Body.JustAttributes() + attributes, attrDiags := block.Body.JustAttributes() + for k, v := range attributes { + requiredProviders[k] = v + } diags = diags.Extend(attrDiags) if diags.HasErrors() { continue diff --git a/rules/terraform_unused_required_providers_test.go b/rules/terraform_unused_required_providers_test.go index 0ac06a7..c846396 100644 --- a/rules/terraform_unused_required_providers_test.go +++ b/rules/terraform_unused_required_providers_test.go @@ -260,6 +260,43 @@ func Test_TerraformUnusedRequiredProvidersRule(t *testing.T) { `, Expected: helper.Issues{}, }, + { + Name: "unused - in multiple terraform blocks", + Content: ` + terraform { + required_providers { + aws = { + source = "hashicorp/aws" + } + } + } + terraform { + required_providers { + null = { + source = "hashicorp/null" + } + } + } + resource "null_resource" "foo" {} + `, + Expected: helper.Issues{ + { + Rule: NewTerraformUnusedRequiredProvidersRule(), + Message: "provider 'aws' is declared in required_providers but not used by the module", + Range: hcl.Range{ + Filename: "module.tf", + Start: hcl.Pos{ + Line: 4, + Column: 7, + }, + End: hcl.Pos{ + Line: 6, + Column: 8, + }, + }, + }, + }, + }, } rule := NewTerraformUnusedRequiredProvidersRule() diff --git a/rules/terraform_workspace_remote.go b/rules/terraform_workspace_remote.go index 4912d01..24632a2 100644 --- a/rules/terraform_workspace_remote.go +++ b/rules/terraform_workspace_remote.go @@ -94,20 +94,15 @@ func (r *TerraformWorkspaceRemoteRule) Check(runner tflint.Runner) error { } var remoteBackend bool - var tf10Support bool + constraints := version.Constraints{} for _, terraform := range body.Blocks { for _, requiredVersion := range terraform.Body.Attributes { err := runner.EvaluateExpr(requiredVersion.Expr, func(v string) error { - constraints, err := version.NewConstraint(v) + c, err := version.NewConstraint(v) if err != nil { return err } - - for _, tf10Version := range tf10Versions { - if constraints.Check(tf10Version) { - tf10Support = true - } - } + constraints = append(constraints, c...) return nil }, nil) if err != nil { @@ -121,6 +116,14 @@ func (r *TerraformWorkspaceRemoteRule) Check(runner tflint.Runner) error { } } } + var tf10Support bool + if constraints.Len() > 0 { + for _, tf10Version := range tf10Versions { + if constraints.Check(tf10Version) { + tf10Support = true + } + } + } if !remoteBackend || !tf10Support { return nil } diff --git a/rules/terraform_workspace_remote_test.go b/rules/terraform_workspace_remote_test.go index c64962d..2ad1f1f 100644 --- a/rules/terraform_workspace_remote_test.go +++ b/rules/terraform_workspace_remote_test.go @@ -296,6 +296,23 @@ resource "kubernetes_secret" "my_secret" { data ] } +}`, + Expected: helper.Issues{}, + }, + { + Name: "required_version does not support 1.0.x by multiple setting", + Content: ` +terraform { + required_version = "< 1.9" + backend "remote" {} +} +terraform { + required_version = ">= 1.1" +} +resource "null_resource" "a" { + triggers = { + w = terraform.workspace + } }`, Expected: helper.Issues{}, },