document the implications of using the insecure overlap strategy for cockroachdb
#1251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ecordell
changed the title
insecure is okay to the crdb readmeinsecure overlap strategy for cockroachdb
ecordell
changed the title
insecure overlap strategy for cockroachdbinsecure overlap strategy for cockroachdb
ecordell
force-pushed
the
insecure-doc
branch
2 times, most recently
from
fee7afa
to
dc48af1
Compare
vroldanbet
reviewed
Apr 10, 2023
There was a problem hiding this comment.
LGTM, left some nits and questions. I think I'd be worth adding this also to https://authzed.com/docs/spicedb/selecting-a-datastore#cockroachdb
internal/datastore/crdb/README.md
Outdated
| ## When is `insecure` overlap a problem? | ||
|
|
||
| Using `insecure` overlap strategy for SpiceDB with CockroachDB means that it is _possible_ that timestamps for two subsequent writes will be out of order. | ||
| When this happens, it's _possible_ for the [New Enemy Problem](https://zanzibar.tech/2B__bAI52Q:0:J) to occur. |
There was a problem hiding this comment.
perhaps https://authzed.com/blog/prevent-newenemy-cockroachdb/ may be an even better link to reference
| - When two writes are made in short succession against CockroachDB | ||
| - And those two writes hit two different gateway nodes | ||
| - And the CRDB gateway node clocks have a delta `D` | ||
| - And the writes touch disjoint sets of relationships |
There was a problem hiding this comment.
couldn't it technically be still possible if the writes touch the same relationships? (so long we hit different gateway nodes)
There was a problem hiding this comment.
Nope, if the the writes land in the same ranges (which they would if any relationships overlap) then the timestamps will get synchronized
internal/datastore/crdb/README.md
Outdated
| - Not allowing the write from `B` within the max_offset time of the CRDB cluster (or the quantization window). | ||
| - Not allowing a Check on a resource within max_offset of its ACL modification (or the quantization window). | ||
|
|
||
| ## Mis-apply Old ACLs to New Content |
There was a problem hiding this comment.
I believe this is part of When does a timestamp reversal matter? section
Suggested change
| ## Mis-apply Old ACLs to New Content | |
| ### Mis-apply Old ACLs to New Content |
internal/datastore/crdb/README.md
Outdated
| Note that this is only possible if `A - T < quantization window`: the check has to happen soon enough after the write for `A` that it's possible that SpiceDB picks a timestamp in between them. | ||
| The default quantization window is `5s`. | ||
|
|
||
| ### Application Mitigations for ACL Update Order |
There was a problem hiding this comment.
nit: may be better to make this a subsection of Neglecting ACL Update Order
Suggested change
| ### Application Mitigations for ACL Update Order | |
| #### Application Mitigations for ACL Update Order |
internal/datastore/crdb/README.md
Outdated
|
|
||
| Same as before, this is only possible if `A - T < quantization window`: Bob's check has to happen soon enough after the write for `A` that it's possible that SpiceDB picks a timestamp in between `A` and `B`, and the default quantization window is `5s`. | ||
|
|
||
| ### Application Mitigations for Misapplying Old ACLs |
There was a problem hiding this comment.
nit: may be better to make this a subsection of Mis-apply Old ACLs to New Content
Suggested change
| ### Application Mitigations for Misapplying Old ACLs | |
| #### Application Mitigations for Misapplying Old ACLs |
internal/datastore/crdb/README.md
Outdated
|
|
||
| Then it's possible that the second write will be assigned a timestamp earlier than the first write. In the next section we'll look at whether that matters for your application, but for now let's look at what makes the above conditions more or less likely: | ||
|
|
||
| - **Clock skew**. A larger clock skew gives a bigger window in which timestamps can be reversed. But note that CRDB enforces a max offset between clocks, and getting within some fraction of that max offset will boot the node from the cluster. |
jakedt
requested changes
Apr 11, 2023
|
@jakedt updated with examples like you suggested |
ecordell
force-pushed
the
insecure-doc
branch
3 times, most recently
from
04a426c
to
bc62d69
Compare
jakedt
previously approved these changes
Apr 12, 2023
|
@ecordell linter error |
ecordell
force-pushed
the
insecure-doc
branch
2 times, most recently
from
91232d7
to
7284223
Compare
|
@jakedt fixed |
jakedt
approved these changes
May 5, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.