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

Use rate limiting Redis instance

See merge request gitlab-org/gitlab!71196

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

ee/spec/services/deployments/auto_rollback_service_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Deployments::AutoRollbackService, :clean_gitlab_redis_cache do
+RSpec.describe Deployments::AutoRollbackService, :clean_gitlab_redis_rate_limiting do
   let_it_be(:maintainer) { create(:user) }
   let_it_be(:project, refind: true) { create(:project, :repository) }
   let_it_be(:environment, refind: true) { create(:environment, project: project) }

lib/gitlab/application_rate_limiter.rb

@@ -73,7 +73,7 @@ module Gitlab
         value = 0
         interval_value = interval || interval(key)
 
-        Gitlab::Redis::Cache.with do |redis|
+        cache_store.with do |redis|
           cache_key = action_key(key, scope)
           value     = redis.incr(cache_key)
           redis.expire(cache_key, interval_value) if value == 1
@@ -109,6 +109,14 @@ 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

@@ -2,9 +2,10 @@
 
 module Gitlab
   module RackAttack
-    # This class is a proxy for all Redis calls made by RackAttack. All the
-    # calls are instrumented, then redirected to ::Rails.cache. This class
-    # instruments the standard interfaces of ActiveRecord::Cache defined in
+    # This class is a proxy for all Redis calls made by RackAttack. All
+    # the calls are instrumented, then redirected to the underlying
+    # store (in `.store). This class instruments the standard interfaces
+    # of ActiveRecord::Cache defined in
     # https://github.com/rails/rails/blob/v6.0.3.1/activesupport/lib/active_support/cache.rb#L315
     #
     # For more information, please see
@@ -14,7 +15,18 @@ module Gitlab
 
       delegate :silence!, :mute, to: :@upstream_store
 
-      def initialize(upstream_store: ::Rails.cache, notifier: ActiveSupport::Notifications)
+      # 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)
         @upstream_store = upstream_store
         @notifier = notifier
       end

lib/gitlab/redis/rate_limiting.rb

@@ -7,6 +7,10 @@ module Gitlab
       def self.config_fallback
         Cache
       end
+
+      def self.cache_store
+        @cache_store ||= ActiveSupport::Cache::RedisCacheStore.new(redis: pool, namespace: Cache::CACHE_NAMESPACE)
+      end
     end
   end
 end

spec/controllers/projects/pipeline_schedules_controller_spec.rb

@@ -397,7 +397,7 @@ RSpec.describe Projects::PipelineSchedulesController do
     end
   end
 
-  describe 'POST #play', :clean_gitlab_redis_cache do
+  describe 'POST #play', :clean_gitlab_redis_rate_limiting do
     let(:ref) { 'master' }
 
     before do

spec/controllers/projects/raw_controller_spec.rb

@@ -84,7 +84,7 @@ RSpec.describe Projects::RawController do
       include_examples 'single Gitaly request'
     end
 
-    context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache do
+    context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_rate_limiting do
       let(:file_path) { 'master/README.md' }
 
       before do

spec/controllers/projects_controller_spec.rb

@@ -1384,12 +1384,12 @@ RSpec.describe ProjectsController do
         end
       end
 
-      context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache do
+      context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_rate_limiting do
         include_examples 'rate limits project export endpoint'
       end
     end
 
-    describe '#download_export', :clean_gitlab_redis_cache do
+    describe '#download_export', :clean_gitlab_redis_rate_limiting do
       let(:action) { :download_export }
 
       context 'object storage enabled' do
@@ -1424,7 +1424,7 @@ RSpec.describe ProjectsController do
           end
         end
 
-        context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache do
+        context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_rate_limiting do
           before do
             allow(Gitlab::ApplicationRateLimiter)
               .to receive(:increment)
@@ -1496,7 +1496,7 @@ RSpec.describe ProjectsController do
         end
       end
 
-      context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache do
+      context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_rate_limiting do
         include_examples 'rate limits project export endpoint'
       end
     end

spec/lib/gitlab/application_rate_limiter_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::ApplicationRateLimiter, :clean_gitlab_redis_cache do
+RSpec.describe Gitlab::ApplicationRateLimiter do
   let(:redis) { double('redis') }
   let(:user) { create(:user) }
   let(:project) { create(:project) }
@@ -20,7 +20,6 @@ RSpec.describe Gitlab::ApplicationRateLimiter, :clean_gitlab_redis_cache do
   subject { described_class }
 
   before do
-    allow(Gitlab::Redis::Cache).to receive(:with).and_yield(redis)
     allow(described_class).to receive(:rate_limits).and_return(rate_limits)
   end
 
@@ -49,73 +48,94 @@ RSpec.describe Gitlab::ApplicationRateLimiter, :clean_gitlab_redis_cache do
     end
   end
 
-  context 'when the key is an array of only ActiveRecord models' do
-    let(:scope) { [user, project] }
+  # 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] }
 
-    let(:cache_key) do
-      "application_rate_limiter:test_action:user:#{user.id}:project:#{project.id}"
+      let(:cache_key) do
+        "application_rate_limiter:test_action:user:#{user.id}:project:#{project.id}"
+      end
+
+      it_behaves_like 'action rate limiter'
     end
 
-    it_behaves_like 'action rate limiter'
-  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] }
 
-  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] }
+      let(:cache_key) do
+        "application_rate_limiter:test_action:project:#{project.id}:commit:#{commit.sha}:#{path}"
+      end
 
-    let(:cache_key) do
-      "application_rate_limiter:test_action:project:#{project.id}:commit:#{commit.sha}:#{path}"
+      it_behaves_like 'action rate limiter'
     end
 
-    it_behaves_like 'action rate limiter'
-  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}" }
+    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(: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
+      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 }
+      context 'without a current user' do
+        let(:current_user) { nil }
 
-      it 'logs information to auth.log' do
-        expect(Gitlab::AuthLogger).to receive(:error).with(base_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)
+          subject.log_request(request, type, current_user)
+        end
       end
-    end
 
-    context 'with a current_user' do
-      let(:current_user) { create(:user) }
+      context 'with a current_user' do
+        let(:current_user) { create(:user) }
 
-      let(:attributes) do
-        base_attributes.merge({
-          user_id: current_user.id,
-          username: current_user.username
-        })
-      end
+        let(:attributes) do
+          base_attributes.merge({
+                                  user_id: current_user.id,
+                                  username: current_user.username
+                                })
+        end
 
-      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(attributes).once
 
-        subject.log_request(request, type, current_user)
+          subject.log_request(request, type, current_user)
+        end
       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
+
+    it_behaves_like 'rate limiting'
+  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_behaves_like 'rate limiting'
+  end
 end

spec/lib/gitlab/rack_attack/instrumented_cache_store_spec.rb

@@ -86,4 +86,17 @@ 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

spec/lib/gitlab/rate_limit_helpers_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::RateLimitHelpers, :clean_gitlab_redis_cache do
+RSpec.describe Gitlab::RateLimitHelpers, :clean_gitlab_redis_rate_limiting do
   let(:limiter_class) do
     Class.new do
       include ::Gitlab::RateLimitHelpers

spec/services/web_hook_service_spec.rb

@@ -392,7 +392,7 @@ RSpec.describe WebHookService do
         end
       end
 
-      context 'when the hook is throttled (via Redis)', :clean_gitlab_redis_cache do
+      context 'when the hook is throttled (via Redis)', :clean_gitlab_redis_rate_limiting do
         before do
           # Set a high interval to avoid intermittent failures in CI
           allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits).and_return(