From 886b5f09d9d86f2775caae55d8ae2a84e80fd78e Mon Sep 17 00:00:00 2001 From: Bruno Ferreira Date: Sun, 9 Feb 2025 00:28:58 +0000 Subject: [PATCH 1/2] create new rule to validate if short circuit validation is being used --- rules/preset.go | 1 + .../terraform_no_short_circuit_evaluation.go | 139 ++++++++++++++++++ ...raform_no_short_circuit_evaluation_test.go | 109 ++++++++++++++ 3 files changed, 249 insertions(+) create mode 100644 rules/terraform_no_short_circuit_evaluation.go create mode 100644 rules/terraform_no_short_circuit_evaluation_test.go diff --git a/rules/preset.go b/rules/preset.go index 17e217b..85c1d00 100644 --- a/rules/preset.go +++ b/rules/preset.go @@ -22,6 +22,7 @@ var PresetRules = map[string][]tflint.Rule{ NewTerraformUnusedDeclarationsRule(), NewTerraformUnusedRequiredProvidersRule(), NewTerraformWorkspaceRemoteRule(), + NewTerraformNoShortCircuitEvaluationRule(), }, "recommended": { NewTerraformDeprecatedIndexRule(), diff --git a/rules/terraform_no_short_circuit_evaluation.go b/rules/terraform_no_short_circuit_evaluation.go new file mode 100644 index 0000000..7f69c12 --- /dev/null +++ b/rules/terraform_no_short_circuit_evaluation.go @@ -0,0 +1,139 @@ +package rules + +import ( + "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" +) + +// TerraformNoShortCircuitEvaluationRule checks for attempts to use short-circuit evaluation +type TerraformNoShortCircuitEvaluationRule struct { + tflint.DefaultRule +} + +// NewTerraformNoShortCircuitEvaluationRule returns a new rule +func NewTerraformNoShortCircuitEvaluationRule() *TerraformNoShortCircuitEvaluationRule { + return &TerraformNoShortCircuitEvaluationRule{} +} + +// Name returns the rule name +func (r *TerraformNoShortCircuitEvaluationRule) Name() string { + return "terraform_no_short_circuit_evaluation" +} + +// Enabled returns whether the rule is enabled by default +func (r *TerraformNoShortCircuitEvaluationRule) Enabled() bool { + return true +} + +// Severity returns the rule severity +func (r *TerraformNoShortCircuitEvaluationRule) Severity() tflint.Severity { + return tflint.WARNING +} + +// Link returns the rule reference link +func (r *TerraformNoShortCircuitEvaluationRule) Link() string { + return project.ReferenceLink(r.Name()) +} + +// Check checks for attempts to use short-circuit evaluation +func (r *TerraformNoShortCircuitEvaluationRule) 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(expr hcl.Expression) hcl.Diagnostics { + if binaryOpExpr, ok := expr.(*hclsyntax.BinaryOpExpr); ok { + if binaryOpExpr.Op == hclsyntax.OpLogicalAnd || binaryOpExpr.Op == hclsyntax.OpLogicalOr { + // Check if left side is a null check + if isNullCheck(binaryOpExpr.LHS) { + // Check if right side references the same variable as the left side + if referencesNullCheckedVar(binaryOpExpr.LHS, binaryOpExpr.RHS) { + if err := runner.EmitIssue( + r, + "Short-circuit evaluation is not supported in Terraform. Use a conditional expression (condition ? true : false) instead.", + binaryOpExpr.Range(), + ); err != nil { + return hcl.Diagnostics{ + { + Severity: hcl.DiagError, + Summary: "Failed to emit issue", + Detail: err.Error(), + }, + } + } + } + } + } + } + return nil + })) + + if diags.HasErrors() { + return diags + } + return nil +} + +// isNullCheck determines if an expression is checking for null +func isNullCheck(expr hcl.Expression) bool { + if binaryOpExpr, ok := expr.(*hclsyntax.BinaryOpExpr); ok { + if binaryOpExpr.Op == hclsyntax.OpEqual || binaryOpExpr.Op == hclsyntax.OpNotEqual { + // Check if either side is a null literal + if isNullLiteral(binaryOpExpr.RHS) || isNullLiteral(binaryOpExpr.LHS) { + return true + } + } + } + return false +} + +// isNullLiteral checks if the expression is a null literal +func isNullLiteral(expr hcl.Expression) bool { + if literalExpr, ok := expr.(*hclsyntax.LiteralValueExpr); ok { + return literalExpr.Val.IsNull() + } + return false +} + +// referencesNullCheckedVar checks if the right side expression references the same variable that was null checked +func referencesNullCheckedVar(nullCheck, expr hcl.Expression) bool { + // Get the variable name from the null check + var varName string + if binaryOpExpr, ok := nullCheck.(*hclsyntax.BinaryOpExpr); ok { + // Try to get variable name from LHS + if scopeTraversalExpr, ok := binaryOpExpr.LHS.(*hclsyntax.ScopeTraversalExpr); ok { + if len(scopeTraversalExpr.Traversal) > 0 { + varName = scopeTraversalExpr.Traversal.RootName() + } + } + // If not found in LHS, try RHS + if varName == "" { + if scopeTraversalExpr, ok := binaryOpExpr.RHS.(*hclsyntax.ScopeTraversalExpr); ok { + if len(scopeTraversalExpr.Traversal) > 0 { + varName = scopeTraversalExpr.Traversal.RootName() + } + } + } + } + + // If we couldn't find a variable name, return false + if varName == "" { + return false + } + + // Check if the expression references the same variable + vars := expr.Variables() + for _, v := range vars { + if len(v) > 0 && v.RootName() == varName { + return true + } + } + return false +} \ No newline at end of file diff --git a/rules/terraform_no_short_circuit_evaluation_test.go b/rules/terraform_no_short_circuit_evaluation_test.go new file mode 100644 index 0000000..9defb3f --- /dev/null +++ b/rules/terraform_no_short_circuit_evaluation_test.go @@ -0,0 +1,109 @@ +package rules + +import ( + "testing" + + hcl "github.com/hashicorp/hcl/v2" + "github.com/terraform-linters/tflint-plugin-sdk/helper" +) + +func Test_TerraformNoShortCircuitEvaluationRule(t *testing.T) { + cases := []struct { + Name string + Content string + Expected helper.Issues + }{ + { + Name: "short circuit with null check and &&", + Content: ` +resource "aws_instance" "example" { + count = var.obj != null && var.obj.enabled ? 1 : 0 +}`, + Expected: helper.Issues{ + { + Rule: NewTerraformNoShortCircuitEvaluationRule(), + Message: "Short-circuit evaluation is not supported in Terraform. Use a conditional expression (condition ? true : false) instead.", + Range: hcl.Range{ + Filename: "resource.tf", + Start: hcl.Pos{Line: 3, Column: 11}, + End: hcl.Pos{Line: 3, Column: 45}, + }, + }, + }, + }, + { + Name: "short circuit with null check and ||", + Content: ` +resource "aws_instance" "example" { + count = var.obj == null || var.obj.enabled ? 1 : 0 +}`, + Expected: helper.Issues{ + { + Rule: NewTerraformNoShortCircuitEvaluationRule(), + Message: "Short-circuit evaluation is not supported in Terraform. Use a conditional expression (condition ? true : false) instead.", + Range: hcl.Range{ + Filename: "resource.tf", + Start: hcl.Pos{Line: 3, Column: 11}, + End: hcl.Pos{Line: 3, Column: 45}, + }, + }, + }, + }, + { + Name: "correct conditional usage", + Content: ` +resource "aws_instance" "example" { + count = var.obj == null ? 0 : (var.obj.enabled ? 1 : 0) +}`, + Expected: helper.Issues{}, + }, + { + Name: "valid use of logical operators with independent values", + Content: ` +resource "aws_instance" "example" { + count = var.value > 3 || var.other_value < 10 ? 1 : 0 +}`, + Expected: helper.Issues{}, + }, + { + Name: "valid use of logical operators with same object", + Content: ` +resource "aws_instance" "example" { + count = var.obj > 3 || var.obj < 10 ? 1 : 0 +}`, + Expected: helper.Issues{}, + }, + { + Name: "multiple null checks in one expression", + Content: ` +resource "aws_instance" "example" { + count = var.obj != null && var.obj.enabled && var.obj.property > 0 ? 1 : 0 +}`, + Expected: helper.Issues{ + { + Rule: NewTerraformNoShortCircuitEvaluationRule(), + Message: "Short-circuit evaluation is not supported in Terraform. Use a conditional expression (condition ? true : false) instead.", + Range: hcl.Range{ + Filename: "resource.tf", + Start: hcl.Pos{Line: 3, Column: 11}, + End: hcl.Pos{Line: 3, Column: 45}, + }, + }, + }, + }, + } + + rule := NewTerraformNoShortCircuitEvaluationRule() + + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { + runner := helper.TestRunner(t, map[string]string{"resource.tf": tc.Content}) + + if err := rule.Check(runner); err != nil { + t.Fatalf("Unexpected error occurred: %s", err) + } + + helper.AssertIssues(t, tc.Expected, runner.Issues) + }) + } +} From 3d769555c91c719f1d1b4cc266e46abeedbd2f55 Mon Sep 17 00:00:00 2001 From: Bruno Ferreira Date: Sun, 9 Feb 2025 00:29:59 +0000 Subject: [PATCH 2/2] add docs --- docs/rules/README.md | 1 + .../terraform_no_short_circuit_evaluation.md | 60 +++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 docs/rules/terraform_no_short_circuit_evaluation.md diff --git a/docs/rules/README.md b/docs/rules/README.md index f052eda..d8b1faf 100644 --- a/docs/rules/README.md +++ b/docs/rules/README.md @@ -17,6 +17,7 @@ All rules are enabled by default, but by setting `preset = "recommended"`, you c |[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|| +|[terraform_no_short_circuit_evaluation](terraform_no_short_circuit_evaluation.md)|Disallow using logical operators with null checks that could lead to errors|✔| |[terraform_required_providers](terraform_required_providers.md)|Require that all providers have version constraints through required_providers|✔| |[terraform_required_version](terraform_required_version.md)|Disallow `terraform` declarations without require_version|✔| |[terraform_standard_module_structure](terraform_standard_module_structure.md)|Ensure that a module complies with the Terraform Standard Module Structure|| diff --git a/docs/rules/terraform_no_short_circuit_evaluation.md b/docs/rules/terraform_no_short_circuit_evaluation.md new file mode 100644 index 0000000..f8d0176 --- /dev/null +++ b/docs/rules/terraform_no_short_circuit_evaluation.md @@ -0,0 +1,60 @@ +# terraform_no_short_circuit_evaluation + +Disallow using logical operators (`&&`, `||`) with null checks that could lead to errors due to lack of short-circuit evaluation. + +> This rule is enabled by "recommended" preset. + +## Example + +```hcl +# This will error if var.obj is null +resource "aws_instance" "example" { + count = var.obj != null && var.obj.enabled ? 1 : 0 +} + +# This is the safe way to write it +resource "aws_instance" "example" { + count = var.obj != null ? var.obj.enabled ? 1 : 0 : 0 +} +``` + +``` +$ tflint +1 issue(s) found: + +Warning: Short-circuit evaluation is not supported in Terraform. Use a conditional expression (condition ? true : false) instead. (terraform_no_short_circuit_evaluation) + + on main.tf line 3: + 3: count = var.obj != null && var.obj.enabled ? 1 : 0 + +Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.1.0/docs/rules/terraform_no_short_circuit_evaluation.md +``` + +## Why + +Unlike many programming languages, Terraform's logical operators (`&&` and `||`) do not short-circuit. This means that in an expression like `var.obj != null && var.obj.enabled`, both sides will be evaluated even if `var.obj` is null, which will result in an error. + +This is a common source of confusion for users coming from other programming languages where short-circuit evaluation is standard behavior. The issue is particularly problematic when checking for null before accessing object attributes. + +## How To Fix + +Use nested conditional expressions instead of logical operators when you need short-circuit behavior. For example: + +```hcl +# Instead of this: +var.obj != null && var.obj.enabled + +# Use this: +var.obj != null ? var.obj.enabled : false + +# For more complex conditions: +var.obj != null ? (var.obj.enabled ? var.obj.value > 0 : false) : false +``` + +You can also use the `try()` function in some cases, though this may mask errors you want to catch: + +```hcl +try(var.obj.enabled, false) +``` + +For more information, see [hashicorp/terraform#24128](https://github.com/hashicorp/terraform/issues/24128). \ No newline at end of file