* storage: allow overriding NonEmpty
Custom store implementations can now bring their own NonEmpty() methods,
which may be more efficient than what the generic method does.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
* runtime: allow passing in custom store builder
Signed-off-by: Stephan Renatus <stephan@styra.com>
---------
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.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>
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>
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>
* 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>
A new optimized read mode has been added to the default in-memory store, where data written to the store is eagerly converted to AST values (the data format used during evaluation). This pre-converted data is faster to read, and won’t cause memory spikes during load; but comes with slower data writes (affects startup and bundle load/update time) and a larger lowest overall memory footprint for OPA. Can be enabled for `opa run`, `opa eval`, and `opa bench` by setting the `—optimize-store-for-read-speed`. See http://localhost:8888/docs/edge/policy-performance/#storage-optimization.
Implements: #4147
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Co-authored-by: Ashutosh Narkar <anarkar4387@gmail.com>
This
1. changes the extension.Handler type to make it more flexible
2. simplifies the extension usage -- it used to be called in many places,
but it could all be handled through util.Unmarshal and util.UnmarshalJSON
instead
We've previously marked it as "EXPERIMENTAL", so we should have enough
leeway to change this now.
NOTE: As a consequence of (2.), we're no longer accepting trailing data for
json files loaded with OPA. I believe it wasn't intentional to ignore bad data
before -- now, it'll be an error.
Signed-off-by: Stephan Renatus <stephan@styra.com>
And enable the `tenv` linter for the future.
Also, bump version of golangci-lint and fix some new
warnings that came from that.
Signed-off-by: Anders Eknert <anders@eknert.com>
Got a few warnings from my IDE about redundant type conversions,
so I decided to look into it. Added the unconvert linter to our
checks, and fixed the violations. Added two ignore comments as I
wasn't sure about whether they'd change the semantics of the code.
Signed-off-by: Anders Eknert <anders@eknert.com>
This commit replaces `os.MkdirTemp` with `t.TempDir` in tests. The
directory created by `t.TempDir` is automatically removed when the test
and all its subtests complete.
Prior to this commit, temporary directory created using `os.MkdirTemp`
needs to be removed manually by calling `os.RemoveAll`, which is omitted
in some tests. The error handling boilerplate e.g.
defer func() {
if err := os.RemoveAll(dir); err != nil {
t.Fatal(err)
}
}
is also tedious, but `t.TempDir` handles this for us nicely.
Reference: https://pkg.go.dev/testing#T.TempDir
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
This commit adds the `prealloc` linter to the list of linters for OPA, and fixes up the miscellaneous locations in the code that the linter found where we could easily preallocate slices.
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Add option to inmem.store which allows disabling the round-tripping
through JSON when adding data to the store.
This option is intended for callers who can guarantee the objects they
pass to Write are JSON objects, and have properly ensured the object
will be only be accessed by store once added.
Fixes#4708.
This is continuance of https://github.com/open-policy-agent/opa/pull/4709,
adding these bits:
* storage/inmem: backwards-compat nitpicks, test adaptations
I might have overshot here, but adding variable-length function parameters
is not a backwards-compatible move. Concretely, if you had been using code like
var x func() storage.Store = inmem.New
going from New() to New(...Opts) would break it.
* storage/inmem: use it where possible without roundtrip
* storage/inmem: deal with nil map
It looks like this is something the roundtrip had guarded us from.
Now, we'll explicitly check this.
This came up when running the bundle tests with roundtripping disabled.
* loader: add StoreWithOpts convenience method
Co-authored-by: Will Beason <willbeason@google.com>
Co-authored-by: Philip Conrad <conradp@chariot-chaser.net>
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
If the bundles being activated share a manifest root prefix, it
would result in overwriting the bundle data based on the activation
order. This happened since the truncate call writes data to the
store based on the top-level keys in the data. When multiple
bundles with overlapping bundle root prefixes are being activated
as part of the same txn, adding data to the store by iterating
over the top-level keys in the data object would result in an unintended
overwrite. The truncate call would be able to properly write
data if it had knowledge of the bundle roots. This commit passes
the bundle roots to the truncate call to assist in writing data
to the store.
Fixes: #4998
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Currently we backup the disk store and apply new bundle policy
and data on the new store. Since truncate is called within
a transaction, any uncommited changes on the store will not
be seen during the backup. For example, if the old bundle
data was erased prior to activating a new bundle, this change
would still be uncommited when the backup is done and as a
result both the old and new data would exist in the store.
To avoid this, we now backup the current store, then commit any
in-flight transactions on the current store and store the
current bundle on the store. The backup can be used if we need
to restore to the orignal store version in case we need to
abort the transaction.
Fixes: #4900
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
When OPA activates a delta bundle, the path of a policy is
used as the ID when UpsertPolicy is called in the disk and
in-memory storage Truncate methods. This will cause the
storage to be updated with IDs that are prefixed with a /, while
the policy that was already in storage will not. This causes bundle
activation to fail if any policy contains a default rule, as the
modules will be duplicated.
This commit changes the policy ID that UpsertPolicy is called with
from disk and in-memory storage's Truncate methods
Fixes#4958
Signed-off-by: Martin Johansen <martinjohansen1705@gmail.com>
If OPA has an activated bundle that owns all roots
and a new bundle with empty roots is to be activated, the
old bundle's data should first be erased from the store.
Currently both the old and new data is kept in the store.
This commit attempts to fix this by providing an indication to
the truncate call about the scenario in which the root is to be
overwritten.
Fixes: #4940
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
While writing data to the in-memory store via truncate op,
OPA reads data at the current path before adding
new data to the store to ensure the path exists.
A bundle that contains data files at non-root locations will
trigger a read on the store for each file and hence for a large
bundle this can cause an increase in the bundle activation time
and also resource usage.
This change attempts to avoid multiple read ops by merging all the data
in the bundle and performing a single write on the store.
This fix was tested by observing the bundle activation time and cpu usage
during bundle activation. The test bundle consisted of multiple data files at
non-root locations. The bundle structure was something like:
a/b/data.json, a/c/data.json etc.
Improvements were seen in both cpu usage and activation time as compared
to the older approach of doing a read while writing each file. This can be
attributed to not reading all data under "a" in the test bundle for
every write which was the case earlier.
Fixes: #4898
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
A data write performed at a non-root nonexistent path
in the im-memory store during a trucnate op would cause a
storage not found error. This change creates a path if one
does not exist before writing data.
Signed-off-by: Ashutosh Narkar <anarkar4387@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>
A bunch of smaller follow-up tasks to #4381.
* storage/disk_test: check invalid patches with wildcard partition, too
* docs/disk: add caveat re: bundles loaded into memory
* storage/disk: auto-manage /system partitions
If these are found in the user-provided partitions, we'll error out.
* storage/disk: pretty-print partitions with "*" instead of %2A
* storage/disk: respect wildcard-replacement in partition validation
It is now allowed to replace a partition like
/foo/bar
by
/foo/*
also if multiple wildcards are used.
Caveats:
You cannot add a wildcard partition like /*/*, since it would overlap
the managed "/system/*" partition.
When attempting to go back from /foo/* to /foo/bar, an error is
raised _unconditionally_ -- we could check the existing data, but
currently don't.
* storage/disk: check prefix when adding wildcard partitions
The previously done check would have falsely returned that there is no problem
when adding a wildcard partition: lookup of "/foo/*" with '*' not interpreted
as a wildcard, but as a string, would yield a not-found, even if there was any
data under /foo/.
Now, we'll check the prefix-until-wildcard. It's more cautious than
theoretically necessary, but safe.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
With this change, the disk backend (badger) becomes available for
use with the OPA runtime properly:
It can be configured using the `storage.disk` key in OPA's config
(see included documentation).
When enabled,
- any data or policies stored with OPA will persist over restarts
- per-query metrics related to disk usage are reported
- Prometheus metrics per storage operation are exported
The main intention behind this feature is to optimize memory usage:
OPA can now operate on more data than fits into the allotted memory
resources. It is NOT meant to be used as a primary source of truth:
there are no backup/restore or desaster recovery procedures -- you
MUST secure the means to restore the data stored with OPA's disk
storage by yourself.
See also #4014. Future improvements around bundle loading are
planned.
Some notes on details:
storage/disk: impose same locking regime used with inmem
With this setup, we'll ensure:
- there is only one open write txn at a time
- there are any number of open read txns at a time
- writes are blocked when reads are inflight
- during a commit (and triggers being run), no read txns can be created
This is to ensure the same atomic policy update semantics when using
'disk" as we have with "inmem". We're basically opting out of badger's
currency control and transactionality guarantees. This is because we
cannot piggy back on that to ensure the atomic update we want.
There might be other ways -- using subscribers, and blocking in some
other place -- but this one seems preferrable since it mirrors inmem.
Part of the problem is ErrTxnTooLarge, and committing and renewing
txns when it occurs: that, which is the prescribed solution to txns
growing too big, also means that reads can see half of the "logical"
transaction having been committed, while the rest is still getting
processed.
Another approach would have been using `WriteBatch`, but that won't
let us read from the batch, only apply Set and Delete operations.
We currently need to read (via an iterator) to figure out if we
need to delete keys to replace something in the store. There is
no DropPrefix operation on the badger txn, or the WriteBatch API.
storage/disk: remove commit-and-renew-txn code for txn-too-big errors
This would break transactional guarantees we care about: while there
can be only one write transaction at a time, read transactions may
happen while a write txn is underway -- with this commit-and-reset
logic, those would read partial data.
Now, the error will be returned to the caller. The maximum txn size
depends on the size of memtables, and could be tweaked manually.
In general, the caller should try to push multiple smaller increments
of the data.
storage/disk: implement noop MakeDir
The MakeDir operation as implemented in the backend-agnostic storage
code has become an issue with the disk store: to write /foo/bar/baz,
we'd have to read /foo (among other subdirs), and that can be _much_
work for the disk backend. With inmem, it's cheap, so this wasn't
problematic before.
Some of the storage/disk/txn.go logic had to be adjusted to properly
do the MakeDir steps implicitly.
The index argument addition to patch() in storage/disk/txn.go was
necessary to keep the error messages conforming to the previous
code path: previously, conflicts (arrays indexed as objects) would
be surfaced in the MakeDir step, now it's entangled with the patch
calculation.
storage/disk: check ctx.Err() in List/Get operations
This won't abort reading a single key, but it will abort iterations.
storage/disk: support patterns in partitions
There is a potential clash here: "*", the path wildcard, is
a valid path section. However, it only affects the case when
a user would want to have a partition at
/foo/*/bar
and would really mean "*", and not the wildcard.
Storing data at /foo/*/bar with a literal "*" won't be treated
differently than storing something at /fo/xyz/bar.
storage/disk: keep per-txn-type histograms of stats
This is done by reading off the metrics on commit, and shovelling
their numbers into the prometheus collector.
NOTE: if you were to share a metrics object among multiple transactions,
the results would be skewed, as it's not reset. However, our server
handlers don't do that.
storage/disk: opt out of badger's conflict detection
With only one write transaction in flight at any time, the situation
that badger guards against cannot happen:
A transaction has written to a key after the current, to-be-committed
transaction has last read that key from the store.
Since it can't happen, we can ignore the bookkeeping involved. This
improves the time it takes to overwrite existing keys.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Currently the "-" character is used to append to
an array. Appends can be also be made by specifying the
array index as long as the specified index is not
greater than the number of elements in the array as
per RFC 6902.
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
`make test` was crashing on Mac OS due to "too many open files",
which could be traced to two different issues.
The first one was the disk based storage being opened but not closed
in some cases. This change takes the number of open file pointers
from >300 to ~30 after the disk based tests have run.
The second issue was a fixture HTTP server allowing keep-alive
connections, and since each test would call the server on a
random port, no connection reuse was possible. Since the test
ran over 400 iterations, and the max open file handles on Mac
OS by default is 256, things broke.
Signed-off-by: Anders Eknert <anders@eknert.com>
* storage/path.Ref: parse int64 into ast.Number
* ast/PtrRef: guard against giant paths
I don't believe this limit is every going to be reached. But CodeQL had flagged
this, and it's not entirely wrong. Let's error our on giant wonky inputs instead
of seeing what'll happen with it eventually.
* ast/parser: fix bad import alias var
The fuzzer came up with that!
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
This commit adds library support for a disk-based storage
implementation that persists policies and data using an embedded
key-value store (badger).
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
This commit just moves some shareable error helpers and the ptr()
function into an internal package so that we can reuse it with the
disk-based store implementation. This commit doesn't change any
existing functionality and no new public APIs are exposed.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
golint is deprecated. The author of the code no longer supports the
codebase. golangci-lint is faster than golint, and is in use by other
opa repositories (e.g. Gatekeeper).
This commit changes tools.go to reference golangci (so it ends up in
vendor) and modifies check-lint to use golangci instead.
Breaking API Changes:
- plugins/rest/rest.go: Fix typo "AllowInsureTLS" -> "AllowInsecureTLS"
- storage/errors.go: Removed unused IndexingNotSupportedErr
Signed-off-by: Will Beason <willbeason@google.com>
When mask rules targeted /result, it was modifying both the result
in the decision logs (intended) and the result in the API
response (unintended). Added a step to deep copy the result only once, if
there is at least one mask rule targeting the result.
Fixes#2752
Signed-off-by: Grant Shively <gshively@godaddy.com>
This is to allow future mutating functions: with value receivers this
mutating any of the member variables is not possible.
Signed-off-by: Teemu Koponen <koponen@styra.com>
At one point the in-memory store implemented an indexing strategy so
that variable bindings could be returned for non-ground references to
base documents. However, we eventually disabled in-memory indexing
and we have not re-added it since
3ebbeede6c. At this point, indexing can
be removed completely.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
This commit adds two checks to the in-memory store to detect improper
use of transactions:
1. Mark aborted/committed transactions as stale and error if an
operation is attempted on a stale transaction. Previously callers
could perform unsafe concurrent operations on stale transactions
without noticiing.
2. Check that supplied transactions are from the underlying store and
not another store. We could consider deprecating the store APIs and
moving them onto the transaction to prevent this kind of mistake in
the future.
With these changes the store will panic on unregister and abort
calls if any of these invariants are violated. Panicing is preferable
to failing silently.
Note, we still have the issue of recursive transactions resulting in
deadlock. Our options there are to implement more sophisticated
locking inside the in-memory store or modify the API so that the store
can update the passed context.Context with a sentinel value.
Fixes#1594
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
These changes update the storage to accept arbitrary key/value
parameters when creating transactions. The container is passed to
trigger callbacks that are invoked when transactions commit.
The use case for this is avoiding parse and compile operations on
modules loaded out of bundles. The parse and compile step on large
bundles can take several seconds. Since the triggers are executed
while the store's read-lock is held, policies queries block.
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>
I don't know why I ended up looking into this, but I've run
golangci-lint run --disable-all -E deadcode
on the code base and removed everything that came up :)
Signed-off-by: Stephan Renatus <srenatus@chef.io>
This should fix#722. It's a slight variation of the code snippet provided
there:
I wasn't sure what the reflect.Interface part was for, so this is using
only reflect.Ptr. Also, there existing tests would fail without the added
check for reflect.Invalid.
Adds a test case for inmem -- in a new method, as I couldn't quite fit it
into the schema of TestInMemoryWrite.
Also, util.Reference() ensures that the returned value is a pointer to
something -- and not a pointer to a pointer to something. While this wasn't
part of the issue #722, it felt weird not to solve the general problem, but
only the edge case. :)
Signed-off-by: Stephan Renatus <srenatus@chef.io>
These changes extend #702 to include writes. If the path segments are
escaped, the should be unescaped during parsing. This allows callers to
write keys like "foo/bar" into storage.
Fixes#695
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
It's often useful to be able to create a hierarchical structure in one
shot in storage. Previously this functionality was implemented in the
server, but it's better off implemented in the storage package.
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Stephan Renatus