Merge branch '31009-limit-project-hooks-services' into 'master'

Don't execute webhooks/services when above limit

See merge request gitlab-org/gitlab!17874

app/helpers/application_settings_helper.rb

@@ -289,7 +289,8 @@ module ApplicationSettingsHelper
       :snowplow_collector_hostname,
       :snowplow_cookie_domain,
       :snowplow_enabled,
-      :snowplow_site_id
+      :snowplow_site_id,
+      :push_event_hooks_limit
     ]
   end
 

app/models/application_setting.rb

@@ -214,6 +214,9 @@ class ApplicationSetting < ApplicationRecord
             length: { maximum: 100, message: N_('is too long (maximum is 100 entries)') },
             allow_nil: false
 
+  validates :push_event_hooks_limit,
+            numericality: { greater_than_or_equal_to: 0 }
+
   SUPPORTED_KEY_TYPES.each do |type|
     validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type }
   end

app/models/application_setting_implementation.rb

@@ -82,6 +82,7 @@ module ApplicationSettingImplementation
         polling_interval_multiplier: 1,
         project_export_enabled: true,
         protected_ci_variables: false,
+        push_event_hooks_limit: 3,
         raw_blob_request_limit: 300,
         recaptcha_enabled: false,
         login_recaptcha_protection_enabled: false,

app/services/git/base_hooks_service.rb

@@ -62,6 +62,8 @@ module Git
     end
 
     def execute_project_hooks
+      return unless params.fetch(:execute_project_hooks, true)
+
       # Creating push_data invokes one CommitDelta RPC per commit. Only
       # build this data if we actually need it.
       project.execute_hooks(push_data, hook_name) if project.has_active_hooks?(hook_name)

app/services/git/process_ref_changes_service.rb

@@ -17,7 +17,7 @@ module Git
       changes_by_action = group_changes_by_action(changes)
 
       changes_by_action.each do |_, changes|
-        process_changes(ref_type, changes) if changes.any?
+        process_changes(ref_type, changes, execute_project_hooks: execute_project_hooks?(changes)) if changes.any?
       end
     end
 
@@ -34,7 +34,11 @@ module Git
       :pushed
     end
 
-    def process_changes(ref_type, changes)
+    def execute_project_hooks?(changes)
+      (changes.size <= Gitlab::CurrentSettings.push_event_hooks_limit) || Feature.enabled?(:git_push_execute_all_project_hooks, project)
+    end
+
+    def process_changes(ref_type, changes, execute_project_hooks:)
       push_service_class = push_service_class_for(ref_type)
 
       changes.each do |change|
@@ -43,7 +47,8 @@ module Git
           current_user,
           change: change,
           push_options: params[:push_options],
-          create_pipelines: change[:index] < PIPELINE_PROCESS_LIMIT || Feature.enabled?(:git_push_create_all_pipelines, project)
+          create_pipelines: change[:index] < PIPELINE_PROCESS_LIMIT || Feature.enabled?(:git_push_create_all_pipelines, project),
+          execute_project_hooks: execute_project_hooks
         ).execute
       end
     end

app/views/admin/application_settings/_performance.html.haml

@@ -20,5 +20,10 @@
       = f.number_field :raw_blob_request_limit, class: 'form-control'
       .form-text.text-muted
         = _('Highest number of requests per minute for each raw path, default to 300. To disable throttling set to 0.')
+    .form-group
+      = f.label :push_event_hooks_limit, class: 'label-bold'
+      = f.number_field :push_event_hooks_limit, class: 'form-control'
+      .form-text.text-muted
+        = _("Number of changes (branches or tags) in a single push to determine whether webhooks and services will be fired or not. Webhooks and services won't be submitted if it surpasses that value.")
 
   = f.submit 'Save changes', class: "btn btn-success"

changelogs/unreleased/31009-limit-project-hooks-services.yml

+---
+title: Don't execute webhooks/services when above limit
+merge_request: 17874
+author:
+type: performance

db/migrate/20191003060227_add_push_event_hooks_limit_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddPushEventHooksLimitToApplicationSettings < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:application_settings, :push_event_hooks_limit, :integer, default: 3)
+  end
+
+  def down
+    remove_column(:application_settings, :push_event_hooks_limit)
+  end
+end

db/schema.rb

@@ -338,6 +338,7 @@ ActiveRecord::Schema.define(version: 2019_10_16_072826) do
     t.boolean "throttle_incident_management_notification_enabled", default: false, null: false
     t.integer "throttle_incident_management_notification_period_in_seconds", default: 3600
     t.integer "throttle_incident_management_notification_per_period", default: 3600
+    t.integer "push_event_hooks_limit", default: 3, null: false
     t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id"
     t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id"
     t.index ["instance_administration_project_id"], name: "index_applicationsettings_on_instance_administration_project_id"

doc/api/settings.md

