From 772754bc41547860405e55868758f0566c878c22 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Tue, 1 Jun 2021 15:31:47 +0200 Subject: [PATCH 1/2] Add webhook rate-limit threshold for Free plan on gitlab.com See https://gitlab.com/gitlab-org/gitlab/-/issues/330133 Changelog: changed --- ...0210601131742_update_web_hook_calls_limit.rb | 17 +++++++++++++++++ db/schema_migrations/20210601131742 | 1 + doc/administration/instance_limits.md | 2 +- doc/user/gitlab_com/index.md | 10 ++++++---- doc/user/project/integrations/webhooks.md | 6 ++++-- 5 files changed, 29 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20210601131742_update_web_hook_calls_limit.rb create mode 100644 db/schema_migrations/20210601131742 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 00000000000000..6af0facd17d499 --- /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 00000000000000..59869b190e57ad --- /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 26a69e92289773..9423045e3b509a 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 b0130795f941e0..223d3363186282 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 33fe615058081f..406b1e9ba6b027 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 -- GitLab From 5d639f3f8fa9391372802d65d3d92c62ede2df41 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Thu, 10 Jun 2021 16:41:50 +0200 Subject: [PATCH 2/2] Use webhook context when logging rate-limit Include the hook metadata in the log, and remove the redundant call to `Gitlab::AppLogger` since `auth.log` is visible in Kibana as well. --- app/services/web_hook_service.rb | 27 ++++++++++---------------- spec/services/web_hook_service_spec.rb | 22 +++++++++++---------- 2 files changed, 22 insertions(+), 27 deletions(-) diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index f258aa13376995..77d2139b3d1de5 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/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 9465fe7f0d66c3..5f53d6f34d81fc 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 -- GitLab