And enable more staticcheck linters. I saw staticcheck failures
mentioned in another PR, so thought I'd check it out.
- `WriteString(fmt.Sprintf)` -> `fmt.Fprintf`
- Rewrite calls to deprecated `*Rule.Path()`
- Don't use `==` to compare `time.Time`
- Use inline ignores over config exclusions of paths
- Remove 'varcheck' ignores as no longer used
- Remove v0 topdown/graphql.go (!)
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
The `json.patch` built-in is quite versatile, and compared to
patching via e.g. `object.union` et. al. often communicates
intent better, IMO. But while it uses some fairly advanced
logic for complex patch operations, it doesn't perform all that
great on simple ones. This is a first and pretty basic attempt
to improve that somewhat by picking the most low-hangig performance
fruits, like avoiding repeated allocations of temporary term pointers.
The main allocation source is the creation of EditTree's, and this
remains a problem. I have created a sync pool but only managed to
get the outermost edit tree to recycle, as I found it really hard
to track where it's safe to release those created in the deeply
nested calls. Additionally, I managed to trigger stack overflows
trying to recycle child trees, so there seems to be some circular
refs? Or I just did something wrong.
If someone wants to look into this and pick up where
I left, that'd be great!
- Add InternedIntRange for testing, primarily
- Intern keys used in json.patch patches
- Clean up json.X built-in benchmarks
- Reduce allocations in edit tree function
- Avoid using intermediate data structures
for JSON patches
- Some unrelated interning fixes to reduce noise
in tests and benchmarks (e.g. do less stuff in
var inits)
Selected benchmark that I used while working on this:
**Before**
```
BenchmarkJSONPatchAddShallowScalar/object-10-16 147853 8008 ns/op 9667 B/op 206 allocs/op
BenchmarkJSONPatchAddShallowScalar/array-10-16 201704 5889 ns/op 7256 B/op 173 allocs/op
BenchmarkJSONPatchAddShallowScalar/set-10-16 182566 6733 ns/op 8103 B/op 156 allocs/op
```
**After**
```
BenchmarkJSONPatchAddShallowScalar/object-10-16 197414 6066 ns/op 7256 B/op 133 allocs/op
BenchmarkJSONPatchAddShallowScalar/array-10-16 278121 4427 ns/op 5285 B/op 100 allocs/op
BenchmarkJSONPatchAddShallowScalar/set-10-16 233884 4839 ns/op 6243 B/op 113 allocs/op
```
Signed-off-by: Anders Eknert <anders.eknert@apple.com>
When `--h2c` is passed, HTTP2 will also be used on the unix domain socket.
Previously, it had no effect on UDS, only on TCP connections.
Fixes#8282.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.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>
* 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>
* v1/plugins: Address race in config access
I ran into this race condition on another PR:
https://github.com/open-policy-agent/opa/actions/runs/16655603110/job/47139789057
I have tried to make all manager.Config access thread-safe by adding new
getters for used values. GetConfig is regrettably based on a JSON
roundtrip deep copy of the config. This us used in tests (fine) but also
in the discovery plugin:
https://github.com/open-policy-agent/opa/blob/2d014a89bbbc307d7204817220146ffae992e838/v1/plugins/discovery/discovery.go#L122
getPluginSet is very tightly coupled to the manager.Config and because
of it's dependencies on status and the other plugins packages, it's hard
to break out.
So, for now, I think this is an improvement and worth getting a second
opinion on before more refactoring.
Signed-off-by: Charlie Egan <charlie@styra.com>
* v1/config: Use add Clone to config
This makes the use of the manager's config more thread-safe and
consistent without more API changes.
Signed-off-by: Charlie Egan <charlie@styra.com>
* topdown: Add clone() funcs for config structs
NamedValueCacheConfig.Clone, InterQueryBuiltinValueCacheConfig.Clone and
InterQueryBuiltinCacheConfig.Clone have been added.
All Clone methods return a deep copy of the struct. This is tested for
missed new fields using PopulateAllFields, a generic function that
stuffs structs with values for all fields.
Signed-off-by: Charlie Egan <charlie@styra.com>
* plugins: Clone new config
Signed-off-by: Charlie Egan <charlie@styra.com>
---------
Signed-off-by: Charlie Egan <charlie@styra.com>
This commit adds an experimental "intermediate results" field to
decision logs, and provides some basic plumbing in the server package
for attaching the intermediate results of an eval to the request
context.
Co-authored-by: Teemu Koponen <koponen@styra.com>
Signed-off-by: Philip Conrad <philip@chariot-chaser.net>
This commit provides an extension mechanism for the server authorizer,
allowing plugins and other server extensions to inform the authorizer
about the methods and paths where it should expect and parse request
bodies.
Signed-off-by: Philip Conrad <philip@chariot-chaser.net>
Before we had introduced `http.ServeMux` as "the router", we had been
using github.com/gorilla/mux. Using the latter, it was possible to
inject middlewares using the mux's `.Use()` method. This mechanism
allowed global middlewares to be injected from `runtime.Params`, for
example.
With `http.ServeMux`, that's no longer possible. However, it was never
an intentionally supported feature in the first place.
So this commit introduces HTTP handler middlewares as extension points.
It's modelled after `(*plugins.Manager).ExtraRoute()`.
Signed-off-by: Stephan Renatus <stephan@styra.com>
When an plugin http handler or some other mechanism wants to do rego
evaluations, too, it's beneficial to share the caches with the server.
This change introduces two new hook types to allow retrieving those
caches during server startup.
Signed-off-by: Stephan Renatus <stephan@styra.com>
This way, the extra handler functions are still covered by prometheus
metrics and opentelemetry spans.
The previous method of directly registering routes with the router
bypassed the server's handler wrapping.
Signed-off-by: Stephan Renatus <stephan@styra.com>
I was curious to see how much work this would entail, and it turned out to
be... some :) Particularly porting some of the features exposed as settings
by gorilla mux, like removing trailing slashes, or escaping `/` in matched
paths.
This change is breaking by necessity, as some public functions previously
accepted arguments straight from the mux library. I don't really see any
way around that if we want to get rid of the dependency. I don't think
that too many external projects use code from the server directly though,
so I'm thinking the impact should be minimal? Happy to hear what others
think.
Signed-off-by: Anders Eknert <anders@styra.com>
The /v1/data endpoint's PUT, PATCH, and DELETE methods all reported
incorrect timer information (zeroed out timers). This was caused by the
Timer.Stop() calls being deferred in those method handlers.
The problem is that the timers do not record a time value until you stop
them. So, when the metrics are reported, all of those timers are still
storing their initial zero values.
The fix was manually calling Timer.Stop() right before metrics collection
on each endpoint, similar to how we handle many other endpoints in the
server.
Signed-off-by: Philip Conrad <philip@chariot-chaser.net>
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>
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>
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 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>
* topdown+rego: allow opt-in for evaluating non-det builtins in PE
Some use cases of PE, notably generating queries that are to be translated
into filters of some sort (think SQL), require the evaluation of non-deterministic
builtins. This is because the result of the builtin informs what queries are
returned.
Imagine that the user associated with a request is known at PE-time, but we need
extra information from an HTTP API to determine the filters that should be applied.
Previously, that was just impossible to do. Now, we can opt-in to evaluate non-det
builtins during PE from the Rego API.
Note that it would probably make sense to include this in the inlining controls, as
sent to the Compile API. (Considered out of scope for this PR.)
Also note that this will take highest precedence over the `ast.IgnoreDuringPartialEval`
map and the "Nondeterministic" value of the registered builtin. If the new option is
provided, both of these are ignored.
Signed-off-by: Stephan Renatus <stephan@styra.com>
* server+rego: expose nondeterministicBuiltins via inlining controls
With `foo.rego` as
```rego
package ex
include if input.fruits.name == object.get(http.send(input.req).body, input.path, "unknown")
```
the following queries show the difference:
```interactive
$ curl -v http://127.0.0.1:8181/v1/compile \
-d '{"input": {"req": {"url": "https://httpbin.org/json", "method":"GET"}, "path": ["slideshow", "title"]}, "query": "data.ex.include", "unknowns": ["input.fruits"]}'
{
"result": {
"queries": [
[
{
"index": 0,
"terms": [
{
"type": "ref",
"value": [
{
"type": "var",
"value": "http"
},
{
"type": "string",
"value": "send"
}
]
},
{
"type": "object",
"value": [
[
{
"type": "string",
"value": "method"
},
{
"type": "string",
"value": "GET"
}
],
[
{
"type": "string",
"value": "url"
},
{
"type": "string",
"value": "https://httpbin.org/json"
}
]
]
},
{
"type": "var",
"value": "__local0__1"
}
]
},
{
"index": 1,
"terms": [
{
"type": "ref",
"value": [
{
"type": "var",
"value": "eq"
}
]
},
{
"type": "ref",
"value": [
{
"type": "var",
"value": "input"
},
{
"type": "string",
"value": "fruits"
},
{
"type": "string",
"value": "name"
}
]
},
{
"type": "call",
"value": [
{
"type": "ref",
"value": [
{
"type": "var",
"value": "object"
},
{
"type": "string",
"value": "get"
}
]
},
{
"type": "ref",
"value": [
{
"type": "var",
"value": "__local0__1"
},
{
"type": "string",
"value": "body"
}
]
},
{
"type": "array",
"value": [
{
"type": "string",
"value": "slideshow"
},
{
"type": "string",
"value": "title"
}
]
},
{
"type": "string",
"value": "unknown"
}
]
}
]
}
]
]
}
}
```
Here, the builtin call to http.send is preserved.
If we also pass `nondeterminsticBuiltins: true` to the options, we get this:
```interactive
$ curl http://127.0.0.1:8181/v1/compile \
-d '{"input": {"req": {"url": "https://httpbin.org/json", "method":"GET"}, "path": ["slideshow", "title"]}, "query": "data.ex.include", "unknowns": ["input.fruits"], "options": {"nondeterministicBuiltins": true}}'
{
"result": {
"queries": [
[
{
"index": 0,
"terms": [
{
"type": "ref",
"value": [
{
"type": "var",
"value": "eq"
}
]
},
{
"type": "ref",
"value": [
{
"type": "var",
"value": "input"
},
{
"type": "string",
"value": "fruits"
},
{
"type": "string",
"value": "name"
}
]
},
{
"type": "string",
"value": "Sample Slide Show"
}
]
}
]
]
}
}
```
Here, all args to http.send have been known at PE time and the call was fully
evaluated.
Signed-off-by: Stephan Renatus <stephan@styra.com>
* cmd/eval: expose --nondeterminstic-builtins for new PE control
```interactive
$ echo '{"req": {"url": "https://httpbin.org/json", "method":"GET"}, "path": ["slideshow", "title"]}'| ./opa_darwin_amd64 eval -fpretty -p -I -d foo.rego -u input.fruits data.ex.include
+---------+-------------------------------------------------------------------------------------+
| Query 1 | http.send({"method": "GET", "url": "https://httpbin.org/json"}, __local0__1) |
| | input.fruits.name = object.get(__local0__1.body, ["slideshow", "title"], "unknown") |
+---------+-------------------------------------------------------------------------------------+
$ echo '{"req": {"url": "https://httpbin.org/json", "method":"GET"}, "path": ["slideshow", "title"]}'| ./opa_darwin_amd64 eval -fpretty -p -I -d foo.rego -u input.fruits data.ex.include --nondeterminstic-builtins
+---------+-----------------------------------------+
| Query 1 | input.fruits.name = "Sample Slide Show" |
+---------+-----------------------------------------+
```
Signed-off-by: Stephan Renatus <stephan@styra.com>
---------
Signed-off-by: Stephan Renatus <stephan@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>
Anders Eknert