ast/index: materialize data refs for rule indexing - #9077
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
anderseknert
left a comment
anderseknert
left a comment
There was a problem hiding this comment.
This is a cool idea, and I can certainly see the appeal for a number of deployments and integrations. Some thoughts / concerns though.
Perhaps I misunderstood something about the implementation, but having OPA do a full recompilation on any data update seems like it could have a negative performance impact that way outweighs the benefit of rule indexing in a non-negligible number of cases? Many deployments run with pretty much static policies but highly dynamic data. Those would now run a constant compilation loop more or less, where previously they didn't compile at all? Thinking OPAL-style deployments and similar.
Unless my concerns above are wrong — I'm no stranger to being wrong, after all — I would feel better about this being opt-in at least initially. I also think any toggle for this would need to be exposed via OPA's server configuration, as that's how most users run OPA.
| | `input.subject in data.computed_members` | no | `data.computed_members` is a rule, not stored data | | ||
|
|
||
| Embedders can tune this: `plugins.RuleIndexData(n)` sets the largest collection | ||
| stored in an index, and `plugins.RuleIndexData(0)` turns off reading data into |
There was a problem hiding this comment.
Who's an "embedder" in this context? The plugins package doesn't strike me as a natural entrypoint for anyone integrating OPA via the rego API, query or whatever. And it's admittedly been a while since I used the OPA SDK, but that reads a configuration file, right? So ideally it'd be a toggle exposed there. That would also allow the most common integration type to have a way to configure this behavior.
It shouldn't be any data update. It's tracking refs that are relevant for rule indexing. (Question: would this not be every update, in the end? Why update data that isn't relevant? We might just as well do without the tracking...) But I take your point that it could affect certain users negatively. OTOH if we make this opt-in, I'm having trouble foreseeing a future where many users benefit from this. I'd hope the overall change is a change for the better 🤔 I'll ponder this a bit, looks like I've got some CI stuff to look into anyways. |
Indeed. Making it opt-in to start out wouldn't necessarily mean that's a permanent decision though. Just thinking there's enough uncertainty about this that we might want to go slow here? Perhaps the number of rules this makes indexable could be taken into account for a default later? 🤔 I imagine the cost/benefit calculation will look different if you have 2 rules where this applies, vs... say 50. |
0a8361d to
19a2991
Compare
|
@anderseknert this now has a twist: It'll do the materialization, but it'll only use it as long as the data hasn't changed. If the data changes, it'll fall back to the old behavior. This should make it not much worse for existing users that have OPAL-ish use cases, but it should make others, like bundle users, benefit greatly. That do you think? |
19a2991 to
c7331f8
Compare
`x in <collection>` records one index entry per value in the collection, and
each went through refindices.insert, whose duplicate check scans everything
recorded for the rule so far. For a collection that is dominated by the
values already appended, so building the index was quadratic in the
collection size: 227ms of scanning for an 8000-value collection, per rule.
Reconcile the "any" entries once, then append the values directly. The scan
only bought duplicate suppression, and a duplicate value is harmless here --
it appends a second, identically prioritized rule node to the same trie
child, which trieTraversalResult.Add already folds together.
Unlike insert, this supersedes every "any" entry for the ref rather than the
first one, so a second local bound to the same ref no longer weakens the
membership constraint into "any value".
values before after
1000 5.9ms 0.28ms
8000 226ms 1.6ms
100000 ~35s 13ms
(extrapolated)
Nobody writes collections this large in a policy, but resolving them from
data (a later change) makes it reachable.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
An expression that reaches into data for the value it compares against was not
indexed at all:
allow if input.subject in data.groups.admins.members
allow if input.tenant == data.config.tenant
The trie keys its children on values known when it is built, and neither side of
these expressions is one -- both are refs. So the membership contributed nothing
and the rule stayed a candidate for every value of input.subject, and the
equality did not even record that it constrains input.tenant. A policy with one
rule per group evaluated all of them.
Resolve those refs while the index is built and put what they hold into the
trie: a collection unrolled into one child per value, exactly like a collection
literal, and a scalar as the value of the equality. A lookup is then the same
single hash lookup that `x in {"a", "b"}` or `x == "b"` gets, however many rules
there are, and needs no access to data at all.
(*Compiler).MaterializeIndexData(resolver, maxCollectionSize) does the
resolving, after Compile, since a caller that writes data and compiles policies
in one transaction can only do one of them first. A ref rooted at a local --
`x := data.config; input.tenant == x.tenant` -- is followed to what the local
aliases, so which spelling the author reached for doesn't decide whether the
data is read in. Unrolling a collection costs
~250 bytes of trie per (rule, value) pair -- roughly 3.5x what the values cost
as data in an AST-backed store -- so maxCollectionSize bounds what one
collection may contribute; 25MB of trie per 100k pairs is the shape of it.
(*Compiler).IndexDataRefs reports the refs the indices read. It is a property of
the policy, reported whether or not their values were materialized: a collection
that is missing, or too large, is one whose data still decides what the indices
should look like. Deriving this from the policy rather than from what was
successfully resolved is what makes "the data showed up later" a case rather
than a hole. Nothing rebuilds a rule index by itself, so a caller has to
materialize again when that data changes.
Values that have moved since are handled the same way, by
(*Compiler).MarkIndexDataStale: a caller that sees data change marks the refs it
changed, which costs a pointer swap, rather than compiling a policy on the path
of a data write. Only the children materialized from a marked ref stop
discriminating -- the rules below them are left as indexed as they were before,
and the rest of the trie keeps pruning -- and the next MaterializeIndexData
reads the new values in and clears the marks.
Two more cases work the same way, per evaluation rather than per compiler: a
`with` statement replacing a ref for one evaluation -- at, above, or below it,
hence refStack.Overlaps rather than Prefixed -- and partial evaluation having to
treat it as unknown. evalResolver reports all three (ast.IndexDataChecker).
Both checks are per ref, which matters most for partial evaluation: the unknowns
are usually input, which says nothing about the data the index read, and
treating it as unknown regardless would leave a rule per group to evaluate --
the cost that materializing it is there to remove.
A ref that rules contribute to is neither read nor reported: isValidIndexRef
rejects virtual refs, because nothing resolves them at lookup time either.
buildRuleIndices runs after SetRuleTree, so that check sees the whole rule tree
-- including general ref rules like `groups[k].members`.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
ast.Compiler.MaterializeIndexData reads the data an index compares against into the index itself, which makes it depend on that data -- and nothing rebuilds a rule index by itself. Wire that up for a running OPA, on by default, bounded by plugins.RuleIndexData: collections up to DefaultRuleIndexDataMaxCollection values are read in, and passing zero turns it off. The manager reads data into every compiler it installs, resolving through the store inside the transaction being committed: at startup, on a policy change, and after a bundle activation, which compiles its modules before its own data reaches the store. A commit that moves data the indices already read does not recompile. It marks those refs (MarkIndexDataStale), so the values read from them stop excluding rules, and the next compilation reads the current ones back in. Recompiling instead would put a policy compilation on the path of every data write: measured on 2000 rules over 50-member collections, a write into one of them went from 1us to 1.8s, which for a deployment with static policies and dynamic data is a compilation loop where there was none. Marking costs 43us at that size, most of it the scan for which refs a write reaches. Two paths lead here. A write through the Data API leaves no compiler on the transaction context, and a delta bundle patches data and hands back the very compiler already installed -- which is why "a compiler was put on the context" isn't enough to tell a freshly compiled one from the one in use, and the check is against the compiler installed. A commit that writes nowhere near data an index read is left alone, and so is every commit when the option is off. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
An external source compiles its rules when a query reaches them -- the
structured-policy plugin with incremental loading returns a few hundred per
lookup -- and ExternalIndex.Tree builds those into a compiler of its own. The
compiler the manager installs never sees them, so none of the data those rules
compare against was read into their indices:
allow if {
input.subject in data.groups["idms:group:1234"].members
...
}
One rule per group means every one of them is a candidate for every subject,
which is what reading the collections in is meant to fix. Measured on five such
rules, the index matched all five; it now matches the one whose group holds the
subject.
Tree already receives a resolver, and it is the evaluator's: save-set aware, and
seeing the data this query sees. So it reads the values in at the moment it
compiles the rules, and nothing needs tracking -- the tree and its indices live
on the externalTreeStack, which belongs to the evaluation. No marking, no
recompiling, no refs to watch.
What it does need from the surrounding compiler is a budget, which is the
operator's to set (plugins.RuleIndexData): MaterializeIndexData stamps it onto
the external nodes of the rule tree, where Tree picks it up. A source that hands
over pre-compiled rules and skips StageBuildRuleIndices builds no indices at
all, and is left alone.
Note that this reads data in on every lookup that expands an external ref, where
the manager's compilers do it once per installed compiler. Those lookups already
compile modules per query, so it adds to a cost that was never amortised; the
budget is what bounds it.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
c7331f8 to
332bc09
Compare
2 participants
Previously, a rule like
would not be subject to rule indexing.
Now, the compiler will materialize the data.admins.members collection, and add one edge to the index per element, treating it the same as a literal collection:
The plugin manager ensures that we re-materialize on data changes for the relevant paths.
Since it's become a low-hanging fruit, the same now happens for constants, e.g.
(Less important, but added for consistency.)