terraform-linters / tflint

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

Errors raised for ignored modules when not installed #1362

Closed tspearconquest closed 2 years ago

tspearconquest commented 2 years ago

We have some privately hosted modules we wrote and included them in other privately hosted modules. For the child modules which have no children of their own, when tflint is run from their repo, no issues are found.

We run tflint in a pre-commit hook like so:

- repo: https://github.com/gruntwork-io/pre-commit
  rev: v0.1.17 # Get the latest from: https://github.com/gruntwork-io/pre-commit/releases
  hooks:
  - id: tflint
    args:
    - "--module"
    - "--config=.tflint.hcl"

This calls tflint on every commit with module inspection enabled. This works fine because locally we have run terraform init for testing our code.

However, we run into problems when our CI runs tflint with module inspection enabled if we have included a private module.

The docker image does not have a terraform command to run terraform init against, and we don't commit the .terraform directory. Therefore the repo is uninitialized when the tflint docker image runs, and so it gives us the below error for our private modules:

Failed to load configurations; modules.tf:1,1-29: `diagnostic_settings` module is not found. Did you run `terraform init`?; :
Error: `diagnostic_settings` module is not found. Did you run `terraform init`?
  on modules.tf line 1, in module "diagnostic_settings":
   1: module "diagnostic_settings" {

We want to have module inspection enabled for scanning modules from public repos, but not the internal modules since they are scanned by tflint on their respective repos, so we created a .tflint.hcl file which was intended to cause tflint to ignore the private modules and only scan for the public ones and we placed it in the repo. You can see it below:

# Ignore the private modules which are scanned in their own pipelines and can't be accessed by the tflint docker image
config {
  ignore_module = {
    "gitlab.com/private/diagnostic_settings_repo" = true
  }
}

# custom terraform rules to be enforced
plugin "azurerm" {
  enabled = true
  version = "0.15.0"
  source  = "github.com/terraform-linters/tflint-ruleset-azurerm"
}

rule "terraform_module_pinned_source" {
  enabled = false
}

Below here you can see in the output from our CI that the .tflint.hcl is properly loaded, but for some reason the ignore rule is not being respected:

$ cd ${TF_ROOT}
$ ls -al
total 84
drwxrwxrwx    5 root     root          4096 Apr 23 22:58 .
drwxrwxrwx    4 root     root          4096 Apr 23 22:58 ..
drwxrwxrwx    6 root     root          4096 Apr 23 22:58 .git
-rw-rw-rw-    1 root     root          2374 Apr 23 22:58 .gitignore
-rw-rw-rw-    1 root     root           192 Apr 23 22:58 .gitlab-ci.yml
-rw-rw-rw-    1 root     root           849 Apr 23 22:58 .kics-config.yaml
-rw-rw-rw-    1 root     root          1482 Apr 23 22:58 .pre-commit-config.yaml
-rw-rw-rw-    1 root     root           454 Apr 23 22:58 .tflint.hcl
-rw-rw-rw-    1 root     root          5368 Apr 23 22:58 README.md
-rw-rw-rw-    1 root     root           124 Apr 23 22:58 _typos.toml
drwxrwxrwx    3 root     root          4096 Apr 23 22:58 examples
-rw-rw-rw-    1 root     root           983 Apr 23 22:58 locals.tf
-rw-rw-rw-    1 root     root          2926 Apr 23 22:58 main.tf
-rw-rw-rw-    1 root     root          1332 Apr 23 22:58 modules.tf
-rwxrwxrwx    1 root     root           118 Apr 23 22:58 pre-commit-init.sh
drwxrwxrwx    2 root     root          4096 Apr 23 22:58 tests
-rw-rw-rw-    1 root     root          3326 Apr 23 22:58 variables.tf
-rw-rw-rw-    1 root     root           158 Apr 23 22:58 versions.tf
$ tflint --init
Installing `azurerm` plugin...
Installed `azurerm` (source: github.com/terraform-linters/tflint-ruleset-azurerm, version: 0.15.0)
$ tflint --module --loglevel=info -f junit | tee tflint.test.xml
22:58:52 config.go:110: [INFO] Load config: .tflint.hcl
22:58:52 loader.go:57: [INFO] Initialize new loader
22:58:52 loader.go:82: [INFO] Load configurations under .
22:58:52 loader.go:97: [INFO] Module inspection is enabled. Building a root module with children...
22:58:52 loader.go:104: [ERROR] Failed to load modules: modules.tf:1,1-29: `diagnostic_settings` module is not found. Did you run `terraform init`?; 
<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite tests="0" failures="0" time="0" name="">
    <properties></properties>
  </testsuite>
</testsuites>Failed to load configurations; modules.tf:1,1-29: `diagnostic_settings` module is not found. Did you run `terraform init`?; :
Error: `diagnostic_settings` module is not found. Did you run `terraform init`?
  on modules.tf line 1, in module "diagnostic_settings":
   1: module "diagnostic_settings" {

So I went to test this out further locally. I deleted my .terraform directory and ran tflint manually specifying the --ignore-module flag and observed the same issue happening.

$ tflint --module --ignore-module=gitlab.com/private/diagnostic_settings_repo --loglevel=info -f junit | tee tflint.test.xml
23:09:30 config.go:110: [INFO] Load config: .tflint.hcl
23:09:30 loader.go:57: [INFO] Initialize new loader
23:09:30 loader.go:82: [INFO] Load configurations under .
23:09:30 loader.go:97: [INFO] Module inspection is enabled. Building a root module with children...
23:09:30 loader.go:104: [ERROR] Failed to load modules: modules.tf:1,1-29: `diagnostic_settings` module is not found. Did you run `terraform init`?; 
<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite tests="0" failures="0" time="0" name="">
    <properties></properties>
  </testsuite>
</testsuites>Failed to load configurations; modules.tf:1,1-29: `diagnostic_settings` module is not found. Did you run `terraform init`?; :
Error: `diagnostic_settings` module is not found. Did you run `terraform init`?
  on modules.tf line 1, in module "diagnostic_settings":
   1: module "diagnostic_settings" {

The documentation indicates to provide the source for the module and I provided the exact same path we used on the source line in the module configuration, so I don't understand why this isn't working.

Any insight will be greatly appreciated.

Version

TFLint version 0.35.0
+ ruleset.azurerm (0.15.0)
bendrucker commented 2 years ago

From a quick scan of the code, this setting does not eliminate the requirement to run terraform init when using module inspection. It prevents the evaluation of the child modules—they're still loaded.

We want to have module inspection enabled for scanning modules from public repos, but not the internal modules since they are scanned by tflint on their respective repos

This is not the intended use of the module inspection feature. Module inspection is designed to catch things like a module that provides an instance_type var and uses it in an aws_instance resource internally. In other words, usage errors.

It is not designed to provide style enforcement. Generic Terraform syntax rules include a condition that skips over any non-root module:

https://github.com/terraform-linters/tflint/blob/9f80183297b8b2b7f62800502bf236fbfc0e607e/rules/terraformrules/terraform_comment_syntax.go#L42-L45

tspearconquest commented 2 years ago

Hi, I am a bit perplexed by the explanation above. Can you provide me some sample code for what you described that this would flag, and why will the module still be loaded if we are specifying to ignore the module?

If this is a case of we're using tflint incorrectly, please help me understand how to get the tflint run to pass when we have a module defined in our code. Should we avoid using module inspection?

Thanks for this info so far :)

bendrucker commented 2 years ago

This is pretty well spelled out in the docs:

https://github.com/terraform-linters/tflint/blob/master/docs/user-guide/module-inspection.md

Module inspection is designed to check the arguments to modules, not the contents of the modules themselves.

Issues must be associated with a variable that was passed to the module. If an issue within a child module is detected in an expression that does not reference a variable (var), it will be discarded.

If this is the behavior you're after, keep using module inspection. If not, you may not be getting anything from it and could just as easily turn it off.

Can you provide me some sample code for what you described that this would flag, and why will the module still be loaded if we are specifying to ignore the module?

Feel free to try to dig into the code if you'd like to understand how this actually works. If you start here you can see what ignores do:

https://github.com/terraform-linters/tflint/blob/3d4cb421d57458ee376dd3eb02a279cbb828d492/cmd/option.go#L18

Versus loading config:

https://github.com/terraform-linters/tflint/blob/master/tflint/loader.go

We'd probably accept a PR skipping over loading ignored modules but that is not the current behavior. However, it doesn't seem like the "private modules" distinction is really relevant here. If you don't run terraform init, all non-local modules will fail to load. If you don't want to run init, you probably don't want module inspection. Having module (attribute) inspection for just local modules doesn't really make sense.

tspearconquest commented 2 years ago

Thanks, I understand now.

You're right about the private modules distinction, it is irrelevant. However I want to clarify that the modules we're referencing aren't local, they are hosted in our gitlab private repo. The issue is not a matter of not wanting to run terraform init, its actually that we can't run it because we're running the tflint-bundle container in our CI pipeline which doesn't have a terraform binary in it, so there's no way to run terraform init before we try to run tflint in this setup.

Hopefully that clears things up. If tflint expects for modules to be installed even when they are ignored, then we need a terraform binary in the image so they can be installed during a pipeline, or tflint should skip loading ignored modules when module inspection is enabled, or we need to accept that we should disable module inspection for containerized tflint checks.

We would like to use module inspection, and would rather not download a terraform binary into the container during the pipeline, for what it's worth. However, it seems for the moment we should probably disable module inspection until one of the above other 2 scenarios happens.

Please let me know if I missed anything.

bendrucker commented 2 years ago

Right, when I say non-local I basically mean anything that's not this:

source = "./path/to/local/module"

This would also be non-local, using the default registry.terraform.io host:

module "vpc" {
  source = "terraform-aws-modules/vpc/aws"
}

Or you can use a custom/private registry host, e.g. Terraform, GitLab, etc.

So while skipping loading for ignored modules would be a valid feature addition, it doesn't seem like the ideal answer to your situation. You'd be left to ignore every module source that is not a relative path.

Instead, if you have something like this:

# main.tf

module "mod" {
  source = "./modules/my-mod"
}

What you'd really want is to recursively visit each Terraform module directory and run TFLint there. This will enforce the terraform_* rules, whereas module inspection will not.

Module inspection intentionally makes no distinctions on the type of module source. The goal of module inspection is to validate that the module is being called correctly and not to enforce rules on the content of the module itself.

tspearconquest commented 2 years ago

Right, so we don't have any local modules we use. All of our modules are hosted in Gitlab's terraform registry and if a module needs to create an external resource for something, the module will call another module for that resource as described below.

Each module is in its own independent repo, there is no sharing between them or usage of local modules when the pipeline runs. We build and publish the modules to the registry, and if we need to reference a different module, we call it from the registry.

So, for example our kubernetes cluster module call looks like this:

module "kubernetes" {
  source  = "gitlab.com/private/kubernetes_repo"
  version = "0.0.17"
}

And inside the kubernetes module is a call to another module we created called diagnostic_settings.

module "diagnostic_settings" {
  source  = "gitlab.com/private/diagnostic_settings_repo"
  version = "0.0.3"
}

These 2 modules are in separate projects in gitlab. When we run the pipeline to create the kubernetes module, tflint runs in a job by itself, and Gitlab does a shallow clone of the repo into a writable directory in the container. We also have jobs that run some other pre-publish checks, and then a job to build the module and a job to publish it. These jobs use artifacts to pass data between them.

We run tflint before any build steps happen, and that seems to be the source of the problem for us. There is no .terraform directory because it doesn't belong in the repo, and while the module will ultimately have a .terraform directory created by the build pulling in the diagnostic_settings module, it's not present when we run tflint.

Based on everything I'm hearing, it seems like turning off module inspection is the right course. It seems to not be meant for this use case.

Thanks for your patience and helping me figure this out. I really appreciate it.

bendrucker commented 2 years ago

Indeed, you'd benefit from module inspection if the modules were installed. If not, go ahead and turn it off.

Based on the discussion it's not clear there's a use case for skipping loading of modules but if someone in the future is interested in tackling a PR we'd accept the change.