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? |
Emyrk
force-pushed
the
stevenmasley/module_output_count
branch
from
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 😢
| if countAttrVal.IsNull() { | ||
| // Defer to the next pass when the count might be known | ||
| countFiltered = append(countFiltered, block) | ||
| continue | ||
| } | ||
|
|
||
| count := 1 |
There was a problem hiding this comment.
Defering expansion when count is unknown.
| // 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 ?
| func TestBlockCountModules(t *testing.T) { | ||
| t.Skip("This test is currently failing, the 'count = 0' module 'bar' is still loaded") | ||
| // `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 is unknown?
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
Emyrk
force-pushed
the
stevenmasley/module_output_count
branch
from
80e01d1 to
6eb6063
Compare
Description
countmeta argument is incorrectly handled when referencing a submodule's output. It does work when referencing within the same scope.This is being caused because
expandBlockCountsdefaults to acount = 1for unknown values. Secondly, unknown values are returned ascty.Nil, so that information is not easily available at this step.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 inconsistent with
terraform plan/applywhich renders 0datablocks in this unit test.I feel a bit iffy trying to solve this without consulting @simar7 and @nikpivkin. My quick solution is presented, however I admit it was done rather quickly and is not correct.
Checklist