Commit 69027a9b authored by Sean McGivern's avatar Sean McGivern

Use rate limiting Redis for ApplicationRateLimiter

Here we can and do use a feature flag. The maximum interval for any rate
limit in this section is 3 minutes, so we don't mind too much about
requests that sneak in the gaps when the feature flag is being changed -
especially as feature flags are cached for a minute in process memory
anyway, so it would be hard to be much more precise.
parent 1c3b893f
---
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
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' 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(:maintainer) { create(:user) }
let_it_be(:project, refind: true) { create(:project, :repository) } let_it_be(:project, refind: true) { create(:project, :repository) }
let_it_be(:environment, refind: true) { create(:environment, project: project) } let_it_be(:environment, refind: true) { create(:environment, project: project) }
......
...@@ -73,7 +73,7 @@ module Gitlab ...@@ -73,7 +73,7 @@ module Gitlab
value = 0 value = 0
interval_value = interval || interval(key) interval_value = interval || interval(key)
Gitlab::Redis::Cache.with do |redis| cache_store.with do |redis|
cache_key = action_key(key, scope) cache_key = action_key(key, scope)
value = redis.incr(cache_key) value = redis.incr(cache_key)
redis.expire(cache_key, interval_value) if value == 1 redis.expire(cache_key, interval_value) if value == 1
...@@ -109,6 +109,14 @@ module Gitlab ...@@ -109,6 +109,14 @@ module Gitlab
private 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) def threshold(key)
value = rate_limit_value_by_key(key, :threshold) value = rate_limit_value_by_key(key, :threshold)
......
...@@ -397,7 +397,7 @@ RSpec.describe Projects::PipelineSchedulesController do ...@@ -397,7 +397,7 @@ RSpec.describe Projects::PipelineSchedulesController do
end end
end end
describe 'POST #play', :clean_gitlab_redis_cache do describe 'POST #play', :clean_gitlab_redis_rate_limiting do
let(:ref) { 'master' } let(:ref) { 'master' }
before do before do
......
...@@ -84,7 +84,7 @@ RSpec.describe Projects::RawController do ...@@ -84,7 +84,7 @@ RSpec.describe Projects::RawController do
include_examples 'single Gitaly request' include_examples 'single Gitaly request'
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
let(:file_path) { 'master/README.md' } let(:file_path) { 'master/README.md' }
before do before do
......
...@@ -1384,12 +1384,12 @@ RSpec.describe ProjectsController do ...@@ -1384,12 +1384,12 @@ RSpec.describe ProjectsController do
end end
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' include_examples 'rate limits project export endpoint'
end end
end end
describe '#download_export', :clean_gitlab_redis_cache do describe '#download_export', :clean_gitlab_redis_rate_limiting do
let(:action) { :download_export } let(:action) { :download_export }
context 'object storage enabled' do context 'object storage enabled' do
...@@ -1424,7 +1424,7 @@ RSpec.describe ProjectsController do ...@@ -1424,7 +1424,7 @@ RSpec.describe ProjectsController do
end end
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 before do
allow(Gitlab::ApplicationRateLimiter) allow(Gitlab::ApplicationRateLimiter)
.to receive(:increment) .to receive(:increment)
...@@ -1496,7 +1496,7 @@ RSpec.describe ProjectsController do ...@@ -1496,7 +1496,7 @@ RSpec.describe ProjectsController do
end end
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' include_examples 'rate limits project export endpoint'
end end
end end
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Gitlab::ApplicationRateLimiter, :clean_gitlab_redis_cache do RSpec.describe Gitlab::ApplicationRateLimiter do
let(:redis) { double('redis') } let(:redis) { double('redis') }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project) } let(:project) { create(:project) }
...@@ -20,7 +20,6 @@ RSpec.describe Gitlab::ApplicationRateLimiter, :clean_gitlab_redis_cache do ...@@ -20,7 +20,6 @@ RSpec.describe Gitlab::ApplicationRateLimiter, :clean_gitlab_redis_cache do
subject { described_class } subject { described_class }
before do before do
allow(Gitlab::Redis::Cache).to receive(:with).and_yield(redis)
allow(described_class).to receive(:rate_limits).and_return(rate_limits) allow(described_class).to receive(:rate_limits).and_return(rate_limits)
end end
...@@ -49,73 +48,94 @@ RSpec.describe Gitlab::ApplicationRateLimiter, :clean_gitlab_redis_cache do ...@@ -49,73 +48,94 @@ RSpec.describe Gitlab::ApplicationRateLimiter, :clean_gitlab_redis_cache do
end end
end end
context 'when the key is an array of only ActiveRecord models' do # Clean up in https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1249
let(:scope) { [user, project] } 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 let(:cache_key) do
"application_rate_limiter:test_action:user:#{user.id}:project:#{project.id}" "application_rate_limiter:test_action:user:#{user.id}:project:#{project.id}"
end
it_behaves_like 'action rate limiter'
end end
it_behaves_like 'action rate limiter' context 'when they key a combination of ActiveRecord models and strings' do
end 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(:cache_key) do
let(:project) { create(:project, :public, :repository) } "application_rate_limiter:test_action:project:#{project.id}:commit:#{commit.sha}:#{path}"
let(:commit) { project.repository.commit } end
let(:path) { 'app/controllers/groups_controller.rb' }
let(:scope) { [project, commit, path] }
let(:cache_key) do it_behaves_like 'action rate limiter'
"application_rate_limiter:test_action:project:#{project.id}:commit:#{commit.sha}:#{path}"
end end
it_behaves_like 'action rate limiter' describe '#log_request' do
end let(:file_path) { 'master/README.md' }
let(:type) { :raw_blob_request_limit }
describe '#log_request' do let(:fullpath) { "/#{project.full_path}/raw/#{file_path}" }
let(:file_path) { 'master/README.md' }
let(:type) { :raw_blob_request_limit }
let(:fullpath) { "/#{project.full_path}/raw/#{file_path}" }
let(:request) do let(:request) do
double('request', ip: '127.0.0.1', request_method: 'GET', fullpath: fullpath) double('request', ip: '127.0.0.1', request_method: 'GET', fullpath: fullpath)
end end
let(:base_attributes) do let(:base_attributes) do
{ {
message: 'Application_Rate_Limiter_Request', message: 'Application_Rate_Limiter_Request',
env: type, env: type,
remote_ip: '127.0.0.1', remote_ip: '127.0.0.1',
request_method: 'GET', request_method: 'GET',
path: fullpath path: fullpath
} }
end end
context 'without a current user' do context 'without a current user' do
let(:current_user) { nil } let(:current_user) { nil }
it 'logs information to auth.log' do it 'logs information to auth.log' do
expect(Gitlab::AuthLogger).to receive(:error).with(base_attributes).once 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
end
context 'with a current_user' do context 'with a current_user' do
let(:current_user) { create(:user) } let(:current_user) { create(:user) }
let(:attributes) do let(:attributes) do
base_attributes.merge({ base_attributes.merge({
user_id: current_user.id, user_id: current_user.id,
username: current_user.username username: current_user.username
}) })
end end
it 'logs information to auth.log' do it 'logs information to auth.log' do
expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once 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 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 end
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' 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 let(:limiter_class) do
Class.new do Class.new do
include ::Gitlab::RateLimitHelpers include ::Gitlab::RateLimitHelpers
......
...@@ -392,7 +392,7 @@ RSpec.describe WebHookService do ...@@ -392,7 +392,7 @@ RSpec.describe WebHookService do
end end
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 before do
# Set a high interval to avoid intermittent failures in CI # Set a high interval to avoid intermittent failures in CI
allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits).and_return( allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits).and_return(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment