Merge branch 'always-use-rate-limiting-redis' into 'master'

Always use rate limiting Redis

See merge request gitlab-org/gitlab!72113

config/feature_flags/development/use_rate_limiting_store_for_application_rate_limiter.yml

----
-name: use_rate_limiting_store_for_application_rate_limiter
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71196
-rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1249
-milestone: '14.4'
-type: development
-group: group::scalability
-default_enabled: false

lib/gitlab/application_rate_limiter.rb

@@ -73,7 +73,7 @@ module Gitlab
         value = 0
         interval_value = interval || interval(key)
 
-        cache_store.with do |redis|
+        ::Gitlab::Redis::RateLimiting.with do |redis|
           cache_key = action_key(key, scope)
           value     = redis.incr(cache_key)
           redis.expire(cache_key, interval_value) if value == 1
@@ -109,14 +109,6 @@ module Gitlab
 
       private
 
-      def cache_store
-        if ::Feature.enabled?(:use_rate_limiting_store_for_application_rate_limiter, default_enabled: :yaml)
-          ::Gitlab::Redis::RateLimiting
-        else
-          ::Gitlab::Redis::Cache
-        end
-      end
-
       def threshold(key)
         value = rate_limit_value_by_key(key, :threshold)
 

lib/gitlab/rack_attack/instrumented_cache_store.rb

@@ -15,18 +15,7 @@ module Gitlab
 
       delegate :silence!, :mute, to: :@upstream_store
 
-      # Clean up in https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1249
-      def self.store
-        if ENV['USE_RATE_LIMITING_STORE_FOR_RACK_ATTACK'] == '1'
-          Gitlab::AuthLogger.info(message: 'Rack::Attack using rate limiting store')
-          ::Gitlab::Redis::RateLimiting.cache_store
-        else
-          Gitlab::AuthLogger.info(message: 'Rack::Attack using cache store')
-          ::Rails.cache
-        end
-      end
-
-      def initialize(upstream_store: self.class.store, notifier: ActiveSupport::Notifications)
+      def initialize(upstream_store: ::Gitlab::Redis::RateLimiting.cache_store, notifier: ActiveSupport::Notifications)
         @upstream_store = upstream_store
         @notifier = notifier
       end

spec/lib/gitlab/application_rate_limiter_spec.rb

@@ -20,6 +20,7 @@ RSpec.describe Gitlab::ApplicationRateLimiter do
   subject { described_class }
 
   before do
+    allow(Gitlab::Redis::RateLimiting).to receive(:with).and_yield(redis)
     allow(described_class).to receive(:rate_limits).and_return(rate_limits)
   end
 
@@ -48,94 +49,73 @@ RSpec.describe Gitlab::ApplicationRateLimiter do
     end
   end
 
-  # Clean up in https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1249
-  shared_examples 'rate limiting' do
-    context 'when the key is an array of only ActiveRecord models' do
-      let(:scope) { [user, project] }
+  context 'when the key is an array of only ActiveRecord models' do
+    let(:scope) { [user, project] }
 
-      let(:cache_key) do
-        "application_rate_limiter:test_action:user:#{user.id}:project:#{project.id}"
-      end
-
-      it_behaves_like 'action rate limiter'
+    let(:cache_key) do
+      "application_rate_limiter:test_action:user:#{user.id}:project:#{project.id}"
     end
 
-    context 'when they key a combination of ActiveRecord models and strings' do
-      let(:project) { create(:project, :public, :repository) }
-      let(:commit) { project.repository.commit }
-      let(:path) { 'app/controllers/groups_controller.rb' }
-      let(:scope) { [project, commit, path] }
+    it_behaves_like 'action rate limiter'
+  end
 
-      let(:cache_key) do
-        "application_rate_limiter:test_action:project:#{project.id}:commit:#{commit.sha}:#{path}"
-      end
+  context 'when they key a combination of ActiveRecord models and strings' do
+    let(:project) { create(:project, :public, :repository) }
+    let(:commit) { project.repository.commit }
+    let(:path) { 'app/controllers/groups_controller.rb' }
+    let(:scope) { [project, commit, path] }
 
