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 #502 (comment)

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.