loader+internal: Add bundle lazy loading mode across the runtime. #7768
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
philipaconrad
force-pushed
the
philip/lazy-bundle-loading
branch
from
cbd81c3 to
d6c782e
Compare
| @@ -46,6 +46,7 @@ func bundleOrDirInfoForRegoVersion(regoVersion ast.RegoVersion, path string, inc | |||
| b, err := loader.NewFileLoader(). | |||
| WithRegoVersion(regoVersion). | |||
| WithSkipBundleVerification(true). | |||
| WithBundleLazyLoadingMode(true). // Bundle lazy loading mode skips parsing data files | |||
There was a problem hiding this comment.
This is the one place we currently set Bundle Lazy Loading mode to true. It has the potential to be helpful in this function because we only care about the metadata, manifest, signatures, and Rego code in this function-- the data isn't inspected at all.
| @@ -122,10 +122,11 @@ func LoadPaths(paths []string, | |||
| asBundle bool, | |||
| bvc *bundle.VerificationConfig, | |||
| skipVerify bool, | |||
| bundleLazyLoading bool, | |||
There was a problem hiding this comment.
I chose to put the bundleLazyLoading parameter immediately after skipVerify where possible. This results in cleaner-looking invocations in places where we're unpacking parameters from an options struct, like in v1/runtime/runtime.go.
There was a problem hiding this comment.
Do you reckon there's a good way to test at least some of this? I mean I have had a hard time coming up with a good test recently, too, #7767, and ended up not doing it... so either way is fine, but perhaps you have an idea.
| @@ -216,6 +217,12 @@ func (c *Compiler) WithBundleVerificationKeyID(keyID string) *Compiler { | |||
| return c | |||
| } | |||
|
|
|||
| // WithBundleLazyLoadingMode sets the additional data to be included in .manifest | |||
There was a problem hiding this comment.
You had me thinking there for a moment. C/P mishap from WithMetadata() below 😄
| @@ -46,6 +46,7 @@ func bundleOrDirInfoForRegoVersion(regoVersion ast.RegoVersion, path string, inc | |||
| b, err := loader.NewFileLoader(). | |||
| WithRegoVersion(regoVersion). | |||
| WithSkipBundleVerification(true). | |||
| WithBundleLazyLoadingMode(true). // Bundle lazy loading mode skips parsing data files | |||
There was a problem hiding this comment.
Hmm this will actually make bundle inspection a tad faster, wouldn't it? Not like it's a performance-critical path, just thinking about the impact of this.
| @@ -124,7 +124,7 @@ p = true { 1 = 2 }` | |||
| store := inmem.New() | |||
|
|
|||
| err := storage.Txn(ctx, store, storage.WriteParams, func(txn storage.Transaction) error { | |||
| loaded, err := LoadPaths(paths, nil, tc.asBundle, nil, true, false, nil, fsys) | |||
| loaded, err := LoadPaths(paths, nil, tc.asBundle, nil, false, true, false, nil, fsys) | |||
There was a problem hiding this comment.
Not now, but eventually, we should figure out these arguments better. nil, nil, false, true, false. It's like morse code or a drum line. 😆
There was a problem hiding this comment.
Yeah, I don't love it either. I think future maintainers might end up rolling all these options up into a params/options struct of some sort.
philipaconrad
force-pushed
the
philip/lazy-bundle-loading
branch
from
d6c782e to
5d2726f
Compare
The only kind of testcase I can think of would be one where we intentionally put non-JSON data into a bundle's data file, and check that the bundle isn't eagerly trying to parse that data. I'll spend a few minutes to see if I can hack up a testcase-- I might be able to stick one in the |
philipaconrad
force-pushed
the
philip/lazy-bundle-loading
branch
from
5d2726f to
288d438
Compare
|
Okay, I've added a test to |
This commit comprehensively plumbs in the bundle lazy loading mode option in the compile, runtime, rego, and bundle packages. It also includes the bare minimum plumbing to allow the path watcher utilities to also toggle the option on. In nearly all places where a default is expected, the lazy loading mode is set to false (disabled) to avoid behavior changes. Signed-off-by: Philip Conrad <[email protected]>
philipaconrad
force-pushed
the
philip/lazy-bundle-loading
branch
from
c7940d7 to
b55440f
Compare
What Changed?
This PR comprehensively plumbs in the bundle lazy loading mode option in the
compile,runtime,rego, andbundlepackages. It also includes the bare minimum plumbing to allow the path watcher utilities to also toggle the option on.In nearly all places where a default is expected, the lazy loading mode is set to
false(disabled) to avoid behavior changes.