External rule sources let wrapping projects inject rules at evaluation
time instead of compile time. The compiler marks external packages in
the rule tree but doesn't index them. When topdown hits an external
node, it calls Lookup to get rules, compiles them on the fly with a
scoped compiler, grafts the result into the tree, and caches it for the
duration of the evaluation.
Sources can be isolated (default, no access to surrounding policy) or
non-isolated (can reference static rules and other external sources).
The ExternalRuleIndexCloser interface handles cleanup after evaluation.
Precompiled rules can skip compiler stages via SkippedStages to avoid
redundant work.
This includes:
* hooks: add BundlePreActivate hook This one is handy when registering
external sources.
* topdown: catch `ir == nil` rule index result
This wouldn't ordinarily happen: the compiler is checking refs before.
But in our use case, the SP rules may be configured to be able to reach
into the surrounding Rego (non-isolated mode). If that happens, the IR
lookup may indeed end up as `nil, nil`.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Expanding the Rego syntax to support not-bodies (not blocks?): `not {...}`
For a not block to successfully evaluate, its body must not successfully evaluate. If evaluation causes iteration, all evaluation paths must fail.
Fixes: #8402
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
The concern expressed in this comment has been determined to be unwarranted, so removing it to not cause future confusion.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Not the best side quest I've been on, but for random reasons I wanted to
test which was covered here and I had one of those "people live like this?"
moments. I resisted the temptation of writing a custom DSL for testing, which
while it had made this a *fun* side quest also a rabbit hole. I do like the
cusom `unpack` function for tests though, and I think we should have that
spread to other tests!
Anyway, 2500+ lines of code less to maintain, any my LOC-based salary
increasingly looking like a bad deal.
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
Some work I did during the holidays as part of improving the performance
of interpolated strings. This change is however not isolated to those, but
updates the `String()` implementation of all AST node types (term values
and policy components). This change also lays the groundwork for migrating
OPA to the `json/v2` package once that's stable. The `json/v2` package
provides low-level functions for zero alloc marshalling via appenders — and
well, here they are. The appenders here should be usable for that purpose with
only a few tweaks needed for the few cases where our `String()` implementations
aren't also valid JSON.
Creating perfectly sized buffers requires knowing the expected length beforehand.
In order to do this, each component now implements not only `encoding.AppendText`
but a new custom `StringLengther` interface, which allows asking any AST node about
its `StringLength()` before `make`ing a buffer of that length.
We could definitely consider adding these to e.g. the `Value` or `Node` interfaces,
but I've left that out of this PR as it's an easy thing to do later should we want
to, and I guess there's always some concerns about changing public interfaces even
when they're not meant to be implemented by external code.
While no `Value` appenders allocate and almost none of the policy appenders do either,
one notable exception is `Module` when there are annotations present, as they are
a bit of a (YAML) special case. It's doable, but as serializing full modules isn't
on a hot path anywhere, I have chosen to defer that work to the future.
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
A mixed bag of improvements I have had around for a while, and would
like to see included in v1.12.0 if possible. I have about twice the
amount of changes **not** included here as they could use some more
testing. These changes should be low risk, I believe.. but obviously
do let me know if you see any potential risks that I don't!
- Add template string benchmarks
- Faster template strings / print eval by not passing bctx in recursion
- Allow passing nil value to `Query.WithQueryTracer` (no-op)
- Reduce allocations in rego v1 compiler stages
- Intern a few more common var name `Value`s
- Remove redundant switch on `scope` in annotations code
- Add a few more benchmarks in the `ast` package
- Performance improvements in type checker, most notably removing
a function literal for checking expression, which only ever had
one implementation. We can extend this later if needed.
- Prefer `NewGenericTransformer` over `&GenericTransformer` for
easier tracking in pprof
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
Parsing is generally fast, so this mainly improves performance
of creating big bundles with many Rego files in them. For Regal's
embedded bundle, loading it from memory would previously take 16
ms on my laptop, and now it takes 9 ms. There are other things
in this process that could be concurrent too, like JSON unmarshalling
of multiple data files. But starting with parsing modules.
This PR adds `errgroup` as a direct dependency (previously indirect)
as it is a nicer way to work with wait groups, and one that can be
useful elsewhere in the codebase (like in the compiler).
Also, and as usual, went off on a bit of a tangent refactoring code
related to the bundle build process, and made sure to use some common
helpers in code where available.
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
- Bump golangci-lint -> 2.6.2
- Fix all `deprecatedComment` "notices should be in a dedicated paragraph, separated from the rest" reports
- Enable `appendCombine` and fix all "appendCombine: can combine chain of X appends into one" notices
- Enable `preferFprint` and fix the few reported issues
- Fix various issues reported only once or twice, like `zeroByteRepeat`
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
Have done this some time in the past, but there was a few
new issues this would highlight now that we're on Go 1.24.
Mostly:
- Use `b.Loop()` in benchmarks
- Use `strings.SplitSeq` where possible
- Remove `omitempty` tag for types that can't be empty
Signed-off-by: Anders Eknert <anders@eknert.com>
Go 1.23 is no longer supported as per Go release policy.
Changes:
- Use Go v1.24.6 as the project SDK requirement
- Apply lint fixes for Go 1.24
- Fix "non-constant format string in call" issues as seen in CI.
Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
Funnily, this started out as an attempt to look into issues reported
with compiling large policy sets... before I realized that it isn't
likely *this* compiler that has perf issues, but the one that "compiles"
bundles as part of activation. So while these fixes likely does little
to address that, there are still some rather nice improvements here, where
the big ones as ususal are mostly just wins from avoiding work where it's
possible.
For benchmarking I've used Regal's embedded bundle, which isn't great to
use over time, as it's a moving target. But since it's a pretty extensive
bundle and one that covers most features of OPA, it's at least good for
1:1 comparisons when testing perf improvements.
```
// 66555594 ns/op 50239492 B/op 1083664 allocs/op - main
// 62569440 ns/op 38723015 B/op 944277 allocs/op - compiler-optimizations pr
```
The B/op / alloc_space improvement is particularly nice here. What's noteworthy
is how relatively little impact that has on performance in this case. That may
be surprising but aligns pretty well with my previous experience of Go code where
a lot of time is spend in recursive walks — that simply takes time, no matter how
much you optimize. Oh well, less memory allocated for this is more memory to spend
elsewhere.
(I'm adding the benchmark used below to Regal in a parallel PR)
Signed-off-by: Anders Eknert <anders@styra.com>
This commit adds support for changing out how bundle storage and
activation work. To allow swapping out bundle activation, two new
`bundle` package functions are provided:
- `RegisterActivator`: Registers a bundle.Activator with a string ID.
- `RegisterDefaultBundleActivator`: Sets the default bundle.Activator to
use by ID.
Behind the scenes, a few new `bundle` package variables are used to
track what bundle activators are available, and which is the preferred
default.
This system allows registering many activators, and allows choosing the
bundle activator to use at activation time. The activator to use is
decided in the following order:
- `(bundle.ActivateOpts).Plugin` is used when non-nil.
- `bundle.bundleExtActivator` is used when an ID was set with
`RegisterDefaultBundleActivator`.
- The default/original bundle activator is used if no other selection
was made.
To support swapping out bundle storage (useful when testing new bundle
designs), a new `bundle` package function is provided:
- `RegisterStoreFunc`: Sets the function to use for creating bundle
storage.
These two features together allow swapping out most of the bundle
activation flow, without requiring deep modification of the `bundle`
package. Lazy bundle loading mode is also enabled across many CLI
commands and other bundle loading points now when a non-default bundle
activator is set.
Signed-off-by: Philip Conrad <philip@chariot-chaser.net>
Co-authored-by: Ashutosh Narkar <anarkar4387@gmail.com>
A number of benchmarks did not have a `for range b.N` (or equivalent) loop in
them, leading to nothing being measured. This PR fixes that, along with some
cleanups in benchmarks found along the way.
Also remove `b.StopTimer` where not absolutely necessary, as that is
[notoriously buggy](https://github.com/golang/go/issues/27217), and had some
benchmarks hang for a very long time.
Signed-off-by: Anders Eknert <anders@styra.com>
Following up on #7566, and now applying the more exciting
modernizations. fmt.Appendf was new to me! But especially
the contains checks are so much better IMHO. I have reviewed
all changes myself and did a few manual changes where it
became obvious that things could be improved a little further.
(the modernize analyzer still has some issues running against
OPA, and I have manually worked around those for the time being)
Signed-off-by: Anders Eknert <anders@styra.com>
to allow for using Rego v1 bundles in `opa build`/`check`/`eval`/`test`.
Before this change, a bundle with `1` as `rego_version`/`file_rego_versions` would be rejected when evaluated with the `--v0-compatible` flag with the error:
```
rego_parse_error: illegal capabilities: rego_v1 feature required for parsing v1 Rego
```
This is fixed by adding the `rego_v1` feature to the `v0` default capabilities applied when using the `--v0-compatible` flag. Note: this allows OPA to accept Rego `v1` modules inside bundles, but modules without a specified Rego version, such as freestanding non-bundle modules or modules inside bundles with no specified Rego version, are parsed as `v0`.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Brace yourselves! For there are many touched files here. No changes
in semantics however.
Spent a long time trying out the various optional rules gocritic
provides, and settled for a few of them. There are more I really
like, but that would take many hours to address across the codebase.
Perhaps others find gocritic too pedantic? If so, we can merge the
fixes without enabling the rule.
Signed-off-by: Anders Eknert <anders@styra.com>
where the bundle has been manually constructed containing v0 Rego modules and
no `rego_version`/`file_rego_versions` fields are declared in the bundle manifest.
This affects bundle deactivation in the bundle store lifecycle used when for
`opa run` in server mode (`-s`).
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
And update code to conform to the rule.
- Replace unnecessary fmt.Sprintf with string concatenation
- Replace fmt.Sprint with more efficient strconv.Itoa
- Replace static fmt.Errorf calls with more efficient errors.New
Thanks @srenatus for pushing me down this rabbit hole!
Signed-off-by: Anders Eknert <anders@styra.com>
Fixing issue where bundle plugin would panic on reconfiguration if module rego-version is missing in bundle manifest.
* Passing runtime rego-version to deactivation options
* Preferring to pull rego-version from parsed modules if present
This solves an edge case when using the OPA SDK, and should not affect standalone OPA.
Fixes: #7297
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
This was originally added to allow Regal to get a serialized AST that included
location data wherever that was possible. Regal is however no longer using OPA's
JSON serialization but its own custom encoder. Attaching these options to every
AST node and term comes with a cost attached, and without any known users of this
feature vs. the many users who care about resource utilization, this feels like
an easy choice.
While it seems unlikely to be users depending on this functionality — in case
someone needs it, the options for serializing AST nodes to JSON can now be set
globally instead. Global state is always awkward, but since JSON marshalling
methods only have access to the node being marshalled and of course, global
state, there's not a whole lot of options if we intend to keep this feature.
Signed-off-by: Anders Eknert <anders@styra.com>
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>
And a few other small fixes in tests. This i not so much
about performance but about choosing the best tool for a
given task :) But that the alternatives are also faster
doesn't hurt either.
Signed-off-by: Anders Eknert <anders@styra.com>
And use them to reduce imperative boilerplate throughout
the codebase.
Additionally, replace use of sort.Slice with slices.SortFunc
which is more efficient since it is generic and as such avoids
allocations related to `interface{}` casts.
Also a few performance-related minor fixes, but not the main
theme of this PR.
```
BenchmarkRegalLintingItself-10 before / after
1832684458 ns/op 3453470360 B/op 66125422 allocs/op
1826601250 ns/op 3449619024 B/op 65999164 allocs/op
````
Signed-off-by: Anders Eknert <anders@styra.com>
All packages, except for `cmd` and `internal`, have been moved into a new `v1` root package.
Old packages are kept for backwards-compatibility reasons. All contained code is replaced with simple type aliases and proxy functions to `v1` implementations.
Old packages default to the Rego v0 syntax, new `v1` packages default to the Rego v1 syntax.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Stephan Renatus