From 8aa1d49b7efe1f9b75e87b5f9873119256386053 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 9 Jun 2022 11:46:22 +1200 Subject: [PATCH 1/5] Add web hook rate limiting columns to database Note, because we enforce a `Changelog` trailer for database changes, this title has been written to mention only the database changes as the feature changes are behind a feature flag. Real commit title: Log webhook rate limits for paid plans Currently only customers on the Free Plan have rate limited webhooks. This change introduces logging of rate limited webhooks of customers on paid plans. The limits are weighted according to the number of customer seats, allowing the limit to scale as the customer grows. The logging will allow us to assess the limits. A future change will remove the logging and implement actual rate limiting. Changelog: added https://gitlab.com/gitlab-org/gitlab/-/issues/337228 --- app/models/hooks/project_hook.rb | 5 - app/models/hooks/web_hook.rb | 17 +- app/services/web_hook_service.rb | 18 +- ...b_hook_calls_med_and_max_to_plan_limits.rb | 8 + ...eb_hook_calls_to_plan_limits_paid_tiers.rb | 77 ++++++++ db/schema_migrations/20220523030804 | 1 + db/schema_migrations/20220523030805 | 1 + db/structure.sql | 4 +- doc/administration/instance_limits.md | 3 +- doc/user/gitlab_com/index.md | 15 +- ee/app/models/hooks/group_hook.rb | 5 - ee/lib/ee/gitlab/web_hooks/rate_limiter.rb | 96 +++++++++ .../ee/gitlab/web_hooks/rate_limiter_spec.rb | 183 ++++++++++++++++++ ee/spec/models/hooks/ee/project_hook_spec.rb | 26 --- ee/spec/models/hooks/group_hook_spec.rb | 19 -- lib/gitlab/application_rate_limiter.rb | 2 + lib/gitlab/web_hooks/rate_limiter.rb | 70 +++++++ .../lib/gitlab/web_hooks/rate_limiter_spec.rb | 123 ++++++++++++ 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/models/hooks/web_hook_spec.rb | 33 ++-- spec/models/plan_limits_spec.rb | 2 + spec/services/web_hook_service_spec.rb | 9 +- 24 files changed, 612 insertions(+), 130 deletions(-) create mode 100644 db/migrate/20220523030804_add_web_hook_calls_med_and_max_to_plan_limits.rb create mode 100644 db/migrate/20220523030805_add_web_hook_calls_to_plan_limits_paid_tiers.rb create mode 100644 db/schema_migrations/20220523030804 create mode 100644 db/schema_migrations/20220523030805 create mode 100644 ee/lib/ee/gitlab/web_hooks/rate_limiter.rb create mode 100644 ee/spec/lib/ee/gitlab/web_hooks/rate_limiter_spec.rb delete mode 100644 ee/spec/models/hooks/ee/project_hook_spec.rb create mode 100644 lib/gitlab/web_hooks/rate_limiter.rb create mode 100644 spec/lib/gitlab/web_hooks/rate_limiter_spec.rb diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb index 9f45160d3a8d83..b7ace34141e8d1 100644 --- a/app/models/hooks/project_hook.rb +++ b/app/models/hooks/project_hook.rb @@ -31,11 +31,6 @@ def pluralized_name _('Webhooks') end - override :rate_limit - def rate_limit - project.actual_limits.limit_for(:web_hook_calls) - end - override :application_context def application_context super.merge(project: project) diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index f239c26773e855..37fd612e65244d 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -127,19 +127,12 @@ def active_state(ignore_flag: false) # @return [Boolean] Whether or not the WebHook is currently throttled. def rate_limited? - return false unless rate_limit - - Gitlab::ApplicationRateLimiter.peek( - :web_hook_calls, - scope: [self], - threshold: rate_limit - ) + rate_limiter.rate_limited? end - # Threshold for the rate-limit. - # Overridden in ProjectHook and GroupHook, other WebHooks are not rate-limited. + # @return [Integer] The rate limit for the WebHook. `0` for no limit. def rate_limit - nil + rate_limiter.limit end # Returns the associated Project or Group for the WebHook if one exists. @@ -180,4 +173,8 @@ def web_hooks_disable_failed? def initialize_url_variables self.url_variables = {} if encrypted_url_variables.nil? end + + def rate_limiter + @rate_limiter ||= Gitlab::WebHooks::RateLimiter.new(self) + end end diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 7caccd13a9979e..f2f94563e56b6f 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -104,7 +104,7 @@ def execute def async_execute Gitlab::ApplicationContext.with_context(hook.application_context) do - break log_rate_limited if rate_limited? + break log_rate_limited if rate_limit! break log_recursion_blocked if recursion_blocked? params = { @@ -215,24 +215,16 @@ def safe_response_body(response) string_size_limit(response_body, RESPONSE_BODY_SIZE_LIMIT) end - def rate_limited? - return false if rate_limit.nil? - - Gitlab::ApplicationRateLimiter.throttled?( - :web_hook_calls, - scope: [hook], - threshold: rate_limit - ) + # Increments rate-limit counter. + # Returns true if hook should be rate-limited. + def rate_limit! + Gitlab::WebHooks::RateLimiter.new(hook).rate_limit! end def recursion_blocked? Gitlab::WebHooks::RecursionDetection.block?(hook) end - def rate_limit - @rate_limit ||= hook.rate_limit - end - def log_rate_limited log_auth_error('Webhook rate limit exceeded') end diff --git a/db/migrate/20220523030804_add_web_hook_calls_med_and_max_to_plan_limits.rb b/db/migrate/20220523030804_add_web_hook_calls_med_and_max_to_plan_limits.rb new file mode 100644 index 00000000000000..c1ed306551f01f --- /dev/null +++ b/db/migrate/20220523030804_add_web_hook_calls_med_and_max_to_plan_limits.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddWebHookCallsMedAndMaxToPlanLimits < Gitlab::Database::Migration[2.0] + def change + add_column :plan_limits, :web_hook_calls_mid, :integer, null: false, default: 0 + add_column :plan_limits, :web_hook_calls_low, :integer, null: false, default: 0 + end +end diff --git a/db/migrate/20220523030805_add_web_hook_calls_to_plan_limits_paid_tiers.rb b/db/migrate/20220523030805_add_web_hook_calls_to_plan_limits_paid_tiers.rb new file mode 100644 index 00000000000000..4596fefb13df25 --- /dev/null +++ b/db/migrate/20220523030805_add_web_hook_calls_to_plan_limits_paid_tiers.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +class AddWebHookCallsToPlanLimitsPaidTiers < Gitlab::Database::Migration[2.0] + restrict_gitlab_migration gitlab_schema: :gitlab_main + + MAX_RATE_LIMIT_NAME = 'web_hook_calls' + MID_RATE_LIMIT_NAME = 'web_hook_calls_mid' + MIN_RATE_LIMIT_NAME = 'web_hook_calls_low' + + UP_FREE_LIMITS = { + MAX_RATE_LIMIT_NAME => 500, + MID_RATE_LIMIT_NAME => 500, + MIN_RATE_LIMIT_NAME => 500 + }.freeze + + UP_PREMIUM_LIMITS = { + MAX_RATE_LIMIT_NAME => 4_000, + MID_RATE_LIMIT_NAME => 2_800, + MIN_RATE_LIMIT_NAME => 1_600 + }.freeze + + UP_ULTIMATE_LIMITS = { + MAX_RATE_LIMIT_NAME => 13_000, + MID_RATE_LIMIT_NAME => 9_000, + MIN_RATE_LIMIT_NAME => 6_000 + }.freeze + + DOWN_FREE_LIMITS = { + # 120 is the value for 'free' migrated in `db/migrate/20210601131742_update_web_hook_calls_limit.rb` + MAX_RATE_LIMIT_NAME => 120, + MID_RATE_LIMIT_NAME => 0, + MIN_RATE_LIMIT_NAME => 0 + }.freeze + + DOWN_PAID_LIMITS = { + MAX_RATE_LIMIT_NAME => 0, + MID_RATE_LIMIT_NAME => 0, + MIN_RATE_LIMIT_NAME => 0 + }.freeze + + def up + apply_limits('free', UP_FREE_LIMITS) + + # Apply Premium limits + apply_limits('bronze', UP_PREMIUM_LIMITS) + apply_limits('silver', UP_PREMIUM_LIMITS) + apply_limits('premium', UP_PREMIUM_LIMITS) + apply_limits('premium_trial', UP_PREMIUM_LIMITS) + + # Apply Ultimate limits + apply_limits('gold', UP_ULTIMATE_LIMITS) + apply_limits('ultimate', UP_ULTIMATE_LIMITS) + apply_limits('ultimate_trial', UP_ULTIMATE_LIMITS) + apply_limits('opensource', UP_ULTIMATE_LIMITS) + end + + def down + apply_limits('free', DOWN_FREE_LIMITS) + + apply_limits('bronze', DOWN_PAID_LIMITS) + apply_limits('silver', DOWN_PAID_LIMITS) + apply_limits('premium', DOWN_PAID_LIMITS) + apply_limits('premium_trial', DOWN_PAID_LIMITS) + apply_limits('gold', DOWN_PAID_LIMITS) + apply_limits('ultimate', DOWN_PAID_LIMITS) + apply_limits('ultimate_trial', DOWN_PAID_LIMITS) + apply_limits('opensource', DOWN_PAID_LIMITS) + end + + private + + def apply_limits(plan_name, limits) + limits.each_pair do |limit_name, limit| + create_or_update_plan_limit(limit_name, plan_name, limit) + end + end +end diff --git a/db/schema_migrations/20220523030804 b/db/schema_migrations/20220523030804 new file mode 100644 index 00000000000000..6a9bdd4f66dc5a --- /dev/null +++ b/db/schema_migrations/20220523030804 @@ -0,0 +1 @@ +80535374849c10d41663d339b95b9ffddbec9b40a8af4585c18602cbe92c14d1 \ No newline at end of file diff --git a/db/schema_migrations/20220523030805 b/db/schema_migrations/20220523030805 new file mode 100644 index 00000000000000..3714e71a3f3d5f --- /dev/null +++ b/db/schema_migrations/20220523030805 @@ -0,0 +1 @@ +92a7ed079521ccb8ab04e59826947778c37bccd30d47f1b0e29727f769e3ff32 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 0b4eb6d55a1caf..5f712fed7caf05 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -18780,7 +18780,9 @@ CREATE TABLE plan_limits ( pipeline_triggers integer DEFAULT 25000 NOT NULL, project_ci_secure_files integer DEFAULT 100 NOT NULL, repository_size bigint DEFAULT 0 NOT NULL, - security_policy_scan_execution_schedules integer DEFAULT 0 NOT NULL + security_policy_scan_execution_schedules integer DEFAULT 0 NOT NULL, + web_hook_calls_mid integer DEFAULT 0 NOT NULL, + web_hook_calls_low 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 7ff22549a84bfa..3058c375c94cda 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -133,8 +133,9 @@ Limit the maximum daily member invitations allowed per group hierarchy. > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61151) in GitLab 13.12. > - [Feature flag removed](https://gitlab.com/gitlab-org/gitlab/-/issues/330133) in GitLab 14.1. +> - [Limit changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/89591) from per-hook to per-top-level namespace in GitLab 15.1. -Limit the number of times any given webhook can be called per minute. +Webhook rate limits apply per minute to all webhooks within a top-level namespace. This only applies to project and group webhooks. Calls over the rate limit are logged into `auth.log`. diff --git a/doc/user/gitlab_com/index.md b/doc/user/gitlab_com/index.md index 31b6dba0ed5929..a60ecc68d5544c 100644 --- a/doc/user/gitlab_com/index.md +++ b/doc/user/gitlab_com/index.md @@ -232,9 +232,22 @@ also load certain page content directly from common public CDN hostnames. The following limits apply for [webhooks](../project/integrations/webhooks.md): +**Rate limits** + +Webhook rate limits apply per minute to all webhooks within a top-level namespace. + +Paid plan limits are stepped according to the number of seats (paid users) that a paid plan customer has. + +| Plan | Default for GitLab.com | +|----------------------|-------------------------| +| Free | `X` | +| Premium | Less than `a` seats the limit is `X`.
Between `b-c` seats the limit is `Y`.
`d` seats or more, the limit is `Z`. | +| Ultimate and open source | Less than `a` seats the limit is `X`.
Between `b-c` seats the limit is `Y`.
`d` seats or more, the limit is `Z`. | + +**Other limits** + | Setting | Default for GitLab.com | |----------------------|-------------------------| -| Webhook rate limit | `120` calls per minute for GitLab Free, unlimited for GitLab Premium and GitLab Ultimate | | Number of webhooks | `100` per project, `50` per group | | Maximum payload size | 25 MB | diff --git a/ee/app/models/hooks/group_hook.rb b/ee/app/models/hooks/group_hook.rb index b681551144bdcb..babfb531b17c0c 100644 --- a/ee/app/models/hooks/group_hook.rb +++ b/ee/app/models/hooks/group_hook.rb @@ -39,11 +39,6 @@ def application_context super.merge(namespace: group) end - override :rate_limit - def rate_limit - group.actual_limits.limit_for(:web_hook_calls) - end - override :parent def parent group diff --git a/ee/lib/ee/gitlab/web_hooks/rate_limiter.rb b/ee/lib/ee/gitlab/web_hooks/rate_limiter.rb new file mode 100644 index 00000000000000..ee9aaaeea55ca9 --- /dev/null +++ b/ee/lib/ee/gitlab/web_hooks/rate_limiter.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +module EE + module Gitlab + module WebHooks + module RateLimiter + extend ::Gitlab::Utils::Override + + LOW_RATE_LIMIT = :web_hook_calls_low + MID_RATE_LIMIT = :web_hook_calls_mid + HIGH_RATE_LIMIT = ::Gitlab::WebHooks::RateLimiter::LIMIT_NAME + + PREMIUM_MID_RANGE = (100..399).freeze + ULTIMATE_MID_RANGE = (1_000..4_999).freeze + + LIMIT_MAP = { + Plan::BRONZE => PREMIUM_MID_RANGE, + Plan::SILVER => PREMIUM_MID_RANGE, + Plan::GOLD => ULTIMATE_MID_RANGE, + Plan::PREMIUM => PREMIUM_MID_RANGE, + Plan::PREMIUM_TRIAL => PREMIUM_MID_RANGE, + Plan::ULTIMATE => ULTIMATE_MID_RANGE, + Plan::ULTIMATE_TRIAL => ULTIMATE_MID_RANGE, + Plan::OPEN_SOURCE => ULTIMATE_MID_RANGE + }.freeze + + override :rate_limit! + def rate_limit! + is_over_limit = super + + # In this first iteration of paid plan webhook rate-limiting, + # only log but allow the webhook to execute. + if is_over_limit && paid_plan? + log_is_over_limit! + return false + end + + is_over_limit + end + + override :rate_limited? + def rate_limited? + return false if no_limit? || paid_plan? + + super + end + + private + + override :limit_name + def limit_name + strong_memoize(:ee_limit_name) do + next super unless paid_plan? + + # Limits for paid plans are stepped based on the number of seats + # for the customer. + case seats.clamp(LIMIT_MAP[plan.name]) <=> seats + when -1 then HIGH_RATE_LIMIT + when 0 then MID_RATE_LIMIT + when 1 then LOW_RATE_LIMIT + end + end + end + + def paid_plan? + plan.paid? + end + + def plan + @plan ||= root_namespace.actual_plan + end + + def seats + @seats ||= root_namespace.gitlab_subscription.seats + end + + # Log at most once per minute (the same interval as webhook rate-limiting). + def log_is_over_limit! + ::Gitlab::ExclusiveLease.throttle(root_namespace.id, period: 1.minute) do + ::Gitlab::AuthLogger.info( + { + message: 'Webhook rate limit would be exceeded', + hook_id: hook.id, + hook_type: hook.type, + root_namespace: root_namespace.full_path_components.first, + plan: plan.name, + limit: limit, + limit_name: limit_name + } + ) + end + end + end + end + end +end diff --git a/ee/spec/lib/ee/gitlab/web_hooks/rate_limiter_spec.rb b/ee/spec/lib/ee/gitlab/web_hooks/rate_limiter_spec.rb new file mode 100644 index 00000000000000..2b04c7accd9cd8 --- /dev/null +++ b/ee/spec/lib/ee/gitlab/web_hooks/rate_limiter_spec.rb @@ -0,0 +1,183 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::WebHooks::RateLimiter, :saas, :clean_gitlab_redis_rate_limiting, :freeze_time do + before_all do + create(:plan_limits, :premium_plan, web_hook_calls_low: 1, web_hook_calls_mid: 2, web_hook_calls: 3) + create(:plan_limits, :ultimate_plan, web_hook_calls_low: 4, web_hook_calls_mid: 5, web_hook_calls: 6) + create(:plan_limits, :opensource_plan, web_hook_calls_low: 7, web_hook_calls_mid: 8, web_hook_calls: 9) + create(:plan_limits, :bronze_plan, web_hook_calls_low: 9, web_hook_calls_mid: 8, web_hook_calls: 7) + create(:plan_limits, :silver_plan, web_hook_calls_low: 6, web_hook_calls_mid: 5, web_hook_calls: 4) + create(:plan_limits, :gold_plan, web_hook_calls_low: 3, web_hook_calls_mid: 2, web_hook_calls: 1) + create(:plan_limits, :premium_trial_plan, web_hook_calls_low: 1, web_hook_calls_mid: 3, web_hook_calls: 2) + create(:plan_limits, :ultimate_trial_plan, web_hook_calls_low: 2, web_hook_calls_mid: 1, web_hook_calls: 3) + end + + let_it_be(:group_premium_plan) { create(:group_with_plan, plan: :premium_plan) } + let_it_be(:group_ultimate_plan) { create(:group_with_plan, plan: :ultimate_plan) } + let_it_be(:group_opensource_plan) { create(:group_with_plan, plan: :opensource_plan) } + let_it_be(:group_bronze_plan) { create(:group_with_plan, plan: :bronze_plan) } + let_it_be(:group_silver_plan) { create(:group_with_plan, plan: :silver_plan) } + let_it_be(:group_gold_plan) { create(:group_with_plan, plan: :gold_plan) } + let_it_be(:group_premium_trial_plan) { create(:group_with_plan, plan: :premium_trial_plan) } + let_it_be(:group_ultimate_trial_plan) { create(:group_with_plan, plan: :ultimate_trial_plan) } + let_it_be(:project_premium_plan) { create(:project, group: group_premium_plan) } + let_it_be(:project_ultimate_plan) { create(:project, group: group_ultimate_plan) } + + let_it_be_with_reload(:project_hook_with_premium_plan) { create(:project_hook, project: project_premium_plan )} + let_it_be_with_reload(:project_hook_with_ultimate_plan) { create(:project_hook, project: project_ultimate_plan )} + let_it_be_with_reload(:group_hook_with_opensource_plan) { create(:group_hook, group: group_opensource_plan )} + let_it_be_with_reload(:group_hook_with_bronze_plan) { create(:group_hook, group: group_bronze_plan )} + let_it_be_with_reload(:group_hook_with_silver_plan) { create(:group_hook, group: group_silver_plan )} + let_it_be_with_reload(:group_hook_with_gold_plan) { create(:group_hook, group: group_gold_plan )} + let_it_be_with_reload(:group_hook_with_premium_trial_plan) { create(:group_hook, group: group_premium_trial_plan )} + let_it_be_with_reload(:group_hook_with_ultimate_trial_plan) { create(:group_hook, group: group_ultimate_trial_plan )} + + describe 'LIMIT_MAP' do + it 'contains all paid plans' do + keys = described_class::LIMIT_MAP.keys + + expect(keys).to match_array(Plan::PAID_HOSTED_PLANS) + end + end + + describe '#rate_limit!' do + def rate_limit! + described_class.new(hook).rate_limit! + end + + context 'when there is no GitLab subscription' do + let(:hook) { project_hook_with_premium_plan } + let(:root_namespace) { hook.parent.root_namespace } + + before do + root_namespace.gitlab_subscription.destroy! + root_namespace.reload + end + + it 'can never be rate-limited' do + expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) + + rate_limit! + end + end + + context 'when there is a GitLab subscription' do + let(:hook) { project_hook_with_premium_plan } + + it 'can be rate limited' do + expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + + rate_limit! + end + end + + describe 'integration-style test of limits' do + using RSpec::Parameterized::TableSyntax + + where(:hook, :seats, :rate_limit_name, :limit) do + ref(:project_hook_with_premium_plan) | 99 | :web_hook_calls_low | 1 + ref(:project_hook_with_premium_plan) | 100 | :web_hook_calls_mid | 2 + ref(:project_hook_with_premium_plan) | 399 | :web_hook_calls_mid | 2 + ref(:project_hook_with_premium_plan) | 400 | :web_hook_calls | 3 + + ref(:project_hook_with_ultimate_plan) | 999 | :web_hook_calls_low | 4 + ref(:project_hook_with_ultimate_plan) | 1_000 | :web_hook_calls_mid | 5 + ref(:project_hook_with_ultimate_plan) | 4_999 | :web_hook_calls_mid | 5 + ref(:project_hook_with_ultimate_plan) | 5_000 | :web_hook_calls | 6 + + ref(:group_hook_with_opensource_plan) | 999 | :web_hook_calls_low | 7 + ref(:group_hook_with_opensource_plan) | 1_000 | :web_hook_calls_mid | 8 + ref(:group_hook_with_opensource_plan) | 4_999 | :web_hook_calls_mid | 8 + ref(:group_hook_with_opensource_plan) | 5_000 | :web_hook_calls | 9 + + ref(:group_hook_with_bronze_plan) | 99 | :web_hook_calls_low | 9 + ref(:group_hook_with_bronze_plan) | 100 | :web_hook_calls_mid | 8 + ref(:group_hook_with_bronze_plan) | 399 | :web_hook_calls_mid | 8 + ref(:group_hook_with_bronze_plan) | 400 | :web_hook_calls | 7 + + ref(:group_hook_with_silver_plan) | 99 | :web_hook_calls_low | 6 + ref(:group_hook_with_silver_plan) | 100 | :web_hook_calls_mid | 5 + ref(:group_hook_with_silver_plan) | 399 | :web_hook_calls_mid | 5 + ref(:group_hook_with_silver_plan) | 400 | :web_hook_calls | 4 + + ref(:group_hook_with_gold_plan) | 999 | :web_hook_calls_low | 3 + ref(:group_hook_with_gold_plan) | 1_000 | :web_hook_calls_mid | 2 + ref(:group_hook_with_gold_plan) | 4_999 | :web_hook_calls_mid | 2 + ref(:group_hook_with_gold_plan) | 5_000 | :web_hook_calls | 1 + + ref(:group_hook_with_premium_trial_plan) | 99 | :web_hook_calls_low | 1 + ref(:group_hook_with_premium_trial_plan) | 100 | :web_hook_calls_mid | 3 + ref(:group_hook_with_premium_trial_plan) | 399 | :web_hook_calls_mid | 3 + ref(:group_hook_with_premium_trial_plan) | 400 | :web_hook_calls | 2 + + ref(:group_hook_with_ultimate_trial_plan) | 999 | :web_hook_calls_low | 2 + ref(:group_hook_with_ultimate_trial_plan) | 1_000 | :web_hook_calls_mid | 1 + ref(:group_hook_with_ultimate_trial_plan) | 4_999 | :web_hook_calls_mid | 1 + ref(:group_hook_with_ultimate_trial_plan) | 5_000 | :web_hook_calls | 3 + end + + with_them do + let(:root_namespace) { hook.parent.root_ancestor } + + before do + allow(root_namespace.gitlab_subscription).to receive(:seats).and_return(seats) + end + + it 'does not log when webhook is under the rate limit' do + expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .exactly(limit).times + .with( + rate_limit_name, + scope: [root_namespace], + threshold: limit + ).and_call_original + expect(Gitlab::AuthLogger).not_to receive(:info) + + limit.times { expect(rate_limit!).to eq(false) } + end + + it 'logs when rate-limited with an ExclusiveLease protection, but does not enforce limits' do + # This test will execute the hook `limit + 2` times. + # + # Executing `limit` times, the webhook will still be + # under the limit. + # Executing twice more will assert that our logging is protected + # by an exclusive lease. We receive a single log message, but the lease + # is called twice. + execution_count = limit + 2 + + expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .exactly(execution_count).times + .with( + rate_limit_name, + scope: [root_namespace], + threshold: limit + ).and_call_original + + expect(Gitlab::ExclusiveLease).to receive(:throttle) + .twice + .with( + root_namespace.id, + period: 1.minute + ).and_call_original + + expect(Gitlab::AuthLogger).to receive(:info) + .once + .with( + message: 'Webhook rate limit would be exceeded', + hook_id: hook.id, + hook_type: hook.type, + root_namespace: root_namespace.full_path_components.first, + plan: root_namespace.actual_plan.name, + limit: limit, + limit_name: rate_limit_name + ) + + execution_count.times { expect(rate_limit!).to eq(false) } + 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 deleted file mode 100644 index 88c87672edda35..00000000000000 --- a/ee/spec/models/hooks/ee/project_hook_spec.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe EE::ProjectHook do - describe '#rate_limit', :saas do - let_it_be(:default_limits) { create(:plan_limits, :free_plan, web_hook_calls: 100) } - let_it_be(:ultimate_limits) { create(:plan_limits, :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 da62a3b4e57ca3..03ca99fa3977e0 100644 --- a/ee/spec/models/hooks/group_hook_spec.rb +++ b/ee/spec/models/hooks/group_hook_spec.rb @@ -11,25 +11,6 @@ subject { build(:group_hook, group: create(:group)) } end - describe '#rate_limit', :saas do - let_it_be(:default_limits) { create(:plan_limits, :free_plan, web_hook_calls: 100) } - let_it_be(:ultimate_limits) { create(:plan_limits, :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 - describe '#parent' do it 'returns the associated group' do group = build(:group) diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index b9aa2e8c9877d2..722ee061eba24b 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -32,6 +32,8 @@ def rate_limits # rubocop:disable Metrics/AbcSize group_testing_hook: { threshold: 5, interval: 1.minute }, profile_add_new_email: { threshold: 5, interval: 1.minute }, web_hook_calls: { interval: 1.minute }, + web_hook_calls_mid: { interval: 1.minute }, + web_hook_calls_low: { interval: 1.minute }, users_get_by_id: { threshold: -> { application_settings.users_get_by_id_limit }, interval: 10.minutes }, username_exists: { threshold: 20, interval: 1.minute }, user_sign_up: { threshold: 20, interval: 1.minute }, diff --git a/lib/gitlab/web_hooks/rate_limiter.rb b/lib/gitlab/web_hooks/rate_limiter.rb new file mode 100644 index 00000000000000..73d59f6f786f4a --- /dev/null +++ b/lib/gitlab/web_hooks/rate_limiter.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module Gitlab + module WebHooks + class RateLimiter + include Gitlab::Utils::StrongMemoize + + LIMIT_NAME = :web_hook_calls + NO_LIMIT = 0 + # SystemHooks (instance admin hooks) and ServiceHooks (integration hooks) + # are not rate-limited. + EXCLUDED_HOOK_TYPES = %w(SystemHook ServiceHook).freeze + + def initialize(hook) + @hook = hook + @parent = hook.parent + end + + # Increments the rate-limit counter. + # Returns true if the hook should be rate-limited. + def rate_limit! + return false if no_limit? + + ::Gitlab::ApplicationRateLimiter.throttled?( + limit_name, + scope: [root_namespace], + threshold: limit + ) + end + + # Returns true if the hook is currently over its rate-limit. + # It does not increment the rate-limit counter. + def rate_limited? + return false if no_limit? + + Gitlab::ApplicationRateLimiter.peek( + limit_name, + scope: [root_namespace], + threshold: limit + ) + end + + def limit + strong_memoize(:limit) do + next NO_LIMIT if hook.class.name.in?(EXCLUDED_HOOK_TYPES) + + root_namespace.actual_limits.limit_for(limit_name) || NO_LIMIT + end + end + + private + + attr_reader :hook, :parent + + def no_limit? + limit == NO_LIMIT + end + + def root_namespace + @root_namespace ||= parent.root_ancestor + end + + def limit_name + LIMIT_NAME + end + end + end +end + +Gitlab::WebHooks::RateLimiter.prepend_mod diff --git a/spec/lib/gitlab/web_hooks/rate_limiter_spec.rb b/spec/lib/gitlab/web_hooks/rate_limiter_spec.rb new file mode 100644 index 00000000000000..b25ce4ea9da7e7 --- /dev/null +++ b/spec/lib/gitlab/web_hooks/rate_limiter_spec.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::WebHooks::RateLimiter, :clean_gitlab_redis_rate_limiting do + let_it_be(:plan) { create(:default_plan) } + let_it_be_with_reload(:project_hook) { create(:project_hook) } + let_it_be_with_reload(:system_hook) { create(:system_hook) } + let_it_be_with_reload(:integration_hook) { create(:jenkins_integration).service_hook } + let_it_be(:limit) { 1 } + + using RSpec::Parameterized::TableSyntax + + describe '#rate_limit!' do + def rate_limit!(hook) + described_class.new(hook).rate_limit! + end + + shared_examples 'a hook that is never rate limited' do + specify do + expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) + + expect(rate_limit!(hook)).to eq(false) + end + end + + context 'when there is no plan limit' do + where(:hook) { [ref(:project_hook), ref(:system_hook), ref(:integration_hook)] } + + with_them { it_behaves_like 'a hook that is never rate limited' } + end + + context 'when there is a plan limit' do + before_all do + create(:plan_limits, plan: plan, web_hook_calls: limit) + end + + where(:hook, :limitless_hook_type) do + ref(:project_hook) | false + ref(:system_hook) | true + ref(:integration_hook) | true + end + + with_them do + if params[:limitless_hook_type] + it_behaves_like 'a hook that is never rate limited' + else + it 'rate limits the hook, returning true when rate limited' do + expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .exactly(3).times + .and_call_original + + freeze_time do + limit.times { expect(rate_limit!(hook)).to eq(false) } + expect(rate_limit!(hook)).to eq(true) + end + + travel_to(1.day.from_now) do + expect(rate_limit!(hook)).to eq(false) + end + end + end + end + end + + describe 'rate limit scope' do + it 'rate limits all hooks from the same namespace', :freeze_time do + create(:plan_limits, plan: plan, web_hook_calls: limit) + project_hook_in_different_namespace = create(:project_hook) + project_hook_in_same_namespace = create(:project_hook, + project: create(:project, namespace: project_hook.project.namespace) + ) + + limit.times { expect(rate_limit!(project_hook)).to eq(false) } + expect(rate_limit!(project_hook)).to eq(true) + expect(rate_limit!(project_hook_in_same_namespace)).to eq(true) + expect(rate_limit!(project_hook_in_different_namespace)).to eq(false) + end + end + end + + describe '#rate_limited?' do + subject { described_class.new(hook).rate_limited? } + + context 'when no plan limit has been defined' do + where(:hook) { [ref(:project_hook), ref(:system_hook), ref(:integration_hook)] } + + with_them do + it { is_expected.to eq(false) } + end + end + + context 'when there is a plan limit' do + before_all do + create(:plan_limits, plan: plan, web_hook_calls: limit) + end + + context 'when hook is not rate-limited' do + where(:hook) { [ref(:project_hook), ref(:system_hook), ref(:integration_hook)] } + + with_them do + it { is_expected.to eq(false) } + end + end + + context 'when hook is rate-limited' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true) + end + + where(:hook, :limitless_hook_type) do + ref(:project_hook) | false + ref(:system_hook) | true + ref(:integration_hook) | true + end + + with_them do + it { is_expected.to eq(!limitless_hook_type) } + end + end + end + end +end diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb index ec2eca9675597e..4253686b843246 100644 --- a/spec/models/hooks/project_hook_spec.rb +++ b/spec/models/hooks/project_hook_spec.rb @@ -31,15 +31,6 @@ end end - describe '#rate_limit' do - let_it_be(:plan_limits) { create(:plan_limits, :default_plan, web_hook_calls: 100) } - let_it_be(:hook) { create(:project_hook) } - - it 'returns the default limit' do - expect(hook.rate_limit).to be(100) - end - end - describe '#parent' do it 'returns the associated project' do project = build(:project) diff --git a/spec/models/hooks/service_hook_spec.rb b/spec/models/hooks/service_hook_spec.rb index 0d65fe302e184c..68c284a913c4da 100644 --- a/spec/models/hooks/service_hook_spec.rb +++ b/spec/models/hooks/service_hook_spec.rb @@ -23,14 +23,6 @@ end end - describe '#rate_limit' do - let(:hook) { build(:service_hook) } - - it 'returns nil' do - expect(hook.rate_limit).to be_nil - end - end - describe '#parent' do let(:hook) { build(:service_hook, integration: integration) } diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb index bf69c7219a81bc..9f5f81dd6c0d70 100644 --- a/spec/models/hooks/system_hook_spec.rb +++ b/spec/models/hooks/system_hook_spec.rb @@ -185,14 +185,6 @@ end end - describe '#rate_limit' do - let(:hook) { build(:system_hook) } - - it 'returns nil' do - expect(hook.rate_limit).to be_nil - end - end - describe '#application_context' do let(:hook) { build(:system_hook) } diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index ab40f962af375c..fb4d1cee606741 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -493,31 +493,30 @@ def run_expectation end describe '#rate_limited?' do - context 'when there are rate limits' do - before do - allow(hook).to receive(:rate_limit).and_return(3) + it 'is false when hook has not been rate limited' do + expect_next_instance_of(Gitlab::WebHooks::RateLimiter) do |rate_limiter| + expect(rate_limiter).to receive(:rate_limited?).and_return(false) end - it 'is false when hook has not been rate limited' do - expect(Gitlab::ApplicationRateLimiter).to receive(:peek).and_return(false) - expect(hook).not_to be_rate_limited - end + expect(hook).not_to be_rate_limited + end - it 'is true when hook has been rate limited' do - expect(Gitlab::ApplicationRateLimiter).to receive(:peek).and_return(true) - expect(hook).to be_rate_limited + it 'is true when hook has been rate limited' do + expect_next_instance_of(Gitlab::WebHooks::RateLimiter) do |rate_limiter| + expect(rate_limiter).to receive(:rate_limited?).and_return(true) end + + expect(hook).to be_rate_limited end + end - context 'when there are no rate limits' do - before do - allow(hook).to receive(:rate_limit).and_return(nil) + describe '#rate_limit' do + it 'returns the hook rate limit' do + expect_next_instance_of(Gitlab::WebHooks::RateLimiter) do |rate_limiter| + expect(rate_limiter).to receive(:limit).and_return(10) end - it 'does not call Gitlab::ApplicationRateLimiter, and is false' do - expect(Gitlab::ApplicationRateLimiter).not_to receive(:peek) - expect(hook).not_to be_rate_limited - end + expect(hook.rate_limit).to eq(10) end end diff --git a/spec/models/plan_limits_spec.rb b/spec/models/plan_limits_spec.rb index 78521e4bdf2163..f9c458b2c801dd 100644 --- a/spec/models/plan_limits_spec.rb +++ b/spec/models/plan_limits_spec.rb @@ -213,6 +213,8 @@ storage_size_limit daily_invites web_hook_calls + web_hook_calls_mid + web_hook_calls_low ci_daily_pipeline_schedule_triggers repository_size security_policy_scan_execution_schedules diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 9f3093d64f3c7b..068550ec234b6c 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -521,7 +521,7 @@ def expect_to_perform_worker(hook) def expect_to_rate_limit(hook, threshold:, throttled: false) expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) - .with(:web_hook_calls, scope: [hook], threshold: threshold) + .with(:web_hook_calls, scope: [hook.parent.root_namespace], threshold: threshold) .and_return(throttled) end @@ -570,13 +570,8 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) end end - context 'when the hook is throttled (via Redis)', :clean_gitlab_redis_rate_limiting do + context 'when the hook is throttled (via Redis)', :clean_gitlab_redis_rate_limiting, :freeze_time 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_to_perform_worker(project_hook).exactly(threshold).times threshold.times { service_instance.async_execute } -- GitLab From 3a01b4742757ad8cf821fed90a1dee78e93420df Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 14 Jun 2022 11:13:45 +1200 Subject: [PATCH 2/5] Revert docs change --- doc/user/gitlab_com/index.md | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/doc/user/gitlab_com/index.md b/doc/user/gitlab_com/index.md index a60ecc68d5544c..dec16f4c108aa1 100644 --- a/doc/user/gitlab_com/index.md +++ b/doc/user/gitlab_com/index.md @@ -232,22 +232,9 @@ also load certain page content directly from common public CDN hostnames. The following limits apply for [webhooks](../project/integrations/webhooks.md): -**Rate limits** - -Webhook rate limits apply per minute to all webhooks within a top-level namespace. - -Paid plan limits are stepped according to the number of seats (paid users) that a paid plan customer has. - -| Plan | Default for GitLab.com | -|----------------------|-------------------------| -| Free | `X` | -| Premium | Less than `a` seats the limit is `X`.
Between `b-c` seats the limit is `Y`.
`d` seats or more, the limit is `Z`. | -| Ultimate and open source | Less than `a` seats the limit is `X`.
Between `b-c` seats the limit is `Y`.
`d` seats or more, the limit is `Z`. | - -**Other limits** - | Setting | Default for GitLab.com | |----------------------|-------------------------| +| Webhook rate limit | `500` calls per minute for GitLab Free, unlimited for GitLab Premium and GitLab Ultimate. Webhook rate limits apply per minute to all webhooks within a top-level namespace. | | Number of webhooks | `100` per project, `50` per group | | Maximum payload size | 25 MB | -- GitLab From 33b5908ed9250972012bcd8ee55408b853dd716e Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 15 Jun 2022 15:21:14 +1200 Subject: [PATCH 3/5] Add reviewer feedback --- ...eb_hook_calls_to_plan_limits_paid_tiers.rb | 4 + doc/administration/instance_limits.md | 2 +- doc/user/gitlab_com/index.md | 2 +- ...ok_calls_to_plan_limits_paid_tiers_spec.rb | 101 ++++++++++++++++++ 4 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 spec/migrations/add_web_hook_calls_to_plan_limits_paid_tiers_spec.rb diff --git a/db/migrate/20220523030805_add_web_hook_calls_to_plan_limits_paid_tiers.rb b/db/migrate/20220523030805_add_web_hook_calls_to_plan_limits_paid_tiers.rb index 4596fefb13df25..842bb297803cfe 100644 --- a/db/migrate/20220523030805_add_web_hook_calls_to_plan_limits_paid_tiers.rb +++ b/db/migrate/20220523030805_add_web_hook_calls_to_plan_limits_paid_tiers.rb @@ -39,6 +39,8 @@ class AddWebHookCallsToPlanLimitsPaidTiers < Gitlab::Database::Migration[2.0] }.freeze def up + return unless Gitlab.com? + apply_limits('free', UP_FREE_LIMITS) # Apply Premium limits @@ -55,6 +57,8 @@ def up end def down + return unless Gitlab.com? + apply_limits('free', DOWN_FREE_LIMITS) apply_limits('bronze', DOWN_PAID_LIMITS) diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index 3058c375c94cda..11689637910b3e 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -135,7 +135,7 @@ Limit the maximum daily member invitations allowed per group hierarchy. > - [Feature flag removed](https://gitlab.com/gitlab-org/gitlab/-/issues/330133) in GitLab 14.1. > - [Limit changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/89591) from per-hook to per-top-level namespace in GitLab 15.1. -Webhook rate limits apply per minute to all webhooks within a top-level namespace. +Webhook rate limits are applied per minute and per top-level namespace. This only applies to project and group webhooks. Calls over the rate limit are logged into `auth.log`. diff --git a/doc/user/gitlab_com/index.md b/doc/user/gitlab_com/index.md index dec16f4c108aa1..d1d1b7f6969d0f 100644 --- a/doc/user/gitlab_com/index.md +++ b/doc/user/gitlab_com/index.md @@ -234,7 +234,7 @@ The following limits apply for [webhooks](../project/integrations/webhooks.md): | Setting | Default for GitLab.com | |----------------------|-------------------------| -| Webhook rate limit | `500` calls per minute for GitLab Free, unlimited for GitLab Premium and GitLab Ultimate. Webhook rate limits apply per minute to all webhooks within a top-level namespace. | +| Webhook rate limit | `500` calls per minute for GitLab Free, unlimited for GitLab Premium and GitLab Ultimate. Webhook rate limits are applied per minute and per top-level namespace. | | Number of webhooks | `100` per project, `50` per group | | Maximum payload size | 25 MB | diff --git a/spec/migrations/add_web_hook_calls_to_plan_limits_paid_tiers_spec.rb b/spec/migrations/add_web_hook_calls_to_plan_limits_paid_tiers_spec.rb new file mode 100644 index 00000000000000..63ad9367503a3b --- /dev/null +++ b/spec/migrations/add_web_hook_calls_to_plan_limits_paid_tiers_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe AddWebHookCallsToPlanLimitsPaidTiers do + let_it_be(:plans) { table(:plans) } + let_it_be(:plan_limits) { table(:plan_limits) } + + context 'when on Gitlab.com' do + let(:free_plan) { plans.create!(name: 'free') } + let(:bronze_plan) { plans.create!(name: 'bronze') } + let(:silver_plan) { plans.create!(name: 'silver') } + let(:gold_plan) { plans.create!(name: 'gold') } + let(:premium_plan) { plans.create!(name: 'premium') } + let(:premium_trial_plan) { plans.create!(name: 'premium_trial') } + let(:ultimate_plan) { plans.create!(name: 'ultimate') } + let(:ultimate_trial_plan) { plans.create!(name: 'ultimate_trial') } + let(:opensource_plan) { plans.create!(name: 'opensource') } + + before do + allow(Gitlab).to receive(:com?).and_return(true) + # 120 is the value for 'free' migrated in `db/migrate/20210601131742_update_web_hook_calls_limit.rb` + plan_limits.create!(plan_id: free_plan.id, web_hook_calls: 120) + plan_limits.create!(plan_id: bronze_plan.id) + plan_limits.create!(plan_id: silver_plan.id) + plan_limits.create!(plan_id: gold_plan.id) + plan_limits.create!(plan_id: premium_plan.id) + plan_limits.create!(plan_id: premium_trial_plan.id) + plan_limits.create!(plan_id: ultimate_plan.id) + plan_limits.create!(plan_id: ultimate_trial_plan.id) + plan_limits.create!(plan_id: opensource_plan.id) + end + + it 'correctly migrates up and down' do + reversible_migration do |migration| + migration.before -> { + expect( + plan_limits.pluck(:plan_id, :web_hook_calls, :web_hook_calls_mid, :web_hook_calls_low) + ).to contain_exactly( + [free_plan.id, 120, 0, 0], + [bronze_plan.id, 0, 0, 0], + [silver_plan.id, 0, 0, 0], + [gold_plan.id, 0, 0, 0], + [premium_plan.id, 0, 0, 0], + [premium_trial_plan.id, 0, 0, 0], + [ultimate_plan.id, 0, 0, 0], + [ultimate_trial_plan.id, 0, 0, 0], + [opensource_plan.id, 0, 0, 0] + ) + } + + migration.after -> { + expect( + plan_limits.pluck(:plan_id, :web_hook_calls, :web_hook_calls_mid, :web_hook_calls_low) + ).to contain_exactly( + [free_plan.id, 500, 500, 500], + [bronze_plan.id, 4_000, 2_800, 1_600], + [silver_plan.id, 4_000, 2_800, 1_600], + [gold_plan.id, 13_000, 9_000, 6_000], + [premium_plan.id, 4_000, 2_800, 1_600], + [premium_trial_plan.id, 4_000, 2_800, 1_600], + [ultimate_plan.id, 13_000, 9_000, 6_000], + [ultimate_trial_plan.id, 13_000, 9_000, 6_000], + [opensource_plan.id, 13_000, 9_000, 6_000] + ) + } + end + end + end + + context 'when on self hosted' do + let(:default_plan) { plans.create!(name: 'default') } + + before do + allow(Gitlab).to receive(:com?).and_return(false) + + plan_limits.create!(plan_id: default_plan.id) + end + + it 'does nothing' do + reversible_migration do |migration| + migration.before -> { + expect( + plan_limits.pluck(:plan_id, :web_hook_calls, :web_hook_calls_mid, :web_hook_calls_low) + ).to contain_exactly( + [default_plan.id, 0, 0, 0] + ) + } + + migration.after -> { + expect( + plan_limits.pluck(:plan_id, :web_hook_calls, :web_hook_calls_mid, :web_hook_calls_low) + ).to contain_exactly( + [default_plan.id, 0, 0, 0] + ) + } + end + end + end +end -- GitLab From 1ecf2865bdd47afd18097c513c43205820bd0fbf Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 16 Jun 2022 10:37:52 +1200 Subject: [PATCH 4/5] Add reviewer feedback --- doc/administration/instance_limits.md | 2 +- doc/user/gitlab_com/index.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index 11689637910b3e..d86414ae285edf 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -135,7 +135,7 @@ Limit the maximum daily member invitations allowed per group hierarchy. > - [Feature flag removed](https://gitlab.com/gitlab-org/gitlab/-/issues/330133) in GitLab 14.1. > - [Limit changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/89591) from per-hook to per-top-level namespace in GitLab 15.1. -Webhook rate limits are applied per minute and per top-level namespace. +Limit the number of times a webhook can be called per minute, per top-level namespace. This only applies to project and group webhooks. Calls over the rate limit are logged into `auth.log`. diff --git a/doc/user/gitlab_com/index.md b/doc/user/gitlab_com/index.md index d1d1b7f6969d0f..d515f9f4558d49 100644 --- a/doc/user/gitlab_com/index.md +++ b/doc/user/gitlab_com/index.md @@ -234,7 +234,7 @@ The following limits apply for [webhooks](../project/integrations/webhooks.md): | Setting | Default for GitLab.com | |----------------------|-------------------------| -| Webhook rate limit | `500` calls per minute for GitLab Free, unlimited for GitLab Premium and GitLab Ultimate. Webhook rate limits are applied per minute and per top-level namespace. | +| Webhook rate limit | `500` calls per minute for GitLab Free, unlimited for GitLab Premium and GitLab Ultimate. Webhook rate limits are applied per top-level namespace. | | Number of webhooks | `100` per project, `50` per group | | Maximum payload size | 25 MB | -- GitLab From f5a3dc76967a39aedd20347a63e530d17ca9f889 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 16 Jun 2022 15:52:05 +1200 Subject: [PATCH 5/5] Add reviewer feedback --- ee/lib/ee/gitlab/web_hooks/rate_limiter.rb | 2 +- ee/spec/lib/ee/gitlab/web_hooks/rate_limiter_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/lib/ee/gitlab/web_hooks/rate_limiter.rb b/ee/lib/ee/gitlab/web_hooks/rate_limiter.rb index ee9aaaeea55ca9..5035681d7b9a22 100644 --- a/ee/lib/ee/gitlab/web_hooks/rate_limiter.rb +++ b/ee/lib/ee/gitlab/web_hooks/rate_limiter.rb @@ -77,7 +77,7 @@ def seats # Log at most once per minute (the same interval as webhook rate-limiting). def log_is_over_limit! ::Gitlab::ExclusiveLease.throttle(root_namespace.id, period: 1.minute) do - ::Gitlab::AuthLogger.info( + ::Gitlab::AppLogger.info( { message: 'Webhook rate limit would be exceeded', hook_id: hook.id, diff --git a/ee/spec/lib/ee/gitlab/web_hooks/rate_limiter_spec.rb b/ee/spec/lib/ee/gitlab/web_hooks/rate_limiter_spec.rb index 2b04c7accd9cd8..d2bc6927ad6270 100644 --- a/ee/spec/lib/ee/gitlab/web_hooks/rate_limiter_spec.rb +++ b/ee/spec/lib/ee/gitlab/web_hooks/rate_limiter_spec.rb @@ -133,7 +133,7 @@ def rate_limit! scope: [root_namespace], threshold: limit ).and_call_original - expect(Gitlab::AuthLogger).not_to receive(:info) + expect(Gitlab::AppLogger).not_to receive(:info) limit.times { expect(rate_limit!).to eq(false) } end @@ -163,7 +163,7 @@ def rate_limit! period: 1.minute ).and_call_original - expect(Gitlab::AuthLogger).to receive(:info) + expect(Gitlab::AppLogger).to receive(:info) .once .with( message: 'Webhook rate limit would be exceeded', -- GitLab