Merge branch 'reduce-redis-storage-session-lookup' into 'master'

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

Closes #219071

See merge request gitlab-org/gitlab!40336

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