Simple count of pre-logged-in-sessions per IP

As noted in https://gitlab.com/gitlab-org/gitlab/-/issues/219071, the
current storage mechanism is a set containing the session id, but this
is never used.
We only need the count to determine when to start potentially invoking
recaptcha.

Keeping the count only is not only more performant for the web app, it
stores significantly less in Redis, particularly for certain
pathological cases

Fixes https://gitlab.com/gitlab-org/gitlab/-/issues/219071

app/controllers/sessions_controller.rb

@@ -156,13 +156,13 @@ class SessionsController < Devise::SessionsController
     (options = request.env["warden.options"]) && options[:action] == "unauthenticated"
   end
 
-  # storing sessions per IP lets us check if there are associated multiple
+  # counting sessions per IP lets us check if there are associated multiple
   # anonymous sessions with one IP and prevent situations when there are
   # multiple attempts of logging in
   def store_unauthenticated_sessions
     return if current_user
 
-    Gitlab::AnonymousSession.new(request.remote_ip, session_id: request.session.id).store_session_id_per_ip
+    Gitlab::AnonymousSession.new(request.remote_ip).count_session_ip
   end
 
   # Handle an "initial setup" state, where there's only one user, it's an admin,
@@ -280,7 +280,7 @@ class SessionsController < Devise::SessionsController
   end
 
   def exceeded_anonymous_sessions?
-    Gitlab::AnonymousSession.new(request.remote_ip).stored_sessions >= MAX_FAILED_LOGIN_ATTEMPTS
+    Gitlab::AnonymousSession.new(request.remote_ip).session_count >= MAX_FAILED_LOGIN_ATTEMPTS
   end
 
   def authentication_method

changelogs/unreleased/reduce-redis-storage-session-lookup.yml

+---
+title: Reduce storage requirements for keeping track of pre-logged-in sessions
+merge_request: 40336
+author:
+type: performance

config/initializers/warden.rb

@@ -19,7 +19,7 @@ Rails.application.configure do |config|
 
   Warden::Manager.after_authentication(scope: :user) do |user, auth, opts|
     ActiveSession.cleanup(user)
-    Gitlab::AnonymousSession.new(auth.request.remote_ip, session_id: auth.request.session.id).cleanup_session_per_ip_entries
+    Gitlab::AnonymousSession.new(auth.request.remote_ip).cleanup_session_per_ip_count
   end
 
   Warden::Manager.after_set_user(scope: :user, only: :fetch) do |user, auth, opts|

lib/gitlab/anonymous_session.rb

@@ -2,35 +2,34 @@
 
 module Gitlab
   class AnonymousSession
-    def initialize(remote_ip, session_id: nil)
+    def initialize(remote_ip)
       @remote_ip = remote_ip
-      @session_id = session_id
     end
 
-    def store_session_id_per_ip
+    def count_session_ip
       Gitlab::Redis::SharedState.with do |redis|
         redis.pipelined do
-          redis.sadd(session_lookup_name, session_id)
+          redis.incr(session_lookup_name)
           redis.expire(session_lookup_name, 24.hours)
         end
       end
     end
 
-    def stored_sessions
+    def session_count
       Gitlab::Redis::SharedState.with do |redis|
-        redis.scard(session_lookup_name)
+        redis.get(session_lookup_name).to_i
       end
     end
 
-    def cleanup_session_per_ip_entries
+    def cleanup_session_per_ip_count
       Gitlab::Redis::SharedState.with do |redis|
-        redis.srem(session_lookup_name, session_id)
+        redis.del(session_lookup_name)
       end
     end
 
     private
 
-    attr_reader :remote_ip, :session_id
+    attr_reader :remote_ip
 
     def session_lookup_name
       @session_lookup_name ||= "#{Gitlab::Redis::SharedState::IP_SESSIONS_LOOKUP_NAMESPACE}:#{remote_ip}"

lib/gitlab/redis/shared_state.rb

@@ -9,7 +9,7 @@ module Gitlab
       SESSION_NAMESPACE = 'session:gitlab'
       USER_SESSIONS_NAMESPACE = 'session:user:gitlab'
       USER_SESSIONS_LOOKUP_NAMESPACE = 'session:lookup:user:gitlab'
-      IP_SESSIONS_LOOKUP_NAMESPACE = 'session:lookup:ip:gitlab'
+      IP_SESSIONS_LOOKUP_NAMESPACE = 'session:lookup:ip:gitlab2'
       DEFAULT_REDIS_SHARED_STATE_URL = 'redis://localhost:6382'
       REDIS_SHARED_STATE_CONFIG_ENV_VAR_NAME = 'GITLAB_REDIS_SHARED_STATE_CONFIG_FILE'
 

