search

antonbabenko / pre-commit-terraform

pre-commit git hooks to take care of Terraform configurations 🇺🇦
MIT License
3.16k stars 535 forks source link

tflint 0.40.0 changes seem to break current terraform_tflint hook configuration #434

Closed tofupup closed 1 year ago

tofupup commented 2 years ago

Describe the bug

tflint release 0.40.0 made some breaking changes that look to impact how it uses --only CLI options to specify rules. I've opened issue https://github.com/terraform-linters/tflint/issues/1513 to see if this is a bug that they will fix, or if pre-commit-terraform will need to modify the terraform_tflint hook to accommodate future versions.

Short summary is tflint has rolled the terraform plugin into the released binary, and now support an option in the configuration file to enable either all or recommended rules. By default, with no configuration file, it will go with recommended. However, if you run tflint 0.40.0 with --only=terraform_naming_convention (not a rule part of the recommended set), the rule does not get run. If you use the all rule preset in the tflint configuration file, the --only= option gets ignored and all rules are run.

Once I get a response on the tflint issue, I'll update here if it looks like changes will be needed. In the interim, if a new docker container is built automatically, it will include tflint 0.40.0, which will no longer run all of the tests expected in terraform_tflint. Pinning the tflint version in the github workflows might be considered until a response in the tflint issue.

How can we reproduce it?

Update tflint to 0.40.0 and run pre-commit run -a terraform_tflint on a file that will trigger rules not in the recommended list.

Files `private-codecommit-repo/main.tf` ```hcl module "private-codecommit-repo" { source = "git::ssh://SOMEUSER@git-codecommit.us-east-2.amazonaws.com/v1/repos/private-codecommit-repo" } variable "var1" { type = string } ``` `.pre-commit-config.yaml`: ```yaml repos: - repo: https://github.com/antonbabenko/pre-commit-terraform rev: v1.75.0 hooks: - id: terraform_fmt - id: terraform_wrapper_module_for_each - id: terraform_validate - id: terraform_docs args: - '--args=--lockfile=false' - id: terraform_tflint args: - '--args=--only=terraform_deprecated_interpolation' - '--args=--only=terraform_deprecated_index' - '--args=--only=terraform_unused_declarations' - '--args=--only=terraform_comment_syntax' - '--args=--only=terraform_documented_outputs' - '--args=--only=terraform_documented_variables' - '--args=--only=terraform_typed_variables' - '--args=--only=terraform_module_pinned_source' - '--args=--only=terraform_naming_convention' - '--args=--only=terraform_required_version' - '--args=--only=terraform_required_providers' - '--args=--only=terraform_standard_module_structure' - '--args=--only=terraform_workspace_remote' - repo: https://github.com/pre-commit/pre-commit-hooks rev: v4.3.0 hooks: - id: check-merge-conflict - id: end-of-file-fixer ```
terraform_tflint with tflint 0.39.3 ```bash ❯ sudo ln -s /usr/bin/tflint-0.39.3 /usr/bin/tflint ❯ tflint -v TFLint version 0.39.3 ❯ pre-commit run -a terraform_tflint Terraform validate with tflint...........................................Failed - hook id: terraform_tflint - exit code: 2 Command 'tflint --init' successfully done: TFLint in private-codecommit-repo/: 7 issue(s) found: Warning: terraform "required_version" attribute is required (terraform_required_version) on line 0: (source code not available) Reference: https://github.com/terraform-linters/tflint/blob/v0.39.3/docs/rules/terraform_required_version.md Notice: module name `private-codecommit-repo` must match the following format: snake_case (terraform_naming_convention) on main.tf line 1: 1: module "private-codecommit-repo" { Reference: https://github.com/terraform-linters/tflint/blob/v0.39.3/docs/rules/terraform_naming_convention.md Warning: Module source "git::ssh://SOMEUSER@git-codecommit.us-east-2.amazonaws.com/v1/repos/private-codecommit-repo" is not pinned (terraform_module_pinned_source) on main.tf line 2: 2: source = "git::ssh://SOMEUSER@git-codecommit.us-east-2.amazonaws.com/v1/repos/private-codecommit-repo" Reference: https://github.com/terraform-linters/tflint/blob/v0.39.3/docs/rules/terraform_module_pinned_source.md Notice: `var1` variable has no description (terraform_documented_variables) on main.tf line 5: 5: variable "var1" { Reference: https://github.com/terraform-linters/tflint/blob/v0.39.3/docs/rules/terraform_documented_variables.md Warning: variable "var1" should be moved from main.tf to variables.tf (terraform_standard_module_structure) on main.tf line 5: 5: variable "var1" { Reference: https://github.com/terraform-linters/tflint/blob/v0.39.3/docs/rules/terraform_standard_module_structure.md Warning: variable "var1" is declared but not used (terraform_unused_declarations) on main.tf line 5: 5: variable "var1" { Reference: https://github.com/terraform-linters/tflint/blob/v0.39.3/docs/rules/terraform_unused_declarations.md Warning: Module should include an empty outputs.tf file (terraform_standard_module_structure) on outputs.tf line 1: (source code not available) Reference: https://github.com/terraform-linters/tflint/blob/v0.39.3/docs/rules/terraform_standard_module_structure.md ```
terraform_tflint with tflint 0.40.0 ```bash ❯ sudo rm /usr/bin/tflint ❯ sudo ln -s /usr/bin/tflint-0.40.0 /usr/bin/tflint ❯ tflint -v TFLint version 0.40.0 + ruleset.terraform (0.1.0-bundled) ❯ pre-commit run -a terraform_tflint Terraform validate with tflint...........................................Failed - hook id: terraform_tflint - exit code: 2 Command 'tflint --init' successfully done: TFLint in private-codecommit-repo/: 3 issue(s) found: Warning: terraform "required_version" attribute is required (terraform_required_version) on line 0: (source code not available) Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.1.0/docs/rules/terraform_required_version.md Warning: Module source "git::ssh://SOMEUSER@git-codecommit.us-east-2.amazonaws.com/v1/repos/private-codecommit-repo" is not pinned (terraform_module_pinned_source) on main.tf line 2: 2: source = "git::ssh://SOMEUSER@git-codecommit.us-east-2.amazonaws.com/v1/repos/private-codecommit-repo" Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.1.0/docs/rules/terraform_module_pinned_source.md Warning: variable "var1" is declared but not used (terraform_unused_declarations) on main.tf line 5: 5: variable "var1" { Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.1.0/docs/rules/terraform_unused_declarations.md ```

