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

perf(misconf): parse rego input once #6615

Merged
merged 4 commits into from
May 7, 2024

Conversation

nikpivkin
Copy link
Contributor

Description

We can pass Rego ast types instead of raw input, which will reduce the time cost of parsing as we apply Rego to the same input data multiple times.

Tested on https://github.com/kubernetes/minikube.

before: time go run ./cmd/trivy conf ~/projects/minikube -q 261.24s user 7.95s system 167% cpu 2:41.04 total
after: time go run ./cmd/trivy conf ~/projects/minikube -q 127.74s user 6.90s system 151% cpu 1:28.74 total

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 May 3, 2024 06:00
@nikpivkin nikpivkin requested a review from simar7 as a code owner May 3, 2024 06:00
Comment on lines +346 to +352
func parseRawInput(input any) (ast.Value, error) {
if err := util.RoundTrip(&input); err != nil {
return nil, err
}

return ast.InterfaceToValue(input)
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why this is better than simply passing the input as-is to the OPA engine? Wouldn't the engine call the roundtripper on its own?

I'm also a little wary of calling this on every single input. It could get very expensive. I almost wonder if we should write some benchmark tests of our own to evaluate this rather than just using Minikube repo as an input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rego is applied to the input data in three places, and each time the input data is parsed. So it makes sense to pass already parsed data. Why it can be expensive? Rego does the same thing.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. After reading the code again, your logic makes sense to me.

We've been bitten by OPA/Rego performance issues in the past, so I'm always a little more skeptical changing things around on that end :)

@simar7 simar7 self-requested a review May 7, 2024 04:20
@simar7 simar7 added this pull request to the merge queue May 7, 2024
Merged via the queue into aquasecurity:main with commit 67c6b1d May 7, 2024
12 checks passed
@nikpivkin nikpivkin deleted the improve-cause-perf branch May 27, 2024 14:14
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.

2 participants