Proper handling of incompatible zedtokens #1723
Conversation
github-actions
bot
added
area/api v1
josephschorr
force-pushed
the
incompatible-zedtokens
branch
3 times, most recently
from
d3b259f
to
64dda15
Compare
josephschorr
force-pushed
the
incompatible-zedtokens
branch
from
64dda15
to
8f8f83c
Compare
josephschorr
force-pushed
the
incompatible-zedtokens
branch
from
8f8f83c
to
8c87502
Compare
|
Rebased |
josephschorr
force-pushed
the
incompatible-zedtokens
branch
2 times, most recently
from
52895fe
to
9bb170d
Compare
josephschorr
force-pushed
the
incompatible-zedtokens
branch
from
9bb170d
to
d15a06b
Compare
|
Rebased |
josephschorr
force-pushed
the
incompatible-zedtokens
branch
2 times, most recently
from
ba65181
to
160e514
Compare
josephschorr
force-pushed
the
incompatible-zedtokens
branch
from
160e514
to
c176cb2
Compare
|
Updated |
josephschorr
force-pushed
the
incompatible-zedtokens
branch
from
c176cb2
to
da44237
Compare
| } | ||
|
|
||
| cds.uniqueID.Store(&uniqueID) |
There was a problem hiding this comment.
I don't think this matters too much here because the value converges, but this can race. Ideally we do a CAS operation.
I'd prefer to see this implemented with sync.Once instead of an atomic pointer. The ID does not change for the lifetime of the server.
There was a problem hiding this comment.
sync.Once won't work because if we get an error, we need to rerun this code the next time its called
There was a problem hiding this comment.
Fair. Can we at least use the CAS operation to store it? if it fails, it probably means some other goroutine already set it.
Another point in favor of the atomic value is it does not rely on any mutex. sync.Once relies on it while it's not initialized, but once it is, the overhead of the mutex disappears.
There was a problem hiding this comment.
Why even do that though? The ID is guaranteed to be the same by the datastore, so no need to compare?
| // UniqueID returns a unique identifier for the datastore. This identifier | ||
| // must be stable across restarts of the datastore if the datastore is | ||
| // persistent. | ||
| UniqueID(context.Context) (string, error) |
There was a problem hiding this comment.
Did you consider alternatives using the UniqueID as a stable identifier for zedtokens?
I assume (not present in the PR body) that the goal is making sure zedtokens from one datastore type are not used in another datastore type. But what happens if you evolve the zedtoken implementation from one version to another for the same datastore, and want to force them to be dropped? the uniqueID wouldn't help, would it?
Shouldn't we store a zedtoken versioning parameter like datastoreType+zedtokenVersion?
There was a problem hiding this comment.
Yes, but then it doesn't meet the goal which is to prevent zedtokens not just across types but instances of the datastore as well
I added the fixes; it was on the commit but not the PR |
There was a problem hiding this comment.
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.
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.
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, and we could find alternative ways for it (e.g. a flag that forces SpiceDB to ignore requested consistency and fallback to full_consistency all the time, or minimize_latency if 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.
| @@ -55,27 +56,47 @@ func RevisionFromContext(ctx context.Context) (datastore.Revision, *v1.ZedToken, | |||
| handle := c.(*revisionHandle) | |||
| rev := handle.revision | |||
| if rev != nil { | |||
| return rev, zedtoken.MustNewFromRevision(rev), nil | |||
| ds := datastoremw.FromContext(ctx) | |||
There was a problem hiding this comment.
This middleware already had a dependency on the datastore middleware, but we never declared it via the middleware framework. Please update the middleware chain to state this middleware has such dependency.
There was a problem hiding this comment.
Are you asking me to add a comment stating that or add something somewhere? (sorry for forgetting)
| // Calculate a revision as we see fit | ||
| databaseRev, err := ds.OptimizedRevision(ctx) | ||
| if err != nil { | ||
| return datastore.NoRevision, false, err | ||
| } | ||
|
|
||
| if requested != nil { | ||
| requestedRev, err := zedtoken.DecodeRevision(requested, ds) | ||
| requestedRev, status, err := zedtoken.DecodeRevision(requested, ds) |
There was a problem hiding this comment.
It does not feel great to return a new return value status when this is something that could be returned via an error condition the callsite can check for. I understand the benefit of the added argument though: it forces the callsite to make a conscious choice about whether to handle it or not.
There was a problem hiding this comment.
Its explicitly not an error though; it only becomes an error if that is the setting on the CLI. I also do like that callers must handle it, as you said.
| if status == zedtoken.StatusMismatchedDatastoreID { | ||
| switch option { | ||
| case TreatMismatchingTokensAsFullConsistency: | ||
| log.Warn().Str("zedtoken", requested.Token).Msg("ZedToken specified references an older datastore and SpiceDB is configured to treat this as a full consistency request") |
There was a problem hiding this comment.
I don't think it's older, just different instance
| log.Warn().Str("zedtoken", requested.Token).Msg("ZedToken specified references an older datastore and SpiceDB is configured to treat this as a full consistency request") | |
| log.Warn().Str("zedtoken", requested.Token).Msg("ZedToken specified references a different datastore instance and SpiceDB is configured to treat this as a full consistency request") |
| return headRev, false, nil | ||
|
|
||
| case TreatMismatchingTokensAsMinLatency: | ||
| log.Warn().Str("zedtoken", requested.Token).Msg("ZedToken specified references an older datastore and SpiceDB is configured to treat this as a min latency request") |
There was a problem hiding this comment.
| log.Warn().Str("zedtoken", requested.Token).Msg("ZedToken specified references an older datastore and SpiceDB is configured to treat this as a min latency request") | |
| log.Warn().Str("zedtoken", requested.Token).Msg("ZedToken specified references a different datastore instance and SpiceDB is configured to treat this as a min latency request") |
| return databaseRev, false, nil | ||
|
|
||
| case TreatMismatchingTokensAsError: | ||
| log.Error().Str("zedtoken", requested.Token).Msg("ZedToken specified references an older datastore and SpiceDB is configured to raise an error in this scenario") |
There was a problem hiding this comment.
I don't think we need to log in this case since it will be surfaced via gRPC error anyway
| log.Error().Str("zedtoken", requested.Token).Msg("ZedToken specified references an older datastore and SpiceDB is configured to raise an error in this scenario") |
| if err != nil { | ||
| return errInvalidZedToken | ||
| } | ||
|
|
||
| if status == zedtoken.StatusMismatchedDatastoreID { | ||
| log.Error().Str("zedtoken", consistency.GetAtExactSnapshot().Token).Msg("ZedToken specified references an older datastore but at-exact-snapshot was requested") |
There was a problem hiding this comment.
I don't think we need to log this, it will surface via gRPC error in the logs
| log.Error().Str("zedtoken", consistency.GetAtExactSnapshot().Token).Msg("ZedToken specified references an older datastore but at-exact-snapshot was requested") |
There was a problem hiding this comment.
Changed to a warning
| TreatMismatchingTokensAsFullConsistency MismatchingTokenOption = iota | ||
|
|
||
| TreatMismatchingTokensAsMinLatency | ||
|
|
||
| TreatMismatchingTokensAsError |
| return &v1.WriteRelationshipsResponse{ | ||
| WrittenAt: zedtoken.MustNewFromRevision(revision), |
There was a problem hiding this comment.
hmm, Im realizing we used MustNewFromRevision everywhere, when it's something that would panic in case of malformed proto encoding. I guess it worked because the middleware prevented any incorrect revision from making it through.
There was a problem hiding this comment.
Yeah, we could move away from the Must approach, but it shouldn't fail in "normal" operation
| @@ -97,9 +97,14 @@ func (ss *schemaServer) ReadSchema(ctx context.Context, _ *v1.ReadSchemaRequest) | |||
| DispatchCount: dispatchCount, | |||
| }) | |||
|
|
|||
| zedToken, err := zedtoken.NewFromRevision(ctx, headRevision, ds) | |||
There was a problem hiding this comment.
at this point if feels like NewFromRevision should be a method in the Datastore interface, but I get the convenience of not relying on the datastore specially for tests
There was a problem hiding this comment.
Yes and no; it uses the datastore's unique ID but its also not strictly tied to the datastore, so I prefer this approach
| if err != nil { | ||
| return status.Errorf(codes.InvalidArgument, "failed to decode start revision: %s", err) | ||
| } | ||
|
|
||
| if tokenStatus == zedtoken.StatusMismatchedDatastoreID { |
There was a problem hiding this comment.
I think context is missing on why we are choosing to fail here. I would recommend adding a comment to clarify, since the implications are not immediately clear.
I'm trying to think what type of scenario are we trying to protect from by failing here, but also potential use-cases we could be hampering by doing so.
- Clearly zedtoken for a different datastore type should fail as those can fail in unexpected ways, even though we could potentially find ways some of them interoperate.
- Same datastore type, but different instances: we can't guarantee to observe the same transactions at the same timestamp in different instances.
In the case of Watch, reusing a zedtoken in different instances could lead to missing events.
The only scenario I can think that could be impacted by this are dual-writing to a new SpiceDB instance and doing an eventual cut-over (e.g. deep schema refactor). API calls with at_least_as_fresh can recover by momentarily switching to minimize_latency until the tokens are refreshed (although this could take very long as it depends on resource changes! the system could be running in minimize for a long time). A cut-over to the Watch API will simply not work, regardless of the changes in this PR, as it can only work if they represent the same state of the world.
Long story short, I think this is the right call, and I wanted to think out lout and leave a trace of the why.
Is it really? We could reduce the size of the ID to just a small prefix if we wish to save some bytes.
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)
It has happened before, sadly.
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. |
josephschorr
force-pushed
the
incompatible-zedtokens
branch
from
da44237
to
a526fc5
Compare
|
Updated to only store 8 bytes of prefix of the datastore ID. Since datastores are rarely updated, this should be fine for detecting changes. |
an older datastore is used All ZedTokens are now minted with the datastore's unique ID included in the ZedToken and that ID is checked when the ZedToken is decoded. In scenarios where the datastore ID does not match, either an error is raised (watch, at_exact_snapshot) or configurable behavior is used (at_least_as_fresh) Fixes authzed#1541
…token Results in smaller tokens but given that datastore IDs are generated, should still minimize the chances of a conflict
josephschorr
force-pushed
the
incompatible-zedtokens
branch
from
a526fc5
to
d19b1f7
Compare
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