Environment information

❯ uname -a
Linux efc04b3806ca 5.10.102.1-microsoft-standard-WSL2 #1 SMP Wed Mar 2 00:30:59 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
GNU bash, version 5.1.16(1)-release (x86_64-pc-linux-gnu)
pre-commit 2.20.0
Terraform v1.2.8
Python 3.10.4
Python 3.10.4
checkov 2.1.204
terraform-docs version v0.16.0 1f686b1 linux/amd64
terragrunt version v0.38.9
terrascan version: v1.15.2
TFLint version 0.40.0
+ ruleset.terraform (0.1.0-bundled)
tfsec v1.27.6
tfupdate 0.6.7
hcledit 0.2.6
file content ```bash ❯ cat .pre-commit-config.yaml repos: - repo: https://github.com/antonbabenko/pre-commit-terraform rev: v1.75.0 hooks: - id: terraform_fmt - id: terraform_wrapper_module_for_each - id: terraform_validate - id: terraform_docs args: - '--args=--lockfile=false' - id: terraform_tflint args: - '--args=--only=terraform_deprecated_interpolation' - '--args=--only=terraform_deprecated_index' - '--args=--only=terraform_unused_declarations' - '--args=--only=terraform_comment_syntax' - '--args=--only=terraform_documented_outputs' - '--args=--only=terraform_documented_variables' - '--args=--only=terraform_typed_variables' - '--args=--only=terraform_module_pinned_source' - '--args=--only=terraform_naming_convention' - '--args=--only=terraform_required_version' - '--args=--only=terraform_required_providers' - '--args=--only=terraform_standard_module_structure' - '--args=--only=terraform_workspace_remote' - repo: https://github.com/pre-commit/pre-commit-hooks rev: v4.3.0 hooks: - id: check-merge-conflict - id: end-of-file-fixer ```
tofupup commented 2 years ago

Feedback from the tflint developers indicates this is a bug in 0.40.0, but don't have a timeline for resolving (they state it's not a quick fix). Recommended workaround is using disabled_by_default setting in .tflint.hcl file.

For pre-commit-terraform, I think this would require some additional code in the terraform_tflint hook to create the needed configuration. Pinning the tflint version recommended would work for now, but depending on how long a patch for this bug takes it may not work long term.

