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

feat(misconf): log causes of HCL file parsing errors #7634

Merged
merged 4 commits into from
Nov 25, 2024

Conversation

nikpivkin
Copy link
Contributor

Description

❯ cat main.tf
resource "aws-s3-bucket" "name" {
  bucket = <
}%

❯ ./trivy conf main.tf -d
...
2024-10-02T15:56:47+06:00       DEBUG   [terraform parser] Parsing FS   module="root" file_path="."
2024-10-02T15:56:47+06:00       DEBUG   [terraform parser] Parsing      module="root" file_path="main.tf"
2024-10-02T15:56:47+06:00       ERROR   [terraform parser] Error parsing file   module="root" file_path="main.tf" cause="  bucket = <" err="main.tf:2,12-13: Invalid expression; Expected the start of an expression, but found an invalid expression token."
...

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
Copy link
Contributor Author

@simar7 I'm concerned that a few lines will make it hard to see where the error occurred. Should we display the whole resource? Or, if the resource is large, just the top part of it and the range of lines where the error occurred, for example:

cause="resource \"aws_s3_bucket\" \"test\" {}\n...  bucket = <"

@simar7
Copy link
Member

simar7 commented Oct 18, 2024

@simar7 I'm concerned that a few lines will make it hard to see where the error occurred. Should we display the whole resource? Or, if the resource is large, just the top part of it and the range of lines where the error occurred, for example:

cause="resource \"aws_s3_bucket\" \"test\" {}\n...  bucket = <"

TBH this was my concern as well. The CLI log output isn't the best way to show such information. I think the best would be to display a code snippet along with start/end line numbers. It would be similar (for the code snippet) for what we do with secret scanning output.

@nikpivkin
Copy link
Contributor Author

nikpivkin commented Oct 24, 2024

I think the best would be to display a code snippet along with start/end line numbers

In that case we should respect the quiet flag and pass it to IaC scanners. Or is there a helper function in Trivy that does not print to stdout if the quiet flag is passed?

@simar7
Copy link
Member

simar7 commented Oct 24, 2024

I think the best would be to display a code snippet along with start/end line numbers

In that case we should respect the quote flag and pass it to IaC scanners. Or is there a helper function in Trivy that does not print to stdout if the quote flag is passed?

What's the quote flag you're referring to?

I think since this is part of the debug output stream, it is fine to be verbose to the extent where we display a code snippet and start/end line numbers as we discussed. That amount of info is sufficient enough for a user to be able to determine where the resource lies.

@nikpivkin
Copy link
Contributor Author

@simar7 My typo, I meant the flag quiet.

@nikpivkin
Copy link
Contributor Author

The CLI log output isn't the best way to show such information

If logging is not the appropriate place, then the only thing left is directly to stdout, which Trivy only does for reports.

@simar7
Copy link
Member

simar7 commented Oct 29, 2024

The CLI log output isn't the best way to show such information

If logging is not the appropriate place, then the only thing left is directly to stdout, which Trivy only does for reports.

I don't have any better idea at this time. We can implement the log output with the start/end line for now and improve it later if need be.

@simar7 simar7 marked this pull request as ready for review November 23, 2024 06:45
@simar7
Copy link
Member

simar7 commented Nov 23, 2024

@nikpivkin I updated the PR, is there anything we need prior to merging it in?

@nikpivkin
Copy link
Contributor Author

No, I forgot to mark this as ready for review after testing

@simar7 simar7 added this pull request to the merge queue Nov 25, 2024
Merged via the queue into aquasecurity:main with commit e9a899a Nov 25, 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.

feat(misconf): Improve terraform plan JSON error logging
2 participants