This commit fixes a request handling bug introduced in #6868, which
caused OPA to treat all incoming chunked requests as if they had
zero-length request bodies.
The fix detects cases where the request body size is unknown in the
DecodingLimits handler, and propagates a request context key down to
the `util.ReadMaybeCompressedBody` function, allowing it to correctly
select between using the original `io.ReadAll` style for chunked
requests, or the newer preallocated buffers approach (for requests of
known size).
This change has a small, but barely visible performance impact for large
requests (<5% increase in GC pauses for a 1GB request JSON blob), and
minimal, if any, effect on RPS under load.
Fixes: #6904
Signed-off-by: Philip Conrad <philip@chariot-chaser.net>
This commit introduces a few major changes:
- (Breaking change) Limits now exist for maximum request body sizes.
- Buffers are preallocated for reading request bodies.
- Buffers are preallocated for decompressing request bodies.
- Gzip decoder instances are reused in a `sync.Pool` across requests.
The effect on garbage collection is dramatically fewer GC pauses, giving
a roughly 9% RPS improvement in load tests with gzipped request bodies.
For larger request sizes, the number of GC pauses is dramatically
reduced, although the peak pause time may increase by a few percent.
Implementation notes:
- The DecodingLimits handler enforces the max request body size both
through a Content-Length check, and a MaxBytesReader wrapper around
the payload.
- The DecodingLimits handler passes the gzip payload size limit down
using a context key.
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
This PR fixes an issue where an OPA running authorization policies would
be unable to handle gzipped request bodies.
Example OPA CLI setup:
opa run -s --authorization=basic
Example request:
echo -n '{}' | gzip | curl -H "Content-Encoding: gzip" --data-binary @- http://127.0.0.1:8181/v1/data
This would result in unhelpful error messages, like:
```json
{
"code": "invalid_parameter",
"message": "invalid character '\\x1f' looking for beginning of value"
}
```
The cause was that the request body handling system in the
`server/authorizer` package did not take gzipped payloads into
account. The fix was to borrow the gzip request body handling function
from `server/server.go`, to transparently decompress the body when
needed.
Fixes: #6804
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
It would be useful if users had the ability to enhance the
decision log with info from the incoming HTTP request such as
headers. This change allows users to configure headers whose
values if present in the incoming HTTP request would be
surfaced via the decision log. This can be extended in the
future to include more context from the request.
Fixes: #6693
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
This change attempts to keep the default decision path used by the server
in sync with the one defined on the manager's config. Currently the
server only updates the default decision path when it's initialized and
when there is a commit on the store. The issue happens when the default
decision path is updated via the discovered config. In this case, the
manager's config is updated but there could be no store txn. Hence
the updated value of default decision path is not taken into account by
the server.
Fixes: #6697
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
In workflow runs like this:
https://github.com/open-policy-agent/opa/actions/runs/7803493290/job/21283458848#step:3:317
We can see two problems. This commit is meant to address them.
First, the test failed with this message:
```
expected unknown certificate authority error but got: Get "https://127.0.0.1:38699/v1/data": write tcp 127.0.0.1:52786->127.0.0.1:38699: write: connection reset by peer
```
Now this step in the test is retried like the other steps in the test
since it can fail too.
Second, the error `failed to reload TLS config` appears many times in
the logs for that test. This issue is caused by the server attempting to
read the new cert, key, and CA contents from disk while they are still
being written to. This PR also introduces a 100ms pause between upto 5
attempts to reload the config for any given change to the state on disk.
This should mean that the error is seen only when is is actually an
issue and the reload has failed after a reasonable time. In most cases,
running locally, the reload happens without error on the first run.
Signed-off-by: Charlie Egan <charlie@styra.com>
This change adds a new flag to `opa run` to allow
users to specify a list of enabled TLS 1.0–1.2 cipher
suites. This allows users to control the cipher suites
the OPA server supports during a TLS handshake.
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>
This was missed in https://github.com/open-policy-agent/opa/pull/6415https://github.com/open-policy-agent/opa/actions/runs/7194437460/job/19595009978?pr=6476
We have a warning of this data race here:
```
==================
WARNING: DATA RACE
Write at 0x00c009b562e8 by goroutine 1815:
github.com/open-policy-agent/opa/server.(*Server).reloadTLSConfig()
/src/server/certs.go:65 +0x608
github.com/open-policy-agent/opa/server.(*Server).getListener.(*Server).certLoopNotify.func2()
/src/server/certs.go:174 +0x434
github.com/open-policy-agent/opa/server.TestCertPoolReloading.func1()
/src/server/server_test.go:5105 +0x4f
github.com/open-policy-agent/opa/server.TestCertPoolReloading.func2()
/src/server/server_test.go:5108 +0x41
Previous read at 0x00c009b562e8 by goroutine 1824:
github.com/open-policy-agent/opa/server.(*Server).getListenerForHTTPSServer.func1()
/src/server/server.go:649 +0x145
crypto/tls.(*Conn).readClientHello()
/usr/local/go/src/crypto/tls/handshake_server.go:149 +0x97d
crypto/tls.(*Conn).serverHandshake()
/usr/local/go/src/crypto/tls/handshake_server.go:42 +0x64
crypto/tls.(*Conn).serverHandshake-fm()
<autogenerated>:1 +0x47
crypto/tls.(*Conn).handshakeContext()
/usr/local/go/src/crypto/tls/conn.go:1552 +0x615
crypto/tls.(*Conn).HandshakeContext()
/usr/local/go/src/crypto/tls/conn.go:1492 +0x16b8
net/http.(*conn).serve()
/usr/local/go/src/net/http/server.go:1891 +0x16c0
net/http.(*Server).Serve.func3()
/usr/local/go/src/net/http/server.go:3086 +0x4f
```
Signed-off-by: Charlie Egan <charlie@styra.com>
The partial query parameter has been marked as deprecated since
v0.23.0. It's also removed from the docs since that time.
Fixes: #2266
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
http_request_duration has fixed, hardcoded number of buckets with no possibility to tweak them
For cases when the most of the latencies are above 1ms, with only 4 available buckets there's no good insight on OPA's performance.
This implementation:
- adds the possibility for the buckets to be configurable in ```server.metrics.prom.http_request_duration_seconds.buckets``` key
- it's not a breaking change, if the buckets are not present in the configuration, the metric is configured with the existing values as a fallback
Signed-off-by: aarnautu <aarnautu@adobe.com>
This test case exercises the scenario when a bundle is being written
to the store (ie. active write transaction) and the OPA server handling
a policy eval request at the same time. The former should not block
the later.
Fixes: #4792
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Currently we parse store modules irrespective of whether
there are modules on the Rego object. This will result in
the compilation of those modules which triggers the bundle
activation flow. Now as part of the module compilation
we interact with the compiler's modules and run compilation
on the input modules. If let's say there are concurrent health
check requests (ie. /v1/health), this could result in a race
during the compilation process while working with the compiler's
modules.
This change avoids this situation by skipping parsing of the store
modules when none are set on the Rego object. The assumption this
change makes is that while using the rego package the compiler and
store are kept in-sync.
Fixes: #5868
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
This came up in the discussion of the previous cleanup PR, but I hadn't
gotten around to include it back then.
Signed-off-by: Stephan Renatus <stephan@styra.com>
Currently if OPA listens on a Unix socket, the socket
file is created with 755 permissions. So if OPA is deployed
on k8s for example and the socket path is shared
via a volume between pods, due to the default permissions,
the socket will not be reachable for the caller. One way around
this is to match the user id for the OPA and caller containers
but that is not always possible.
This change adds a new flag to the OPA runtime that allows
to configure the permission of the socket file. In the
k8s scenario, if the file permission is updated to 777
for instance, the caller will be able to connect to OPA via
the socket.
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Currently OPA's decision logs do not include the trace
and span identifier associated with a given request
handled by the server. This information if available
can be helpful to correlate logs and trace data.
This change updates the decision log format to now
include the trace and span identifier if present.
Fixes: #5230
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Currently when OPA's HTTP server rejects requests per
the authz policy, this is not accounted for via the management APIs.
This change adds that count in the metric registry that is
part of the Status API for more visibility.
Fixes: #3378
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
When using OPA TLS authorization, authz policy authors will now have
access to the client certificates presented as part of the TLS
connection. This new data will be available under the key `client_certificates`.
The existing functionality where `identity` is set to the Subject RDN Sequence
is left unchanged.
When using x.509 SVIDs the SPIFFEID is presented as in the SANs of the
certificate. So this change makes that data available should users
require it. It also makes other information about the client cert
available to authz policy too.
Signed-off-by: Charlie Egan <charlie@styra.com>
It is available for the following REST API endpoints:
- GET & POST HTTP methods on /v0/data & /v1/data endpoints
- POST HTTP method on /v1/compile endpoint
HTTP clients can optionally:
- send 'Accept-Encoding: gzip' header and expect a gzip compressed body and a Content-Encoding: gzip response header. The server will send the content encoded as gzip only after a threshold defined by server.encoding.gzip.min_length (default value is 1024). If the size is below the threshold, the body is not compressed
- send 'Content-Encoding: gzip' header and a gzip compressed body and expect the server to correctly interpret the request
Fixes#5310
Signed-off-by: aarnautu <aarnautu@adobe.com>
Similar to how https://github.com/open-policy-agent/opa/pull/5596
allowed streaming JSON in the response, do the same for the request.
This has potentially bigger benefits, as the incoming request is
commonly larger than the response (admission control, terraform, etc).
Signed-off-by: Anders Eknert <anders@styra.com>
Just s small cleanup in the server handler code. Ctx is for
request-scoped data, so let's put the decision ID there, too.
Our Eval and Log helper methods have way too many arguments already.
Signed-off-by: Stephan Renatus <stephan@styra.com>
When sending JSON back to the client — and we do a lot of that, use
the streaming implementation of json.Encode rather than marshalling
the data into an intermediate byte array.
One curious detail here is that the streaming implementation
uses newlines to mark the end of the stream, so a few unit tests had
to be updated to expect this. Previously we would only emit a trailing
newline if "pretty" was configured.
Signed-off-by: Anders Eknert <anders@styra.com>
* [rego] Check store modules before skipping parsing
Fixes https://github.com/open-policy-agent/opa/issues/5511
This change will cause the operation to be timed if the store
modules have already all been compiled and there are no
rawModules. This might be undesirable.
Signed-off-by: Charlie Egan <charlie@styra.com>
Since "document missing or undefined" caused confusion, we can be
more helpful and report whether the default decision document is
missing, or whether it's there but undefined. I would probably have
preferred for undefined to just result in an empty/undefined result,
but that'd be quite a breaking change at this point in time, so
hopefully this can at least make things a little less convoluted.
Fixes#5344
Signed-off-by: Anders Eknert <anders@eknert.com>
This is the OPA side of #4290. It will allow the envoy plugin to wire
the TraceProvider into the gRPC handlers.
Signed-off-by: vinhph0906 <vinhph0906@gmail.com>
Today it is not possible to correlate the decision log with
other types of logs (server, print, etc.) when the server log
level is >= INFO. The log correlation could be helpful in
troubleshooting.
A solution is to add a common attribute in all logs to make
the log correlation possible, so adding the req_id attribute
on decision logs, when server log level is >= INFO, will make it
possible.
Fixes: #5006
* Add documentation about decision log req_id attribute
The documentation purpose is to explain the relation with
others logs, how it could be used, and when it is included
on decision logs.
Signed-off-by: Humberto Corrêa da Silva <humbertoc_silva@hotmail.com>
This change allows rules to have string prefixes in their heads -- we've
come to call them "ref heads".
String prefixes means that where before, you had
package a.b.c
allow = true
you can now have
package a
b.c.allow = true
This allows for more concise policies, and different ways to structure
larger rule corpuses.
Backwards-compatibility:
- There are code paths that accept ast.Module structs that don't necessarily
come from the parser -- so we're backfilling the rule's Head.Reference
field from the Name when it's not present.
This is exposed through (Head).Ref() which always returns a Ref.
This also affects the `opa parse` "pretty" output:
With x.rego as
package x
import future.keywords
a.b.c.d if true
e[x] if true
we get
$ opa parse x rego
module
package
ref
data
"x"
import
ref
future
"keywords"
rule
head
ref
a
"b"
"c"
"d"
true
body
expr index=0
true
rule
head
ref
e
x
true
body
expr index=0
true
Note that
Name: e
Key: x
becomes
Reference: e[x]
in the output above (since that's how we're parsing it, back-compat edge cases aside)
- One special case for backcompat is `p[x] { ... }`:
rule | ref | key | value | name
------------------------+-------+-----+-------+-----
p[x] { ... } | p | x | nil | "p"
p contains x if { ... } | p | x | nil | "p"
p[x] if { ... } | p[x] | nil | true | ""
For interpreting a rule, we now have the following procedure:
1. if it has a Key, it's a multi-value rule; and its Ref defines the set:
Head{Key: x, Ref: p} ~> p is a set
^-- we'd get this from `p contains x if true`
or `p[x] { true }` (back compat)
2. if it has a Value, it's a single-value rule; its Ref may contain vars:
Head{Ref: p.q.r[s], Value: 12} ~> body determines s, `p.q.r.[s]` is 12
^-- we'd get this from `p.q.r[s] = 12 { s := "whatever" }`
Head{Key: x, Ref: p[x], Value: 3} ~> `p[x]` has value 3, `x` is determined
by the rule body
^-- we'd get this from `p[x] = 3 if x := 2`
or `p[x] = 3 { x := 2 }` (back compat)
Here, the Key isn't used, it's present for backwards compatibility: for ref-
less rule heads, `p[x] = 3` used to be a partial object: key x, value 3,
name "p"
- The destinction between complete rules and partial object rules disappears.
They're both single-value rules now.
- We're now outputting the refs of the rules completely in error messages, as
it's hard to make sense of "rule r" when there's rule r in package a.b.c and
rule b.c.r in package a.
Restrictions/next steps:
- Support for ref head rules in the REPL is pretty poor so far. Anything that
works does so rather accidentally. You should be able to work with policies
that contain ref heads, but you cannot interactively define them.
This is because before, we'd looked at REPL input like
p.foo.bar = true
and noticed that it cannot be a rule, so it's got to be a query. This is no
longer the case with ref heads.
- Currently vars in Refs are only allowed in the last position. This is expected
to change in the future.
- Also, for multi-value rules, we can not have a var at all -- so the following
isn't supported yet:
p.q.r[s] contains t if { ... }
-----
Most of the work happens when the RuleTree is derived from the ModuleTree -- in
the RuleTree, it doesn't matter if a rule was `p` in `package a.b.c` or `b.c.p`
in `package a`.
As such, the planner and wasm compiler hasn't seen that many adaptations:
- We're putting rules into the ruletree _including_ the var parts, so
p.q.a = 1
p.q.[x] = 2 { x := "b" }
end up in two different leaves:
p
`-> q
`-> a = 1
`-> [x] = 2`
- When planing a ref, we're checking if a rule tree node's children have
var keys, and plan "one level higher" accordingly:
Both sets of rules, p.q.a and p.q[x] will be planned into one function
(same as before); and accordingly return an object {"a": 1, "b": 2}
- When we don't have vars in the last ref part, we'll end up planning
the rules separately. This will have an effect on the IR.
p.q = 1
p.r = 2
Before, these would have been one function; now, it's two. As a result,
in Wasm, some "object insertion" conflicts can become "var assignment
conflicts", but that's in line with the now-new view of "multi-value"
and "single-value" rules, not partial {set/obj} vs complete.
* planner: only check ref.GroundPrefix() for optimizations
In a previous commit, we've only mapped
p.q.r[7]
as p.q.r; and as such, also need to lookup the ref
p.q.r[__local0__]
via p.q.r
(I think. Full disclosure: there might be edge cases here that are unaccounted
for, but right now, I'm aiming for making the existing tests green...)
New compiler stage:
In the compiler, we're having a new early rewriting step to ensure that the
RuleTree's keys are comparible. They're ast.Value, but some of them cause us
grief:
- ast.Object cannot be compared structurally; so
_, ok := map[ast.Value]bool{ast.NewObject([2]*ast.Term{ast.StringTerm("foo"), ast.StringTerm("bar")}): true}[ast.NewObject([2]*ast.Term{ast.StringTerm("foo"), ast.StringTerm("bar")})]
`ok` will never be true here.
- ast.Ref is a slice type, not hashable, so adding that to the RuleTree would
cause a runtime panic:
p[y.z] { y := input }
is now rewritten to
p[__local0__] { y := input; __local0__ := y.z }
This required moving the InitLocalVarGen stage up the chain, but as it's still
below ResolveRefs, we should be OK.
As a consequence, we've had to adapt `oracle` to cope with that rewriting:
1. The compiler rewrites rule head refs early because the rule tree expects
only simple vars, no refs, in rule head refs. So `p[x.y]` becomes
`p[local] { local = x.y }`
2. The oracle circles in on the node it's finding the definition for based
on source location, and the logic for doing that depends on unaltered
modules.
So here, (2.) is relaxed: the logic for building the lookup node stack can
now cope with generated statements that have been appended to the rule bodies.
There is a peculiarity about ref rules and extents:
See the added tests: having a ref rule implies that we get an empty object
in the full extent:
package p
foo.bar if false
makes the extent of data.p: {"foo": {}}
This is somewhat odd, but also follows from the behaviour we have right now
with empty modules:
package p.foo
bar if false
this also gives data.p the extent {"foo": {}}.
This could be worked around by recording, in the rule tree, when a node was
added because it's an intermediary with no values, but only children.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
This would be useful for us for two immediate use cases:
1. Show how and why rules failed in more detail in verbose tooling,
we can show the unification happening step by step.
2. We can trace which parts of the input document were used, if we
add `Location` info those terms.
However, I think it's generally useful for debugging tools.
This increases verbosity in the explain logs, so we decided to add a new explain
mode `debug` in addition to the existing `full`, `notes`, `fails`, `off` modes.
This can be set using the `--explain=debug` flag on the CLI, or by using `trace
debug` in the REPL.
Signed-off-by: Jasper Van der Jeugt <m@jaspervdj.be>
This commit integrates the non-deterministic builtins caching system
into decision logging, both in the server and sdk packages. Some
reworking of the NDBCache's serialization format were required to
accommodate this. The feature is disabled by default, and must be
opted into by user configuration.
The feature can be enabled via a top-level config key:
nd_builtin_cache=true
The NDBCache is exposed to the masking system under the
`/nd_builtin_cache` path, which allows masking or dropping sensitive
values from decision logs selectively.
Note: If a decision log event exceeds the `upload_size_limit_bytes`
value for the OPA instance, OPA will reattempt uploading it, after
dropping the NDBCache from the event. This behavior will trigger a log
error, and will increment the `decision_logs_nd_builtin_cache_dropped`
metrics counter.
Fixes: #1514
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Before, we'd only check if the NEW policy path was owned by a bundle. Now,
we'll also check if the to-be-updated policy is owned by a bundle. If so,
return an error.
Fixes#4846
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Before, the option InterQueryCache(...) passed to the authorizer's config
had set it to `nil` -- it wasn't set up yet.
Now, the ordering allows for caching in the system authz policies.
Fixes#4829.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Currently bundles are loaded into memory entirely
even when disk storage is used. Then the parsed content
is written to the store. Deserializing data into Go structs
is memory consuming and even if user has configured disk
storage, OPA is still bound by the amount of memory
assigned to it. This change adds a new lazy loading mode
wherein the entire data is not deserialized while bundle
reading and hence if the bundle contains large data files
and the user has enabled disk storage, OPA should be
able to handle this scenario w/o running OOM.
Fixes: #4539
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
This commit adds a new flag to the opa bench command which
allows users to run benchmarks against a running OPA server.
This mode can be used to evaluate the additional overhead the
server is going to introduce.
Co-authored-by: Anders Eknert anders@eknert.com
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Currently a write transaction on the store is opened
inside the query API handler (ie. v1DataPost). What this
means is if the user wants to evaluate a policy while
OPA is activating a bundle, the api request would block
trying to open a write txn on the store till the bundle
was activated. If OPA is activating a large bundle,
the write lock is held longer and hence the client would
notice the latency in OPA's API response. This change updates
the query handler to open a read txn on the store which was
the case pre-v0.39.0.
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Philip Conrad