terraform-linters / tflint

A Pluggable Terraform Linter
Mozilla Public License 2.0
4.86k stars 354 forks source link

`terraform_deprecated_interpolation`: deeply inspect map/list attributes #1257

Closed morremeyer closed 2 years ago

morremeyer commented 2 years ago

terraform_deprecated_interpolation does not detect interpolation everywhere in a configuration. Take the following two examples:

data "terraform_remote_state" "remote_state" {
  backend = "remote"

  config = {
    organization = "Organization"
    workspaces = {
      name = "${var.environment}"
    }
  }
}

and

provider "aws" {
  region = var.region
  default_tags {
    tags = {
      Owner          = "owner"
      Application    = "application"
      Environment    = "${var.environment}"
      Provisioned-by = "terraform"
    }
  }
}

In both, the "${var.environment}" is not detected as deprecated interpolation-only variable.

Interestingly, the same applies to terraform fmt, which does also not detect this. This might be related as tflint uses terraform built-in as far as I’m aware.

Version

❯ tflint -v
TFLint version 0.33.0
+ ruleset.aws (0.8.0)
❯ terraform -v
Terraform v1.0.11
on darwin_amd64
bendrucker commented 2 years ago

There is nothing special about either of these two block types so I have a hard time seeing why this would be. That rule attempts to walk all expressions:

https://github.com/terraform-linters/tflint/blob/a18d0944774d34ed10a86ff1c80c97e1e9e47000/tflint/runner_walk.go#L143-L186

That certainly includes walking provider and data blocks. It's conceivable that Terraform parses them differently I guess.

bendrucker commented 2 years ago

Ok, did a bit of debugging here. The key characteristic of the examples is not their block types, it's the fact that it's an object expression.

The rule is looking for expressions of type *hclsyntax.TemplateWrapExpr. That applies when an attribute is set to an unnecessarily quoted expression directly. But if the attribute is assigned to an object or another collection type that contains the expression, there's no match.

I seem to recall the logic for that rule was lifted from Terraform. terraform fmt could do better here and making the TFLint rule more comprehensive than fmt is also ok, as long as the rule is faithful to the written guidance in the Terraform docs.

In this case, the expression is actually a *hclsyntax.ObjectConsExpr. And expr.(*hclsyntax.ObjectConsExpr).Items[0].ValueExpr is a *hclsyntax.TemplateWrapExpr. Don't have time to take this any further at the moment, but in theory it would be doable to traverse the data in known collection expression types.

bendrucker commented 2 years ago

Similar to WalkExpressions we'd probably want to implement the complex expression walking in the runner and then call it in the rule.

morremeyer commented 2 years ago

Thanks for analyzing it! I’ll see if we can contribute that change to terraform fmt directly and adapt it for tflint.

bendrucker commented 2 years ago

It's a real tricky one for sure. There are presumably a fair number of collection types, e.g. a distinction between objects (known keys) and maps (unknown). Then there's sets/lists/tuples. It's almost certain that this is technically possible already, but I can imagine that implementing this elegantly could involve enhancing the hcl packages too.

wata727 commented 2 years ago

For deeper walking, we can use hclsyntax.Walk. https://github.com/hashicorp/hcl/blob/v2.13.0/hclsyntax/walk.go

However, since json.Walk does not exist, it is difficult to fully meet the requirements.