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.

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 9 commits into
base: main
Choose a base branch
from

Conversation

lennysgarage
Copy link
Contributor

@lennysgarage lennysgarage commented Jan 29, 2025

PR Template:

Describe your changes

  • Adds a zookie to each create/del/check/lookup requests as an optional field
  • Adds a zookie to each create/del/check/lookup response
    • Zookie is always returned from a read/write request. (Even if full consistency is enabled.)
  • Tests updated to at least work with zookies. 1 test case so far to verify read after write consistency with zookie (TestSpiceDbRepository_CheckPermissionZookie)

Example output of running a create & check immediately:

CreateTuples:
{ 
  "createdAt": { 
    "token": "GgYKBENNY0c=" 
  } 
}
Check (passing zookie in request):
{
  "allowed": "ALLOWED_TRUE",
  "checkedAt": {
    "token": "GgYKBENNY0c="
  }
}

Ticket reference (if applicable)

Fixes # https://issues.redhat.com/browse/RHCLOUD-37466

Checklist

  • Are the agreed upon acceptance criteria fulfilled?

  • Was the 4-eye-principle applied? (async PR review, pairing, ensembling)

  • Do your changes have passing automated tests and sufficient observability?

  • Are the work steps you introduced repeatable by others, either through automation or documentation?

    • If automation is possible but not done due to other constraints, a ticket to the tech debt sprint is added
    • An SOP (Standard Operating Procedure) was created
  • The Changes were automatically built, tested, and - if needed, behind a feature flag - deployed to our production environment. (Please check this when the new deployment is done and you could verify it.)

  • Are the agreed upon coding/architectural practices applied?

  • Are security needs fullfilled? (e.g. no internal URL)

  • Is the corresponding Ticket in the right state? (should be on "review" now, put to done when this change made it to production)

  • For changes to the public API / code dependencies: Was the whole team (or a sufficient amount of ppl) able to review?

Signed-off-by: Jonathan Marcantonio <jmarcant@redhat.com>
Signed-off-by: Jonathan Marcantonio <jmarcant@redhat.com>
Signed-off-by: Jonathan Marcantonio <jmarcant@redhat.com>
Copy link

github-actions bot commented Jan 29, 2025

The latest Buf updates on your PR. Results from workflow buf-pull-request / build (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedJan 30, 2025, 8:30 PM

Signed-off-by: Jonathan Marcantonio <jmarcant@redhat.com>
… requests

Signed-off-by: Jonathan Marcantonio <jmarcant@redhat.com>
Copy link
Contributor

@merlante merlante left a comment

Choose a reason for hiding this comment

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

Looks really good @lennysgarage. It has everything I was expecting. Let's wait for a couple of more reviews on this one.

internal/service/lookup_test.go Show resolved Hide resolved
@@ -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?

deploy/kessel-relations.yaml Outdated Show resolved Hide resolved

// The Zookie is used to provide consistency between write and read requests.
message Zookie {
string token = 1 [(buf.validate.field).string.min_len = 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Are zookies always of conistent length? if not, a length check of "1" is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my own testing with the zookie, no it's not always a consistent length. The length of the token does vary and I've set it up similarly to authzed's docs. They have a token field with a min_bytes of 1 which is equivalent to min_len of 1. Doc ref: https://buf.build/authzed/api/docs/main:authzed.api.v1#authzed.api.v1.ZedToken

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the implementation of the zookie is storage-dependent in SpiceDB, so even if they were a consistent length for Postgres, they wouldn't necessarily be for, say, CRDB. And another Zanzibar implementation may go a different direction entirely.

min_len = 1 ensures that it's not empty without asserting anything about the contents, which makes sense to me.

Signed-off-by: Jonathan Marcantonio <jmarcant@redhat.com>
Signed-off-by: Jonathan Marcantonio <jmarcant@redhat.com>
Signed-off-by: Jonathan Marcantonio <jmarcant@redhat.com>
Signed-off-by: Jonathan Marcantonio <jmarcant@redhat.com>
@wscalf
Copy link
Contributor

wscalf commented Jan 30, 2025

Two thoughts around usecases for fullyconsistent alongside zookies:

  • Moving more toward Zanzibar..compliance? Normalness?..we'll need to be able to do fully consistent checks for non-view operations. Can we do that with this API change? It seems like it's either all fully consistent (by configuration) or minimize latency/zookie depending on whether or not one's passed.
  • ReadRelationships may need to be fully consistent more often than not (or even always.) Rationale: the main usecase for directly reading relationships (vs subjects/resources) is if a client intends to follow it with a WriteRelationships call (+ precondition?) to make some change relative to the current state. A stale read would be a problem there. (Though open to there being other usecases. Also not sure anything calls ReadRelationships atm, or that anything outside the management fabric should.)

@lennysgarage
Copy link
Contributor Author

Two thoughts around usecases for fullyconsistent alongside zookies:

  • Moving more toward Zanzibar..compliance? Normalness?..we'll need to be able to do fully consistent checks for non-view operations. Can we do that with this API change? It seems like it's either all fully consistent (by configuration) or minimize latency/zookie depending on whether or not one's passed.

Thanks for raising these points and with this change, no we won't be able to do that. We don't expose the consistency level in the API, so like you said we're either always fully consistent or minimize latency/zookie. I guess we would need to implement a consistency level similar to spicedb to determine when we want fully consistent checks versus ones with a zookie/no zookie.

  • ReadRelationships may need to be fully consistent more often than not (or even always.) Rationale: the main usecase for directly reading relationships (vs subjects/resources) is if a client intends to follow it with a WriteRelationships call (+ precondition?) to make some change relative to the current state. A stale read would be a problem there. (Though open to there being other usecases. Also not sure anything calls ReadRelationships atm, or that anything outside the management fabric should.)

ReadRelationships can be passed a zookie in the request as well. Since we're not exposing the consistency to the user we don't have a way for them to determine what they're going to do next (unless we modify the api like above). If the main usecase is for directly following up with a WriteRelationships call then it would be appropriate to change to full consistency always.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants