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

Migrate Existing Session Data to Redis and Begin Reading It #58019

Merged
merged 30 commits into from
May 13, 2024

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Apr 15, 2024

Follow-up to #57972. We have been successfully writing session data to Redis, so now it's time to start using it.

This PR switches us from a cookie-based session store to a Redis-based one provided by the redis-actionpack gem. It also includes some temporary logic which will gracefully migrate any sessions that are still trying to store data in the cookie into redis.

Links

Testing story

Tested locally and and on an adhoc that user registration, authentication, and deauthentication all work as expected. Also verified that I can start a session on the staging branch, then switch to this branch and restart the server to pick up the changes, and then continue my session in the browser with no disruption.

Deployment strategy

We should plan to deploy this during a low traffic time when we are able to quickly respond with a revert in case things go wrong.

Follow-up work

After all (or most) sessions have been successfully migrated into Redis, we can remove the migration logic and use a vanilla un-customized Redis-based session store.

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@Hamms Hamms marked this pull request as ready for review April 15, 2024 22:55
@Hamms Hamms requested a review from a team April 16, 2024 00:31
Comment on lines 37 to 38
request.set_header("action_dispatch.request.unsigned_session_cookie", {})
write_session(request, session_id, session_data.except("session_id"), request.session_options)
Copy link
Contributor

Choose a reason for hiding this comment

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

Between these two lines of code, the session only exists in memory, is there anything bad that happens if we clear the cookie and then fail to write the session? Or is it fine if we throw a 500 with the cookie cleared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good catch! I'm not entirely sure how the 500 error response interacts with manually-set headers like this, but my expectation is that we would indeed lose the cookie data in that case. I'll swap these lines, and add an error handler just in case.

@@ -1,10 +1,10 @@
require 'cdo/cookie_helpers'
require_relative '../custom_session_store/mirror_cookies_in_redis_store'
require_relative '../custom_session_store/migrate_cookies_to_redis_store'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is mirror_cookies_in_redis_store still in use? Does that file need to go away here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't; I had it in my head that I was going to clean that up later when I eventually remove the MigrateCookiesStore logic, but I guess there's no reason not to do so now

@sureshc
Copy link
Contributor

sureshc commented Apr 16, 2024

Do we need to update our Rack Request extension? It seems to try to decrypt the session cookie to extract the user_id, I'm guessing so that Pegasus knows who the user is.

def user_id_from_session_cookie
session_cookie_key = "_learn_session"
session_cookie_key += "_#{rack_env}" unless rack_env?(:production)
message = CGI.unescape(cookies[session_cookie_key].to_s)
key_generator = ActiveSupport::KeyGenerator.new(
CDO.dashboard_secret_key_base,
iterations: 1000
)
encryptor = ActiveSupport::MessageEncryptor.new(
key_generator.generate_key('encrypted cookie')[0, ActiveSupport::MessageEncryptor.key_len],
key_generator.generate_key('signed encrypted cookie')
)
return nil unless cookie = encryptor.decrypt_and_verify(message)
return nil unless warden = cookie['warden.user.user.key']
warden.first.first
rescue
return nil
end

@sureshc
Copy link
Contributor

sureshc commented Apr 16, 2024

This is a foundational change to our web applications. Are there ways we can reduce risk? Can we roll out this change to a subset of users/sessions and increase that percentage over time to all users? Do we have easy ways to rollback without a deploy? I'm assuming/hoping that any unanticipated negative impacts of this change would actually be smaller than the several hundred requests a day that are failing due to Cookie overflow 😅

@Hamms
Copy link
Contributor Author

Hamms commented Apr 16, 2024

Do we need to update our Rack Request extension? It seems to try to decrypt the session cookie to extract the user_id, I'm guessing so that Pegasus knows who the user is.

Oh damn, yes we do. I did not know this existed; I'm hoping that Drone tests will catch this now that they're configured correctly.

Can we roll out this change to a subset of users/sessions and increase that percentage over time to all users?

Not easily; we'd have to implement a session store that supports both cookie-based and redis-based sessions simultaneously, which would involve significantly more custom logic than what we've done so far.

Do we have easy ways to rollback without a deploy?

No, not at all. I'm planning to put some work into implementing another custom session store that will allow us to rollback this change at all, but by default attempting to switch back from a Redis-based session storage strategy that just puts a string in the cookie to a Cookie-based session storage strategy that expects the cookie to contain a Hash would be extremely disruptive.

…58232)

* Update Legacy `user_id` Implementation to Support New Session Store

In response to us moving session data out of the cookie and into a
persistent store, also update the legacy implementation of `user_id` we
have in Rack to read the user id from that store rather than the cookie.

* add comments

* reimplement private method rather than calling it in a hacky way

* Add unit tests for Cdo::RequestExtension

In addition to adding a unit test for the new piece of functionality
being added in this PR, also add some tests for existing functionality
which has been heretofore untested.

* use dynamic rather than hardcoded test values, since the underlying data being tested gets regularly updated
@Hamms Hamms merged commit 78e1d98 into staging May 13, 2024
1 of 2 checks passed
@Hamms Hamms deleted the read-session-data-from-redis branch May 13, 2024 21:33
Hamms added a commit that referenced this pull request May 13, 2024
Hamms added a commit that referenced this pull request May 13, 2024
Hamms added a commit that referenced this pull request May 14, 2024
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.

None yet

4 participants