-      it_behaves_like 'action rate limiter'
+    let(:cache_key) do
+      "application_rate_limiter:test_action:project:#{project.id}:commit:#{commit.sha}:#{path}"
     end
 
-    describe '#log_request' do
-      let(:file_path) { 'master/README.md' }
-      let(:type) { :raw_blob_request_limit }
-      let(:fullpath) { "/#{project.full_path}/raw/#{file_path}" }
-
-      let(:request) do
-        double('request', ip: '127.0.0.1', request_method: 'GET', fullpath: fullpath)
-      end
-
-      let(:base_attributes) do
-        {
-          message: 'Application_Rate_Limiter_Request',
-          env: type,
-          remote_ip: '127.0.0.1',
-          request_method: 'GET',
-          path: fullpath
-        }
-      end
-
-      context 'without a current user' do
-        let(:current_user) { nil }
+    it_behaves_like 'action rate limiter'
+  end
 
-        it 'logs information to auth.log' do
-          expect(Gitlab::AuthLogger).to receive(:error).with(base_attributes).once
+  describe '#log_request' do
+    let(:file_path) { 'master/README.md' }
+    let(:type) { :raw_blob_request_limit }
+    let(:fullpath) { "/#{project.full_path}/raw/#{file_path}" }
 
-          subject.log_request(request, type, current_user)
-        end
-      end
+    let(:request) do
+      double('request', ip: '127.0.0.1', request_method: 'GET', fullpath: fullpath)
+    end
 
-      context 'with a current_user' do
-        let(:current_user) { create(:user) }
+    let(:base_attributes) do
+      {
+        message: 'Application_Rate_Limiter_Request',
+        env: type,
+        remote_ip: '127.0.0.1',
+        request_method: 'GET',
+        path: fullpath
+      }
+    end
 
-        let(:attributes) do
-          base_attributes.merge({
-                                  user_id: current_user.id,
-                                  username: current_user.username
-                                })
-        end
+    context 'without a current user' do
+      let(:current_user) { nil }
 
-        it 'logs information to auth.log' do
-          expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
+      it 'logs information to auth.log' do
+        expect(Gitlab::AuthLogger).to receive(:error).with(base_attributes).once
 
-          subject.log_request(request, type, current_user)
-        end
+        subject.log_request(request, type, current_user)
       end
     end
-  end
 
-  context 'when use_rate_limiting_store_for_application_rate_limiter is enabled' do
-    before do
-      stub_feature_flags(use_rate_limiting_store_for_application_rate_limiter: true)
-      allow(Gitlab::Redis::RateLimiting).to receive(:with).and_yield(redis)
-    end
+    context 'with a current_user' do
+      let(:current_user) { create(:user) }
 
-    it_behaves_like 'rate limiting'
-  end
+      let(:attributes) do
+        base_attributes.merge({
+                                user_id: current_user.id,
+                                username: current_user.username
+                              })
+      end
 
-  context 'when use_rate_limiting_store_for_application_rate_limiter is disabled' do
-    before do
-      stub_feature_flags(use_rate_limiting_store_for_application_rate_limiter: false)
-      allow(Gitlab::Redis::Cache).to receive(:with).and_yield(redis)
-    end
+      it 'logs information to auth.log' do
+        expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
 
-    it_behaves_like 'rate limiting'
+        subject.log_request(request, type, current_user)
+      end
+    end
   end
 end

spec/lib/gitlab/rack_attack/instrumented_cache_store_spec.rb

@@ -86,17 +86,4 @@ RSpec.describe Gitlab::RackAttack::InstrumentedCacheStore do
       test_proc.call(subject)
     end
   end
-
-  # Remove in https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1249
-  describe '.store' do
-    it 'uses the rate limiting store when USE_RATE_LIMITING_STORE_FOR_RACK_ATTACK is set' do
-      stub_env('USE_RATE_LIMITING_STORE_FOR_RACK_ATTACK', '1')
-
-      expect(described_class.store).to eq(Gitlab::Redis::RateLimiting.cache_store)
-    end
-
-    it 'uses the cache store' do
-      expect(described_class.store).to eq(Rails.cache)
-    end
-  end
 end