Plan file inspection

[wata727]

This idea is to inspect configuration based on the passed plan file. If a plan file can be used, deeper inspection such as name duplication checking becomes possible.

A parser for the plan file is in Terraform itself, so we can reuse it.

[richardjennings]

👍 I would be interested in working on this

[wata727]

@richardjennings Sorry for the late reply. Are you still interested in this? If so, I'd love to help. Ask anything if you have a question.

[richardjennings]

@wata727 yes - I have started having a look today.

[richardjennings]

@wata727 After looking at this a bit - I think the best option is a Loader implementation that reads the plan zip file contents supplementing variable definitions with any explicitly specified in the plan.

[wata727]

I think so too. It's probably a good idea to pass the plan object to Runner so that each rule can use it.

[richardjennings]

@wata727 - Given the delay in showing any feedback I have pushed a 'draft' which is not a finished article.

https://github.com/richardjennings/tflint/pull/1/commits/b242846879efde2470e228b57f127cf7da15ed5b

I have added multi-module functionality so that 'runners' can be applied to all modules / usage defined in the plan.

This essentially means a one hit like tflint --plan=tflint/test-fixtures/plans/a.plan is intended to lint all the Terraform config code in the plan, making this a potentially ideal candidate for a pre-apply step in a CI/CD pipeline.

Some shoe-horning, no tests as yet and I have not reconciled new behaviours with all of the configuration options.

I appreciate that I have not included the plan as an object Runners can utilise directly. I can see this could provide quite substantial capabilities such as pre-apply 'Security Hub' type auditing.

Thoughts?

[wata727]

Interesting, but the pattern that generates runners from the plan file may not be able to handle additional data, such as the change sets provided by the plan.

For example, if the resource added by terraform apply is known from the plan file, it is possible to check whether the unique ID of the resource already exists in the cloud.

I believe that the runner should be extended to handle plans.Plan objects to allow such inspections. What do you think?

[richardjennings]

@wata727 The first focus here for me was adding compatibility with alternative usages of Terraform; specifically Terragrunt. Terragrunt configuration is not understood by TFLint, but the resulting plans that are generated using terragrunt plan(-all) are now which I think is very useful.

I totally agree that adding a Plan object to runners opens a door to a lot of utility. I am tempted to suggest that would be best placed as a follow up PR. Would you accept a PR with the current behaviour change refined/tested ?

[wata727]

I agree that it is worthwhile to provide an analysis based on the plan file when considering another Terraform usage, such as Terragrunt.

One concern is that the plan-based analysis may not be enough to inspect modules. The current module inspection considers the module's arguments. For instance, only expressions affected by module arguments are inspected. See https://github.com/terraform-linters/tflint/issues/502#issuecomment-546701798

Unfortunately, I couldn't test the example implementation in detail because it caused a panic in my plan file, but it probably has this issue.

[richardjennings]

@wata727 There is an example a.plan in test-fixtures/plans which I think demonstrates the issue with module arguments is not applicable, 2 instance_type lint errors are output for the test module referenced from the root module without any arguments.

This is because all modules are treated as root modules where as the current code base only walks into modules via variables - i.e I believe it is something I considered. Could you have a look at a.plan ?

[wata727]

I checked the example of a.plan, but I think that in this example, the issue of "test" module should not be reported.

The problem is that there is no way to properly ignore issues from third-party modules when all modules are treated as root.

[richardjennings]

@wata727 I would see scanning 3rd party modules in the plan as a useful feature - mostly for security reasons. A module blacklist could work to optionally ignore?

[wata727]

Umm... I think it depends on the situation. For example, it would be nonsense to be unable to use third-party modules due to naming convention violations.

There are concerns that some behavior is different from normal inspection and may cause confusion, but I think it is not a bad idea to release it as an experimental feature.

[richardjennings]

@wata727 Apologies for the delayed response.

The issue I ran into is that the Plan file records TF version info which must match the version of TF imported by TFLint.

I was not able to come up with a solution to this constraint - other than having builds of TFLint tracking Terrafrom releases.

[bendrucker]

Take a look at the JSON output format:

https://www.terraform.io/docs/internals/json-format.html#plan-representation

IMO we should not be doing any parsing of Terraform binary output files. Terraform rightly doesn't provide any cross-version compatibility for those, which creates the constraint you've identified.

Possibly relevant, some Terratest PRs have been open for this for a while:

https://github.com/gruntwork-io/terratest/pull/484

[richardjennings]

@bendrucker looks like a workable approach. It is a bit of a shame really as a self-contained bundle be it binary or otherwise is ideal in my mind as a separation of concerns. I believe a JSON representation has a dependency on the original src where a plan zip is self-contained. I'll take my name off of 'working on this'. Cheers.

[wyardley]

Would this also support detecting issues when the resource is created by a module, or is that a separate issue / enhancement request?

i.e., if I have a module which creates a resource, and pass in the value to the resource via the module, currently, tflint won't follow the chain to see whether the resource parameter / attribute is valid.

[bendrucker]

Would this also support detecting issues when the resource is created by a module, or is that a separate issue / enhancement request?

Separate, unrelated issue.

if I have a module which creates a resource, and pass in the value to the resource via the module, currently, tflint won't follow the chain to see whether the resource parameter / attribute is valid.

It does do this currently, see the --modules flag:

https://github.com/terraform-linters/tflint/blob/master/docs/guides/advanced.md#module-inspection

Rules run on the child modules and when issues are emitted, the relevant expression is checked for variable references and TFLint re-emits issues referencing the attribute as specified in the calling module.

If you don't think that's working as documented, please open another issue with example code.

[bendrucker]

Plan file inspection could make some unknown values into known ones. Particularly any attribute coming from a data source or via existing state.

[wyardley]

if I have a module which creates a resource, and pass in the value to the resource via the module, currently, tflint won't follow the chain to see whether the resource parameter / attribute is valid.

It does do this currently, see the --modules flag:

https://github.com/terraform-linters/tflint/blob/master/docs/guides/advanced.md#module-inspection

Thanks - I hadn't found the more detailed docs, just the info in the CLI help output and the main README. My use case is, I think, slightly different, so I can open a question issue to see if what I'm doing is supposed to work.

[bendrucker]

Based on the discussion in https://github.com/terraform-linters/tflint/issues/937, TFLint is not going to move forward with plan file inspection. There's a lot of work to depend on Terraform's CLI through officially supported libraries rather than using the now internalized Terraform packages directly. Plan-oriented rules are an entirely different problem/tool than config-oriented ones and increasing the scope of TFLint to take on plans is something we don't have the time to do.

For policy enforcement, tools like Sentinel and Open Policy Agent offer the ability to author rules against JSON plan output.

TFLint is going to focus on static analysis of the configuration itself and the style and usage suggestions that can be made without access to state. Like all static analysis it will have limitations when it comes to dynamic behavior but it will continue to run in any environment and be suitable for editor integration.