Merge branch '73343-cleanup-feature-flag' into 'master'

Remove rate limiter feature flag

See merge request gitlab-org/gitlab!73939

config/feature_flags/development/rate_limiter_safe_increment.yml

----
-name: rate_limiter_safe_increment
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73343
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/285352
-milestone: '14.5'
-type: development
-group: group::project management
-default_enabled: false

lib/gitlab/application_rate_limiter.rb

@@ -79,28 +79,6 @@ module Gitlab
           increment(key, options[:scope]) > threshold_value
       end
 
-      # Increments the given cache key and increments the value by 1 with the
-      # expiration interval defined in `.rate_limits`.
-      #
-      # @param key [Symbol] Key attribute registered in `.rate_limits`
-      # @option scope [Array<ActiveRecord>] Array of ActiveRecord models to scope throttling to a specific request (e.g. per user per project)
-      #
-      # @return [Integer] incremented value
-      def increment(key, scope)
-        return safe_increment(key, scope) if Feature.enabled?(:rate_limiter_safe_increment, default_enabled: :yaml)
-
-        value = 0
-        interval_value = interval(key)
-
-        ::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
-        end
-
-        value
-      end
-
       # Increments a cache key that is based on the current time and interval.
       # So that when time passes to the next interval, the key changes and the count starts again from 0.
       #
@@ -110,7 +88,7 @@ module Gitlab
       # @option scope [Array<ActiveRecord>] Array of ActiveRecord models to scope throttling to a specific request (e.g. per user per project)
       #
       # @return [Integer] incremented value
-      def safe_increment(key, scope)
+      def increment(key, scope)
         interval_value = interval(key)
 
         period_key, time_elapsed_in_period = Time.now.to_i.divmod(interval_value)

spec/lib/gitlab/application_rate_limiter_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Gitlab::ApplicationRateLimiter do
 
   subject { described_class }
 
