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>
This was something I originally intended to use in another project,
but since this turned out to be a better implementation (subject of
course to review!) in terms of both performance and simplicity, I
figured we might as well use it here too. Some changes include:
- `MustParse` to parse known valid versions
- Allocates nothing in any operations other than 1 alloc in `String()`
- Ignores empty `PreRelease` and `Metadata` fields in serialization
- `encoding.TextAppender` implementation to serialize without allocating
- A whole bunch of benchmarks
Signed-off-by: Anders Eknert <anders@eknert.com>
This was previously done in several places in a somewhat convoluted way,
which probably made sense at some point. As expected however, a few small
local variations had emerged, and while nothing critical, this code wasn't
very nice to work with.
Some comments stated as the rationale for the design was avoiding allocations,
but those were nowhere to be seen when measured now, meaning there was no good
reason to have it remain this way! It was *quite* nice to be able to merge
the numbers comparsion functions (in particular) together into one!
We now have a unified way for comparing Number values throughout the AST package,
and as an added bonus, `1.0 == 1` is now true consistently for Rego.
See for example @srenatus example in https://github.com/open-policy-agent/opa/issues/4797
```
$ opa eval -fpretty 'count({1.0, 1})'
2
```
Doing the same now gives:
```
$ go run main.go eval -fpretty 'count({1.0, 1})'
1
$ go run main.go eval -fpretty 'count({1.0, 1, 1.000, 1.00000})'
1
```
What I have left out for now is however _presentation_. Meaning that
even though 1.0 and 1 is now treated as the same value, you may still
see either '1' or 1.0' (or whatever) displayed, depending on what was
parsed. Should be easy to fix, but could perhaps be perceived as
surprising... so holding off on that until we've had a discussion on
the topic.
Signed-off-by: Anders Eknert <anders@eknert.com>
Due the way that wasmtime-go does its versioning, it seems to fly under
the radar of dependabot: that will never propose major version bumps, I
suppose.
Signed-off-by: Stephan Renatus <stephan@styra.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>
- build with 1.25.1 (.go-version)
- go-compat test run with 1.24.*
- adjust prometheus metrics test
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
* server: port compile API
Also adds e2e tests: These include coverage for ucast in the prisma
setting, and thus require some JS runtime.
* e2e: selectively skip e2e Compile API tests
...for macos runs, and for the go-compat suites.
* server: accept timer_rego_external_resolve_ns metrics with value 0
When running the tests in a loop for a while, I would see values of 0ns
for this metric. However, comparing with its non-zero values, which are
often 41 or 42ns, it seems like this is just not happening in this code
path. So if "almost nothing" actually goes below 1ns, it's OK.
* e2e: split dep-heavy e2e tests into their own go module
* Makefile: export DOCKER_RUNNING (make e2e read it)
---------
Co-authored-by: Philip Conrad <philip@chariot-chaser.net>
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Previously, we've allowed overriding the GitHub API endpoint. Now, we
can also allow overriding the GitHub Repo slug used in the API call.
A concrete use is EOPA checking for new EOPA versions, as opposed to
comparing its version against OPA's version.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.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>
This allows simple setups -- those feeding the OPA discovery plugin with
a static JSON file -- to still do env variable replacements.
This should be possible already, by using a policy to construct the
disco config, but it becomes easier now.
Co-authored-by: Teemu Koponen <koponen@styra.com>
Signed-off-by: Stephan Renatus <stephan@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>
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 <philip@chariot-chaser.net>
Closing the circle here, or something.
Not a lot of Rego used in OPA yet, but some in examples and tests. The little
there is should be linted though, and it'd be good if any new addition of policies got
linted by default. But more than anything, the "ignore configuration" provided here
avoids having developers seeing thousands of issues reported by Regal when they
open the OPA project in VS Code or their editor of choice.
Someone might want to look into un-ignoring the doc directory at some point, as it's
probably a good idea to have the docs follow best practices.
Signed-off-by: Anders Eknert <anders@styra.com>
Since we don't use the HTTP API in Regal, I hadn't looked at this from
a performance POV before, and it was a fun side quest :)
A pretty decent reduction of the baseline cost for the most common
request type, v1/data POST. Changes:
- Add NoOp implementation of `Metrics` for when metrics aren't needed
- Cheaper `metrics.New` instantiation avoiding unnecessary lock
- Avoid cost of decision logging if decision logging isn't enabled
- Only call request.URL.Query() if URL.RawQuery isn't empty, avoiding
allocating an empty map with each request
While the target was v1/data POST handling, some of the changes above
have a positive impact on all or most handlers. All changes have been
run through the existing tests, and a new benchmark to measure the impact
of the fixes have been added, showing:
```
13063 ns/op 15162 B/op 195 allocs/op - main
12796 ns/op 14856 B/op 189 allocs/op - avoid r.URL.Query() when no query provided
12541 ns/op 14483 B/op 187 allocs/op - decisionLogger.Log early exit if not enabled
12133 ns/op 14235 B/op 180 allocs/op - get revisions and init logger only if needed
11098 ns/op 14207 B/op 180 allocs/op - more efficient metrics.New() (without locking)
10683 ns/op 13171 B/op 169 allocs/op - use no-op metrics implementation when metrics aren't requested
```
Even if the impact is pretty good, it's worth noting that most of the improvements
above are only seen when decision logging is turned off. While this is the common case
for development, it's not in production. Getting the baseline cost down is important still
as there should be no cost paid for features unused.
Signed-off-by: Anders Eknert <anders@styra.com>
This PR adds interning of strings representing common integer values,
which greatly speeds up "to string" operations on numbers, and updates
some built-ins commonly used for this to make use of interned values
where possible.
This is "light" version of a previous PR that did this more aggressively,
but also came with more caveats. Importantly, interning of new strings is
now never done at "runtime", but only allowed at init time. The API for
interning is marked experimental and should not relied upon by anyone
who expects a stable API.
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>
This change still follows the approach thought up in #6996, but now does
it more consistently: the extra args accumulated through (multiple)
with-replacements using variables are now put into the funcstackj, and
consistently affect the planning of functions in "higher" gens.
Fixes#5311.
Signed-off-by: Stephan Renatus <stephan@styra.com>
This updates gqlparser from version ~v2.5.1 and the June 2018 GraphQL spec to
v2.5.26 and the October 2021 GraphQL spec.
Signed-off-by: Rob Myers <1243316+robmyersrobmyers@users.noreply.github.com>
When planning rules like these:
```
package authz
p.allow[action][resource] if { action := "list"; resource := "fruit" }
p.unrelated.eat.veggies if true
resp := p[input.rule][input.action][input.resource]
```
we ended up with a broken CallDynamic statement. Since the first ref
rule is planned as `g0.data.authz.p.allow` and builds an object return
value, and the second rule is planned as
`g0.data.authz.p.unrelated.eat.veggies` with a boolean return value, we cannot
dynamically dispatch their calls.
With this change, the previously existing "unbalanced ruletrie" check now
also hits before reaching the end of the ref. It'll catch this situation
and avoid optimizing the dispatch. We'll end up with a longer, less
efficient, but correct plan.
Signed-off-by: Stephan Renatus <stephan@styra.com>
The previous check there was running into false positives, as the added
test case showed. We should only count relevant ruletrie child nodes.
Signed-off-by: Stephan Renatus <stephan@styra.com>
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>
And many smaller performance improvements. The indexer recycling results
is one of the most impactful performance improvements as of yet, and alone
saves more than 2 million allocations in the Regal lint benchmark. The indexer
is also more efficient, as `values` are no longer stored on the struct. Thanks
@tsandall for that code!
Also included a bunch of small improvements from my perf branches.
**Before**
```
1209043041 ns/op 3255157224 B/op 64026192 allocs/op
```
**After**
```
1197131792 ns/op 3194124864 B/op 61876276 allocs/op
```
Signed-off-by: Anders Eknert <anders@styra.com>
I'll follow up with another PR to modify this to allow passing
a custom compiler, and whatever else we need in Regal. But moving
files *and* modifying them in the same change is rarely great for
reviewing. So this does nothing but move the package and adjust
the pointer to the package in cmd/oracle.
Fixes#7265
Signed-off-by: Anders Eknert <anders@styra.com>
Adds the "deployment.environment" resource attribute to those that can
be configured for OpenTelemetry. This was done as some collectors,
including Datadog, require this value to properly classify traces.
Note: the "deployment.environment" attribute is being deprecated in
future versions of the OTel schemas and this may need to be
updated when that library is upgraded.
Fixes#7322
Signed-off-by: Brian Cullen <brianc@kahoot.com>
Stephan Renatus