@@ -289,6 +289,7 @@ are listed in the descriptions of the relevant settings.
 | `prometheus_metrics_enabled`             | boolean          | no                                   | Enable Prometheus metrics. |
 | `protected_ci_variables`                 | boolean          | no                                   | Environment variables are protected by default. |
 | `pseudonymizer_enabled`                  | boolean          | no                                   | **(PREMIUM)** When enabled, GitLab will run a background job that will produce pseudonymized CSVs of the GitLab database that will be uploaded to your configured object storage directory.
+| `push_event_hooks_limit`                 | integer          | no                                   | Number of changes (branches or tags) in a single push to determine whether webhooks and services will be fired or not. Webhooks and services won't be submitted if it surpasses that value. |
 | `recaptcha_enabled`                      | boolean          | no                                   | (**If enabled, requires:** `recaptcha_private_key` and `recaptcha_site_key`) Enable reCAPTCHA. |
 | `recaptcha_private_key`                  | string           | required by: `recaptcha_enabled`     | Private key for reCAPTCHA. |
 | `recaptcha_site_key`                     | string           | required by: `recaptcha_enabled`     | Site key for reCAPTCHA. |

doc/user/project/integrations/project_services.md

@@ -56,6 +56,16 @@ Click on the service links to see further configuration instructions and details
 | [Redmine](redmine.md) | Redmine issue tracker |
 | [YouTrack](youtrack.md) | YouTrack issue tracker |
 
+## Push hooks limit
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/31009) in GitLab 12.4.
+
+If a single push includes changes to more than three branches or tags, services
+supported by `push_hooks` and `tag_push_hooks` events won't be executed.
+
+The number of branches or tags supported can be changed via
+[`push_event_hooks_limit` application setting](../../../api/settings.md#list-of-settings-that-can-be-accessed-via-api-calls).
+
 ## Services templates
 
 Services templates is a way to set some predefined values in the Service of

doc/user/project/integrations/webhooks.md

@@ -107,6 +107,9 @@ detailed commit data is expensive. Note that despite only 20 commits being
 present in the `commits` attribute, the `total_commits_count` attribute will
 contain the actual total.
 
+Also, if a single push includes changes for more than three (by default, depending on
+[`push_event_hooks_limit` setting](../../../api/settings.md#list-of-settings-that-can-be-accessed-via-api-calls)) branches, this hook won't be executed.
+
 **Request header**:
 
 ```
@@ -190,6 +193,10 @@ X-Gitlab-Event: Push Hook
 
 Triggered when you create (or delete) tags to the repository.
 
+NOTE: **Note:**
+If a single push includes changes for more than three (by default, depending on
+[`push_event_hooks_limit` setting](../../../api/settings.md#list-of-settings-that-can-be-accessed-via-api-calls)) tags, this hook won't be executed.
+
 **Request header**:
 
 ```

lib/api/settings.rb

@@ -101,6 +101,7 @@ module API
       optional :polling_interval_multiplier, type: BigDecimal, desc: 'Interval multiplier used by endpoints that perform polling. Set to 0 to disable polling.'
       optional :project_export_enabled, type: Boolean, desc: 'Enable project export'
       optional :prometheus_metrics_enabled, type: Boolean, desc: 'Enable Prometheus metrics'
+      optional :push_event_hooks_limit, type: Integer, desc: "Number of changes (branches or tags) in a single push to determine whether webhooks and services will be fired or not. Webhooks and services won't be submitted if it surpasses that value."
       optional :recaptcha_enabled, type: Boolean, desc: 'Helps prevent bots from creating accounts'
       given recaptcha_enabled: ->(val) { val } do
         requires :recaptcha_site_key, type: String, desc: 'Generate site key at http://www.google.com/recaptcha'

locale/gitlab.pot

@@ -11193,6 +11193,9 @@ msgstr ""
 msgid "Number of LOCs per commit"
 msgstr ""
 
+msgid "Number of changes (branches or tags) in a single push to determine whether webhooks and services will be fired or not. Webhooks and services won't be submitted if it surpasses that value."
+msgstr ""
+
 msgid "Number of commits per MR"
 msgstr ""
 

spec/models/application_setting_spec.rb

@@ -56,6 +56,10 @@ describe ApplicationSetting do
     it { is_expected.not_to allow_value(nil).for(:protected_paths) }
     it { is_expected.to allow_value([]).for(:protected_paths) }
 
+    it { is_expected.to allow_value(3).for(:push_event_hooks_limit) }
+    it { is_expected.not_to allow_value('three').for(:push_event_hooks_limit) }
+    it { is_expected.not_to allow_value(nil).for(:push_event_hooks_limit) }
+
     context "when user accepted let's encrypt terms of service" do
       before do
         setting.update(lets_encrypt_terms_of_service_accepted: true)

spec/requests/api/settings_spec.rb

@@ -72,7 +72,8 @@ describe API::Settings, 'Settings' do
             default_branch_protection: ::Gitlab::Access::PROTECTION_DEV_CAN_MERGE,
             local_markdown_version: 3,
             allow_local_requests_from_web_hooks_and_services: true,
-            allow_local_requests_from_system_hooks: false
+            allow_local_requests_from_system_hooks: false,
+            push_event_hooks_limit: 2
           }
 
         expect(response).to have_gitlab_http_status(200)
@@ -102,6 +103,7 @@ describe API::Settings, 'Settings' do
         expect(json_response['local_markdown_version']).to eq(3)
         expect(json_response['allow_local_requests_from_web_hooks_and_services']).to eq(true)
         expect(json_response['allow_local_requests_from_system_hooks']).to eq(false)
+        expect(json_response['push_event_hooks_limit']).to eq(2)
       end
     end
 

spec/services/git/base_hooks_service_spec.rb

@@ -8,7 +8,6 @@ describe Git::BaseHooksService do
 
   let(:user) { create(:user) }
   let(:project) { create(:project, :repository) }
-
   let(:oldrev) { Gitlab::Git::BLANK_SHA }
   let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0
   let(:ref) { 'refs/tags/v1.1.0' }
@@ -26,7 +25,17 @@ describe Git::BaseHooksService do
 
     let(:project) { create(:project, :repository) }
 
-    subject { TestService.new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }) }
+    let(:params) do
+      {
+        change: {
+          oldrev: oldrev,
+          newrev: newrev,
+          ref: ref
+        }
+      }
+    end
+
+    subject { TestService.new(project, user, params) }
 
     context '#execute_hooks' do
       before do