-  describe '.throttled?' do
+  describe '.throttled?', :clean_gitlab_redis_rate_limiting do
     let(:rate_limits) do
       {
         test_action: {
@@ -56,136 +56,51 @@ RSpec.describe Gitlab::ApplicationRateLimiter do
       end
     end
 
-    context 'when rate_limiter_safe_increment is disabled' do
-      let(:redis) { double('redis') }
-      let(:key) { rate_limits.keys[0] }
+    shared_examples 'throttles based on key and scope' do
+      let(:start_time) { Time.current.beginning_of_hour }
 
-      before do
-        allow(Gitlab::Redis::RateLimiting).to receive(:with).and_yield(redis)
-
-        stub_feature_flags(rate_limiter_safe_increment: false)
-      end
-
-      shared_examples 'action rate limiter' do
-        it 'increases the throttle count and sets the expiration time' do
-          expect(redis).to receive(:incr).with(cache_key).and_return(1)
-          expect(redis).to receive(:expire).with(cache_key, 120)
-
-          expect(subject.throttled?(key, scope: scope)).to be_falsy
+      it 'returns true when threshold is exceeded' do
+        travel_to(start_time) do
+          expect(subject.throttled?(:test_action, scope: scope)).to eq(false)
         end
 
-        it 'returns true if the key is throttled' do
-          expect(redis).to receive(:incr).with(cache_key).and_return(2)
-          expect(redis).not_to receive(:expire)
+        travel_to(start_time + 1.minute) do
+          expect(subject.throttled?(:test_action, scope: scope)).to eq(true)
 
-          expect(subject.throttled?(key, scope: scope)).to be_truthy
-        end
-
-        context 'when throttling is disabled' do
-          it 'returns false and does not set expiration time' do
-            expect(redis).not_to receive(:incr)
-            expect(redis).not_to receive(:expire)
-
-            expect(subject.throttled?(key, scope: scope, threshold: 0)).to be_falsy
-          end
+          # Assert that it does not affect other actions or scope
+          expect(subject.throttled?(:another_action, scope: scope)).to eq(false)
+          expect(subject.throttled?(:test_action, scope: [user])).to eq(false)
         end
       end
 
-      context 'when the key is an array of only ActiveRecord models' do
-        let(:scope) { [user, project] }
+      it 'returns false when interval has elapsed' do
+        travel_to(start_time) do
+          expect(subject.throttled?(:test_action, scope: scope)).to eq(false)
 
-        let(:cache_key) do
-          "application_rate_limiter:test_action:user:#{user.id}:project:#{project.id}"
+          # another_action has a threshold of 3 so we simulate 2 requests
+          expect(subject.throttled?(:another_action, scope: scope)).to eq(false)
+          expect(subject.throttled?(:another_action, scope: scope)).to eq(false)
         end
 
-        it_behaves_like 'action rate limiter'
-
-        context 'when a scope attribute is nil' do
-          let(:scope) { [user, nil] }
-
-          let(:cache_key) do
-            "application_rate_limiter:test_action:user:#{user.id}"
-          end
-
-          it_behaves_like 'action rate limiter'
-        end
-      end
-
-      context 'when the key is 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
-
-        it_behaves_like 'action rate limiter'
-
-        context 'when a scope attribute is nil' do
-          let(:scope) { [project, commit, nil] }
-
-          let(:cache_key) do
-            "application_rate_limiter:test_action:project:#{project.id}:commit:#{commit.sha}"
-          end
+        travel_to(start_time + 2.minutes) do
+          expect(subject.throttled?(:test_action, scope: scope)).to eq(false)
 
-          it_behaves_like 'action rate limiter'
+          # Assert that another_action has its own interval that hasn't elapsed
+          expect(subject.throttled?(:another_action, scope: scope)).to eq(true)
         end
       end
     end
 
-    context 'when rate_limiter_safe_increment is enabled', :clean_gitlab_redis_rate_limiting do
-      before do
-        stub_feature_flags(rate_limiter_safe_increment: true)
-      end
-
-      shared_examples 'throttles based on key and scope' do
-        let(:start_time) { Time.current.beginning_of_hour }
-
-        it 'returns true when threshold is exceeded' do
-          travel_to(start_time) do
-            expect(subject.throttled?(:test_action, scope: scope)).to eq(false)
-          end
-
-          travel_to(start_time + 1.minute) do
-            expect(subject.throttled?(:test_action, scope: scope)).to eq(true)
-
-            # Assert that it does not affect other actions or scope
-            expect(subject.throttled?(:another_action, scope: scope)).to eq(false)
-            expect(subject.throttled?(:test_action, scope: [user])).to eq(false)
-          end
-        end
-
-        it 'returns false when interval has elapsed' do
-          travel_to(start_time) do
-            expect(subject.throttled?(:test_action, scope: scope)).to eq(false)
-
-            # another_action has a threshold of 3 so we simulate 2 requests
-            expect(subject.throttled?(:another_action, scope: scope)).to eq(false)
-            expect(subject.throttled?(:another_action, scope: scope)).to eq(false)
-          end
-
-          travel_to(start_time + 2.minutes) do
-            expect(subject.throttled?(:test_action, scope: scope)).to eq(false)
-
-            # Assert that another_action has its own interval that hasn't elapsed
-            expect(subject.throttled?(:another_action, scope: scope)).to eq(true)
-          end
-        end
-      end
-
-      context 'when using ActiveRecord models as scope' do
-        let(:scope) { [user, project] }
+    context 'when using ActiveRecord models as scope' do
+      let(:scope) { [user, project] }
 
-        it_behaves_like 'throttles based on key and scope'
-      end
+      it_behaves_like 'throttles based on key and scope'
+    end
 
-      context 'when using ActiveRecord models and strings as scope' do
-        let(:scope) { [project, 'app/controllers/groups_controller.rb'] }
+    context 'when using ActiveRecord models and strings as scope' do
+      let(:scope) { [project, 'app/controllers/groups_controller.rb'] }
 
-        it_behaves_like 'throttles based on key and scope'
-      end
+      it_behaves_like 'throttles based on key and scope'
     end
   end