fix(terraform): `evaluateStep` to correctly set `EvalContext` for multiple instances of blocks #8555

Emyrk · 2025-03-14T23:50:29Z

Before this change, all data blocks set their values in the EvalContext with the key b.Labels()[0]. If the block is an instance of a block (via count param), the label looks like <name>[<idx>].

This is different to how expandBlockCounts handles this.

https://github.com/aquasecurity/trivy/blob/main/pkg/iac/scanners/terraform/parser/evaluator.go#L422-L422

expandBlockCounts handles this correctly, as an hcl.TraverseIndex expects a tuple (assuming the key is a number).

This PR updates evaluateStep to override the <name> in the EvalContext with a new tuple containing every instance of the data block that is found.

TODO:

This same fix probably needs to be applied for for_each and maybe dynamic blocks.

Testing when count = 0 likely also needs to be tested. I think this is already ok, because if count = 0, then expandBlockCounts will not set the EvalContext. And the block is omitted from the list with any key index. Worst case, the key isNull and it is saved under the name without a tuple. So the context has a value, but not a tuple type, so traversals will still fail.

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).

Referencing a block that uses `count`, and accessing it's arguments returns a nil value.

Instances of a block were being saved as "<name>[<idx>] = val" to the eval context. `expandBlocks` correctly saves it as a tuple "<name> = [val, ...]"

Emyrk · 2025-03-14T23:57:53Z

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

+			if rawKey.Type().Equals(cty.Number) {
+				existing := valueMap[ref.NameLabel()]
+				asInt, _ := rawKey.AsBigFloat().Int64()
+
+				valueMap[ref.NameLabel()] = insertTupleElement(existing, int(asInt), b.Values())
+			}
+
+			// The default behavior, that being no key or an unimplemented key type, is to
+			// save the block as the name value. The name label value contains the key.
+			//
+			// Eg: If the block has the index '4', the string [4] is contained
+			//     within 'b.Labels()[1]'
+			// TODO: This is always done to maintain backwards compatibility.
+			//       I think this can be omitted if the switch statement above
+			//      sets the appropriate tuple value in the valueMap.
 			valueMap[b.Labels()[1]] = b.Values()


I really think if this if statement branch is executed, we should not do valueMap[b.Labels()[1]] = b.Values().

In an abundance of caution, I kept the previous context setting as well. The previous context is incorrect for an hcl traversal though. So I feel like it was never being hit.

How the code would look if we remove the current behavior entirely.

if rawKey.Type().Equals(cty.Number) { existing := valueMap[ref.NameLabel()] asInt, _ := rawKey.AsBigFloat().Int64() valueMap[ref.NameLabel()] = insertTupleElement(existing, int(asInt), b.Values()) } else { valueMap[b.Labels()[1]] = b.Values() }

I think the old behavior should be removed after adding the handling of all cases.

@nikpivkin by "all the cases", do you mean handling for_each and possibly dynamic cases as well? (TBD if dynamic needs any changes, I am unsure off the top of my head)

nikpivkin · 2025-03-19T12:41:13Z

Hi @Emyrk !

Indeed, I was not even aware of this problem. Now the last part of the path when inserting values into a context contains an index, which is incorrect. Your approach looks good, but this problem applies not only to data blocks, but to all types of blocks with for_each and count meta-arguments. When using for_each, instances of the block should be exported as an object, and the index can be a number, so we should distinguish between these cases.

If you don't mind I can change this PR to add support for the rest of the cases.

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

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

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

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

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

Emyrk · 2025-03-19T15:42:19Z

Indeed, I was not even aware of this problem. Now the last part of the path when inserting values into a context contains an index, which is incorrect. Your approach looks good, but this problem applies not only to data blocks, but to all types of blocks with for_each and count meta-arguments. When using for_each, instances of the block should be exported as an object, and the index can be a number, so we should distinguish between these cases.

If you don't mind I can change this PR to add support for the rest of the cases.

Yes! I was aware this problem is larger than my fix. I'm still getting my footing/bearings in this codebase, so just wanted to tackle this in the smallest incremental improvement I could contribute.