@@ -83,5 +92,21 @@ describe Git::BaseHooksService do
         end
       end
     end
+
+    context 'execute_project_hooks param set to false' do
+      before do
+        params[:execute_project_hooks] = false
+
+        allow(project).to receive(:has_active_hooks?).and_return(true)
+        allow(project).to receive(:has_active_services?).and_return(true)
+      end
+
+      it 'does not execute hooks and services' do
+        expect(project).not_to receive(:execute_hooks)
+        expect(project).not_to receive(:execute_services)
+
+        subject.execute
+      end
+    end
   end
 end

spec/services/git/process_ref_changes_service_spec.rb

@@ -28,12 +28,66 @@ describe Git::ProcessRefChangesService do
     it "calls #{push_service_class}" do
       expect(push_service_class)
         .to receive(:new)
+        .with(project, project.owner, hash_including(execute_project_hooks: true))
         .exactly(changes.count).times
         .and_return(service)
 
       subject.execute
     end
 
+    context 'changes exceed push_event_hooks_limit' do
+      def multiple_changes(change, count)
+        Array.new(count).map.with_index do |n, index|
+          { index: index, oldrev: change[:oldrev], newrev: change[:newrev], ref: "#{change[:ref]}#{n}" }
+        end
+      end
+
+      let(:push_event_hooks_limit) { 3 }
+
+      let(:changes) do
+        multiple_changes(
+          { oldrev: '123456', newrev: '789012', ref: "#{ref_prefix}/test" },
+          push_event_hooks_limit + 1
+        )
+      end
+
+      before do
+        stub_application_setting(push_event_hooks_limit: push_event_hooks_limit)
+      end
+
+      context 'git_push_execute_all_project_hooks is disabled' do
+        before do
+          stub_feature_flags(git_push_execute_all_project_hooks: false)
+        end
+
+        it "calls #{push_service_class} with execute_project_hooks set to false" do
+          expect(push_service_class)
+            .to receive(:new)
+            .with(project, project.owner, hash_including(execute_project_hooks: false))
+            .exactly(changes.count).times
+            .and_return(service)
+
+          subject.execute
+        end
+      end
+
+      context 'git_push_execute_all_project_hooks is enabled' do
+        before do
+          stub_feature_flags(git_push_execute_all_project_hooks: true)
+        end
+
+        it "calls #{push_service_class} with execute_project_hooks set to true" do
+          expect(push_service_class)
+            .to receive(:new)
+            .with(project, project.owner, hash_including(execute_project_hooks: true))
+            .exactly(changes.count).times
+            .and_return(service)
+
+          subject.execute
+        end
+      end
+    end
+
     context 'pipeline creation' do
       context 'with valid .gitlab-ci.yml' do
         before do

spec/workers/post_receive_spec.rb

@@ -93,6 +93,8 @@ describe PostReceive do
     end
 
     context 'with changes' do
+      let(:push_service) { double(execute: true) }
+
       before do
         allow_any_instance_of(Gitlab::GitPostReceive).to receive(:identify).and_return(project.owner)
         allow(Gitlab::GlRepository).to receive(:parse).and_return([project, Gitlab::GlRepository::PROJECT])