spec/controllers/sessions_controller_spec.rb

@@ -229,7 +229,7 @@ RSpec.describe SessionsController do
 
           context 'when there are more than 5 anonymous session with the same IP' do
             before do
-              allow(Gitlab::AnonymousSession).to receive_message_chain(:new, :stored_sessions).and_return(6)
+              allow(Gitlab::AnonymousSession).to receive_message_chain(:new, :session_count).and_return(6)
             end
 
             it 'displays an error when the reCAPTCHA is not solved' do
@@ -241,7 +241,7 @@ RSpec.describe SessionsController do
             end
 
             it 'successfully logs in a user when reCAPTCHA is solved' do
-              expect(Gitlab::AnonymousSession).to receive_message_chain(:new, :cleanup_session_per_ip_entries)
+              expect(Gitlab::AnonymousSession).to receive_message_chain(:new, :cleanup_session_per_ip_count)
 
               succesful_login(user_params)
 

spec/lib/gitlab/anonymous_session_spec.rb

@@ -8,45 +8,36 @@ RSpec.describe Gitlab::AnonymousSession, :clean_gitlab_redis_shared_state do
 
   subject { new_anonymous_session }
 
-  def new_anonymous_session(session_id = default_session_id)
-    described_class.new('127.0.0.1', session_id: session_id)
+  def new_anonymous_session
+    described_class.new('127.0.0.1')
   end
 
-  describe '#store_session_id_per_ip' do
+  describe '#store_session_ip' do
     it 'adds session id to proper key' do
-      subject.store_session_id_per_ip
+      subject.count_session_ip
 
       Gitlab::Redis::SharedState.with do |redis|
-        expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [default_session_id]
+        expect(redis.get("session:lookup:ip:gitlab2:127.0.0.1").to_i).to eq 1
       end
     end
 
     it 'adds expiration time to key' do
       Timecop.freeze do
-        subject.store_session_id_per_ip
+        subject.count_session_ip
 
         Gitlab::Redis::SharedState.with do |redis|
-          expect(redis.ttl("session:lookup:ip:gitlab:127.0.0.1")).to eq(24.hours.to_i)
+          expect(redis.ttl("session:lookup:ip:gitlab2:127.0.0.1")).to eq(24.hours.to_i)
         end
       end
     end
 
-    it 'adds id only once' do
-      subject.store_session_id_per_ip
-      subject.store_session_id_per_ip
-
-      Gitlab::Redis::SharedState.with do |redis|
-        expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [default_session_id]
-      end
-    end
-
     context 'when there is already one session' do
-      it 'adds session id to proper key' do
-        subject.store_session_id_per_ip
-        new_anonymous_session(additional_session_id).store_session_id_per_ip
+      it 'increments the session count' do
+        subject.count_session_ip
+        new_anonymous_session.count_session_ip
 
         Gitlab::Redis::SharedState.with do |redis|
-          expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to contain_exactly(default_session_id, additional_session_id)
+          expect(redis.get("session:lookup:ip:gitlab2:127.0.0.1").to_i).to eq(2)
         end
       end
     end
@@ -55,24 +46,22 @@ RSpec.describe Gitlab::AnonymousSession, :clean_gitlab_redis_shared_state do
   describe '#stored_sessions' do
     it 'returns all anonymous sessions per ip' do
       Gitlab::Redis::SharedState.with do |redis|
-        redis.sadd("session:lookup:ip:gitlab:127.0.0.1", default_session_id)
-        redis.sadd("session:lookup:ip:gitlab:127.0.0.1", additional_session_id)
+        redis.set("session:lookup:ip:gitlab2:127.0.0.1", 2)
       end
 
-      expect(subject.stored_sessions).to eq(2)
+      expect(subject.session_count).to eq(2)
     end
   end
 
   it 'removes obsolete lookup through ip entries' do
     Gitlab::Redis::SharedState.with do |redis|
-      redis.sadd("session:lookup:ip:gitlab:127.0.0.1", default_session_id)
-      redis.sadd("session:lookup:ip:gitlab:127.0.0.1", additional_session_id)
+      redis.set("session:lookup:ip:gitlab2:127.0.0.1", 2)
     end
 
-    subject.cleanup_session_per_ip_entries
+    subject.cleanup_session_per_ip_count
 
     Gitlab::Redis::SharedState.with do |redis|
-      expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [additional_session_id]
+      expect(redis.exists("session:lookup:ip:gitlab2:127.0.0.1")).to eq(false)
     end
   end
 end