ZedTokens: SpiceDBAuthorizer cleanup #18898

geropl · 2023-10-09T15:05:30Z

Description

Follow-up for: #18893

This PR:

split the EnhancedLogContext as suggested here
Cleanup of the SpiceDbAuthorizer API/bindings as suggested here
Passes ZedTokens to readPermissions calls as well (turns out we are calling them more often then expected)

Summary generated by Copilot

`🤖 Generated by Copilot at dcd42e3`

Refactor the server component to use SpiceDB for authorization and simplify the log context module. Add TypeScript bindings and code generation for SpiceDB protobuf files.

Related Issue(s)

Fixes #

How to test

Documentation

Preview status

Gitpod was successfully deployed to your preview environment.

🏷️ Name - gpl-738-zedtokens-4
🔗 URL - gpl-738-zedtokens-4.preview.gitpod-dev.com/workspaces.
📚 Documentation - See our internal documentation for information on how to interact with your preview environment.
📦 Version - gpl-738-zedtokens-4-gha.18348
🗒️ Logs - GCP Logs Explorer

Build Options

Build

/werft with-werft
Run the build with werft instead of GHA
leeway-no-cache
/werft no-test
Run Leeway with --dont-test

Publish

/werft publish-to-npm
/werft publish-to-jb-marketplace

Installer

analytics=segment
with-dedicated-emulation
workspace-feature-flags
Add desired feature flags to the end of the line above, space separated

Preview Environment / Integration Tests

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-large-vm
/werft with-gce-vm
If enabled this will create the environment on GCE infra
with-integration-tests=all
Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh. If enabled, with-preview and with-large-vm will be enabled.
with-monitoring

/hold

svenefftinge · 2023-10-11T06:44:14Z

components/server/src/authorization/spicedb-authorizer.ts

+        await this.tokenCache.set(
+            ...updates.map<ZedTokenCacheKV>((u) => [
+                u.relationship?.resource,
+                writtenAt, // Make sure that in case we don't get a writtenAt token here, we at least invalidate the cache


I would always expect a token on successful writes. Did you see cases where no writtenAt was returned?

The API gives no guarantee, so I decided to not assume things. 🤷

I doubt it happens, but let's at least log a warning in those cases

svenefftinge · 2023-10-11T06:54:08Z

components/server/src/authorization/spicedb-authorizer.ts

+        // following https://github.com/authzed/spicedb/blob/786555c24af98abfe3f832c94dbae5ca518dcf50/pkg/zedtoken/zedtoken.go#L64-L100
+        const decodedBytes = base64decode(token);
+        const decodedToken = DecodedZedToken.decode(Buffer.from(decodedBytes, "utf8")).v1;
+        if (!decodedToken) {


is that not an error case?

Yes, this is a technical error. But it would not break in a user-facing way - except we got a bunch of these, which we need to detect via metrics+alerts.

I think we need to decide (and document) how we see log levels being used, because I heart people (over time) argue for very different perspectives. 👍

Probably similar case to the above. I think we should at least log an error

svenefftinge · 2023-10-11T07:02:24Z

components/server/src/authorization/spicedb-authorizer.ts

 }
+
+type ZedTokenCacheKV = [objectRef: v1.ObjectReference | undefined, token: string | undefined];
+interface ZedTokenCache {


I don't think there's a need for an interface

I did not touch the code here yet. Let's do the rollout first.

svenefftinge · 2023-10-11T07:02:40Z

components/server/src/authorization/spicedb-authorizer.ts

+export class RequestLocalZedTokenCache implements ZedTokenCache {
+    constructor() {}
+
+    async get(objectRef: v1.ObjectReference | undefined): Promise<string | undefined> {


none of these methods need to be async

Also let's remove the unused arguments. We can introduce them when we need them

I'd rather go with the current version and get data. 🙂 Removal is very quick.

I thought the reason for this PR is clean up?

svenefftinge · 2023-10-11T07:04:57Z

components/server/src/authorization/spicedb-authorizer.ts

+        return getContext().zedToken?.token;
+    }
+
+    async set(...kvs: ZedTokenCacheKV[]) {


wouldn't it be enough to just pass in a single token and only set it if it is greater than the existing one?

We get a bunch of tokens in some cases. As mentioned above, happy to remove all of this once we decided the current implementation is sufficient.

only because you want to update them per resource which we currently don't do/need

svenefftinge · 2023-10-11T07:06:09Z

components/server/src/authorization/spicedb-authorizer.ts

+        }
+    }
+
+    async consistency(resourceRef: v1.ObjectReference | undefined): Promise<v1.Consistency> {


nit: I would pass in the zedtoken and move this method to the authorizer. it seems unrelated to the caching aspect.

For this cache implementation: Yes. For others definitely not. Again, happy to remove/fold in if we don't need that stuff. 🤞

svenefftinge

Looks good!

(minus the comments regarding the token cache, which we can tackle in a follow up)

geropl · 2023-10-11T07:19:32Z

/unhold

roboquat added do-not-merge/work-in-progress do-not-merge/hold size/XXL labels Oct 9, 2023

geropl mentioned this pull request Oct 9, 2023

[server] SpiceDB: Add request-level caching based on AsyncLocalStorage+ZedTokens #18893

Merged

14 tasks

Base automatically changed from gpl/738-zedtokens-3 to main October 10, 2023 06:55

geropl added 3 commits October 10, 2023 12:39

[server] Extract RequextContext from EnhancedLogContext

69d1582

[spicedb] Cleanup SpiceDBAuthorizer

a4dd2b6

[server] SpiceDB: Pass ZedToken to read requests as well

ea0acbf

geropl force-pushed the gpl/738-zedtokens-4 branch from dcd42e3 to ea0acbf Compare October 10, 2023 12:58

geropl changed the title ~~[WIP] zedtokens 4~~ ZedTokens: SpiceDBAuthorizer cleanup Oct 10, 2023

geropl marked this pull request as ready for review October 11, 2023 06:33

geropl requested a review from a team as a code owner October 11, 2023 06:33

roboquat removed the do-not-merge/work-in-progress label Oct 11, 2023

github-actions bot added the team: team-experience label Oct 11, 2023

svenefftinge reviewed Oct 11, 2023

View reviewed changes

svenefftinge approved these changes Oct 11, 2023

View reviewed changes

roboquat removed the do-not-merge/hold label Oct 11, 2023

roboquat merged commit 9bde303 into main Oct 11, 2023

roboquat deleted the gpl/738-zedtokens-4 branch October 11, 2023 07:20

ZedTokens: SpiceDBAuthorizer cleanup #18898

ZedTokens: SpiceDBAuthorizer cleanup #18898

Uh oh!

Conversation

geropl commented Oct 9, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

🤖 Generated by Copilot at dcd42e3

Related Issue(s)

How to test

Documentation

Preview status

Build Options

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svenefftinge left a comment

Choose a reason for hiding this comment

Uh oh!

geropl commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

geropl commented Oct 9, 2023 •

edited by github-actions bot

Loading

`🤖 Generated by Copilot at dcd42e3`