sdk: fix bug while activating the bundle plugin #6689
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
For some reason the bundle plugin activation with multiple optimized bundles, builded in v1 syntax, results in parsing error. During the activation of the bundle, when we need to parse the rego modules, the ParserOptions is not forwarded to the `bundle.ActivateOps`, so the v1 syntax does not seem to be properly parsed. Signed-off-by: Francisco Rodrigues <ednofco@gmail.com>
c499d9a
to
7dd1dd8
Compare
…he function of multiple consecutive bundle updates. Signed-off-by: Johan Fylling <johan.dev@fylling.se>
sdk/opa_test.go
Outdated
| @@ -2844,3 +2844,54 @@ func toMetricMap(metrics []*promdto.MetricFamily) map[string]bool { | |||
| } | |||
| return metricMap | |||
| } | |||
|
|
|||
| func TestActivateOptimizedV1Bundles(t *testing.T) { | |||
There was a problem hiding this comment.
We should rename this to TestActivateV1Bundles.
Colloquially, an "optimized" bundle is one that has been constructed through an optimized OPA build (e.g. opa build is used with the -O flag).
There was a problem hiding this comment.
@johanfylling I've renamed the test and simplified the test structure to better reflect this.
| Compiler: compiler, | ||
| Metrics: p.status[name].Metrics, | ||
| Bundles: map[string]*bundle.Bundle{name: b}, | ||
| ParserOptions: p.manager.ParserOptions(), |
Conflicts: plugins/bundle/plugin_test.go
ed3dc24
to
dad69b8
Compare
The bug in activating bundles in the SDK is not necessarily related to optimized bundles, but specifically for v1 bundles. So the test was renamed to `TestActivateV1Bundles` and the test structure simplified to better describe the scenario. Signed-off-by: Francisco Rodrigues <ednofco@gmail.com>
dad69b8
to
2d4a6ff
Compare
johanfylling
approved these changes
Apr 11, 2024
tsidebottom
pushed a commit
to tsidebottom/opa-
that referenced
this pull request
Apr 17, 2024
…policy-agent#6689) Fixing issue where active parser options aren't propagated to module reload during bundle activation. Signed-off-by: Francisco Rodrigues <ednofco@gmail.com> Signed-off-by: Thomas Sidebottom <thomas.sidebottom@va.gov>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For some reason the bundle plugin activation with multiple optimized bundles, builded in v1 syntax, results in parsing error.
During the activation of the bundle, when we need to parse the rego modules, the ParserOptions is not forwarded to the
bundle.ActivateOps, so the v1 syntax does not seem to be properly parsed.Why the changes in this PR are needed?
The OPA SDK does not initialize when provided multiple bundles that were optimized with v1 syntax.
What are the changes in this PR?
Basically, I just forward the ParserOptions from the plugin manager to the
bundle.ActivateOptsstruct.Notes to assist PR review:
I am not totally sure about what the root problem is, but I was able to provide a failing test
and the fix.
It might be related to the issue #6450
Further comments: