diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb index 9f45160d3a8d839258dbaaea193e78563b516016..b7ace34141e8d1ef77b15c2b4207533de9612e54 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 f239c26773e855449afa2b7bbf5632bc52c201de..37fd612e65244d6afb95cec900cc81948e0d4ec9 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 7caccd13a9979e3c8ece2e0b2bcec40386a64866..f2f94563e56b6f8586ebf4fd02de5244503bae45 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 0000000000000000000000000000000000000000..c1ed306551f01f3b6bc5c128ed3afc000e77c932 --- /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 0000000000000000000000000000000000000000..842bb297803cfe0bd7e192232e12028dc66054c5 --- /dev/null +++ b/db/migrate/20220523030805_add_web_hook_calls_to_plan_limits_paid_tiers.rb @@ -0,0 +1,81 @@ +# 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 + return unless Gitlab.com? + + 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 + return unless Gitlab.com? + + 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 0000000000000000000000000000000000000000..6a9bdd4f66dc5ada7b2ea9d1e95aac324d24e656 --- /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 0000000000000000000000000000000000000000..3714e71a3f3d5f98804ccbedece6904be8063e35 --- /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 0b4eb6d55a1caf71e338a3155116f26607ca33db..5f712fed7caf05eb88633836e26322ab2dbf81d6 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 7ff22549a84bfa7dfb0360ddf11d74c0cdf87af8..d86414ae285edf3785e7bdc165bdc3b53625b344 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. +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 31b6dba0ed59294b68ae25e6a2f7e64c83078f7e..d515f9f4558d494bd42658ece26b1ebc8f5bc57b 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 | `120` calls per minute for GitLab Free, unlimited for GitLab Premium and GitLab Ultimate | +| 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 | diff --git a/ee/app/models/hooks/group_hook.rb b/ee/app/models/hooks/group_hook.rb index b681551144bdcb5609be687dc68503c9eeeba364..babfb531b17c0c70d57456d7f31ac9f083ec8b26 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 0000000000000000000000000000000000000000..5035681d7b9a225dc0c2dc511a3804e545e58b1f --- /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::AppLogger.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 0000000000000000000000000000000000000000..d2bc6927ad62702cd8df6d175a732e6f467b76b5 --- /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::AppLogger).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::AppLogger).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 88c87672edda3556160db05d6285730f87dc8a20..0000000000000000000000000000000000000000 --- 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 da62a3b4e57ca3627a1a7361033a3a60c0a55d68..03ca99fa3977e0c5ccd2f4db4adf99c9aab7aab8 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 b9aa2e8c9877d2e9436f73cbb5d6a45612692be0..722ee061eba24bad616df99a62f895c38cd04ab4 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 0000000000000000000000000000000000000000..73d59f6f786f4a523cdd98bb1e0d816e0ff4d70e --- /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 0000000000000000000000000000000000000000..b25ce4ea9da7e7f5463943ec28f8e27d20efaf34 --- /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/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 0000000000000000000000000000000000000000..63ad9367503a3b13ecf35f70ac8a960c05b5daa0 --- /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 diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb index ec2eca9675597e779b0953d1379d382cbc2a38be..4253686b8432467df77e1c8e0266df2b8bafc23d 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 0d65fe302e184c99aa21952c72d389171bd521fd..68c284a913c4da8cc1f7570c417e47920d7ea858 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 bf69c7219a81bcb6995754779088c4aab2c83c2d..9f5f81dd6c0d70579c07831ce87d12949df49b3e 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 ab40f962af375cdf0a7ad5856549ab0236a57b3e..fb4d1cee6067412ff72a2af07911a29fc68592a7 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 78521e4bdf2163f86597bdf6e20f1216f9882e12..f9c458b2c801ddf5ee3cb1293a867693947ccbb2 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 9f3093d64f3c7b95dd7be031a8415eaaeadbcb4d..068550ec234b6c257bd96ca1ea27b3651588532d 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 }