introduce caveat support in CockroachDB

[vroldanbet]

Closes #916

What

introduces support for caveats in CockroachDB datastore

How

• following a similar design to namespaces
• introduce corresponding migration (add new table, add new columns to relation_tuple
• Watch API implementation was adjusted to read changes on caveats from the After payload which we weren't using beore because everything came via the Primary Key
• adjusted tests comparing revisions from reads and writes, since that assumption does not hold with CRDB (they are monotonically increasing tho)
• added new assertion in test suite to make sure TOUCH operations that only change caveat name of a relationship also works as intended
• removed flag from NewPGXExecutor given now PSQL and CRDB are aligned
• using caveat names as overlapping key

[ecordell]

this is allocating an extra slice to track overlap keys, why not just loop over the names similar to how we do for namespaces?

there will almost always be only 1 key (and the "keyset" only stores it once), so it's not much help to store it N times. The default is even simpler - we discard the keys entirely (just write to a single static key instead).

[vroldanbet]

That is correct, it's a tradeoff I deliberately took, although I'm not 100% sold given the overhead. The goal was to spare us a bunch of repeated lines of code. This the executeWithOverlappingKeys could be used in other methods of the readerwriter implementation.

I'm not sure I follow what you mean the keyset stores it just once. Isn't each caveat name added to the keyset to compute the overlapping?

• WriteCaveats accepts a slice of caveats. Each name is an overlapping key.
• DeleteCaveats also accepts a slice of caveats, same here

[ecordell]

I'm not sure I follow what you mean the keyset stores it just once. Isn't each caveat name added to the keyset to compute the overlapping?

There's two types of keyset: static and prefix.

If it's a static keyset, then addOverlapKey is a no-op, we always touch the same key.

If it's a prefix keyset, then addOverlapKey extracts the prefix (i.e. mynamespace/<whatever>) and only stores the prefix.

In theory you can make requests with multiple prefixes, in practice it's always just 1 prefix.

By collecting up the keys like this, you'll build a big list in memory only to throw it away (because we actually only need the on single prefix from the request, not an array of all of the caveat names)

[vroldanbet]

@ecordell I've updated the code to handle overlapping key one level above and avoid the extra allocation

In theory you can make requests with multiple prefixes, in practice it's always just 1 prefix.

I'd like to understand this better. Given WriteCaveats will be handled by API WriteSchema, wouldn't it be possible to send a schema write request with caveats / namespaces with different prefixes? If so, doesn't that require us to loop over all namespaces and caveats to make sure we don't miss any overlapping key?

[ecordell]

If so, doesn't that require us to loop over all namespaces and caveats to make sure we don't miss any overlapping key?

Yes, and the "keyer" is for tracking all prefixes from the request. But most deployments will either not use prefixes at all (authzed dedicated, spicedb users) or will have a single prefix enforced a layer up (authzed serverless). And in SpiceDB deployments that do use prefixes, the default "overlap strategy" is static (meaning all namespaces write to a single static key).

If you use the "prefix" overlap strategy, then prefixes in SpiceDB w/ CRDB are really "new enemy protection domains" rather than just for organization. I don't think we have any telemetry on it, but I doubt there are many (if any) SpiceDB users who have bothered - it's only really useful if you are trying to enforce multitenancy on top of SpiceDB and/or you're hitting QPS limits with static (and can untangle the namespaces in some reasonable way / don't care about newenemy problem)

[vroldanbet]

Thanks for the context, helps a lot! Intentionally using prefix separation to reduce the bottleneck caused by serialization errors could indeed be an option for certain SpiceDB deployments, but still to be seen in real-life (beyond Authzed Serverless 😄 ). In any case, my take away is that, while it is relatively rare, we still need to make sure we account for all potential prefixes in an incoming request.

[ecordell]

LGTM