diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index f258aa133769955bde30512e710e5ed531bf8101..77d2139b3d1de5f94fbe956658ab9fe3cb487684 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -91,12 +91,10 @@ def execute end def async_execute - if rate_limited?(hook) - log_rate_limit(hook) - else - Gitlab::ApplicationContext.with_context(hook.application_context) do - WebHookWorker.perform_async(hook.id, data, hook_name) - end + Gitlab::ApplicationContext.with_context(hook.application_context) do + break log_rate_limit if rate_limited? + + WebHookWorker.perform_async(hook.id, data, hook_name) end end @@ -177,7 +175,7 @@ def safe_response_body(response) response.body.encode('UTF-8', invalid: :replace, undef: :replace, replace: '') end - def rate_limited?(hook) + def rate_limited? return false unless Feature.enabled?(:web_hooks_rate_limit, default_enabled: :yaml) return false if rate_limit.nil? @@ -192,18 +190,13 @@ def rate_limit @rate_limit ||= hook.rate_limit end - def log_rate_limit(hook) - payload = { + def log_rate_limit + Gitlab::AuthLogger.error( 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) + hook_name: hook_name, + **Gitlab::ApplicationContext.current + ) end end diff --git a/db/migrate/20210601131742_update_web_hook_calls_limit.rb b/db/migrate/20210601131742_update_web_hook_calls_limit.rb new file mode 100644 index 0000000000000000000000000000000000000000..6af0facd17d499e1d22e013146f2055243639d77 --- /dev/null +++ b/db/migrate/20210601131742_update_web_hook_calls_limit.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class UpdateWebHookCallsLimit < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + def up + return unless Gitlab.com? + + create_or_update_plan_limit('web_hook_calls', 'free', 120) + end + + def down + return unless Gitlab.com? + + create_or_update_plan_limit('web_hook_calls', 'free', 0) + end +end diff --git a/db/schema_migrations/20210601131742 b/db/schema_migrations/20210601131742 new file mode 100644 index 0000000000000000000000000000000000000000..59869b190e57ad21db591b8cd4799230a8920f80 --- /dev/null +++ b/db/schema_migrations/20210601131742 @@ -0,0 +1 @@ +63cd83e097a24b39a399918422950caacb6aed8d05d0d8b7bcf66f9155a0d04e \ No newline at end of file diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index 26a69e92289773d0573cca7a61a7da9bc8844339..9423045e3b509a755c5fda47619241f3619488df 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -112,7 +112,7 @@ 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 +### Webhook rate limit > - [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. diff --git a/doc/user/gitlab_com/index.md b/doc/user/gitlab_com/index.md index b0130795f941e0c2b9511cdac77b11a229223487..223d3363186282932b0e56b74d65f0ca206cdb6a 100644 --- a/doc/user/gitlab_com/index.md +++ b/doc/user/gitlab_com/index.md @@ -153,11 +153,13 @@ content directly from common public CDN hostnames. ## Webhooks -A limit of: +The following limits apply for [Webhooks](../project/integrations/webhooks.md): -- 100 webhooks applies to projects. -- 50 webhooks applies to groups. **(BRONZE ONLY)** -- Payload is limited to 25MB. +| Setting | GitLab.com | Default | +| ------- | ---------- | ------- | +| [Webhook rate limit](../../administration/instance_limits.md#webhook-rate-limit) | `120` calls per minute for Free tier, unlimited for all paid tiers | Unlimited +| [Number of webhooks](../../administration/instance_limits.md#number-of-webhooks) | `100` per-project, `50` per-group | `100` per-project, `50` per-group +| Maximum payload size | `25 MB` | `25 MB` ## Shared runners diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index 33fe615058081f65cffd713f9b0dfbd7e05fb567..406b1e9ba6b027289772b90a9b11ed07f9180f5d 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -34,8 +34,10 @@ Webhooks are available: - Per project, at a project's **Settings > Webhooks** menu. **(FREE)** - Additionally per group, at a group's **Settings > Webhooks** menu. **(PREMIUM)** -NOTE: -On GitLab.com, the [maximum number of webhooks and their size](../../../user/gitlab_com/index.md#webhooks) per project, and per group, is limited. +GitLab.com enforces various [webhook limits](../../../user/gitlab_com/index.md#webhooks), including: + +- The maximum number of webhooks and their size, both per project, and per group. +- The number of webhook calls per minute. ## Possible uses for webhooks diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 9465fe7f0d66c37967e462300e31fc71412cbe17..5f53d6f34d81fc7323adf5d6618327efcb91c9b0 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -375,15 +375,18 @@ 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) - payload = { - message: 'Webhook rate limit exceeded', - hook_id: project_hook.id, - hook_type: 'ProjectHook', - hook_name: 'push_hooks' - } - - expect(Gitlab::AuthLogger).to receive(:error).with(payload) - expect(Gitlab::AppLogger).to receive(:error).with(payload) + expect(Gitlab::AuthLogger).to receive(:error).with( + include( + message: 'Webhook rate limit exceeded', + hook_id: project_hook.id, + hook_type: 'ProjectHook', + hook_name: 'push_hooks', + "correlation_id" => kind_of(String), + "meta.project" => project.full_path, + "meta.related_class" => 'ProjectHook', + "meta.root_namespace" => project.root_namespace.full_path + ) + ) service_instance.async_execute end @@ -403,7 +406,6 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) 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