Merge branch '330133-webhook-rate-limit-free-plan' into 'master'

Add webhook rate-limit threshold for Free plan on gitlab.com

See merge request gitlab-org/gitlab!62918

app/services/web_hook_service.rb

@@ -91,12 +91,10 @@ class WebHookService
   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 @@ class WebHookService
     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 @@ class WebHookService
     @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

db/migrate/20210601131742_update_web_hook_calls_limit.rb

+# 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

db/schema_migrations/20210601131742

+63cd83e097a24b39a399918422950caacb6aed8d05d0d8b7bcf66f9155a0d04e
\ No newline at end of file

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.

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
 

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
 

spec/services/web_hook_service_spec.rb

@@ -375,15 +375,18 @@ RSpec.describe WebHookService do
         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 @@ RSpec.describe WebHookService do
 
         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