Skip to content

Save session data directly in a cookie #2732

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

Merged
merged 1 commit into from
Mar 31, 2025
Merged

Conversation

jianglai
Copy link
Collaborator

@jianglai jianglai commented Mar 29, 2025

The new CookieSessionMetadata makes the Nomulus EPP endpoint stateless. The session data is stored in the cookie, which has the same lifetime as the connection/session itself. This makes the session information global and therefore the release process seamless as no session needs to be re-established via proxy restart, as long as the proxy stays connected to the client, and no period during which stale session information exists on the proxy but not on the newly deployed backend itself exists. It also makes it possible to horizontally scale the frontend deployment like other stateless services.

Also removed a redundant JDK version check in tests as we require at least JDK 21 now. There are some JDK versions which are not all digits (e.g. 21.0.5-ea) that cause the parsing to fail.

TESTED=Deployed to crash. Verified that proxy restart is no longer needed and no "no logged in" errors appeared during a release.

This change is Reviewable

@jianglai jianglai force-pushed the session branch 5 times, most recently from 3200288 to 1db27c1 Compare March 29, 2025 17:46
Copy link
Collaborator Author

@jianglai jianglai left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 10 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jianglai)

Copy link
Collaborator Author

@jianglai jianglai left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jianglai)

@jianglai jianglai requested a review from ptkach March 29, 2025 21:15
@jianglai jianglai enabled auto-merge March 29, 2025 21:15
Copy link
Collaborator

@ptkach ptkach left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 10 files at r1, 1 of 1 files at r3, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jianglai)


core/src/main/java/google/registry/flows/CookieSessionMetadata.java line 68 at r7 (raw file):

                String sessionInfo = decode(matcher.group(1));
                matcher = REGISTRAR_ID_PATTERN.matcher(sessionInfo);
                if (matcher.find()) {

This kind of approach (parsing string,then regexping) sometimes can lead to errors that hard to find, so I think we might want to have some logging to all of the patter matchers to make sure that we can trace back what's been matched in the cookie and that it worked as expected.

Copy link
Collaborator

@ptkach ptkach left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @jianglai)


core/src/main/java/google/registry/flows/CookieSessionMetadata.java line 69 at r8 (raw file):

              if (matcher.find()) {
                String sessionInfo = decode(matcher.group(1));
                logger.atInfo().log("SESSION INFO: %s", sessionInfo);

As this doesn't actually provide any clarity on what matchers actually matched. I think logger.atInfo().log("SESSION INFO: %s", data.toString()); at the bottom would be more useful.

Copy link
Collaborator Author

@jianglai jianglai left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ptkach)


core/src/main/java/google/registry/flows/CookieSessionMetadata.java line 68 at r7 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

This kind of approach (parsing string,then regexping) sometimes can lead to errors that hard to find, so I think we might want to have some logging to all of the patter matchers to make sure that we can trace back what's been matched in the cookie and that it worked as expected.

Done.


core/src/main/java/google/registry/flows/CookieSessionMetadata.java line 69 at r8 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

As this doesn't actually provide any clarity on what matchers actually matched. I think logger.atInfo().log("SESSION INFO: %s", data.toString()); at the bottom would be more useful.

We want the raw cookie because we can use it to deduce which part of the parsing went wrong.

As far logging the data itself, it is actually already logged here.

Copy link
Collaborator

@ptkach ptkach left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 10 files at r1, 1 of 2 files at r5, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jianglai)

@jianglai jianglai added this pull request to the merge queue Mar 31, 2025
Merged via the queue into google:master with commit 4999a72 Mar 31, 2025
8 of 9 checks passed
@jianglai jianglai deleted the session branch March 31, 2025 17:12
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.

2 participants