This commit fixes another data race found by running go test
-race. The server was reading state modified by the trigger handler
without using a mutex or being inside of a transaction.
Note, we should think about how we can move away from relying on the
server transactions to implement critical sections in the server as
this is bound to bite us in the future.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Previously we let it use the default namespace, which meant that
every cached evaluation would use the same query on the compiler..
which isn't correct. They need to be unique per path.
We'll now use a hash of the path (since it needs to be a valid var).
While doing this the logic for the Rego opts was refactored in
`makeRego` to only define the list a single time.. this should help
reduce the risk of any regressions in the future.
Fixes: #2247
Signed-off-by: Patrick East <east.patrick@gmail.com>
Previously we would iterate over all modules on the compiler and then
look them up by id in the store. This causes problems when there are
partial compilation results on the compiler that have not been put in
the store.
To correct this we will now reverse how it iterates, instead iterating
over a list of policies from the store and looking up their AST from
the compiler as needed.
Fixes: #2036
Signed-off-by: Patrick East <east.patrick@gmail.com>
This deprecates `/health?bundle` in favor of (the plural)
`/health?bundles`.
Both have the same API result, taking into account *all* bundle
statuses. This just lessen any confusion with regards to whether
`bundle` means a single bundle or all. It fits better with the new
`plugins` option.
Signed-off-by: Patrick East <east.patrick@gmail.com>
The health check now supports a `?plugin` option which will make the
response depend on whether or not all configured plugins are in an OK
state.
The `bundle` parameter will now use the bundle *and* discovery plugin
statuses to determine if the bundles are ready. This corrects an issue
where discovery bundles, and bundles defined by the discovery dynamic
config, were not included with `/health?bundle=true` checks.
The URL parameter parsing has also changed to allow for omitting the
value for the `bundle` option. It will default to `true` so that
`/health?bundle=true` can be shortened to `/health?bundle`.
Fixes: #2010Fixes: #2015
Signed-off-by: Patrick East <east.patrick@gmail.com>
The logger was swapping `.`'s with `/`'s but this isn't safe when a
valid path should be /foo/a.b.c/main. The server was already doing
the right thing by passing in the url path where applicable, or only
specifying a query instead of the path.
This might affect anyone using the decision logger golang API passing
in something in dot-notation and expecting it to come out with paths.
Anyone using the HTTP server should be unaffected.
Fixes: #2031
Signed-off-by: Patrick East <east.patrick@gmail.com>
The metric was using `metrics.RegoQueryParse` but it isn't actually
doing the query parse there. It is parsing the input document.
Signed-off-by: Patrick East <east.patrick@gmail.com>
This adds in caching of prepared queries for versioned and unversioned
data queries (POST/GET to `/`, `/data`, and `/v1/data`).
The cache has a max size of 100 and just starts acting as a circular
buffer for queries (FIFO, no smarts for LRU caching or anything). It
seems like it would be unlikely for these API's to hit the cache max
size. Most OPA use-cases have a single query that is re-used over
and over with different inputs.
There is a new metric `counter_server_query_cache_hit` which will
show whether or not a request used the query cache or not. It is there
primarily to help explain away why sometimes a handful of the other
metrics aren't there (the query parse/compile/etc).
In the future this could be added to the query API's too. This change
does not touch anything other than the "data" API's.
Closes: #1567
Signed-off-by: Patrick East <east.patrick@gmail.com>
The issue was that with bundles loaded from the file system we would
not initialize the mutex used for checking bundle status.
This fixes the initialization and prevents the error. Health status
works as expected now.
Fixes: #1703
Signed-off-by: Patrick East <east.patrick@gmail.com>
Previously the server didn't wrap the error messages which made it
hard to determine the source of internal errors coming back from OPA
when deployed with a custom decision logger.
Fixes#1367
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
* Remove metric provider config to avoid introducing new public
interfaces. Since there is only one provider (prometheus) and it
doesn't have any configurable settings, remove the configuration
changes for now. We can always add these in the future.
* Remove dummy metric provider implementation. This isn't needed now
that we're using the metrics.Metrics interface instead of
metrics.GlobalMetrics.
* Remove metrics.GlobalMetrics in favour of metrics.Metrics. Move the
HTTP handler instrumentation interfaces into the server package to
avoid coupling the metrics package to the net/http package.
* Refactor the prometheus provider to implement the metrics.Metrics
interface. Since the prometheus registry can error on Gather()
calls, the provider has been updated to accept a logger and use ti
when the Gather() call fails. This doesn't affect any public
interfaces so it can be revisited in future if needed. Alteratnively
we could add a Gather() interface onto metrics.Metrics which could
return the error.
* Refactor status plugin to include metrics in status update by
default. Users implementing the status API are likely to need
performance metrics to gauge the OPA's health. Moreover if they are
implementing the status API it's unlikely they will want to poll the
/metrics endpoint on the OPA HTTP API (which may not even be
exposed.)
* Move the prometheus endpoint test case into the e2e package so the
server package has no dependencies on prometheus anymore.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Prometheus metrics can give much of insight into OPA's health.
Run-time metrics are a natural part of the application state
so having them in status update seems like a right change
that can help server understand what's going in with the OPA
instance.
The commit also encapsulates all prometheus-related code in one package
and abstracts it with generic interface so that it would be possible
to add other metrics providers
Addresses #1606
Signed-off-by: Stan Lagun <stan@styra.com>
This commit removes the deprecated diagnostic feature from the
server. The feature has been deprecated since November 2018 and it was
essentially unused at the time so it should be safe to
remove. Removing the diagnostic support from the server saves having
to perform an extra policy evaluation in the server.
Once the buffer is removed from the runtime.Params struct the related
issue can be closed (there is still one known user of that so it has
been left intact for backwards compatibility.)
Ref #1052
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
This commit adds `http_request_cancellations` counter to the Prometheus
metrics. Having such a counter helps identify requests that were timed
out or the client just got disconnected before the request result was
computed.
Signed-off-by: Stan Lagun <stan@styra.com>
This change brings in support for multiple bundles to be downloaded
and activated OPA.
This is enabled by using the new config option `bundles` to define
the bundles, and deprecates the older `bundle` option.
The new `bundles` keyword and structure is propagated through to the
decision logs, status API, provenance, stored manifests, etc. Check
out the doc changes for all the updated structures.
That being said any existing configuration using `bundle` will *not*
see the new structure, everything is intended to be backwards
compatible (almost to a fault).
Fixes: #721
Signed-off-by: Patrick East <east.patrick@gmail.com>
This will allow others consuming OPA as a library to have easier
access to manage/query the bundle manifests.
Signed-off-by: Patrick East <east.patrick@gmail.com>
This patch moves the check for unsafe built-ins from the server to the
compiler, so it can be used by the Go API as well as the HTTP API.
This was previously discussed in #1570.
Signed-off-by: Jasper Van der Jeugt <jasper@fugue.co>
These changes update the server to pass the server's open transaction
to the decision logger. This prevents the same goroutine from
recursively opening a new transcation when the log masking decision is
evaluated.
Alternatively we could update the server to close it's transaction
before logging the decision however this could lead to the log masking
decision being generated from a different policy revision. Another
alternative would be extend the storage layer to support recursive
transactions however this would be quite a bit more work.
We should investigate whether we can cheaply detect recursive
transactions in the store to avoid potential deadlocks in the future.
Also, delete opa binary that was accidentally committed to the repo.
Fixes#1543
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
To do this we needed a way to get the actual address that was bound.
To do that we needed to refactor the server and runtime code a tad
to let us create our own `net.Listener`s and get their address _after_
they had been started. The code is pretty much 1:1 with what is in the
official `http` package.
Now when the tests run using the helpers to stand up server runtimes
they should all be on separate ports.. in theory we could run the
unit tests in parallel without concern (for the e2e parts anyway).
Fixes: #1533
Signed-off-by: Patrick East <east.patrick@gmail.com>
The server was leaking the write transaction when parsing failed which
caused subsequent updates to block. In future we should refactor the
handler implementations to avoid having to manually abort on each
error path.
Fixes#1478
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
There is a new parameter for /health REST API which will include the
Configured bundle activation in the response. Example:
GET /health?bundle=true HTTP/1.1
Without the parameter the behavior stays the same, with it the server
will respond with 500’s until the status has been updated with an
activation time.
The docs for kubernetes ready probe has been updated to show this as
it makes for a better ready check than the original behavior when
remote bundles are being used.
Fixes: #1153
Signed-off-by: Patrick East <east.patrick@gmail.com>
Add the new 'provenance' query parameter to the v1DataGet() and
v1DataPost() routines.
Set the fields of the DataResponseV1 struct with the version/build
values for possible return with the result of the data operations.
Add a unit test by setting version info, than ensuring we get the
strings in the response. Note that the revision is left unset.
Signed-off-by: Peter W. Morreale <pmorreale@statestreet.com>
These changes update the server to compare the body of the HTTP
request with the existing policy in the store. If the bytes are equal,
the server returns immediately.
These changes are being made to workaround poor parser performance
given deeply nested JSON objects that are commonly found in Kubernetes
use cases. Specifically, we've observed very high parse times (e.g.,
over a second) when processing policies in production
environments. The problem becomes worse when kube-mgmt is being used
because it periodically resyncs the policies causing load even if the
policies are not changing.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
To make this work it adds in a new shim interface for the server
to interact with the `http.Server`(s). We mock one out and can then
have it return errors on `Shutdown`.
Signed-off-by: Patrick East <east.patrick@gmail.com>
There is a new CLI option to configure the grace period:
`--shutdown-grace-period`
The option defaults to 10 seconds.
When a SIGINT or SIGTERM is sent the runtime will catch them and
attempt to gracefully shutdown the http servers. If the timeout is reached
it will log an error and continue with exiting (in a less graceful way).
Unit testing is a little bit light on this. Its touching some parts of the
code that aren't super easy to mock out. We can probably refactor
things in the future to make it easier.
Fixes: #1291
Signed-off-by: Patrick East <east.patrick@gmail.com>
In the Data API, the `path` attribute of `PATCH` operations was
not handling escaped `~` and `/` values (escaped as `~0` and `~1`,
respectively) according to the JSON-Pointer RFC.
The API was using (and continues to support) URL escaping of these
characters (so `%7E` and `%2F`, respectively), but this was not
documented and is not strictly correct according to the documentation.
However, this is being left unchanged to preserve backwards compatibility
for anyone who stumbled across this behaviour.
With this change, `~0` and `~1` in the `path` attribute of `PATCH`
operations will be unescaped into `~` and `/` as defined in the RFC.
Signed-off-by: Geoff Baskwill <me@geoffbaskwill.ca>
Previously, the decision logger interface did not allow plugin
implementations to return an error. In some cases, implementations may
prefer to make OPA fail-closed if the event cannot be emitted.
This is a backwards incompatible change to the custom decision logger
API that was added in v0.10.3 and it deprecates the old diagnostic
interface as well.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Previously the build version was recorded in the version package and
then different components would report it in an ad-hoc manner, e.g.,
the REPL has a module that generates a virtual doc with the version
info in it, the server was using templating to do the same, etc.
These changes remove the special code from the REPL and server
implementations to report the version. Instead the runtime writes the
version into /system/version at boot.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
This change appends a newline to pretty printed API results to improve readability on the command line. Also, fix tests that were asserting on string equality instead of JSON value equality.
Signed-off-by: Josh Marshall <joshua.r.marshall.1991@gmail.com>
These changes update the server to check the bundle scope before
writing any policy and data. This way the caller won't accidentally
write policy and data into OPA only to have it overwritten the next
time a bundle is downloaded and activated.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Previously there were no checks in place to ensure that base and
virtual documents do not overlap. As a result, if users loaded raw
JSON and rules into OPA that overlapped, the evaluation results were
not well defined. With these changes, we can detect the overlap and
reject updates (to policies or data) that would cause inconsistent
results.
Fixes#1207
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Previously paths and queries were modelled with the same
attribute. This was going to cause headaches down the road for
decision log consumers that need to be able to deal with both kinds of
policy invocations.
As part of these changes, the decision logging test in the server has
been refactored to make it a bit more maintainable.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Previously, the server was not including a decision ID in log events
for errors. This commit changes that.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Previously the input attribute was no represented as *interface{}
which makes it impossible to differentiate between null and undefined
input. This commit changes that. Eventually we should just get rid of
server.Info in favour of the decision log event structure.
This change requires a release note.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
These changes add a new metric into the server that calculates the
time taken from when the handler is invoked to the point where the
decison log is emitted. Hopefully this will prevent us from missing
important periods in the request path.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Fixes#1081
curl -G -v "http://localhost:8181/v1/query" --data-urlencode "q=^ -i"
* Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8181 (#0)
> GET /v1/query?q=%5E%20-i HTTP/1.1
> Host: localhost:8181
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 400 Bad Request
< Content-Type: application/json
< Date: Sat, 15 Dec 2018 09:13:58 GMT
< Content-Length: 118
<
{
"code": "invalid_parameter",
"message": "1 error occurred: 1:1: rego_parse_error: no match found\n\t^ -i\n\t^"
* Connection #0 to host localhost left intact
}%
Signed-off-by: repenno <rapenno@gmail.com>
Query API should return HTTP 400 if query does not parse
Fixes#1081
curl -G -v "http://localhost:8181/v1/query" --data-urlencode "q=^ -i"
* Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8181 (#0)
> GET /v1/query?q=%5E%20-i HTTP/1.1
> Host: localhost:8181
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 400 Bad Request
< Content-Type: application/json
< Date: Sat, 15 Dec 2018 09:13:58 GMT
< Content-Length: 118
<
{
"code": "invalid_parameter",
"message": "1 error occurred: 1:1: rego_parse_error: no match found\n\t^ -i\n\t^"
* Connection #0 to host localhost left intact
}%
Signed-off-by: repenno <rapenno@gmail.com>
Query API should return HTTP 400 if query does not parse
Fixes#1081
Reworked fix based on comments and current code.
v1QueryGet/Post already call ast.parseBody through validateQuery,
therefore the solution:
- Avoids double parsing query
- Keeps original query string around so logs can work
- Performs http error checking at the server.go level
- Enhanced testing to make sure all messages are sane
Optionally error checking in server.go could be factored in a
unified function.
Signed-off-by: repenno <rapenno@gmail.com>
Query API should return HTTP 400 if query does not parse
Fixes#1081
Reworked fix based on comments and current code.
v1QueryGet/Post already call ast.parseBody through validateQuery,
therefore the solution:
- Avoids double parsing query
- Keeps original query string around so logs can work
- Performs http error checking at the server.go level
- Enhanced testing to make sure all messages are sane
Optionally error checking in server.go could be factored in a
unified function.
Signed-off-by: repenno <rapenno@gmail.com>
Query API should return HTTP 400 if query does not parse
Fixes#1081
Reworked fix based on comments and current code.
v1QueryGet/Post already call ast.parseBody through validateQuery,
therefore the solution:
- Avoids double parsing query
- Keeps original query string around so logs can work
- Performs http error checking at the server.go level
- Enhanced testing to make sure all messages are sane
- Fixed existing server_test.go tests affected by requested changes
Optionally error checking in server.go could be factored in a
unified function.
Signed-off-by: repenno <rapenno@gmail.com>
* identifier: add TLSBased
This is only the identifier, the server setup still has to be done.
Note that it diverges a little from what was proposed in the issue:
not every client cert needs to have a CN record -- so instead, we'll
use whatever is the cert's subject as client identity.
* Drive-by fix: identifier_test: don't use same package for TokenBased
tests.
* server: require and verify client cert for AuthenticationTLS
* server: allow setting CA pool via --tls-ca-cert-file
* server: expose new authentication via parameter
* [nit] server: simplify getListenerForHTTPServer
* server_test: use httptest for integration-y TLS tests
* book/security: mention TLS authn with example
Signed-off-by: Stephan Renatus <srenatus@chef.io>
These changes refactor the discovery implementation a bit to improve
test coverage and remove duplication of common logic shared with the
bundle plugin.
Specifically, the downloading logic has been moved into a separate
package that is shared by bundle and discovery. Second, test coverage in
the discovery implementation is increased from ~15% to ~85%.
These changes also include a few functional improvements:
- The default decision paths can be updated dynamically
- The decision logger can be enabled dynamically
- Discovery downloading errors are reported in status updates
- Discovery bundle is evaluated with all runtime params
- Custom plugins can be created dynamically
- Status updates include both discovery and bundle status
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
This allows callers to embed the OPA HTTP server inside another HTTP
server (e.g., under a path prefix like /opa).
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Previously, OPA would serve POST requests with an empty URL path by
querying data.system.main and returning the generated value. In some
cases, it's useful to be able to reconfigure OPA to use a different
document to serve these kinds of requests. The same goes for the OPA
authorization policy.
These changes make the default decision and default authorization
decision paths configurable.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
These changes add support for accessing runtime information inside of
policies. In some cases, policies need to access environment variables
or configuration that OPA was booted with. These changes add a built-in
function that allows policies to gain access to this information. The
built-in function itself is relatively trivial. Most of the required
changes were plumbing the runtime information from the entrypoint down
into the evaluation engine. The alternative would have been to introduce
a global variable containing this information however that would be have
been harder to reason about in library integrations.
Fixes#420
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Torin Sandall