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>
- 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 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>
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>
The topdown Cancel machinery is there because it's cheap to check. ctx.Err() is
not.
This change moves the "Is the context the cause for cancellation?" check into
the branch were evaluation has been aborted through the topdown.Cancel call.
When evaluation has already been cancelled, an expensive check no longer
matters much -- when it's still ongoing, it'll affect the overall performance.
Signed-off-by: Stephan Renatus <stephan@styra.com>
Per discussion in https://github.com/open-policy-agent/opa/pull/7188, do *not* adjust the timing of cache scans to be more frequent; we want to wait at least staleEntryEvictionTimePeriodSeconds between cache scans, even if we hold the lock for a substantial period of time.
Signed-off-by: Evan Anderson <evan@stacklok.com>
For the first time, down under a 100 million allocations
when running `regal lint bundle` 🎈
**main**
```
BenchmarkLintAllEnabled-10 1 2538350916 ns/op 6182626816 B/op 108424249 allocs/op
```
**pr**
```
BenchmarkLintAllEnabled-10 1 2282894416 ns/op 5310032744 B/op 93674054 allocs/op
```
But there's more to it than just the number of allocations:
```
➜ hyperfine -i --warmup 1 'regal lint bundle' 'regal-new lint bundle'
Benchmark 1: regal lint bundle
Time (mean ± σ): 2.822 s ± 0.055 s [User: 19.299 s, System: 0.603 s]
Range (min … max): 2.743 s … 2.961 s 10 runs
Benchmark 2: regal-new lint bundle
Time (mean ± σ): 2.373 s ± 0.040 s [User: 15.940 s, System: 0.575 s]
Range (min … max): 2.315 s … 2.435 s 10 runs
Summary
regal-new lint bundle ran
1.19 ± 0.03 times faster than regal lint bundle
```
Most notable changes:
- Reuse trieTraversalResult in indexing, as these were expensive
and short-lived. This had the most dramatic impact on the number
of reduced allocations of all the changes here.
- Optimize *set, *object and *Array operations to minimize
allocations by using "primitive" form iteration instead of
the function literal counterparts internally, and to only
reset the sort guard when needed.
- New Array.Equal implementation does not remove any allocations
as the old implementation didn't allocate either. It did however
perform much better for the case where the compared arrays were
not equal.
Signed-off-by: Anders Eknert <anders@styra.com>
# Conflicts:
# topdown/casts.go
This changes the `eval_cancel_error` logic to differentiate between
context.Canceled and context.DeadlineExceeded errors, because they are
distinct errors. By differentiating here, callers will be able to
determine if a call was stopped because a caller stopped the request, or
because the request went over some imposed deadline.
Signed-off-by: Mike Chittenden <mchittenden@digitalocean.com>
This isn't needed anymore, so now we don't.
Also enabled the copyloopvar linter in case we
accidentally do this in the future.
Signed-off-by: Anders Eknert <anders@styra.com>
**main**
```
BenchmarkLintAllEnabled-10 1 2640715625 ns/op 6385110200 B/op 116296633 allocs/op
```
**pr**
```
BenchmarkLintAllEnabled-10 1 2597179708 ns/op 6183614112 B/op 108421141 allocs/op
```
(I renamed the benchmark, but this is the same as "regal linting itself"
used in the past)
Another 8 million allocations cut off from `regal lint bundle`,
and a whopping 10% improvements to wall clock time!
The most significant improvement is the Equal implementation for
refs, since that is called all over the place. But there are many
other fixes here, and they all contribute something substantial
(and fixes that only have had marginal impact have been left out).
Signed-off-by: Anders Eknert <anders@styra.com>
The previous version has been failing without any good reason for me,
so let's try this.
About the version pick: It's not the latest version (v1.62.0 at the
moment), because that would introduce a new revive rule,
redeclares-builtin-id, and that flags every variable called `min` or
`max` in the code base. I had started addressing these, but they were
just too many.
The new issues related to this version are mostly that it complains
whenever it finds a non-static string that makes its way into a printf-
like function. However, that's a common pattern in some place here, so
I've sprinkled some nolint:govet on it.
Signed-off-by: Stephan Renatus <stephan@styra.com>
Concluding my quest to reduce the number of allocations in the
hot path for `regal lint` for this time around. This PR mainly
does so by reusing pointers to boolean and integer terms where
these are determined not to be mutated later.
The result is another ~4 million allocations reduced when
linting Regal against its own bundle. These improvements should
however help reduce allocations in pretty much any evaluation.
**opa main**
```
BenchmarkRegalLintingItself-10 1 3195257584 ns/op 6496097784 B/op 120108808 allocs/op
```
**PR branch**
```
BenchmarkRegalLintingItself-10 1 3132126333 ns/op 6376318224 B/op 116163318 allocs/op
```
Signed-off-by: Anders Eknert <anders@styra.com>
* test: Parallelize package level tests in high-cost packages.
This commit adds `t.Parallel()` calls to the beginning of many tests
across several Go packages in OPA. The slowest packages (taking ~10s or
more) have been instrumented where possible as a proof-of-concept. On a
machine with many cores, the tests now will complete as fast as the
slowest test per package, instead of the sum of all the tests in a
particular package.
* server/server_test: Remove 3x tests from parallel set.
This commit fixes a data race that could occur in the `server` package
tests, because 3x tests were modifying package variables under
`internal/version`. These tests now run sequentially, and are not
included in the parallel test set.
* plugins/bundle/plugin_test: Remove 2x tests from the parallel set.
Two tests in this package modified a package variable directly, and as
such cannot be safely run in parallel with each other or any other tests
in the package.
* topdown/*_test: t.Parallel refactors.
This commit wraps up a large batch of fairly mechanical refactorings to
add t.Parallel() annotations to almost every test under `topdown`. The
tests that could not be safely parallelized now have explicit warning
comments on them describing why they are not safe to run in parallel.
* storage/disk: t.Parallel refactors.
This commit bundles up test parallelization changes for the
`storage/disk` package, dramatically reducing its execution time.
* topdown/net_test: Remove sub-test parallelization.
* rego: t.Parallel refactors.
This commit includes a bundle of t.Parallel refactoring changes for the
`rego` package, including a timer-related bugfix, and a slight change on
a cancellation test to reduce its overall cost during test runs (the
logic is preserved, but the mandatory timeouts are lower now).
* test: Fixes for sporadic test breakages.
---------
Signed-off-by: Philip Conrad <philip@chariot-chaser.net>
The construction of intermediate throwaway `ast.Array`s caused almost
5 million allocations when running `regal lint` against its own bundle.
While the results are likely more impressive for Regal policies than the
average one, this is still a substantial improvement across a large group
of policies.
**regal lint OPA main**
```
BenchmarkRegalLintingItself-10 1 3317838417 ns/op 6611412800 B/op 124873123 allocs/op
```
**regal lint this branch**
```
BenchmarkRegalLintingItself-10 1 3140384416 ns/op 6590159176 B/op 120098613 allocs/op
```
Signed-off-by: Anders Eknert <anders@styra.com>
- Updated extractNumAndUnit to recognize and correctly parse scientific notation, including cases with 'e' or 'E' followed by an exponent (e.g., "1e10", "3.2E4").
- Ensures that 'e' or 'E' is treated as part of the number if followed by digits, enhancing compatibility with scientific notation inputs.
- Scientific notation with units now also parses correctly (e.g., "1e10GB" extracts "1e10" as the number and "GB" as the unit).
- Maintains behavior for cases without units, allowing either the number or unit portion to be empty, while improving overall string parsing logic.
- Added tests for scientific notation parsing in `units.parse_bytes`, including both SI and binary units (e.g., KB, MiB, GiB, KiB).
- Included cases for uppercase, lowercase, and mixed case formats with scientific notation.
- Added validation for numbers without units defaulting to bytes.
Fixes#7142
Signed-off-by: bakar <berdan.akar@rise-world.com>
This commit adds a utility for explicitly creating cancelable timers, to avoid
possible memory leaks caused by some `<-time.After` timer receives in select
statements never being GC'd properly. This issue is fixed in Go 1.23, but
since we're still on Go 1.21, this will resolve the possibility of leaks in
the mean time.
Signed-off-by: Philip Conrad <philip@chariot-chaser.net>
I figured I'd test this out anyway, and this seemed like
a good case given that there was an actual issue on this.
Testing response times with OPA running as a server, and
the first request is ~800 ms while the following ones are
~10 ms.
Fixes#7011
Signed-off-by: Anders Eknert <anders@styra.com>
No functional changes, just
- using ast.String(..) instead of ast.StringTerm(..).Value
- using const where package-level strings never change (metrics names)
Signed-off-by: Stephan Renatus <stephan@styra.com>
This commit adds a new inter-query value cache that built-in
functions can use to cache information across queries.
For example, the `regex` and `glob` builtins can use this
to cache compiled regex and glob match patterns respectively.
The number of entries in the cache can be configured via the OPA
config. By default there is no limit.
Fixes: #6908
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Fixes incorrect `Timer` measuring in case of error. Without calling `Stop()` method when e.g. `eval_cancel_error` happens the last delta is not added to the accumulated `Timer` value.
I discovered it by investigating `eval_cancel_error` when the caller gave up but according to OPA metrics it looked like `http.send` did not cause it (I would expect near 10s value in `timer_rego_builtin_http_send_ns` too):
```
{"counter_rego_builtin_http_send_interquery_cache_hits":1,"counter_server_query_cache_hit":1,"timer_rego_builtin_http_send_ns":124580,"timer_rego_input_parse_ns":4771,"timer_rego_query_eval_ns":9770617400,"timer_server_handler_ns":9770659804}
```
Signed-off-by: lukyer <lukyer@gmail.com>
Fixing issue where ref-head rules could put evaluation result scoped by call-site ref unification into global virtual-cache, which would later erroneously be read by ref to same rule/virtual document but with different "unification scope".
Fixes: #6926
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Fixes https://github.com/open-policy-agent/opa/issues/6925
This change modifies the `io.jwt.encode_sign` built-in to marshal the
parameters as JSON before signing the JWT. The previous implementation
was using String(), which was a sort of pseudo-JSON that was almost
correct but used the wrong JSON syntax for the set type.
I have chosen to marshal the headers in the same way as the payload as
the 'crit' header can also be an array type. I marshal the signature the
same way for consistency with the other two.
Signed-off-by: Charlie Egan <charlie@styra.com>
Fixing possible memory leak where caches grow uncontrollably when large amounts of regexes or globs are generated or originate from the input document.
Fixes: #6828
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Fixing an issue where a non-collection `every`-domain didn’t fail evaluation.
Removing a possible attack surface, where an attacker with the ability to craft portions of the input document could replace a value with an expected collection type, that is known to be processed by an `every`-statement, with a non-collection value and thereby would cause the policy to accept a query that should otherwise be rejected.
Fixes: #6762
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Fixing two issues where Early Exit was being suppressed when it shouldn't have been:
1. A cache hit for a rule/function discards EE for the call-site.
2. Non-EE rule/func discards EE for call-site.
Fixes: #6566
To support uses cases where OPA is used for signing s3 requests whose payload is
not known upfront or payload is big enough (big file upload) to be sent over wire,
this PR adds support for unsigned payloads.
AWS signer has configurable option to use unsigned payload where the
x-amz-content-sha256 is set to "UNSIGNED-PAYLOAD" and is included as part
of signing process. This PR provides an option for unsigned payload if
aws_config.disable_payload_signing is set to true. If payload signing is
disabled, SignV4 method will not compute the content sha from the request body
but instead use "UNSIGNED-PAYLOAD" string literal for x-amz-content-sha256
header during signature computation.
References:
https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.htmlhttps://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-auth-using-authorization-header.html
Signed-off-by: Prasanth Jayachandran <p_jayachandran@apple.com>
Currently the `raise_error` flag is not honored during the
input validation step. So `http.send` will return an error if
input validation fails irrespective of the `raise_error` flag
status. This change attempts to fix that.
Also the description of the `raise_error` flag is updated to
reflect actual behavior.
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Regularly clean up of cache entries that have expired for a more efficient use of memory.
Introduce two new parameters to tune clean up frequency and threshold for forced FIFO eviction.
Fixes#5320
Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>
Currently while all the headers are signed the headers in the
returned object are missing all the original user provided headers
This means that if you pass the object directly to http.send
amazon will fail the request because the signed data doesn't match
the canonical request. Users can work around it by using object.union
to restore the original headers, but would be nice to avoid that extra
step
Signed-off-by: Peter <c2zwdjnlcg@users.noreply.github.com>
Anders Eknert