refactor(auth, gateway): use user_id over account_name #1674

jonaro00 · 2024-03-08T13:22:17Z

Description of change

Rename AccountName -> UserId but use a type alias instead of a newtype with no parsing logic (IMO user_id should not be stricly parsed to allow more flexibility)
Still set the old XShuttleAccountName header manually (for old deployers), but with the new user id value ✔️
Use the claim to set user_id field in deployer, like the other backends do ✔️
Posting a user still uses account name, returns struct with a user id
Keeping get user by account name for console
Admin call to gw still includes account_name
Admin command logic not updated
Auth's create user CLI command inserts user with blank name and the provided string as user_id ✔️
New projects in gw get a blank account_name ✔️

How has this been tested? (if applicable)

✔️ = tested

chesedo

Seems good. The metrics notes can be ignored - I just want to make sure we update them all

auth/migrations/0004_user_ids_part2.sql

auth/src/api/builder.rs

chesedo · 2024-03-08T16:31:53Z

auth/src/api/builder.rs

                delete(delete_subscription),
            )
            .route_layer(from_extractor::<Metrics>())
            .layer(
                TraceLayer::new(|request| {
                    request_span!(
                        request,
-                        request.params.account_name = field::Empty,
+                        request.params.user_id = field::Empty,


Note: this will affect metrics

auth/src/api/handlers.rs

chesedo · 2024-03-08T16:41:28Z

common/src/backends/headers.rs

@@ -33,15 +33,15 @@ impl Header for XShuttleAdminSecret {
    }
 }

-pub static X_SHUTTLE_ACCOUNT_NAME: HeaderName = HeaderName::from_static("x-shuttle-account-name");
+pub static X_SHUTTLE_USER_ID: HeaderName = HeaderName::from_static("x-shuttle-user-id");


Changing the header name will break old deployers iirc. So we can't do that

~~This one is only used between auth and gateway, so we can.~~ Nevermind, it is being used for setting the account.name trace field in deployer. However, if we're changing the value that is going to be recorded there (user_xyz instead of github-123), then it makes sense to change the name of the field (like I have in https://github.com/shuttle-hq/shuttle/pull/1674/files#diff-223aa4e48caca5d2fae2a4a306bcd0c99b8480713749f509eb60e858b9f73c04R166). Old deployers don't fail if it goes missing. It is possible to set both the old and new names+values of this header for backwards compat (it becomes pretty ugly tho... we can instead set both fields to the new user id, which is much simpler. Edit: did it).

Fixed in 1edf407

chesedo · 2024-03-08T16:42:48Z

gateway/src/api/latest.rs

    let posthog_client = state.posthog_client.clone();
    tokio::spawn(async move {
-        let event = async_posthog::Event::new("shuttle_api_start_deployment", &account_name);
+        let event = async_posthog::Event::new("shuttle_api_start_deployment", &user_id);


Note: will affect metrics

chesedo · 2024-03-08T16:43:16Z

gateway/src/api/latest.rs

@@ -869,9 +869,9 @@ impl ApiBuilder {
            TraceLayer::new(|request| {
                request_span!(
                    request,
-                    account.name = field::Empty,
+                    account.user_id = field::Empty,


Note: will affect metrics

gateway/src/project.rs

oddgrd

The code changes LGTM, but we should figure out exactly how our metrics will be affected by these changes, and what we will need to do after the release.

auth/migrations/0004_user_ids_part2.sql

deployer/src/handlers/mod.rs

common/src/models/user.rs

chesedo

Quick extra question

auth/src/lib.rs

This reverts commit 0be50c3.

jonaro00 · 2024-03-14T09:22:42Z

.circleci/config.yml

-              only: main
+          # filters:
+          #   branches:
+          #     only: main


This reverts commit bc82a89.

jonaro00 added 6 commits March 8, 2024 13:03

feat(auth): user_id not null, remove migration insertions

76273d3

fix(auth): insert user_id on command inserts

28e82d4

feat: rename account_name to user_id in most places

877930f

nits

e1c20ba

nit2

a816e73

fix: auth tests almost working

5445696

chesedo reviewed Mar 8, 2024

View reviewed changes

jonaro00 added 5 commits March 8, 2024 23:28

yeet: auth/refresh

bcde733

fix: span

3e205d1

fix: auth tests

00c3162

feat: set old account name header for tracing in old deployers

8caf01b

nit: clarify start_last_deploy

617d9c6

oddgrd reviewed Mar 11, 2024

View reviewed changes

auth/migrations/0004_user_ids_part2.sql Outdated Show resolved Hide resolved

deployer/src/handlers/mod.rs Show resolved Hide resolved

common/src/models/user.rs Show resolved Hide resolved

jonaro00 added 2 commits March 11, 2024 14:12

fix: sql comment

c8fe94b

fix: userid comment

2900c3d

chesedo reviewed Mar 12, 2024

View reviewed changes

auth/src/lib.rs Show resolved Hide resolved

jonaro00 added 11 commits March 12, 2024 14:59

feat(deployer): use claim instead of user id header for tracing

1edf407

nit: remove request.path tracing field

0be50c3

Revert "nit: remove request.path tracing field"

0931bf3

This reverts commit 0be50c3.

less clone

3c27279

feat(auth): keep get account by name endpoint

30a966d

fmt

9f0be65

ci: unstable

f33f9da

clippy

443cdc8

fix: migration drift fixes

fa81d18

fix: migration drift fixes 2

d702495

revert: migration drift fixes

0e8e6ef

jonaro00 commented Mar 14, 2024

View reviewed changes

.circleci/config.yml

only: main

# filters:

# branches:

# only: main

Copy link

Member Author

jonaro00 Mar 14, 2024

Choose a reason for hiding this comment

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

reminder

jonaro00 added 2 commits March 14, 2024 15:43

fix: endpoint ordering

51b1c0c

test: set empty field on endpoint

bc82a89

Revert "test: set empty field on endpoint"

b312146

This reverts commit bc82a89.

jonaro00 merged commit 4937f4b into main Mar 15, 2024
29 of 32 checks passed

jonaro00 deleted the token-user-id branch March 15, 2024 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(auth, gateway): use user_id over account_name #1674

refactor(auth, gateway): use user_id over account_name #1674

jonaro00 commented Mar 8, 2024 •

edited

Loading

chesedo left a comment

chesedo Mar 8, 2024

chesedo Mar 8, 2024

jonaro00 Mar 8, 2024 •

edited

Loading

jonaro00 Mar 12, 2024

chesedo Mar 8, 2024

chesedo Mar 8, 2024

oddgrd left a comment

chesedo left a comment

jonaro00 Mar 14, 2024

refactor(auth, gateway): use user_id over account_name #1674

refactor(auth, gateway): use user_id over account_name #1674

Conversation

jonaro00 commented Mar 8, 2024 • edited Loading

Description of change

How has this been tested? (if applicable)

chesedo left a comment

Choose a reason for hiding this comment

chesedo Mar 8, 2024

Choose a reason for hiding this comment

chesedo Mar 8, 2024

Choose a reason for hiding this comment

jonaro00 Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

jonaro00 Mar 12, 2024

Choose a reason for hiding this comment

chesedo Mar 8, 2024

Choose a reason for hiding this comment

chesedo Mar 8, 2024

Choose a reason for hiding this comment

oddgrd left a comment

Choose a reason for hiding this comment

chesedo left a comment

Choose a reason for hiding this comment

jonaro00 Mar 14, 2024

Choose a reason for hiding this comment

jonaro00 commented Mar 8, 2024 •

edited

Loading

jonaro00 Mar 8, 2024 •

edited

Loading