spicedb
spicedb copied to clipboard
authzed
Proper handling of incompatible zedtokens
NOTE: ZedTokens are a bit longer now as a result of this change, but should still be well within the 1024 limit previously defined
Fixes #1541
Some early feedback, I'll continue tomorrow. Please describe in the PR body what problems are you trying to solve and design choices you took to come up with this solution. It helps folks reviewing the PR with the right context 🙏🏻
I added the fixes; it was on the commit but not the PR
We are transferring UUIDs in every API call for no reason other than to prevent an eventual migration within the same datastore type. That's extremely wasteful.
Is it really? We could reduce the size of the ID to just a small prefix if we wish to save some bytes.
It's still within the zedtoken spec limit, but it is a very high price to pay for an incredible rare event (a legit one, don't get me wrong). We are storing UUIDs in the datastore as strings, which is the least compact way of storing them and transferring them over the wire. And we are forcing everybody to see the data transferred increase significantly (and latency!) to prevent a scenario that the system will not be subjected to in, I'll dare to say, practically 100% of its lifespan.
Except users have already been subject to it - this was spurred by two, different reports: one where a user moved between datastore types, but another where they moved between datastores of the same type (but accidentally)
We need to rethink this. I get the zedtoken is a stateless token and that it needs to self-contain this information, but I think we can get 99% there by defining the datastore type as a small attribute in the token. I understand migrating from different instances of the same datastore can be problematic, but no one asked for this,
It has happened before, sadly.
and we could find alternative ways for it (e.g. a flag that forces SpiceDB to ignore requested consistency and fallback to
full_consistencyall the time, orminimize_latencyif the customer can take the hit of ignoring the new enemy problem during the migration). I'd argue that by adding such a flag we could completely avoid having to add the datastore type too.
This PR does add that flag, but we need a means of tracking to ensure that zedtokens are not used across instances of a datastore. The fact that we allow it now is, technically speaking, a small but real risk.
Updated to only store 8 bytes of prefix of the datastore ID. Since datastores are rarely updated, this should be fine for detecting changes.