bundle: Add info about the correct rego version to parse modules on the store #7278
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,8 @@ import ( | |
| // BundlesBasePath is the storage path used for storing bundle metadata | ||
| var BundlesBasePath = storage.MustParsePath("/system/bundles") | ||
|
|
||
| var ModulesInfoBasePath = storage.MustParsePath("/system/modules") | ||
|
|
||
| // Note: As needed these helpers could be memoized. | ||
|
|
||
| // ManifestStoragePath is the storage path used for the given named bundle manifest. | ||
|
|
@@ -59,6 +61,14 @@ func metadataPath(name string) storage.Path { | |
| return append(BundlesBasePath, name, "manifest", "metadata") | ||
| } | ||
|
|
||
| func moduleRegoVersionPath(id string) storage.Path { | ||
| return append(ModulesInfoBasePath, strings.Trim(id, "/"), "rego_version") | ||
| } | ||
|
|
||
| func moduleInfoPath(id string) storage.Path { | ||
| return append(ModulesInfoBasePath, strings.Trim(id, "/")) | ||
| } | ||
|
|
||
| func read(ctx context.Context, store storage.Store, txn storage.Transaction, path storage.Path) (interface{}, error) { | ||
| value, err := store.Read(ctx, txn, path) | ||
| if err != nil { | ||
|
|
@@ -166,6 +176,16 @@ func eraseWasmModulesFromStore(ctx context.Context, store storage.Store, txn sto | |
| return suppressNotFound(err) | ||
| } | ||
|
|
||
| func eraseModuleRegoVersionsFromStore(ctx context.Context, store storage.Store, txn storage.Transaction, modules []string) error { | ||
| for _, module := range modules { | ||
| err := store.Write(ctx, txn, storage.RemoveOp, moduleInfoPath(module), nil) | ||
| if err := suppressNotFound(err); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // ReadWasmMetadataFromStore will read Wasm module resolver metadata from the store. | ||
| func ReadWasmMetadataFromStore(ctx context.Context, store storage.Store, txn storage.Transaction, name string) ([]WasmResolver, error) { | ||
| path := wasmEntrypointsPath(name) | ||
|
|
@@ -470,7 +490,7 @@ func activateBundles(opts *ActivateOpts) error { | |
| remainingAndExtra[name] = mod | ||
| } | ||
|
|
||
| _, err = compileModules(opts.Compiler, opts.Metrics, snapshotBundles, remainingAndExtra, opts.legacy, opts.AuthorizationDecisionRef) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -626,7 +646,7 @@ func eraseBundles(ctx context.Context, store storage.Store, txn storage.Transact | |
| return nil, err | ||
| } | ||
|
|
||
| remaining, removed, err := erasePolicies(ctx, store, txn, parserOpts, roots) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -649,6 +669,11 @@ func eraseBundles(ctx context.Context, store storage.Store, txn storage.Transact | |
| } | ||
| } | ||
|
|
||
| err = eraseModuleRegoVersionsFromStore(ctx, store, txn, removed) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return remaining, nil | ||
| } | ||
|
|
||
|
|
@@ -668,44 +693,94 @@ func eraseData(ctx context.Context, store storage.Store, txn storage.Transaction | |
| return nil | ||
| } | ||
|
|
||
| type moduleInfo struct { | ||
| RegoVersion ast.RegoVersion `json:"rego_version"` | ||
| } | ||
|
|
||
| func readModuleInfoFromStore(ctx context.Context, store storage.Store, txn storage.Transaction) (map[string]moduleInfo, error) { | ||
| versions := map[string]moduleInfo{} | ||
|
|
||
| value, err := read(ctx, store, txn, ModulesInfoBasePath) | ||
| if suppressNotFound(err) != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if value != nil { | ||
| 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") | ||
| } | ||
| } | ||
|
|
||
| return versions, nil | ||
| } | ||
|
|
||
| func erasePolicies(ctx context.Context, store storage.Store, txn storage.Transaction, parserOpts ast.ParserOptions, roots map[string]struct{}) (map[string]*ast.Module, []string, error) { | ||
|
|
||
| ids, err := store.ListPolicies(ctx, txn) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| modulesInfo, err := readModuleInfoFromStore(ctx, store, txn) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to read module info from store: %w", err) | ||
| } | ||
|
|
||
| getRegoVersion := func(modId string) (ast.RegoVersion, bool) { | ||
| info, ok := modulesInfo[modId] | ||
| if !ok { | ||
| return ast.RegoUndefined, false | ||
| } | ||
| return info.RegoVersion, true | ||
| } | ||
|
|
||
| remaining := map[string]*ast.Module{} | ||
| var removed []string | ||
|
|
||
| for _, id := range ids { | ||
| bs, err := store.GetPolicy(ctx, txn, id) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| 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 👍 . |
||
| parserOptsCpy.RegoVersion = regoVersion | ||
| } | ||
|
|
||
| module, err := ast.ParseModuleWithOpts(id, string(bs), parserOptsCpy) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| path, err := module.Package.Path.Ptr() | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| deleted := false | ||
| for root := range roots { | ||
| if RootPathsContain([]string{root}, path) { | ||
| if err := store.DeletePolicy(ctx, txn, id); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| deleted = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if deleted { | ||
| removed = append(removed, id) | ||
| } else { | ||
| remaining[id] = module | ||
| } | ||
| } | ||
|
|
||
| return remaining, removed, nil | ||
| } | ||
|
|
||
| func writeManifestToStore(opts *ActivateOpts, name string, manifest Manifest) error { | ||
|
|
@@ -758,6 +833,12 @@ func writeDataAndModules(ctx context.Context, store storage.Store, txn storage.T | |
| if err := store.UpsertPolicy(ctx, txn, path, mf.Raw); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if regoVersion, err := b.RegoVersionForFile(mf.Path, ast.RegoUndefined); err == nil && regoVersion != ast.RegoUndefined { | ||
| if err := write(ctx, store, txn, moduleRegoVersionPath(path), regoVersion); err != nil { | ||
| return fmt.Errorf("failed to write rego version for '%s' in bundle '%s': %w", mf.Path, name, err) | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| params.BasePaths = *b.Manifest.Roots | ||
|
|
@@ -766,6 +847,25 @@ 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 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. |
||
| for _, f := range b.Raw { | ||
| if strings.HasSuffix(f.Path, RegoExt) { | ||
| p, err := getFileStoragePath(f.Path) | ||
| if err != nil { | ||
| return fmt.Errorf("failed get storage path for module '%s' in bundle '%s': %w", f.Path, name, err) | ||
| } | ||
|
|
||
| if m := f.module; m != nil { | ||
| // 'f.module.Path' contains the module's path as it relates to the bundle root, and can be used for looking up the rego-version. | ||
| // 'f.Path' can differ, based on how the bundle reader was initialized. | ||
| if regoVersion, err := b.RegoVersionForFile(f.module.Path, ast.RegoUndefined); err == nil && regoVersion != ast.RegoUndefined { | ||
| if err := write(ctx, store, txn, moduleRegoVersionPath(p.String()), regoVersion); err != nil { | ||
| return fmt.Errorf("failed to write rego version for '%s' in bundle '%s': %w", f.Path, name, err) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -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) (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. |
||
|
|
||
| m.Timer(metrics.RegoModuleCompile).Start() | ||
| defer m.Timer(metrics.RegoModuleCompile).Stop() | ||
|
|
@@ -809,28 +909,38 @@ func compileModules(compiler *ast.Compiler, m metrics.Metrics, bundles map[strin | |
| modules[name] = module | ||
| } | ||
|
|
||
| moduleIDToRegoVersion := map[string]ast.RegoVersion{} | ||
|
|
||
| // 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. |
||
| for _, mf := range b.Modules { | ||
| modules[mf.Path] = mf.Parsed | ||
| } | ||
| } else { | ||
| for name, module := range b.ParsedModules(bundleName) { | ||
| modules[name] = module | ||
|
|
||
| p, err := getFileStoragePath(name) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| moduleIDToRegoVersion[strings.TrimLeft(p.String(), "/")] = module.RegoVersion() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if compiler.Compile(modules); compiler.Failed() { | ||
| return nil, compiler.Errors | ||
| } | ||
|
|
||
| if authorizationDecisionRef.Equal(ast.EmptyRef()) { | ||
| return moduleIDToRegoVersion, nil | ||
| } | ||
|
|
||
| return moduleIDToRegoVersion, iCompiler.VerifyAuthorizationPolicySchema(compiler, authorizationDecisionRef) | ||
| } | ||
|
|
||
| func writeModules(ctx context.Context, store storage.Store, txn storage.Transaction, compiler *ast.Compiler, m metrics.Metrics, bundles map[string]*Bundle, extraModules map[string]*ast.Module, legacy bool) error { | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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