Feel free to contribute to my branch directly if you want to fix it all in 1 go. We could also solve the for_each in a second PR. I will defer to your judgement.

Signed-off-by: nikpivkin <[email protected]>

…for_each Signed-off-by: nikpivkin <[email protected]>

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

Emyrk · 2025-03-20T18:31:54Z

@nikpivkin This also exists with module outputs:

https://github.com/aquasecurity/trivy/blob/main/pkg/iac/scanners/terraform/parser/evaluator.go#L245

nikpivkin · 2025-03-20T18:36:51Z

@Emyrk Yes, I will make the fixes for the modules in the other PR as it requires a slightly different approach. I already have a draft.

Emyrk · 2025-03-20T19:09:22Z

@Emyrk Yes, I will make the fixes for the modules in the other PR as it requires a slightly different approach. I already have a draft.

Whoops! @nikpivkin I will revert my test and change, and keep this PR to what it was before my comment.

This reverts commit f96d147.

…uts" This reverts commit bac0ac4.

nikpivkin · 2025-03-20T19:17:24Z

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

 	// lastState needs to be captured after applying outputs – so that they
 	// don't get treated as changes – but before running post-submodule
 	// evaluation, so that changes from that can trigger re-evaluations of
 	// the submodule if/when they feed back into inputs.
-	e.ctx.Set(outputs, "module", sm.definition.Name)
+	ref := sm.definition.Definition.Reference()
+	e.ctx.Set(blockInstanceValues(sm.definition.Definition, valueMap, outputs), "module", ref.NameLabel())


My draft has exactly the same implementation :)

nikpivkin · 2025-03-21T06:06:28Z

@simar7 Can you take a look?

simar7 · 2025-04-02T02:01:42Z

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

+	}
+
+	for _, tt := range tests {
+		tt := tt


capturing loop variable is no longer required https://go.dev/blog/loopvar-preview

@Emyrk can we remove this then?

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

simar7 · 2025-04-02T03:38:38Z

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

+		idx, _ := key.AsBigFloat().Int64()
+		return insertTupleElement(typeValues[ref.NameLabel()], int(idx), b.Values())


Should we check if idx is within bounds before we invoke insertTupleElement?

AsBigFloat() will panic if it is not valid type. Maybe we should assert it first?

Element may be larger than the tuple capacity when the context does not yet contain all instances of the block, and then we need to grow it

AsBigFloat() will panic if it is not valid type. Maybe we should assert it first?

We check that it is a number. Also the key cannot be unknown, it is either Nil or a known value.

Emyrk · 2025-04-03T20:49:55Z

Really sorry, I made a mistake and force pushed to this branch by accident. I got the branch back to the state that was reviewed 🤦‍♂️

simar7 · 2025-04-08T03:22:17Z

Really sorry, I made a mistake and force pushed to this branch by accident. I got the branch back to the state that was reviewed 🤦‍♂️

No worries @Emyrk - were there any other updates to the comments I had left or you have pushed everything you had?

Emyrk · 2025-04-08T16:51:34Z

@simar7 just read through all the comments. Added 1 little test syntax tweak.

Everything I have for this issue is pushed up 👍

simar7

lgtm, thanks for the PR! @nikpivkin could you also take another look at it?

Emyrk added 8 commits March 14, 2025 12:09

test(terraform): unit test that shows example of breaking derefs

be674f7

Referencing a block that uses `count`, and accessing it's arguments returns a nil value.

simplier example

b43cac8

most simple example

6921256

chore: add tool to serialize back to hcl, nice for debugging

04605e2

chore(terraform): save instances of data block to eval ctx as tuple

0103bd8

Instances of a block were being saved as "<name>[<idx>] = val" to the eval context. `expandBlocks` correctly saves it as a tuple "<name> = [val, ...]"

add unit tests for ctylist

ce933c9

remove dead code

6fe1164

remove dead code

332c4ad

Emyrk requested review from simar7 and nikpivkin as code owners March 14, 2025 23:50

some formatting

8f8e652

Emyrk commented Mar 14, 2025

View reviewed changes