Merge branch 'security-cancel-pipelines-for-deleted-project' into 'master'

Cancel alive jobs on project deletion [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/security/gitlab!1220

app/models/ci/pipeline.rb

@@ -16,6 +16,7 @@ module Ci
     include ShaAttribute
     include FromUnion
     include UpdatedAtFilterable
+    include EachBatch
 
     MAX_OPEN_MERGE_REQUESTS_REFS = 4
 

app/models/commit_status.rb

@@ -55,6 +55,7 @@ class CommitStatus < ApplicationRecord
   scope :for_ids, -> (ids) { where(id: ids) }
   scope :for_ref, -> (ref) { where(ref: ref) }
   scope :by_name, -> (name) { where(name: name) }
+  scope :in_pipelines, ->(pipelines) { where(pipeline: pipelines) }
 
   scope :for_project_paths, -> (paths) do
     where(project: Project.where_full_path_in(Array(paths)))

app/services/ci/abort_project_pipelines_service.rb

+# frozen_string_literal: true
+
+module Ci
+  class AbortProjectPipelinesService
+    # Danger: Cancels in bulk without callbacks
+    # Only for pipeline abandonment scenarios (current example: project delete)
+    def execute(project)
+      return unless Feature.enabled?(:abort_deleted_project_pipelines, default_enabled: :yaml)
+
+      pipelines = project.all_pipelines.cancelable
+      bulk_abort!(pipelines, status: :canceled)
+
+      ServiceResponse.success(message: 'Pipelines canceled')
+    end
+
+    private
+
+    def bulk_abort!(pipelines, status:)
+      pipelines.each_batch do |pipeline_batch|
+        CommitStatus.in_pipelines(pipeline_batch).in_batches.update_all(status: status) # rubocop: disable Cop/InBatches
+        pipeline_batch.update_all(status: status)
+      end
+    end
+  end
+end

app/services/ci/cancel_user_pipelines_service.rb

@@ -6,6 +6,7 @@ module Ci
     # This is a bug with CodeReuse/ActiveRecord cop
     # https://gitlab.com/gitlab-org/gitlab/issues/32332
     def execute(user)
+      # TODO: fix N+1 queries https://gitlab.com/gitlab-org/gitlab/-/issues/300685
       user.pipelines.cancelable.find_each(&:cancel_running)
 
       ServiceResponse.success(message: 'Pipeline canceled')

app/services/projects/destroy_service.rb

@@ -21,11 +21,14 @@ module Projects
     def execute
       return false unless can?(current_user, :remove_project, project)
 
+      project.update_attribute(:pending_delete, true)
       # Flush the cache for both repositories. This has to be done _before_
       # removing the physical repositories as some expiration code depends on
       # Git data (e.g. a list of branch names).
       flush_caches(project)
 
+      ::Ci::AbortProjectPipelinesService.new.execute(project)
+
       Projects::UnlinkForkService.new(project, current_user).execute
 
       attempt_destroy(project)

changelogs/unreleased/security-cancel-pipelines-for-deleted-project.yml

+---
+title: Cancel running and pending jobs when a project is deleted
+merge_request: 1220
+author:
+type: security

config/feature_flags/development/abort_deleted_project_pipelines.yml

+---
+name: abort_deleted_project_pipelines
+introduced_by_url: https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/1220
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/301106
+milestone: '13.9'
+type: development
+group: group::continuous integration
+default_enabled: true

lib/gitlab/ci/pipeline/chain/validate/abilities.rb

@@ -10,6 +10,10 @@ module Gitlab
             include Chain::Helpers
 
             def perform!
+              if project.pending_delete?
+                return error('Project is deleted!')
+              end
+
               unless project.builds_enabled?
                 return error('Pipelines are disabled!')
               end

spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb

@@ -74,6 +74,14 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do
     it 'does not break the chain' do
       expect(step.break?).to eq false
     end
+
+    context 'when project is deleted' do
+      before do
+        project.update!(pending_delete: true)
+      end
+
+      specify { expect(step.perform!).to contain_exactly('Project is deleted!') }
+    end
   end
 
   describe '#allowed_to_write_ref?' do

spec/models/ci/pipeline_spec.rb

@@ -34,6 +34,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
   it { is_expected.to have_many(:auto_canceled_jobs) }
   it { is_expected.to have_many(:sourced_pipelines) }
   it { is_expected.to have_many(:triggered_pipelines) }
+  it { is_expected.to have_many(:pipeline_artifacts) }
 
   it { is_expected.to have_one(:chat_data) }
   it { is_expected.to have_one(:source_pipeline) }
@@ -41,14 +42,15 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
   it { is_expected.to have_one(:source_job) }
   it { is_expected.to have_one(:pipeline_config) }
 
-  it { is_expected.to validate_presence_of(:sha) }
-  it { is_expected.to validate_presence_of(:status) }
-
   it { is_expected.to respond_to :git_author_name }
   it { is_expected.to respond_to :git_author_email }
   it { is_expected.to respond_to :short_sha }
   it { is_expected.to delegate_method(:full_path).to(:project).with_prefix }
-  it { is_expected.to have_many(:pipeline_artifacts) }
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:sha) }
+    it { is_expected.to validate_presence_of(:status) }
+  end
 
   describe 'associations' do
     it 'has a bidirectional relationship with projects' do

spec/services/ci/abort_project_pipelines_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::AbortProjectPipelinesService do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:pipeline) { create(:ci_pipeline, :running, project: project) }
+  let_it_be(:build) { create(:ci_build, :running, pipeline: pipeline) }
+
+  describe '#execute' do
+    it 'cancels all running pipelines and related jobs' do
+      result = described_class.new.execute(project)
+
+      expect(result).to be_success
+      expect(pipeline.reload).to be_canceled
+      expect(build.reload).to be_canceled
+    end
+
+    it 'avoids N+1 queries' do
+      control_count = ActiveRecord::QueryRecorder.new { described_class.new.execute(project) }.count
+
+      pipelines = create_list(:ci_pipeline, 5, :running, project: project)
+      create_list(:ci_build, 5, :running, pipeline: pipelines.first)
+
+      expect { described_class.new.execute(project) }.not_to exceed_query_limit(control_count)
+    end
+  end
+
+  context 'when feature disabled' do
+    before do
+      stub_feature_flags(abort_deleted_project_pipelines: false)
+    end
+
+    it 'does not abort the pipeline' do
+      result = described_class.new.execute(project)
+
+      expect(result).to be(nil)
+      expect(pipeline.reload).to be_running
+      expect(build.reload).to be_running
+    end
+  end
+end

spec/services/projects/destroy_service_spec.rb

@@ -69,6 +69,12 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do
       destroy_project(project, user, {})
     end
 
+    it 'performs cancel for project ci pipelines' do
+      expect(::Ci::AbortProjectPipelinesService).to receive_message_chain(:new, :execute).with(project)
+
+      destroy_project(project, user, {})
+    end
+
     context 'when project has remote mirrors' do
       let!(:project) do
         create(:project, :repository, namespace: user.namespace).tap do |project|