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

Aborting query-eval if the compiler has errors #5949

Conversation

johanfylling
Copy link
Contributor

@johanfylling johanfylling commented May 24, 2023

Fixes: #5947

@netlify
Copy link

netlify bot commented May 24, 2023

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 8a991b6
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/646f676c0df79300083c7fd2
😎 Deploy Preview https://deploy-preview-5949--openpolicyagent.netlify.app/docs/edge/policy-reference
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@johanfylling johanfylling force-pushed the discovery_bundle_deletion_panic branch from ec996ca to 2d0426c Compare May 24, 2023 15:57
if q.compiler != nil && len(q.compiler.Errors) > 0 {
return &Error{
Code: InternalErr,
Message: "compiler has errors",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we aggregate them or something? This error message alone doesn't help much figuring out what's wrong, I think.

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'm actually a bit uncertain as to what's the best option here. This error will be returned on the /v1/data endpoint, and it might be a security risk to forward compilation errors to the querying client .. (or I'm just way to cautious 😅).

The compiler errors are reported for each bundle on the /v1/status endpoint.

.. but now that I'm thinking about it, that doesn't help you much when you're not running OPA in server mode 🤔.

Copy link
Member

Choose a reason for hiding this comment

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

We can always expose more debug info in the future if we think it's necessary and/or would assist with issue triage.

Copy link
Member

Choose a reason for hiding this comment

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

And since these errors will be exposed via the Status API we still have some insight here.

@ashutosh-narkar ashutosh-narkar force-pushed the discovery_bundle_deletion_panic branch from 8a991b6 to 28f75b5 Compare May 25, 2023 17:28
@johanfylling johanfylling merged commit 6109e6a into open-policy-agent:main May 25, 2023
@johanfylling johanfylling deleted the discovery_bundle_deletion_panic branch May 25, 2023 17:38
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.

Removing a bundle from discovery might cause panic at eval-time
3 participants