From e55bb6a34a2dbb8162071fc052d2f7fe1311a8da Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Thu, 6 May 2021 16:59:44 +0200 Subject: [PATCH 1/8] Apply rate-limiting to webhook executions This applies a rate limit to WebHookWorker jobs, scoped to the current hook ID, with thresholds defined in plan limits, and a hard-coded interval of 1 minute. Changelog: performance --- app/workers/web_hook_worker.rb | 42 ++++++ .../unreleased/329207-rate-limit-webhooks.yml | 5 + .../development/web_hooks_rate_limit.yml | 8 ++ ...31747_add_web_hook_calls_to_plan_limits.rb | 7 + db/schema_migrations/20210503131747 | 1 + db/structure.sql | 3 +- doc/administration/instance_limits.md | 15 ++ ee/app/workers/ee/web_hook_worker.rb | 27 ++++ ee/spec/factories/group_hooks.rb | 1 + ee/spec/workers/ee/web_hook_worker_spec.rb | 119 ++++++++++++++++ lib/gitlab/application_rate_limiter.rb | 1 + spec/workers/web_hook_worker_spec.rb | 133 ++++++++++++++++++ 12 files changed, 361 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/329207-rate-limit-webhooks.yml create mode 100644 config/feature_flags/development/web_hooks_rate_limit.yml create mode 100644 db/migrate/20210503131747_add_web_hook_calls_to_plan_limits.rb create mode 100644 db/schema_migrations/20210503131747 create mode 100644 ee/app/workers/ee/web_hook_worker.rb create mode 100644 ee/spec/workers/ee/web_hook_worker_spec.rb create mode 100644 spec/workers/web_hook_worker_spec.rb diff --git a/app/workers/web_hook_worker.rb b/app/workers/web_hook_worker.rb index dffab61dd0e1a4..5c9de3ab4cd15b 100644 --- a/app/workers/web_hook_worker.rb +++ b/app/workers/web_hook_worker.rb @@ -11,6 +11,46 @@ class WebHookWorker sidekiq_options retry: 4, dead: false + class << self + def perform_async(hook_id, data, hook_name) + return super unless rate_limited?(hook_id) + + Gitlab::AuthLogger.error( + message: 'Webhook rate limit exceeded', + hook_id: hook_id, + hook_name: hook_name + ) + + # Also log into application log for now, so we can use this information + # to determine suitable limits for gitlab.com + Gitlab::AppLogger.error( + message: 'Webhook rate limit exceeded', + class: name, + hook_id: hook_id, + hook_name: hook_name + ) + end + + private + + def rate_limited?(hook_id) + return false unless Feature.enabled?(:web_hooks_rate_limit, default_enabled: :yaml) + + hook = WebHook.find(hook_id) + limits = current_plan_limits(hook) + + Gitlab::ApplicationRateLimiter.throttled?( + :web_hook_calls, + scope: [hook], + threshold: limits.web_hook_calls + ) + end + + def current_plan_limits(_hook) + Plan.default.actual_limits + end + end + def perform(hook_id, data, hook_name) hook = WebHook.find(hook_id) data = data.with_indifferent_access @@ -19,3 +59,5 @@ def perform(hook_id, data, hook_name) end end # rubocop:enable Scalability/IdempotentWorker + +WebHookWorker.prepend_if_ee('EE::WebHookWorker') diff --git a/changelogs/unreleased/329207-rate-limit-webhooks.yml b/changelogs/unreleased/329207-rate-limit-webhooks.yml new file mode 100644 index 00000000000000..0f8786ba30c8d7 --- /dev/null +++ b/changelogs/unreleased/329207-rate-limit-webhooks.yml @@ -0,0 +1,5 @@ +--- +title: Apply rate-limiting to webhook executions +merge_request: +author: +type: performance diff --git a/config/feature_flags/development/web_hooks_rate_limit.yml b/config/feature_flags/development/web_hooks_rate_limit.yml new file mode 100644 index 00000000000000..193d51bb250513 --- /dev/null +++ b/config/feature_flags/development/web_hooks_rate_limit.yml @@ -0,0 +1,8 @@ +--- +name: web_hooks_rate_limit +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61151 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/330133 +milestone: '13.12' +type: development +group: group::ecosystem +default_enabled: false diff --git a/db/migrate/20210503131747_add_web_hook_calls_to_plan_limits.rb b/db/migrate/20210503131747_add_web_hook_calls_to_plan_limits.rb new file mode 100644 index 00000000000000..cb23c9391eaf06 --- /dev/null +++ b/db/migrate/20210503131747_add_web_hook_calls_to_plan_limits.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddWebHookCallsToPlanLimits < ActiveRecord::Migration[6.0] + def change + add_column :plan_limits, :web_hook_calls, :integer, null: false, default: 0 + end +end diff --git a/db/schema_migrations/20210503131747 b/db/schema_migrations/20210503131747 new file mode 100644 index 00000000000000..52771dcb5ded2a --- /dev/null +++ b/db/schema_migrations/20210503131747 @@ -0,0 +1 @@ +583c350d82c4d02e910f2c16ed2ec55ccdc880c87b55bf7bd6be3e1839958732 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 976ac394aaeb91..15aad6803cb898 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -16038,7 +16038,8 @@ CREATE TABLE plan_limits ( terraform_module_max_file_size bigint DEFAULT 1073741824 NOT NULL, helm_max_file_size bigint DEFAULT 5242880 NOT NULL, ci_registered_group_runners integer DEFAULT 1000 NOT NULL, - ci_registered_project_runners integer DEFAULT 1000 NOT NULL + ci_registered_project_runners integer DEFAULT 1000 NOT NULL, + web_hook_calls integer DEFAULT 0 NOT NULL ); CREATE SEQUENCE plan_limits_id_seq diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index 044a7403e8bda1..b71f9524f24fa9 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -205,6 +205,21 @@ Plan.default.actual_limits.update!(group_hooks: 100) Set the limit to `0` to disable it. +## Webhook calls + +Limit the number of times any given webhook can be called per minute. + +Calls over the rate limit are logged into `auth.log`. + +```ruby +# If limits don't exist for the default plan, you can create one with: +# Plan.default.create_limits! + +Plan.default.actual_limits.update!(web_hook_calls: 10) +``` + +Set the limit to `0` to disable it. + ## Pull Mirroring Interval > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/237891) in GitLab 13.7. diff --git a/ee/app/workers/ee/web_hook_worker.rb b/ee/app/workers/ee/web_hook_worker.rb new file mode 100644 index 00000000000000..4abab1ba1015cf --- /dev/null +++ b/ee/app/workers/ee/web_hook_worker.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module EE + module WebHookWorker + extend ActiveSupport::Concern + + class_methods do + extend ::Gitlab::Utils::Override + + private + + override :current_plan_limits + def current_plan_limits(hook) + case hook + when GroupHook + hook.group.actual_limits + when ProjectHook + hook.project.actual_limits + when ServiceHook + hook.service.project.actual_limits + else + super + end + end + end + end +end diff --git a/ee/spec/factories/group_hooks.rb b/ee/spec/factories/group_hooks.rb index 94558e3218bca5..28360349179d09 100644 --- a/ee/spec/factories/group_hooks.rb +++ b/ee/spec/factories/group_hooks.rb @@ -3,6 +3,7 @@ FactoryBot.define do factory :group_hook do url { generate(:url) } + group trait :all_events_enabled do push_events { true } diff --git a/ee/spec/workers/ee/web_hook_worker_spec.rb b/ee/spec/workers/ee/web_hook_worker_spec.rb new file mode 100644 index 00000000000000..4ad2dde4a68d1b --- /dev/null +++ b/ee/spec/workers/ee/web_hook_worker_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe EE::WebHookWorker do + let_it_be(:group_premium) { create(:group_with_plan, plan: :premium_plan) } + let_it_be(:group_ultimate) { create(:group_with_plan, plan: :ultimate_plan) } + let_it_be(:project_premium) { create(:project, group: group_premium) } + let_it_be(:project_ultimate) { create(:project, group: group_ultimate) } + let_it_be(:service_premium) { create(:service, project: project_premium) } + let_it_be(:service_ultimate) { create(:service, project: project_ultimate) } + + let_it_be(:premium_hooks) do + [ + create(:group_hook, group: group_premium), + create(:project_hook, project: project_premium), + create(:service_hook, service: service_premium) + ] + end + + let_it_be(:ultimate_hooks) do + [ + create(:group_hook, group: group_ultimate), + create(:project_hook, project: project_ultimate), + create(:service_hook, service: service_ultimate) + ] + end + + let_it_be(:default_hooks) do + [ + create(:group_hook), + create(:project_hook), + create(:service_hook) + ] + end + + let_it_be(:system_hook) { create(:system_hook) } + + describe '.perform_async' do + def perform_async(hook) + WebHookWorker.perform_async(hook.id, {}, 'push_hooks') + end + + def expect_rate_limiter(hook, threshold:, throttled: false) + expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .with(:web_hook_calls, scope: [hook], threshold: threshold) + end + + context 'when rate limiting is not configured' do + it 'all hooks use a threshold of 0' do + (premium_hooks + ultimate_hooks + default_hooks + [system_hook]).each do |hook| + expect_rate_limiter(hook, threshold: 0) + + perform_async(hook) + end + end + end + + context 'when rate limiting is configured on each plan' do + let_it_be(:premium_plan) { create(:premium_plan) } + let_it_be(:ultimate_plan) { create(:ultimate_plan) } + let_it_be(:premium_limits) { create(:plan_limits, plan: premium_plan, web_hook_calls: 250) } + let_it_be(:ultimate_limits) { create(:plan_limits, plan: ultimate_plan, web_hook_calls: 500) } + let_it_be(:default_limits) { create(:plan_limits, :default_plan, web_hook_calls: 100) } + + it 'hooks with a Premium plan use the configured threshold' do + premium_hooks.each do |hook| + expect_rate_limiter(hook, threshold: 250) + + perform_async(hook) + end + end + + it 'hooks with an Ultimate plan use the configured threshold' do + ultimate_hooks.each do |hook| + expect_rate_limiter(hook, threshold: 500) + + perform_async(hook) + end + end + + it 'hooks with the default plan use the configured threshold' do + default_hooks.each do |hook| + expect_rate_limiter(hook, threshold: 100) + + perform_async(hook) + end + end + + it 'system hooks use the default threshold' do + expect_rate_limiter(system_hook, threshold: 100) + + perform_async(system_hook) + end + + context 'on gitlab.com' do + let_it_be(:free_plan) { create(:free_plan) } + let_it_be(:free_limits) { create(:plan_limits, plan: free_plan, web_hook_calls: 50) } + + before do + allow(Gitlab).to receive(:com?).and_return(true) + end + + it 'hooks with a Free plan use the configured threshold' do + default_hooks.each do |hook| + expect_rate_limiter(hook, threshold: 50) + + perform_async(hook) + end + end + + it 'system hooks use the default threshold' do + expect_rate_limiter(system_hook, threshold: 100) + + perform_async(system_hook) + end + end + end + end +end diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index f74edf2b767d09..f91a56a0cd2020 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -34,6 +34,7 @@ def rate_limits group_import: { threshold: -> { application_settings.group_import_limit }, interval: 1.minute }, group_testing_hook: { threshold: 5, interval: 1.minute }, profile_add_new_email: { threshold: 5, interval: 1.minute }, + web_hook_calls: { interval: 1.minute }, profile_resend_email_confirmation: { threshold: 5, interval: 1.minute }, update_environment_canary_ingress: { threshold: 1, interval: 1.minute }, auto_rollback_deployment: { threshold: 1, interval: 3.minutes } diff --git a/spec/workers/web_hook_worker_spec.rb b/spec/workers/web_hook_worker_spec.rb new file mode 100644 index 00000000000000..33be9ba6056d1b --- /dev/null +++ b/spec/workers/web_hook_worker_spec.rb @@ -0,0 +1,133 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe WebHookWorker do + include AfterNextHelpers + + let_it_be(:hook) { create(:project_hook) } + let_it_be(:data) { { foo: 'bar' } } + let_it_be(:hook_name) { 'push_hooks' } + + describe '.perform_async' do + def perform_async(hook) + described_class.perform_async(hook.id, data, hook_name) + end + + def expect_job_for(hook) + expect(described_class.jobs).to include( + include('args' => [hook.id, data.stringify_keys, hook_name]) + ) + end + + def expect_jobs(count) + expect(described_class.jobs.count).to be(count) + end + + def expect_rate_limiter(threshold:, throttled: false) + expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .with(:web_hook_calls, scope: [hook], threshold: threshold) + .and_return(throttled) + end + + context 'when rate limiting is not configured' do + it 'queues a job' do + expect_rate_limiter(threshold: 0) + + perform_async(hook) + + expect_job_for(hook) + end + end + + context 'when rate limiting is configured' do + let_it_be(:rate_limit) { 3 } + let_it_be(:plan_limits) { create(:plan_limits, :default_plan, web_hook_calls: rate_limit) } + + it 'queues a job' do + expect_rate_limiter(threshold: 3) + + perform_async(hook) + + expect_job_for(hook) + end + + context 'when the hook is throttled (via mock)' do + before do + expect_rate_limiter(threshold: rate_limit, throttled: true) + end + + it 'does not queue a job and logs an error' do + expect(Gitlab::AuthLogger).to receive(:error).with( + message: 'Webhook rate limit exceeded', + hook_id: hook.id, + hook_name: hook_name + ) + + expect(Gitlab::AppLogger).to receive(:error).with( + message: 'Webhook rate limit exceeded', + class: 'WebHookWorker', + hook_id: hook.id, + hook_name: hook_name + ) + + perform_async(hook) + + expect_jobs(0) + end + end + + context 'when the hook is throttled (via Redis)', :clean_gitlab_redis_cache do + before do + # Set a high interval to avoid intermittent failures in CI + allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits).and_return( + web_hook_calls: { interval: 1.day } + ) + + rate_limit.times { perform_async(hook) } + + expect_jobs(rate_limit) + end + + it 'stops queueing jobs and logs errors' do + expect(Gitlab::AuthLogger).to receive(:error).twice + expect(Gitlab::AppLogger).to receive(:error).twice + + 2.times { perform_async(hook) } + + expect_jobs(rate_limit) + end + + it 'still queues jobs for other hooks' do + other_hook = create(:project_hook) + + perform_async(other_hook) + + expect_jobs(rate_limit + 1) + expect_job_for(other_hook) + end + end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(web_hooks_rate_limit: false) + end + + it 'queues a job without tracking the call' do + expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) + + perform_async(hook) + + expect_job_for(hook) + end + end + end + end + + describe '#perform' do + it 'delegates to WebHookService' do + expect_next(WebHookService, hook, data.with_indifferent_access, hook_name).to receive(:execute) + + subject.perform(hook.id, data, hook_name) + end + end +end -- GitLab From fec888b81e2a1a2a00826df270b9c2312e06116d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=A4=96=20GitLab=20Bot=20=F0=9F=A4=96?= Date: Thu, 6 May 2021 15:34:18 +0000 Subject: [PATCH 2/8] Apply 1 suggestion(s) to 1 file(s) --- changelogs/unreleased/329207-rate-limit-webhooks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/329207-rate-limit-webhooks.yml b/changelogs/unreleased/329207-rate-limit-webhooks.yml index 0f8786ba30c8d7..134cdc67c35fd0 100644 --- a/changelogs/unreleased/329207-rate-limit-webhooks.yml +++ b/changelogs/unreleased/329207-rate-limit-webhooks.yml @@ -1,5 +1,5 @@ --- title: Apply rate-limiting to webhook executions -merge_request: +merge_request: 61151 author: type: performance -- GitLab From fd2f44b01969eaf300503c95eaa71c11ad0fb684 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Fri, 7 May 2021 16:54:14 +0200 Subject: [PATCH 3/8] Fix plan limits spec --- spec/models/plan_limits_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/models/plan_limits_spec.rb b/spec/models/plan_limits_spec.rb index 4259c8b708b3bd..b8c723b3847f94 100644 --- a/spec/models/plan_limits_spec.rb +++ b/spec/models/plan_limits_spec.rb @@ -210,6 +210,7 @@ ci_active_jobs storage_size_limit daily_invites + web_hook_calls ] + disabled_max_artifact_size_columns end -- GitLab From 1de8c1917a7957e555f07d58f372779a7ba89ae0 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Fri, 7 May 2021 19:07:48 +0200 Subject: [PATCH 4/8] Move rate-limiting into service, only apply to project and group hooks --- app/models/hooks/project_hook.rb | 6 + app/models/hooks/web_hook.rb | 5 + app/services/web_hook_service.rb | 34 +++++- app/workers/web_hook_worker.rb | 42 ------- doc/administration/instance_limits.md | 31 ++--- ee/app/models/hooks/group_hook.rb | 6 + ee/app/workers/ee/web_hook_worker.rb | 27 ----- ee/spec/models/hooks/ee/project_hook_spec.rb | 26 ++++ ee/spec/models/hooks/group_hook_spec.rb | 19 +++ ee/spec/workers/ee/web_hook_worker_spec.rb | 119 ------------------ spec/models/hooks/project_hook_spec.rb | 9 ++ spec/models/hooks/service_hook_spec.rb | 8 ++ spec/models/hooks/system_hook_spec.rb | 8 ++ spec/services/web_hook_service_spec.rb | 102 +++++++++++++++- spec/workers/web_hook_worker_spec.rb | 121 +------------------ 15 files changed, 235 insertions(+), 328 deletions(-) delete mode 100644 ee/app/workers/ee/web_hook_worker.rb create mode 100644 ee/spec/models/hooks/ee/project_hook_spec.rb delete mode 100644 ee/spec/workers/ee/web_hook_worker_spec.rb diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb index 83858ee0c77143..7566b7d23e1940 100644 --- a/app/models/hooks/project_hook.rb +++ b/app/models/hooks/project_hook.rb @@ -4,6 +4,7 @@ class ProjectHook < WebHook include TriggerableHooks include Presentable include Limitable + extend ::Gitlab::Utils::Override self.limit_scope = :project @@ -33,6 +34,11 @@ def pluralized_name def web_hooks_disable_failed? Feature.enabled?(:web_hooks_disable_failed, project) end + + override :rate_limit + def rate_limit + project.actual_limits.limit_for(:web_hook_calls) + end end ProjectHook.prepend_if_ee('EE::ProjectHook') diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 445381343186ea..efddb605ce79d0 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -75,6 +75,11 @@ def enable! update!(recent_failures: 0, disabled_until: nil, backoff_count: 0) end + # Overriden in ProjectHook and GroupHook, other webhooks are not rate-limited. + def rate_limit + nil + end + private def web_hooks_disable_failed? diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 0535bc625ac1fc..bdc97300a36b76 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -90,7 +90,24 @@ def execute end def async_execute - WebHookWorker.perform_async(hook.id, data, hook_name) + if rate_limited?(hook) + Gitlab::AuthLogger.error( + message: 'Webhook rate limit exceeded', + hook_id: hook.id, + hook_name: hook_name + ) + + # Also log into application log for now, so we can use this information + # to determine suitable limits for gitlab.com + Gitlab::AppLogger.error( + message: 'Webhook rate limit exceeded', + class: self.class.name, + hook_id: hook.id, + hook_name: hook_name + ) + else + WebHookWorker.perform_async(hook.id, data, hook_name) + end end private @@ -169,4 +186,19 @@ def safe_response_body(response) response.body.encode('UTF-8', invalid: :replace, undef: :replace, replace: '') end + + def rate_limited?(hook) + return false unless Feature.enabled?(:web_hooks_rate_limit, default_enabled: :yaml) + return false if rate_limit.nil? + + Gitlab::ApplicationRateLimiter.throttled?( + :web_hook_calls, + scope: [hook], + threshold: rate_limit + ) + end + + def rate_limit + @rate_limit ||= hook.rate_limit + end end diff --git a/app/workers/web_hook_worker.rb b/app/workers/web_hook_worker.rb index 5c9de3ab4cd15b..dffab61dd0e1a4 100644 --- a/app/workers/web_hook_worker.rb +++ b/app/workers/web_hook_worker.rb @@ -11,46 +11,6 @@ class WebHookWorker sidekiq_options retry: 4, dead: false - class << self - def perform_async(hook_id, data, hook_name) - return super unless rate_limited?(hook_id) - - Gitlab::AuthLogger.error( - message: 'Webhook rate limit exceeded', - hook_id: hook_id, - hook_name: hook_name - ) - - # Also log into application log for now, so we can use this information - # to determine suitable limits for gitlab.com - Gitlab::AppLogger.error( - message: 'Webhook rate limit exceeded', - class: name, - hook_id: hook_id, - hook_name: hook_name - ) - end - - private - - def rate_limited?(hook_id) - return false unless Feature.enabled?(:web_hooks_rate_limit, default_enabled: :yaml) - - hook = WebHook.find(hook_id) - limits = current_plan_limits(hook) - - Gitlab::ApplicationRateLimiter.throttled?( - :web_hook_calls, - scope: [hook], - threshold: limits.web_hook_calls - ) - end - - def current_plan_limits(_hook) - Plan.default.actual_limits - end - end - def perform(hook_id, data, hook_name) hook = WebHook.find(hook_id) data = data.with_indifferent_access @@ -59,5 +19,3 @@ def perform(hook_id, data, hook_name) end end # rubocop:enable Scalability/IdempotentWorker - -WebHookWorker.prepend_if_ee('EE::WebHookWorker') diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index b71f9524f24fa9..cb230bb51f340d 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -112,6 +112,22 @@ Limit the maximum daily member invitations allowed per group hierarchy. - GitLab.com: Free members may invite 20 members per day. - Self-managed: Invites are not limited. +### Webhook calls + +Limit the number of times any given webhook can be called per minute. +This only applies to project and group webhooks. + +Calls over the rate limit are logged into `auth.log`. + +```ruby +# If limits don't exist for the default plan, you can create one with: +# Plan.default.create_limits! + +Plan.default.actual_limits.update!(web_hook_calls: 10) +``` + +Set the limit to `0` to disable it. + ## Gitaly concurrency limit Clone traffic can put a large strain on your Gitaly service. To prevent such workloads from overwhelming your Gitaly server, you can set concurrency limits in Gitaly's configuration file. @@ -205,21 +221,6 @@ Plan.default.actual_limits.update!(group_hooks: 100) Set the limit to `0` to disable it. -## Webhook calls - -Limit the number of times any given webhook can be called per minute. - -Calls over the rate limit are logged into `auth.log`. - -```ruby -# If limits don't exist for the default plan, you can create one with: -# Plan.default.create_limits! - -Plan.default.actual_limits.update!(web_hook_calls: 10) -``` - -Set the limit to `0` to disable it. - ## Pull Mirroring Interval > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/237891) in GitLab 13.7. diff --git a/ee/app/models/hooks/group_hook.rb b/ee/app/models/hooks/group_hook.rb index f87c537e427223..5f0d4b0d050800 100644 --- a/ee/app/models/hooks/group_hook.rb +++ b/ee/app/models/hooks/group_hook.rb @@ -4,6 +4,7 @@ class GroupHook < WebHook include CustomModelNaming include TriggerableHooks include Limitable + extend ::Gitlab::Utils::Override self.limit_name = 'group_hooks' self.limit_scope = :group @@ -36,4 +37,9 @@ def pluralized_name def web_hooks_disable_failed? Feature.enabled?(:web_hooks_disable_failed, group) end + + override :rate_limit + def rate_limit + group.actual_limits.limit_for(:web_hook_calls) + end end diff --git a/ee/app/workers/ee/web_hook_worker.rb b/ee/app/workers/ee/web_hook_worker.rb deleted file mode 100644 index 4abab1ba1015cf..00000000000000 --- a/ee/app/workers/ee/web_hook_worker.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -module EE - module WebHookWorker - extend ActiveSupport::Concern - - class_methods do - extend ::Gitlab::Utils::Override - - private - - override :current_plan_limits - def current_plan_limits(hook) - case hook - when GroupHook - hook.group.actual_limits - when ProjectHook - hook.project.actual_limits - when ServiceHook - hook.service.project.actual_limits - else - super - end - end - end - end -end diff --git a/ee/spec/models/hooks/ee/project_hook_spec.rb b/ee/spec/models/hooks/ee/project_hook_spec.rb new file mode 100644 index 00000000000000..a05ba710e6cfa4 --- /dev/null +++ b/ee/spec/models/hooks/ee/project_hook_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe EE::ProjectHook do + describe '#rate_limit' do + let_it_be(:default_limits) { create(:plan_limits, :default_plan, web_hook_calls: 100) } + let_it_be(:ultimate_limits) { create(:plan_limits, plan: create(:ultimate_plan), web_hook_calls: 500) } + + let_it_be(:group) { create(:group) } + let_it_be(:group_ultimate) { create(:group_with_plan, plan: :ultimate_plan) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:project_ultimate) { create(:project, group: group_ultimate) } + + let_it_be(:hook) { create(:project_hook, project: project) } + let_it_be(:hook_ultimate) { create(:project_hook, project: project_ultimate) } + + it 'returns the default limit for a project without a plan' do + expect(hook.rate_limit).to be(100) + end + + it 'returns the configured limit for a project with the Ultimate plan' do + expect(hook_ultimate.rate_limit).to be(500) + end + end +end diff --git a/ee/spec/models/hooks/group_hook_spec.rb b/ee/spec/models/hooks/group_hook_spec.rb index 692385a3aa2129..374b526407a43a 100644 --- a/ee/spec/models/hooks/group_hook_spec.rb +++ b/ee/spec/models/hooks/group_hook_spec.rb @@ -10,4 +10,23 @@ it_behaves_like 'includes Limitable concern' do subject { build(:group_hook, group: create(:group)) } end + + describe '#rate_limit' do + let_it_be(:default_limits) { create(:plan_limits, :default_plan, web_hook_calls: 100) } + let_it_be(:ultimate_limits) { create(:plan_limits, plan: create(:ultimate_plan), web_hook_calls: 500) } + + let_it_be(:group) { create(:group) } + let_it_be(:group_ultimate) { create(:group_with_plan, plan: :ultimate_plan) } + + let_it_be(:hook) { create(:group_hook, group: group) } + let_it_be(:hook_ultimate) { create(:group_hook, group: group_ultimate) } + + it 'returns the default limit for a group without a plan' do + expect(hook.rate_limit).to be(100) + end + + it 'returns the configured limit for a group with the Ultimate plan' do + expect(hook_ultimate.rate_limit).to be(500) + end + end end diff --git a/ee/spec/workers/ee/web_hook_worker_spec.rb b/ee/spec/workers/ee/web_hook_worker_spec.rb deleted file mode 100644 index 4ad2dde4a68d1b..00000000000000 --- a/ee/spec/workers/ee/web_hook_worker_spec.rb +++ /dev/null @@ -1,119 +0,0 @@ -# frozen_string_literal: true -require 'spec_helper' - -RSpec.describe EE::WebHookWorker do - let_it_be(:group_premium) { create(:group_with_plan, plan: :premium_plan) } - let_it_be(:group_ultimate) { create(:group_with_plan, plan: :ultimate_plan) } - let_it_be(:project_premium) { create(:project, group: group_premium) } - let_it_be(:project_ultimate) { create(:project, group: group_ultimate) } - let_it_be(:service_premium) { create(:service, project: project_premium) } - let_it_be(:service_ultimate) { create(:service, project: project_ultimate) } - - let_it_be(:premium_hooks) do - [ - create(:group_hook, group: group_premium), - create(:project_hook, project: project_premium), - create(:service_hook, service: service_premium) - ] - end - - let_it_be(:ultimate_hooks) do - [ - create(:group_hook, group: group_ultimate), - create(:project_hook, project: project_ultimate), - create(:service_hook, service: service_ultimate) - ] - end - - let_it_be(:default_hooks) do - [ - create(:group_hook), - create(:project_hook), - create(:service_hook) - ] - end - - let_it_be(:system_hook) { create(:system_hook) } - - describe '.perform_async' do - def perform_async(hook) - WebHookWorker.perform_async(hook.id, {}, 'push_hooks') - end - - def expect_rate_limiter(hook, threshold:, throttled: false) - expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) - .with(:web_hook_calls, scope: [hook], threshold: threshold) - end - - context 'when rate limiting is not configured' do - it 'all hooks use a threshold of 0' do - (premium_hooks + ultimate_hooks + default_hooks + [system_hook]).each do |hook| - expect_rate_limiter(hook, threshold: 0) - - perform_async(hook) - end - end - end - - context 'when rate limiting is configured on each plan' do - let_it_be(:premium_plan) { create(:premium_plan) } - let_it_be(:ultimate_plan) { create(:ultimate_plan) } - let_it_be(:premium_limits) { create(:plan_limits, plan: premium_plan, web_hook_calls: 250) } - let_it_be(:ultimate_limits) { create(:plan_limits, plan: ultimate_plan, web_hook_calls: 500) } - let_it_be(:default_limits) { create(:plan_limits, :default_plan, web_hook_calls: 100) } - - it 'hooks with a Premium plan use the configured threshold' do - premium_hooks.each do |hook| - expect_rate_limiter(hook, threshold: 250) - - perform_async(hook) - end - end - - it 'hooks with an Ultimate plan use the configured threshold' do - ultimate_hooks.each do |hook| - expect_rate_limiter(hook, threshold: 500) - - perform_async(hook) - end - end - - it 'hooks with the default plan use the configured threshold' do - default_hooks.each do |hook| - expect_rate_limiter(hook, threshold: 100) - - perform_async(hook) - end - end - - it 'system hooks use the default threshold' do - expect_rate_limiter(system_hook, threshold: 100) - - perform_async(system_hook) - end - - context 'on gitlab.com' do - let_it_be(:free_plan) { create(:free_plan) } - let_it_be(:free_limits) { create(:plan_limits, plan: free_plan, web_hook_calls: 50) } - - before do - allow(Gitlab).to receive(:com?).and_return(true) - end - - it 'hooks with a Free plan use the configured threshold' do - default_hooks.each do |hook| - expect_rate_limiter(hook, threshold: 50) - - perform_async(hook) - end - end - - it 'system hooks use the default threshold' do - expect_rate_limiter(system_hook, threshold: 100) - - perform_async(system_hook) - end - end - end - end -end diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb index 69fbc4c3b4fe4d..88149465232139 100644 --- a/spec/models/hooks/project_hook_spec.rb +++ b/spec/models/hooks/project_hook_spec.rb @@ -30,4 +30,13 @@ expect(described_class.tag_push_hooks).to eq([hook]) end end + + describe '#rate_limit' do + let_it_be(:hook) { create(:project_hook) } + let_it_be(:plan_limits) { create(:plan_limits, :default_plan, web_hook_calls: 100) } + + it 'returns the default limit' do + expect(hook.rate_limit).to be(100) + end + end end diff --git a/spec/models/hooks/service_hook_spec.rb b/spec/models/hooks/service_hook_spec.rb index f7045d7ac5e92d..60c0eb265ce8a4 100644 --- a/spec/models/hooks/service_hook_spec.rb +++ b/spec/models/hooks/service_hook_spec.rb @@ -22,4 +22,12 @@ hook.execute(data) end end + + describe '#rate_limit' do + let(:hook) { build(:service_hook) } + + it 'returns nil' do + expect(hook.rate_limit).to be_nil + end + end end diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb index 02e630cbf2727c..a72034f1ac5005 100644 --- a/spec/models/hooks/system_hook_spec.rb +++ b/spec/models/hooks/system_hook_spec.rb @@ -169,4 +169,12 @@ hook.async_execute(data, hook_name) end end + + describe '#rate_limit' do + let(:hook) { build(:system_hook) } + + it 'returns nil' do + expect(hook.rate_limit).to be_nil + end + end end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 46dab4fa1715ce..ac57a1cb81a77b 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -5,8 +5,9 @@ RSpec.describe WebHookService do include StubRequests - let(:project) { create(:project) } - let(:project_hook) { create(:project_hook) } + let_it_be(:project) { create(:project) } + let_it_be(:project_hook) { create(:project_hook) } + let(:headers) do { 'Content-Type' => 'application/json', @@ -321,12 +322,101 @@ end describe '#async_execute' do - let(:system_hook) { create(:system_hook) } + def expect_worker(hook) + expect(WebHookWorker).to receive(:perform_async).with(hook.id, data, 'push_hooks') + end + + def expect_rate_limiter(hook, threshold:, throttled: false) + expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .with(:web_hook_calls, scope: [hook], threshold: threshold) + .and_return(throttled) + end + + context 'when rate limiting is not configured' do + it 'queues a worker without tracking the call' do + expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) + expect_worker(project_hook) + + service_instance.async_execute + end + end + + context 'when rate limiting is configured' do + let_it_be(:threshold) { 3 } + let_it_be(:plan_limits) { create(:plan_limits, :default_plan, web_hook_calls: threshold) } + + it 'queues a worker and tracks the call' do + expect_rate_limiter(project_hook, threshold: threshold) + expect_worker(project_hook) + + service_instance.async_execute + end + + context 'when the hook is throttled (via mock)' do + before do + expect_rate_limiter(project_hook, threshold: threshold, throttled: true) + end + + it 'does not queue a worker and logs an error' do + expect(WebHookWorker).not_to receive(:perform_async) + + expect(Gitlab::AuthLogger).to receive(:error).with( + message: 'Webhook rate limit exceeded', + hook_id: project_hook.id, + hook_name: 'push_hooks' + ) + + expect(Gitlab::AppLogger).to receive(:error).with( + message: 'Webhook rate limit exceeded', + class: 'WebHookService', + hook_id: project_hook.id, + hook_name: 'push_hooks' + ) + + service_instance.async_execute + end + end + + context 'when the hook is throttled (via Redis)', :clean_gitlab_redis_cache do + before do + # Set a high interval to avoid intermittent failures in CI + allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits).and_return( + web_hook_calls: { interval: 1.day } + ) + + expect_worker(project_hook).exactly(threshold).times + + threshold.times { service_instance.async_execute } + end + + it 'stops queueing workers and logs errors' do + expect(Gitlab::AuthLogger).to receive(:error).twice + expect(Gitlab::AppLogger).to receive(:error).twice + + 2.times { service_instance.async_execute } + end - it 'enqueue WebHookWorker' do - expect(WebHookWorker).to receive(:perform_async).with(project_hook.id, data, 'push_hooks') + it 'still queues workers for other hooks' do + other_hook = create(:project_hook) - described_class.new(project_hook, data, 'push_hooks').async_execute + expect_worker(other_hook) + + described_class.new(other_hook, data, :push_hooks).async_execute + end + end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(web_hooks_rate_limit: false) + end + + it 'queues a worker without tracking the call' do + expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) + expect_worker(project_hook) + + service_instance.async_execute + end + end end end end diff --git a/spec/workers/web_hook_worker_spec.rb b/spec/workers/web_hook_worker_spec.rb index 33be9ba6056d1b..becc7461f2a75d 100644 --- a/spec/workers/web_hook_worker_spec.rb +++ b/spec/workers/web_hook_worker_spec.rb @@ -4,130 +4,15 @@ RSpec.describe WebHookWorker do include AfterNextHelpers - let_it_be(:hook) { create(:project_hook) } + let_it_be(:project_hook) { create(:project_hook) } let_it_be(:data) { { foo: 'bar' } } let_it_be(:hook_name) { 'push_hooks' } - describe '.perform_async' do - def perform_async(hook) - described_class.perform_async(hook.id, data, hook_name) - end - - def expect_job_for(hook) - expect(described_class.jobs).to include( - include('args' => [hook.id, data.stringify_keys, hook_name]) - ) - end - - def expect_jobs(count) - expect(described_class.jobs.count).to be(count) - end - - def expect_rate_limiter(threshold:, throttled: false) - expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) - .with(:web_hook_calls, scope: [hook], threshold: threshold) - .and_return(throttled) - end - - context 'when rate limiting is not configured' do - it 'queues a job' do - expect_rate_limiter(threshold: 0) - - perform_async(hook) - - expect_job_for(hook) - end - end - - context 'when rate limiting is configured' do - let_it_be(:rate_limit) { 3 } - let_it_be(:plan_limits) { create(:plan_limits, :default_plan, web_hook_calls: rate_limit) } - - it 'queues a job' do - expect_rate_limiter(threshold: 3) - - perform_async(hook) - - expect_job_for(hook) - end - - context 'when the hook is throttled (via mock)' do - before do - expect_rate_limiter(threshold: rate_limit, throttled: true) - end - - it 'does not queue a job and logs an error' do - expect(Gitlab::AuthLogger).to receive(:error).with( - message: 'Webhook rate limit exceeded', - hook_id: hook.id, - hook_name: hook_name - ) - - expect(Gitlab::AppLogger).to receive(:error).with( - message: 'Webhook rate limit exceeded', - class: 'WebHookWorker', - hook_id: hook.id, - hook_name: hook_name - ) - - perform_async(hook) - - expect_jobs(0) - end - end - - context 'when the hook is throttled (via Redis)', :clean_gitlab_redis_cache do - before do - # Set a high interval to avoid intermittent failures in CI - allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits).and_return( - web_hook_calls: { interval: 1.day } - ) - - rate_limit.times { perform_async(hook) } - - expect_jobs(rate_limit) - end - - it 'stops queueing jobs and logs errors' do - expect(Gitlab::AuthLogger).to receive(:error).twice - expect(Gitlab::AppLogger).to receive(:error).twice - - 2.times { perform_async(hook) } - - expect_jobs(rate_limit) - end - - it 'still queues jobs for other hooks' do - other_hook = create(:project_hook) - - perform_async(other_hook) - - expect_jobs(rate_limit + 1) - expect_job_for(other_hook) - end - end - - context 'when the feature flag is disabled' do - before do - stub_feature_flags(web_hooks_rate_limit: false) - end - - it 'queues a job without tracking the call' do - expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) - - perform_async(hook) - - expect_job_for(hook) - end - end - end - end - describe '#perform' do it 'delegates to WebHookService' do - expect_next(WebHookService, hook, data.with_indifferent_access, hook_name).to receive(:execute) + expect_next(WebHookService, project_hook, data.with_indifferent_access, hook_name).to receive(:execute) - subject.perform(hook.id, data, hook_name) + subject.perform(project_hook.id, data, hook_name) end end end -- GitLab From 4903209ae3f7641fbd76b0413ca0bd03dd1b9560 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Tue, 11 May 2021 13:04:47 +0200 Subject: [PATCH 5/8] Fix and clean up specs --- spec/services/web_hook_service_spec.rb | 36 ++++++++++++-------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index ac57a1cb81a77b..95cb07f86e787a 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -6,7 +6,7 @@ include StubRequests let_it_be(:project) { create(:project) } - let_it_be(:project_hook) { create(:project_hook) } + let_it_be_with_reload(:project_hook) { create(:project_hook, project: project) } let(:headers) do { @@ -61,12 +61,8 @@ end describe '#execute' do - before do - project.hooks << [project_hook] - end - context 'when token is defined' do - let(:project_hook) { create(:project_hook, :token) } + let_it_be(:project_hook) { create(:project_hook, :token) } it 'POSTs to the webhook URL' do stub_full_request(project_hook.url, method: :post) @@ -90,8 +86,8 @@ end context 'when auth credentials are present' do - let(:url) {'https://example.org'} - let(:project_hook) { create(:project_hook, url: 'https://demo:demo@example.org/') } + let_it_be(:url) {'https://example.org'} + let_it_be(:project_hook) { create(:project_hook, url: 'https://demo:demo@example.org/') } it 'uses the credentials' do stub_full_request(url, method: :post) @@ -105,8 +101,8 @@ end context 'when auth credentials are partial present' do - let(:url) {'https://example.org'} - let(:project_hook) { create(:project_hook, url: 'https://demo@example.org/') } + let_it_be(:url) {'https://example.org'} + let_it_be(:project_hook) { create(:project_hook, url: 'https://demo@example.org/') } it 'uses the credentials anyways' do stub_full_request(url, method: :post) @@ -148,7 +144,7 @@ end context 'when url is not encoded' do - let(:project_hook) { create(:project_hook, url: 'http://server.com/my path/') } + let_it_be(:project_hook) { create(:project_hook, url: 'http://server.com/my path/') } it 'handles exceptions' do expect(service_instance.execute).to eq(status: :error, message: 'bad URI(is not URI?): "http://server.com/my path/"') @@ -322,11 +318,11 @@ end describe '#async_execute' do - def expect_worker(hook) + def expect_to_perform_worker(hook) expect(WebHookWorker).to receive(:perform_async).with(hook.id, data, 'push_hooks') end - def expect_rate_limiter(hook, threshold:, throttled: false) + def expect_to_rate_limit(hook, threshold:, throttled: false) expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) .with(:web_hook_calls, scope: [hook], threshold: threshold) .and_return(throttled) @@ -335,7 +331,7 @@ def expect_rate_limiter(hook, threshold:, throttled: false) context 'when rate limiting is not configured' do it 'queues a worker without tracking the call' do expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) - expect_worker(project_hook) + expect_to_perform_worker(project_hook) service_instance.async_execute end @@ -346,15 +342,15 @@ def expect_rate_limiter(hook, threshold:, throttled: false) let_it_be(:plan_limits) { create(:plan_limits, :default_plan, web_hook_calls: threshold) } it 'queues a worker and tracks the call' do - expect_rate_limiter(project_hook, threshold: threshold) - expect_worker(project_hook) + expect_to_rate_limit(project_hook, threshold: threshold) + expect_to_perform_worker(project_hook) service_instance.async_execute end context 'when the hook is throttled (via mock)' do before do - expect_rate_limiter(project_hook, threshold: threshold, throttled: true) + expect_to_rate_limit(project_hook, threshold: threshold, throttled: true) end it 'does not queue a worker and logs an error' do @@ -384,7 +380,7 @@ def expect_rate_limiter(hook, threshold:, throttled: false) web_hook_calls: { interval: 1.day } ) - expect_worker(project_hook).exactly(threshold).times + expect_to_perform_worker(project_hook).exactly(threshold).times threshold.times { service_instance.async_execute } end @@ -399,7 +395,7 @@ def expect_rate_limiter(hook, threshold:, throttled: false) it 'still queues workers for other hooks' do other_hook = create(:project_hook) - expect_worker(other_hook) + expect_to_perform_worker(other_hook) described_class.new(other_hook, data, :push_hooks).async_execute end @@ -412,7 +408,7 @@ def expect_rate_limiter(hook, threshold:, throttled: false) it 'queues a worker without tracking the call' do expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) - expect_worker(project_hook) + expect_to_perform_worker(project_hook) service_instance.async_execute end -- GitLab From 42c9e00b487d9b928179a3e650ac515d74685a38 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Tue, 11 May 2021 13:05:39 +0200 Subject: [PATCH 6/8] Fix typo in comment --- app/models/hooks/web_hook.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index efddb605ce79d0..02b4feb4ccc277 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -75,7 +75,7 @@ def enable! update!(recent_failures: 0, disabled_until: nil, backoff_count: 0) end - # Overriden in ProjectHook and GroupHook, other webhooks are not rate-limited. + # Overridden in ProjectHook and GroupHook, other webhooks are not rate-limited. def rate_limit nil end -- GitLab From 73c994d550eed78e9b3297a80cc2a98da782d1fd Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Tue, 11 May 2021 13:12:38 +0200 Subject: [PATCH 7/8] Clean up logger calls --- app/services/web_hook_service.rb | 30 ++++++++++++++------------ spec/services/web_hook_service_spec.rb | 13 +++++------ 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index bdc97300a36b76..645767200e07d6 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -91,20 +91,7 @@ def execute def async_execute if rate_limited?(hook) - Gitlab::AuthLogger.error( - message: 'Webhook rate limit exceeded', - hook_id: hook.id, - hook_name: hook_name - ) - - # Also log into application log for now, so we can use this information - # to determine suitable limits for gitlab.com - Gitlab::AppLogger.error( - message: 'Webhook rate limit exceeded', - class: self.class.name, - hook_id: hook.id, - hook_name: hook_name - ) + log_rate_limit(hook) else WebHookWorker.perform_async(hook.id, data, hook_name) end @@ -201,4 +188,19 @@ def rate_limited?(hook) def rate_limit @rate_limit ||= hook.rate_limit end + + def log_rate_limit(hook) + payload = { + message: 'Webhook rate limit exceeded', + hook_id: hook.id, + hook_type: hook.type, + hook_name: hook_name + } + + Gitlab::AuthLogger.error(payload) + + # Also log into application log for now, so we can use this information + # to determine suitable limits for gitlab.com + Gitlab::AppLogger.error(payload) + end end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 95cb07f86e787a..f9c09086cf25b5 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -356,18 +356,15 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) it 'does not queue a worker and logs an error' do expect(WebHookWorker).not_to receive(:perform_async) - expect(Gitlab::AuthLogger).to receive(:error).with( + payload = { message: 'Webhook rate limit exceeded', hook_id: project_hook.id, + hook_type: 'ProjectHook', hook_name: 'push_hooks' - ) + } - expect(Gitlab::AppLogger).to receive(:error).with( - message: 'Webhook rate limit exceeded', - class: 'WebHookService', - hook_id: project_hook.id, - hook_name: 'push_hooks' - ) + expect(Gitlab::AuthLogger).to receive(:error).with(payload) + expect(Gitlab::AppLogger).to receive(:error).with(payload) service_instance.async_execute end -- GitLab From b098eb84f65cbf169c47f04c0acefdc95975ff4b Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Wed, 12 May 2021 12:46:59 +0200 Subject: [PATCH 8/8] Add version history and FF info to docs --- doc/administration/instance_limits.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index cb230bb51f340d..9ea76ff615135b 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -114,6 +114,12 @@ Limit the maximum daily member invitations allowed per group hierarchy. ### Webhook calls +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61151) in GitLab 13.12. +> - [Deployed behind a feature flag](../user/feature_flags.md), disabled by default. +> - Disabled on GitLab.com. +> - Not recommended for production use. +> - To use in GitLab self-managed instances, ask a GitLab administrator to [enable it](#enable-or-disable-rate-limiting-for-webhooks). **(FREE SELF)** + Limit the number of times any given webhook can be called per minute. This only applies to project and group webhooks. @@ -128,6 +134,27 @@ Plan.default.actual_limits.update!(web_hook_calls: 10) Set the limit to `0` to disable it. +- **Default rate limit**: Disabled. + +#### Enable or disable rate limiting for webhooks **(FREE SELF)** + +Rate limiting for webhooks is under development and not ready for production use. It is +deployed behind a feature flag that is **disabled by default**. +[GitLab administrators with access to the GitLab Rails console](../administration/feature_flags.md) +can enable it. + +To enable it: + +```ruby +Feature.enable(:web_hooks_rate_limit) +``` + +To disable it: + +```ruby +Feature.disable(:web_hooks_rate_limit) +``` + ## Gitaly concurrency limit Clone traffic can put a large strain on your Gitaly service. To prevent such workloads from overwhelming your Gitaly server, you can set concurrency limits in Gitaly's configuration file. -- GitLab