Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(terraform): do not re-expand dynamic blocks #6151

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

nikpivkin
Copy link
Contributor

Description

Related issues

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

@nikpivkin nikpivkin marked this pull request as ready for review February 19, 2024 06:13
@knqyf263 knqyf263 removed their request for review February 20, 2024 06:43
t.Run("arg is map and ref to each.key", func(t *testing.T) {
src := `locals {
buckets = {
bucket1 = "bucket1"
Copy link
Member

@simar7 simar7 Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bucket1 = "bucket1"
bucket1key = "bucket1val"

Should we make the assertion a little easier to understand? If it's a map, the each object returns a each.key object returns a key and a each.value returns the value as described here In this case the assertion would strictly look for the following:

		assert.Equal(t, `this["bucket1key"]`, bucket.NameLabel())

as an example.

Copy link
Contributor Author

@nikpivkin nikpivkin Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Done 56e86aa

buckets = []
}
resource "aws_s3_bucket" "this" {
for_each = loca.buckets
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for_each = loca.buckets
for_each = local.buckets

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 56e86aa

@@ -1,94 +0,0 @@
package parser
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the method for validating a collection item. See comment #6151 (comment)

Comment on lines -287 to -288
if err := validateForEachArg(forEachVal); err != nil {
e.debug.Log(`"for_each" argument is invalid: %s`, err.Error())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the other cases as checked in the validateForEachArg()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need to check that the argument is a collection so that we can iterate over it. Does it make sense to validate the elements of the collection on the Trivy side? I think the user should pass a valid config.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah fair enough. I was more concerned on what happens if the user does pass us an invalid configuration (which is common) anyway. Looks like we still handle that fine as the debug logs have the following line:

2024-02-22T17:45:05.909-0700    DEBUG   [misconf] 45:05.909212000 terraform.parser.<root>          error parsing 'main.tf': main.tf:2,12-15: Invalid expression; Expected the start of an expression, but found an invalid expression token.

@simar7 simar7 added this pull request to the merge queue Feb 23, 2024
@simar7 simar7 removed this pull request from the merge queue due to a manual request Feb 23, 2024
@@ -905,6 +906,114 @@ data "http" "example" {

func TestForEach(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I forgot to add one thing: can we have these tests in a table instead of a bunch of t.Run()s? Looks like they have quite a bit of similarity between the cases that we can refactor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 6c11ee0

@nikpivkin nikpivkin requested a review from simar7 February 27, 2024 06:23
@simar7 simar7 added this pull request to the merge queue Feb 27, 2024
Merged via the queue into aquasecurity:main with commit 64926d8 Feb 27, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(misconf): improve false negatives with terraform dynamic blocks
2 participants