bundle: Add info about the correct rego version to parse modules on the store #7278
Conversation
…store Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
| module, err := ast.ParseModuleWithOpts(id, string(bs), parserOpts) | ||
|
|
||
| parserOptsCpy := parserOpts | ||
| if regoVersion, ok := getRegoVersion(id); ok { |
There was a problem hiding this comment.
We're doing a store read on every policy id. Is there a performance hit here especially for large bundles? Can we read the entire data once and then use that in the loop? I don't think there would be much performance overhead. Maybe I'm missing something here and we need to structure the data this way. Multiple bundles, maybe?
There was a problem hiding this comment.
This got shifted around a bit when making the tests green, but there should be no issue in moving it back to reading the full thing 👍 .
| @@ -766,6 +827,23 @@ func writeDataAndModules(ctx context.Context, store storage.Store, txn storage.T | |||
| if err != nil { | |||
| return fmt.Errorf("store truncate failed for bundle '%s': %v", name, err) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Not sure I understand why we're doing the store write here? Couldn't we just build a mapping in compileModules? It would be helpful to understand the purpose of the code below.
There was a problem hiding this comment.
Unfortunately not. The bundle-store/compiler doesn't necessarily agree with the bundle on module naming; if the bundle reader was initialized with a bundle name, it'll get prefixed to the module name during truncation, but if the reader wasn't, the prefix isn't included. This means that, for raw bundles, if the bundle reader wasn't initialized with a bundle name (outside of the store's control), then the modules are compiled with the bundle name as name prefix, but when truncated into the store, they won't have that prefix.
There are a couple of tests that implicitly cause this state (that failed before); I'll add an explicit test.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
johanfylling
changed the title
v1/bundle/store.go
Outdated
| bs, err := json.Marshal(value) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("corrupt rego version") | ||
| } | ||
|
|
||
| err = util.UnmarshalJSON(bs, &versions) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("corrupt rego version") | ||
| } |
There was a problem hiding this comment.
Why do we need to do this roundtripping, exactly? I'd suspect it's a little more annoying, but not harder, to do this via
if m, ok := value.(map[string]any); ok {
versions := make(map[string]moduleInfo, len(m))
for k, v := range m {
if m0, ok := v.(map[string]any); ok {
if ver, ok := v["rego_version"]; ok {
if vs, ok := ver.(int) { // float64?
versions[k] = moduleInfo{RegoVersion: fromInt(vs)}
}
}
}
}
return versions, nil
}
return nil, fmt.Errorf("corrupt rego version")
v1/bundle/store.go
Outdated
| // include all the new bundle modules | ||
| for bundleName, b := range bundles { | ||
| if legacy { | ||
| // FIXME: Do we need to account for legacy mode in moduleIdToRegoVersion? |
There was a problem hiding this comment.
We can avoid this as this has been deprecated for a while now.
v1/bundle/store.go
Outdated
| @@ -792,7 +892,7 @@ func writeData(ctx context.Context, store storage.Store, txn storage.Transaction | |||
| return nil | |||
| } | |||
|
|
|||
| func compileModules(compiler *ast.Compiler, m metrics.Metrics, bundles map[string]*Bundle, extraModules map[string]*ast.Module, legacy bool, authorizationDecisionRef ast.Ref) error { | |||
| func compileModules(compiler *ast.Compiler, m metrics.Metrics, bundles map[string]*Bundle, extraModules map[string]*ast.Module, legacy bool, authorizationDecisionRef ast.Ref) (map[string]ast.RegoVersion, error) { | |||
There was a problem hiding this comment.
If we're not using the returned value in the call site, let's not return it.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Co-authored-by: Stephan Renatus <stephan@styra.com> Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
…he store (open-policy-agent#7278) Fixing an issue where the rego-version for individual modules was lost during bundle deactivation (bundle lifecycle) if this version diverged from the active runtime rego-version. This could cause reloading of v0 bundles to fail when OPA was not running with the `--v0-compatible` flag. Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com> Co-authored-by: Johan Fylling <johan.dev@fylling.se> (cherry picked from commit 5d5329d)
…he store (#7278) Fixing an issue where the rego-version for individual modules was lost during bundle deactivation (bundle lifecycle) if this version diverged from the active runtime rego-version. This could cause reloading of v0 bundles to fail when OPA was not running with the `--v0-compatible` flag. Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com> Co-authored-by: Johan Fylling <johan.dev@fylling.se> (cherry picked from commit 5d5329d)
Fixing an issue where the rego-version for individual modules were lost during bundle deactivation (bundle lifecycle) if this version diverged from the active runtime rego-version. This could cause reloading of v0 bundles to fail when OPA was not running with the
v0-compatibleflag.