Optimize and improve code for built-in functions#1788
Optimize and improve code for built-in functions#1788anderseknert merged 1 commit intoopen-policy-agent:mainfrom
Conversation
* Register Regal's built-in functions globally only once and in a single location. The various `rego` APIs for working with custom functions haven't been a great experience for us to work with, and consolidating our code for this simplifies things quite a bit. Note that we've tried to do this in the past but never got it quite working. Seems like it does now. * Set `CanSkipBctx` to `true` on all the OPA built-in functions that we use internally, as we don't need the cancellation or inter-query caches, and building the context for each call isn't cheap. * Pass a no-op external cancel function to eval to avoid having OPA start one per each file linted. While working on this I also found an optimization opportunity in OPA, which I will submit a PR for too. Signed-off-by: Anders Eknert <anders.eknert@apple.com>
anderseknert
force-pushed
the
safety-theatre-opt-out
branch
from
1661373 to
88e637b
Compare
| "github.com/open-policy-agent/regal/pkg/config" | ||
| "github.com/open-policy-agent/regal/pkg/roast/transform" | ||
|
|
||
| _ "github.com/open-policy-agent/regal/pkg/builtins" |
There was a problem hiding this comment.
We have this required now in many places, what guards are there (or could there be) for in future if we need this _ import but forget to do it?
There was a problem hiding this comment.
Since they are functions, missing this import would typically result in OPA not compiling a policy that makes use of them. Certainly a risk, and you're right this is hard to future-proof, but at least it should fail rather loudly.
I believe the best remedy is to have a single entry point for all evaluation throughout the Regal codebase. As you know, I've been rather conscious about this already, and we're now down to... 3 or so? But ideally only a single Regal module wraps rego1 directly, and any code doing evals route through that. At that point, it should be simple to assert programmatically that no other code imports rego directly.
Footnotes
-
Tbh, I'm increasingly considering the feasibility of using the even more "low level"
queryAPI (whichregois largely a wrapper around) directly in Regal, as I think we're well past the point of convenience being more important than control. And as I think this PR demonstrates, the convenience is often quite inconvenient (for our use case). ↩
| // While this comes with some overhead, it too is rather insignificant. It does however mean | ||
| // that the type of multi-threaded evaluations we typically in the linter will have pprof | ||
| // report 99% of all "blocked" time here, making it much harder to identify blocking we do | ||
| // ourselves / care about. |
There was a problem hiding this comment.
Still sounds like something we could fix upstream. "unreadable pprof output" sounds like reason enough
There was a problem hiding this comment.
Indeed. This was just an easier fix for the time being :)
3 participants
regoAPIs for working with custom functions haven't been a great experience for us to work with, and consolidating our code for this simplifies things quite a bit. Note that we've tried to do this in the past but never got it quite working. Seems like it does now.CanSkipBctxtotrueon all the OPA built-in functions that we use internally, as we don't need the cancellation or inter-query caches, and building the context for each call isn't cheap.While working on this I also found an optimization opportunity in OPA, which I will submit a PR for too.