yermulnik commented 2 years ago

@tofupup Thanks for the update.

I think this would require some additional code in the terraform_tflint hook to create the needed configuration.

What would be your rough estimate on the effort to implement this? I'm leaning to the second option to pin max allowed tflint version as a short-term solution to minimize effort and code changes. Maybe even constrain the max version allowed for when tflint hook config has --only in there only, and not restrict 0.40.0 when --only is not used? (not sure this is a good quirk though) 🤔

tofupup commented 2 years ago

What would be your rough estimate on the effort to implement this?

I think a basic pass would take a couple of hours at most. After some testing tonight, we could generate a temporary static .tflint.hcl file that enables the plugin and sets disabled_by_default = true. Any --only= arg would need to be changed to a --enable-rule arg via simple substitution, but otherwise the calling syntax would stay the same.

That said...

I'm leaning to the second option to pin max allowed tflint version as a short-term solution to minimize effort and code changes. Maybe even constrain the max version allowed for when tflint hook config has --only in there only, and not restrict 0.40.0 when --only is not used? (not sure this is a good quirk though) 🤔

I guess it comes down to how much ownership pre-commit-terraform needs to have on the tflint configuration/functionality, since in general the hook is just passing arguments. Even if the --only option is "fixed", in testing for the above it looks like there's at least a couple of other behavior changes. For example, if specifying a config file for tflint, starting at 0.40.0 if the config file doesn't specifically enable the new terraform rules module, your config may not function as it did previously (looks like it just picks up the new recommended group of rules). Or if the --loglevel option was specified, tflint will just error out starting with 0.40.0 (you have to use an env var). It makes it hard to constrain the max version on certain CLI options. And it's not reasonable to own mapping any new/future tflint config/CLI changes.

Looking at the terraform-aws-modules org, all of the .pre-commit-config.yaml configs only use --only options for terraform_tflint. So the above quick fix would help all of these as the new tflint gets pushed out to repositories (looks like 0.40.0 is already the version installed by homebrew). Of course, all of these modules lock to a specific pre-commit-terraform rev that won't have any check/notice in the hook.

Part of the problem is using --only doesn't give you any notice it's not doing what you expect, you just get less/different rules running than expected. I'm a little less concerned with something like --loglevel since it's throws an error.

Maybe just an added section to the README that lets people know a change has happened to tflint options, to downgrade or how to fix the options, and a version warning in the hook to double check behavior if any args are set and the version is over 0.40.0? Since it seems like the most likely case, where an older version of the hook is used with a new local install of tflint (unless the container is used), maybe this is the best that can be done.

Sorry, kind of rambling as I don't know what the right path is. I ran into this issue while running pre-commit-terraform against some work code, and losing an hour figuring out why I was getting less tflint rules triggered, and I assume others will run into the same.

yermulnik commented 2 years ago

I guess it comes down to how much ownership pre-commit-terraform needs to have on the tflint configuration/functionality, since in general the hook is just passing arguments.

Yep, I hold the same view of «the hook is just passing arguments», so we should keep as minimal intrusion as possible. The main goal of pre-commit is to identify simple issues before submitting code for review and hence to be an intermediate in between commit hook and the tools used to lint/improve code as opposite to interfere and change user defined tool configuration implicitly/silently. It's up to user to look after their hook configs while it's up to us to indicate and flag changes which may lead to incorrect functionality. Hence I full support what you wrote in the paragraph quoted below:

Maybe just an added section to the README that lets people know a change has happened to tflint options, to downgrade or how to fix the options, and a version warning in the hook to double check behavior if any args are set and the version is over 0.40.0? Since it seems like the most likely case, where an older version of the hook is used with a new local install of tflint (unless the container is used), maybe this is the best that can be done.

Also would be keen what @antonbabenko's and @MaxymVlasov's thoughts on this are.

antonbabenko commented 2 years ago

First, I highly appreciate the detailed input from everyone!

I think that since we are not making a lot of assumptions and modifications of the arguments passed to tflint, we should do little to no modifications in the hook but leave a note that version 0.40.0 is buggy and ask users to update to 0.40.1 (once https://github.com/terraform-linters/tflint/pull/1514 is merged).

yermulnik commented 2 years ago

FYI: https://github.com/terraform-linters/tflint-ruleset-terraform/releases/tag/v0.1.1 + https://github.com/terraform-linters/tflint/releases/tag/v0.40.1