From 2b18257293f88b50428c512ce621f4fea67401c0 Mon Sep 17 00:00:00 2001 From: Bruno Ferreira Date: Mon, 18 Apr 2022 00:24:15 +0100 Subject: [PATCH] feat: add new rule to check comparisons with empty list --- docs/rules/README.md | 2 + docs/rules/terraform_empty_list_check.md | 36 ++++++++ rules/provider.go | 1 + .../terraform_empty_list_check.go | 88 +++++++++++++++++++ .../terraform_empty_list_check_test.go | 85 ++++++++++++++++++ test.tf | 7 ++ 6 files changed, 219 insertions(+) create mode 100644 docs/rules/terraform_empty_list_check.md create mode 100644 rules/terraformrules/terraform_empty_list_check.go create mode 100644 rules/terraformrules/terraform_empty_list_check_test.go create mode 100644 test.tf diff --git a/docs/rules/README.md b/docs/rules/README.md index c0ead1cea..d2fe831b4 100644 --- a/docs/rules/README.md +++ b/docs/rules/README.md @@ -11,6 +11,7 @@ Below is a list of available rules. |[terraform_deprecated_interpolation](terraform_deprecated_interpolation.md)|Disallow deprecated (0.11-style) interpolation|✔| |[terraform_documented_outputs](terraform_documented_outputs.md)|Disallow `output` declarations without description|| |[terraform_documented_variables](terraform_documented_variables.md)|Disallow `variable` declarations without description|| +|[terraform_empty_list_check](terraform_empty_list_check.md)|Disallow comparisons with `[]` when checking if a list is empty|| |[terraform_module_pinned_source](terraform_module_pinned_source.md)|Disallow specifying a git or mercurial repository as a module source without pinning to a version|✔| |[terraform_module_version](terraform_module_version.md)|Checks that Terraform modules sourced from a registry specify a version|✔| |[terraform_naming_convention](terraform_naming_convention.md)|Enforces naming conventions for resources, data sources, etc|| @@ -21,3 +22,4 @@ Below is a list of available rules. |[terraform_unused_declarations](terraform_unused_declarations.md)|Disallow variables, data sources, and locals that are declared but never used|| |[terraform_unused_required_providers](terraform_unused_required_providers.md)|Check that all `required_providers` are used in the module|| |[terraform_workspace_remote](terraform_workspace_remote.md)|`terraform.workspace` should not be used with a "remote" backend with remote execution|✔| + diff --git a/docs/rules/terraform_empty_list_check.md b/docs/rules/terraform_empty_list_check.md new file mode 100644 index 000000000..50c5e2b05 --- /dev/null +++ b/docs/rules/terraform_empty_list_check.md @@ -0,0 +1,36 @@ +# terraform_empty_list_check + +Disallow comparisons with `[]` when checking if a list is empty. + +## Example + +```hcl +variable "my_list" { + type = list(string) +} +resource "aws_db_instance" "mysql" { + count = var.my_list == [] ? 0 : 1 + instance_class = "m4.2xlarge" +} +``` + +``` +$ tflint +1 issue(s) found: + +Warning: List is compared with [] instead of checking if length is 0. (terraform_empty_list_check) + + on test.tf line 5: + 5: count = var.my_list == [] ? 0 : 1 + +Reference: https://github.com/terraform-linters/tflint/blob/master/docs/rules/terraform_empty_list_check.md + +``` + +## Why + +The `==` operator can only return true when the two operands have identical types, and the type of `[]` alone (without any further type conversions) is an empty tuple rather than a list of objects, strings, numbers or any other type. Therefore, a comparison with a single `[]` with the goal of checking if a list is empty, will always return false. + +## How To Fix + +Check if a list is empty by checking its length instead. For example: `length(var.my_list) == 0`. diff --git a/rules/provider.go b/rules/provider.go index f7192a6e0..17fc06a7e 100644 --- a/rules/provider.go +++ b/rules/provider.go @@ -21,6 +21,7 @@ var DefaultRules = []Rule{ terraformrules.NewTerraformDeprecatedInterpolationRule(), terraformrules.NewTerraformDocumentedOutputsRule(), terraformrules.NewTerraformDocumentedVariablesRule(), + terraformrules.NewTerraformEmptyListCheckRule(), terraformrules.NewTerraformModulePinnedSourceRule(), terraformrules.NewTerraformModuleVersionRule(), terraformrules.NewTerraformNamingConventionRule(), diff --git a/rules/terraformrules/terraform_empty_list_check.go b/rules/terraformrules/terraform_empty_list_check.go new file mode 100644 index 000000000..7756ae415 --- /dev/null +++ b/rules/terraformrules/terraform_empty_list_check.go @@ -0,0 +1,88 @@ +package terraformrules + +import ( + "log" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/terraform-linters/tflint/tflint" +) + +// TerraformEmptyListCheckRule checks whether is there a comparison with an empty list +type TerraformEmptyListCheckRule struct{} + +// NewTerraformCommentSyntaxRule returns a new rule +func NewTerraformEmptyListCheckRule() *TerraformEmptyListCheckRule { + return &TerraformEmptyListCheckRule{} +} + +// Name returns the rule name +func (r *TerraformEmptyListCheckRule) Name() string { + return "terraform_empty_list_check" +} + +// Enabled returns whether the rule is enabled by default +func (r *TerraformEmptyListCheckRule) Enabled() bool { + return true +} + +// Severity returns the rule severity +func (r *TerraformEmptyListCheckRule) Severity() tflint.Severity { + return tflint.WARNING +} + +// Link returns the rule reference link +func (r *TerraformEmptyListCheckRule) Link() string { + return tflint.ReferenceLink(r.Name()) +} + +// Check checks whether the list is being compared with static empty list +func (r *TerraformEmptyListCheckRule) Check(runner *tflint.Runner) error { + if !runner.TFConfig.Path.IsRoot() { + // This rule does not evaluate child modules. + return nil + } + + log.Printf("[TRACE] Check `%s` rule for `%s` runner", r.Name(), runner.TFConfigPath()) + + files := make(map[string]*struct{}) + for _, variable := range runner.TFConfig.Module.Variables { + files[variable.DeclRange.Filename] = nil + } + + for filename := range files { + if err := r.checkEmptyList(runner, filename); err != nil { + return err + } + } + + return nil +} + +func (r *TerraformEmptyListCheckRule) checkEmptyList(runner *tflint.Runner, filename string) error { + return runner.WalkExpressions(func(expr hcl.Expression) error { + if conditionalExpr, ok := expr.(*hclsyntax.ConditionalExpr); ok { + if binaryOpExpr, ok := conditionalExpr.Condition.(*hclsyntax.BinaryOpExpr); ok { + if binaryOpExpr.Op.Type.FriendlyName() == "bool" { + if right, ok := binaryOpExpr.RHS.(*hclsyntax.TupleConsExpr); ok { + checkEmptyList(right, runner, r, binaryOpExpr) + } + if left, ok := binaryOpExpr.LHS.(*hclsyntax.TupleConsExpr); ok { + checkEmptyList(left, runner, r, binaryOpExpr) + } + } + } + } + return nil + }) +} + +func checkEmptyList(tupleConsExpr *hclsyntax.TupleConsExpr, runner *tflint.Runner, r *TerraformEmptyListCheckRule, binaryOpExpr *hclsyntax.BinaryOpExpr) { + if len(tupleConsExpr.Exprs) == 0 { + runner.EmitIssue( + r, + "List is compared with [] instead of checking if length is 0.", + binaryOpExpr.Range(), + ) + } +} diff --git a/rules/terraformrules/terraform_empty_list_check_test.go b/rules/terraformrules/terraform_empty_list_check_test.go new file mode 100644 index 000000000..9a433f5df --- /dev/null +++ b/rules/terraformrules/terraform_empty_list_check_test.go @@ -0,0 +1,85 @@ +package terraformrules + +import ( + "testing" + + hcl "github.com/hashicorp/hcl/v2" + "github.com/terraform-linters/tflint/tflint" +) + +func Test_TerraformEmptyListCheckRule(t *testing.T) { + cases := []struct { + Name string + Content string + Expected tflint.Issues + }{ + { + Name: "comparing with [] is not recommended", + Content: ` +variable "my_list" { + type = list(string) +} +resource "aws_db_instance" "mysql" { + count = var.my_list == [] ? 0 : 1 + instance_class = "m4.2xlarge" +}`, + Expected: tflint.Issues{ + { + Rule: NewTerraformEmptyListCheckRule(), + Message: "List is compared with [] instead of checking if length is 0.", + Range: hcl.Range{ + Filename: "resource.tf", + Start: hcl.Pos{Line: 6, Column: 10}, + End: hcl.Pos{Line: 6, Column: 27}, + }, + }, + }, + }, + { + Name: "negatively comparing with [] is not recommended", + Content: ` +variable "my_list" { + type = list(string) +} +resource "aws_db_instance" "mysql" { + count = var.my_list != [] ? 1 : 0 + instance_class = "m4.2xlarge" +}`, + Expected: tflint.Issues{ + { + Rule: NewTerraformEmptyListCheckRule(), + Message: "List is compared with [] instead of checking if length is 0.", + Range: hcl.Range{ + Filename: "resource.tf", + Start: hcl.Pos{Line: 6, Column: 10}, + End: hcl.Pos{Line: 6, Column: 27}, + }, + }, + }, + }, + { + Name: "checking if length is 0 is recommended", + Content: ` +variable "my_list" { + type = list(string) +} +resource "aws_db_instance" "mysql" { + count = length(var.my_list) == 0 ? 1 : 0 + instance_class = "m4.2xlarge" +}`, + Expected: tflint.Issues{}, + }, + } + + rule := NewTerraformEmptyListCheckRule() + + for _, tc := range cases { + runner := tflint.TestRunner(t, map[string]string{"resource.tf": tc.Content}) + + if err := rule.Check(runner); err != nil { + t.Fatalf("Unexpected error occurred: %s", err) + } + + tflint.AssertIssues(t, tc.Expected, runner.Issues) + } +} diff --git a/test.tf b/test.tf new file mode 100644 index 000000000..95a55093c --- /dev/null +++ b/test.tf @@ -0,0 +1,7 @@ +variable "my_list" { + type = list(string) +} +resource "aws_db_instance" "mysql" { + count = var.my_list == [] ? 0 : 1 + instance_class = "m4.2xlarge" +}