Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RHCLOUD-37466 - Implement zookie into api #352

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

RHCLOUD-37466 - Implement zookie into api #352

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cb090f2
Add zookie proto def to api
lennysgarage Jan 27, 2025
5bc00f9
Disable fully consistent mode
lennysgarage Jan 27, 2025
04229bb
Expose zookie in API + update tests
lennysgarage Jan 29, 2025
aebe3ee
Update test to use same zookie...
lennysgarage Jan 29, 2025
df51211
Add basic test cases that ensure zookie's handle read after write for…
lennysgarage Jan 29, 2025
01e911a
Keep ephemeral env fullyConsistent
lennysgarage Jan 30, 2025
4215432
Add a new enemy problem-like test
lennysgarage Jan 30, 2025
1c6b17e
Compare returned zookie.
lennysgarage Jan 30, 2025
e9c6135
Check for full consistency
lennysgarage Jan 30, 2025
21a9c9e
Full consistency field for check requests
lennysgarage Jan 31, 2025
31e1718
Implement a CheckForUpdate rpc for full consistency checks
lennysgarage Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 63 additions & 37 deletions api/kessel/relations/v1beta1/check.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions api/kessel/relations/v1beta1/check.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ message CheckRequest {
ObjectReference resource = 1 [(buf.validate.field).required = true];
string relation = 2 [(buf.validate.field).string.min_len = 1];
SubjectReference subject = 3 [(buf.validate.field).required = true];
Zookie zookie = 4;
}

message CheckResponse {
Expand All @@ -36,5 +37,6 @@ message CheckResponse {
// e.g. ALLOWED_CONDITIONAL = 3;
}
Allowed allowed = 1;
Zookie checked_at = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any value to having checked_at, deleted_at, etc., rather than just zookie? Just wondering why you named them like this?

I'm wondering whether we want to give the impression that zookies are like timestamps, or to stay completely opaque as to what they are. I don't know the answer, just thinking out loud.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great question, it would not make much of a difference if it were all just zookie. The reasoning is due to the fact that I was following how authzed had setup their zedtoken. With looked_up_at for lookup responses https://buf.build/authzed/api/docs/main:authzed.api.v1#authzed.api.v1.LookupResourcesResponse and checked_at for https://buf.build/authzed/api/docs/main:authzed.api.v1#authzed.api.v1.CheckPermissionResponse etc.

Considering it is just an opaque string, I think having names that sound like timestamps can be misleading. I think it would be good to just have a single name like zookie for requests and response fields and won't have users thinking they can grab some extra timestamp value from what it is. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that spicedb do it like this. :) I'm on the fence with this one, so whichever way you like is good with me. If nobody else comments, feel free to resolve one way or the other.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether we want to give the impression that zookies are like timestamps, or to stay completely opaque as to what they are. I don't know the answer, just thinking out loud.

This – we should be very opaque about them IMO, because we can't guarantee they'll always be timestamps.

See my comment below on the "Zookie" name itself – I think we need a little less specific of a field name for these because across other FGA projects, the token we'd use isn't actually a logical timestamp, so there is arguably a better name for this. Maybe simply "consistency_token" or "read_token" (similar to what you had originally).

}

70 changes: 59 additions & 11 deletions api/kessel/relations/v1beta1/common.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions api/kessel/relations/v1beta1/common.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,8 @@ message ObjectType {
string namespace = 1 [(buf.validate.field).string.min_len = 1];
string name = 2 [(buf.validate.field).string.min_len = 1];
}

// The Zookie is used to provide consistency between write and read requests.
message Zookie {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about exposing the "Zookie" name (and the exact concept) itself directly in the API.

What I mean by this is that we will probably benefit from making a bit more generic concept that fits better with other Zanzibar/FGA implementations, but still provides the same or similar API semantics.

e.g. something like maybe Revision or ReadToken or ConsistencyToken ... etc. We need to convey, "Here's your response and a token. You can use this token later to get data at least as recent as from this request. We promise nothing else about what this token means."

This value needn't be a logical timestamp as in a Zookie. It could simply be an opaque string that means "for this request, use full consistency," which would be compatible with OpenFGA, for example. OSO Cloud's "consistency token" is similar as well – it just guarantees reading-your-writes, but otherwise doesn't refer to a particular snapshot (in their case looks like a particular shard). For completeness, Permify, SpiceDB, and likely Warrant (now WorkOS FGA) use more by-the-book zookies.

The guarantees may not all be rigorously the same with different implementations, so I don't want to go down that rabbit hole. But for now we have to remain open to working with different implementations, even if there is the caveat that they have slightly different semantics. (In these examples though, for what we need, I think they would all work.)

This point is true regardless of whether we expose this in protos or not – even in our internal abstraction, we should remain as agnostic of the "Zanzibars" as possible until we absolutely have to commit to limiting that.

string token = 1 [(buf.validate.field).string.min_len = 1];
lennysgarage marked this conversation as resolved.
Show resolved Hide resolved
}
Loading