fix(terraform): 'count' meta argument sourced from submodule output #8899

Emyrk · 2025-05-21T15:16:49Z

Description

count & for_each meta 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 expandBlockCounts defaults to a count = 1 for 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 inconsistent with terraform plan/apply which renders 0 data blocks in this unit test.

The solution presented allows expandBlocks to be called during evaluateSteps, which occurs again after submodules are expanded.

There is still an issue if the count/for_each argument is used in a module block, meaning a submodule depends on another module output. Since submodule evaluation will not happen again. See the test TestBlockCountModules. It is skipped for now.

Checklist

I've read the guidelines for contributing to this repository.
I've followed the conventions in the PR title.
I've added tests that prove my fix is effective or that my feature works.
I've updated the documentation with the relevant information (if needed).
I've added usage information (if the PR introduces new options)
I've included a "before" and "after" example to the description (if the PR is a user interface change).

Note: I updated this PR to the latest main, and moved the fork origin to a new organization. I was previously using my personal github org.

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

…put_count

Emyrk · 2025-05-21T15:19:11Z

pkg/iac/scanners/terraform/parser/parser_test.go

+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)
+}


Does evaluating sub-modules also need to occur in the inner loop? Defering them if their count/for_each is unknown?

in the inner loop

What do you mean?

I mean evaluateSteps when I referred to the "inner loop".

Right now the procedure is:

evaluateSteps

expandBlocks x2

evaluateSubmodules

evaluateSteps

The test case I referenced here has the count for the submodule only being available after step 3. So evaluateSubmodules needs to occur again.

But you could imagine a nested submodule with depth >1, so it needs to be done in the loop with maxContextIterations

All of this is related to your graph suggestion.

Emyrk · 2025-05-21T15:19:15Z

pkg/iac/scanners/terraform/parser/evaluator.go

+		// Always attempt to expand any blocks that might now be expandable
+		// due to new context being set.
+		e.blocks = e.expandBlocks(e.blocks)


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 ?

I think this just wasn't thought of in the initial development of the evaluator. I want to test this on a repository with a large number of modules and check the performance.

@nikpivkin let me know if I can do anything to support you 👍

nikpivkin · 2025-05-23T07:38:23Z

@simar7 The Terraform configuration evaluation logic is getting more and more complicated and confusing. What if we apply graph theory to solve this problem, as done in terraform or in infracost? To do this, we would need to walk through all the blocks, extract references from expressions, and populate the graph. Then apply topological sorting to get the traversal order and after that iterate over the blocks, expand the blocks and evaluate the values. Wdyt?

simar7 · 2025-05-24T00:23:18Z

@simar7 The Terraform configuration evaluation logic is getting more and more complicated and confusing. What if we apply graph theory to solve this problem, as done in terraform or in infracost? To do this, we would need to walk through all the blocks, extract references from expressions, and populate the graph. Then apply topological sorting to get the traversal order and after that iterate over the blocks, expand the blocks and evaluate the values. Wdyt?

Yes I agree in principle but at this time, we have no set priorities to do so. I suggest we start an issue and document such shortfalls as part of it so we can start tracking the importance of having a graph based engine.

nikpivkin · 2025-05-24T14:59:18Z

pkg/iac/scanners/terraform/parser/evaluator.go

+		if !forEachVal.IsKnown() {
+			// Defer the expansion of the block if it is unknown. It might be known at a later
+			// execution step.
+			forEachFiltered = append(forEachFiltered, block)


Need to check a case where for_each/count refers to an unknown value, such as a data source. In such a case one instance of the block will be created?

For count, the previous behavior was to assume count = 1. So this would change the count index value from 1 -> unknown and leave the block unexpanded.

For for_each, the previous behavior was to remove the block and log a debug message that it failed to expand. The new behavior is to leave the block with the index as the unknown.

In both cases, the new behavior has the key being unknown and I would imagine references to the block using a key would break. This feels correct though?

The count one assuming count = 1 feels incorrect. And the for_each just leaves the remaining block.

To get the same behavior as before, a last pass would have to be done to either remove the blocks or set count to 1 once the eval is done.

Emyrk · 2025-06-03T14:50:52Z

@nikpivkin this is ready again for review

Emyrk added 13 commits April 9, 2025 10:00

fix(terraform): expand count blocks can depend on submodule returns

4f7ff4c

Do not expand unknown `count` blocks. Run `expandBlocks` in eval to allow submodule returns to affect the `count` when using module outputs.

update unit test

cd4d4b3

test(terraform): add counter example test

fc4b5f5

The depth of submodule evalutation is still limited

fixup test

9b757cc

test(terraform): add failing unit test with ambition to resolve

c6831f9

Module counts are incorrectly being handled

chore: check for unknown and null to match upstream change

6eb6063

Merge remote-tracking branch 'aqua/main' into stevenmasley/module_out…

1e0cfa6

…put_count

chore: fix expand for_each using module output

073fdef

test rename

f0b293a

update skip comment

86b3b93

linting

36938e3

Merge remote-tracking branch 'aqua/main' into stevenmasley/module_out…

6b67628

…put_count

chore: leave null counts as 1, not sure what to do with them

a979854

Emyrk requested review from simar7 and nikpivkin as code owners May 21, 2025 15:16

Emyrk commented May 21, 2025

View reviewed changes

nikpivkin reviewed May 24, 2025

View reviewed changes

Emyrk mentioned this pull request May 27, 2025

Dynamic parameters coder/trivy#2

Draft

Emyrk requested a review from nikpivkin May 29, 2025 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(terraform): 'count' meta argument sourced from submodule output #8899

fix(terraform): 'count' meta argument sourced from submodule output #8899

Uh oh!

Emyrk commented May 21, 2025 •

edited

Loading

Uh oh!

Emyrk May 21, 2025

Uh oh!

nikpivkin May 24, 2025

Uh oh!

Emyrk May 27, 2025

Uh oh!

Emyrk May 21, 2025

Uh oh!

nikpivkin May 24, 2025

Uh oh!

Emyrk May 27, 2025

Uh oh!

nikpivkin commented May 23, 2025

Uh oh!

simar7 commented May 24, 2025

Uh oh!

nikpivkin May 24, 2025

Uh oh!

Emyrk May 27, 2025

Uh oh!

Emyrk commented Jun 3, 2025

Uh oh!

Uh oh!

fix(terraform): 'count' meta argument sourced from submodule output #8899

Are you sure you want to change the base?

fix(terraform): 'count' meta argument sourced from submodule output #8899

Uh oh!

Conversation

Emyrk commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikpivkin commented May 23, 2025

Uh oh!

simar7 commented May 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emyrk commented Jun 3, 2025

Uh oh!

Uh oh!

Emyrk commented May 21, 2025 •

edited

Loading