test(terraform): test to demonstrate 'count' meta argument incorrectl… #8479
Conversation
|
@nikpivkin and @simar7, what are your thoughts on this bug? The unit test is a good example and self description. It fails on the latest The test only passes because I added in I don't have a good solution for a more robust fix. |
|
Hi @Emyrk ! Thanks. I'll look into it. |
|
@nikpivkin any updates on your thoughts? |
29a795b to
6b58370
Compare
| // A pointer for this module is needed up front to correctly set the module parent hierarchy. | ||
| // The actual instance is created at the end, when all terraform blocks | ||
| // are evaluated. | ||
| rootModule := new(terraform.Module) | ||
|
|
||
| submodules := e.evaluateSubmodules(ctx, rootModule, fsMap) | ||
|
|
||
| e.logger.Debug("Starting post-submodules evaluation...") | ||
| e.evaluateSteps() | ||
|
|
||
| e.logger.Debug("Module evaluation complete.") | ||
| // terraform.NewModule must be called at the end, as `e.blocks` can be | ||
| // changed up until the last moment. | ||
| *rootModule = *terraform.NewModule(e.projectRootPath, e.modulePath, e.blocks, e.ignores) |
There was a problem hiding this comment.
This feels really unfortunate 😢
| // Always attempt to expand any blocks that might now be expandable | ||
| // due to new context being set. | ||
| e.blocks = e.expandBlocks(e.blocks) |
There was a problem hiding this comment.
This is the biggest change. It allows expandBlocks to occur in evaluateSteps.
From my understanding of expandBlocks this should be a safe operation. Is there a reason it is currently called outside this context? @nikpivkin ?
Do not expand unknown `count` blocks. Run `expandBlocks` in eval to allow submodule returns to affect the `count` when using module outputs.
The depth of submodule evalutation is still limited
Module counts are incorrectly being handled
80e01d1 to
6eb6063
Compare
| func TestBlockCountModules(t *testing.T) { | ||
| t.Skip( | ||
| "This test is currently failing. " + | ||
| "The count passed to `module bar` is not being set correctly. " + | ||
| "The count value is sourced from the output of `module foo`. " + | ||
| "Submodules cannot be dependent on the output of other submodules right now. ", | ||
| ) | ||
| // `count` meta attributes are incorrectly handled when referencing | ||
| // a module output. | ||
| files := map[string]string{ | ||
| "main.tf": ` | ||
| module "foo" { | ||
| source = "./modules/foo" | ||
| } | ||
| module "bar" { | ||
| source = "./modules/foo" | ||
| count = module.foo.staticZero | ||
| } | ||
| `, | ||
| "modules/foo/main.tf": ` | ||
| output "staticZero" { | ||
| value = 0 | ||
| } | ||
| `, | ||
| } | ||
|
|
||
| modules := parse(t, files) | ||
| require.Len(t, modules, 2) | ||
| } |
There was a problem hiding this comment.
Does evaluating sub-modules also need to occur in the inner loop? Defering them if their count/for_each is unknown?
|
@nikpivkin I updated this PR against the latest main. It solves an issue using It does not solve submodules dependent on each other 😢. |
|
@nikpivkin and @simar7 sorry to keep pinging you on this. Just want to make sure it does not slip through the cracks |
Description
count&for_eachmeta argument is incorrectly handled when referencing a submodule's output. The expansion arguments only work when referencing within the same scope.This is being caused because
expandBlockCountsdefaults to acount = 1for unknown values.ExpandingBlocks happens before submodule evaluation:
https://github.com/aquasecurity/trivy/blob/main/pkg/iac/scanners/terraform/parser/evaluator.go#L139-L142
So expanded blocks cannot reference submodule exports. This is inc 8000 onsistent with
terraform plan/applywhich renders 0datablocks in this unit test.The solution presented allows
expandBlocksto be called duringevaluateSteps, which occurs again after submodules are expanded.There is still an issue if the
count/for_eachargument is used in a module block, meaning a submodule depends on another module output. Since submodule evaluation will not happen again. See the testTestBlockCountModules. It is skipped for now.Checklist