diff --git a/app/events/ci/job_artifacts_deleted_event.rb b/app/events/ci/job_artifacts_deleted_event.rb new file mode 100644 index 0000000000000000000000000000000000000000..2972342cae6969fe4d13ee1f849348dd523d63b2 --- /dev/null +++ b/app/events/ci/job_artifacts_deleted_event.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module Ci + class JobArtifactsDeletedEvent < ::Gitlab::EventStore::Event + def schema + { + 'type' => 'object', + 'required' => ['job_ids'], + 'properties' => { + 'job_ids' => { + 'type' => 'array', + 'properties' => { + 'job_id' => { 'type' => 'integer' } + } + } + } + } + end + end +end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index c24a72db454e8b4fd7be6b3f06bf8b8d33bcbf81..401471d02d9f6b2af1b9f8dad81aa1997793e43d 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -413,6 +413,8 @@ - 1 - - security_auto_fix - 1 +- - security_findings_delete_by_job_id + - 1 - - security_scans - 2 - - self_monitoring_project_create diff --git a/ee/app/services/ee/ci/job_artifacts/destroy_batch_service.rb b/ee/app/services/ee/ci/job_artifacts/destroy_batch_service.rb index d6ab9f0bfd1451fd26b1f00beaa05b2df955c890..852a8719a86a239a667fead77b7f856726809ff5 100644 --- a/ee/app/services/ee/ci/job_artifacts/destroy_batch_service.rb +++ b/ee/app/services/ee/ci/job_artifacts/destroy_batch_service.rb @@ -8,22 +8,18 @@ module DestroyBatchService private - override :destroy_related_records - def destroy_related_records(artifacts) - ::Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/346236') do - destroy_security_findings(artifacts) - end - end - override :after_batch_destroy_hook def after_batch_destroy_hook(artifacts) - insert_geo_event_records(artifacts) - end - - def destroy_security_findings(artifacts) - job_ids = artifacts.map(&:job_id) + # This DestroyBatchService is used from different services. + # One of them is when pipeline is destroyed, and then eventually call DestroyBatchService via DestroyAssociationsService. + # So in this case even if it is invoked after a transaction but it is still under Ci::Pipeline.transaction. + Sidekiq::Worker.skipping_transaction_check do + ::Gitlab::EventStore.publish( + ::Ci::JobArtifactsDeletedEvent.new(data: { job_ids: artifacts.map(&:job_id) }) + ) + end - ::Security::Finding.by_build_ids(job_ids).delete_all + insert_geo_event_records(artifacts) end def insert_geo_event_records(artifacts) diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index f068e3c7eabbb19b5967527957626df14e5bef92..c91c630329742a08542040e93c4107d24606c3d3 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -1282,6 +1282,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: security_findings_delete_by_job_id + :worker_name: Security::Findings::DeleteByJobIdWorker + :feature_category: :vulnerability_management + :has_external_dependencies: + :urgency: :low + :resource_boundary: :cpu + :weight: 1 + :idempotent: true + :tags: [] - :name: set_user_status_based_on_user_cap_setting :worker_name: SetUserStatusBasedOnUserCapSettingWorker :feature_category: :users diff --git a/ee/app/workers/security/findings/delete_by_job_id_worker.rb b/ee/app/workers/security/findings/delete_by_job_id_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..fe295a1c8b2dae482ee3fb03420b3964a77b9c5b --- /dev/null +++ b/ee/app/workers/security/findings/delete_by_job_id_worker.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Security + module Findings + class DeleteByJobIdWorker + include ApplicationWorker + include Gitlab::EventStore::Subscriber + + feature_category :vulnerability_management + urgency :low + worker_resource_boundary :cpu + data_consistency :always + + idempotent! + + def handle_event(event) + ::Security::Finding.by_build_ids(event.data[:job_ids]).delete_all + end + end + end +end diff --git a/ee/lib/ee/gitlab/event_store.rb b/ee/lib/ee/gitlab/event_store.rb index 1d29fc4b3e29de43839c2e1772b34d9e1ce8a52d..7389a88bcd5da74ae10e923e6a0ceef4b3a54805 100644 --- a/ee/lib/ee/gitlab/event_store.rb +++ b/ee/lib/ee/gitlab/event_store.rb @@ -24,6 +24,7 @@ def configure!(store) # Add EE only subscriptions here: store.subscribe ::GitlabSubscriptions::NotifySeatsExceededWorker, to: ::Members::MembersAddedEvent + store.subscribe ::Security::Findings::DeleteByJobIdWorker, to: ::Ci::JobArtifactsDeletedEvent end end end diff --git a/ee/spec/lib/gitlab/event_store_spec.rb b/ee/spec/lib/gitlab/event_store_spec.rb index ae0fa2c0cb8171e98e3129917e74bab2c9f60ed3..684ad264c4d5d65f821d211b6827721350a1fef5 100644 --- a/ee/spec/lib/gitlab/event_store_spec.rb +++ b/ee/spec/lib/gitlab/event_store_spec.rb @@ -9,7 +9,8 @@ expect(instance.subscriptions.keys).to include( ::Ci::PipelineCreatedEvent, - ::Members::MembersAddedEvent + ::Members::MembersAddedEvent, + ::Ci::JobArtifactsDeletedEvent ) end end diff --git a/ee/spec/services/ee/ci/job_artifacts/destroy_all_expired_service_spec.rb b/ee/spec/services/ee/ci/job_artifacts/destroy_all_expired_service_spec.rb index 4c1386a7c23b1bb7ea6937cdea1933885c3acce7..b48bd783f20707c02ab60656f63e2b8a5fc31a48 100644 --- a/ee/spec/services/ee/ci/job_artifacts/destroy_all_expired_service_spec.rb +++ b/ee/spec/services/ee/ci/job_artifacts/destroy_all_expired_service_spec.rb @@ -20,11 +20,25 @@ context 'when artifact is expired' do context 'when artifact is not locked' do let!(:artifact) { create(:ci_job_artifact, :expired, job: job, locked: job.pipeline.locked) } + let(:event_data) { { job_ids: [artifact.job_id] } } - it 'destroys job artifact and the security finding' do + it 'destroys job artifact and the security finding', :sidekiq_inline do expect { subject }.to change { Ci::JobArtifact.count }.by(-1) .and change { Security::Finding.count }.by(-1) end + + it 'publishes Ci::JobArtifactsDeletedEvent' do + event = double(:event) + + expect(Ci::JobArtifactsDeletedEvent) + .to receive(:new) + .with(data: event_data) + .and_return(event) + + expect(Gitlab::EventStore).to receive(:publish).with(event) + + subject + end end context 'when artifact is locked' do @@ -55,23 +69,6 @@ end end - context 'when failed to destroy artifact' do - before do - stub_const('Ci::JobArtifacts::DestroyAllExpiredService::LOOP_LIMIT', 10) - expect(Ci::DeletedObject) - .to receive(:bulk_import) - .once - .and_raise(ActiveRecord::RecordNotDestroyed) - end - - let!(:artifact) { create(:ci_job_artifact, :expired, job: job, locked: job.pipeline.locked) } - - it 'raises an exception but destroys the security_finding object regardless' do - expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed) - .and change { Security::Finding.count }.by(-1) - end - end - context 'when there are artifacts more than batch sizes' do before do stub_const('Ci::JobArtifacts::DestroyAllExpiredService::BATCH_SIZE', 1) @@ -83,13 +80,13 @@ let!(:second_security_scan) { create(:security_scan, build: second_job) } let!(:second_security_finding) { create(:security_finding, scan: second_security_scan) } - it 'destroys all expired artifacts' do + it 'destroys all expired artifacts', :sidekiq_inline do expect { subject }.to change { Ci::JobArtifact.count }.by(-2) .and change { Security::Finding.count }.by(-2) end end - context 'when some artifacts are locked' do + context 'when some artifacts are locked', :sidekiq_inline do let!(:artifact) { create(:ci_job_artifact, :expired, job: job, locked: job.pipeline.locked) } let!(:second_artifact) { create(:ci_job_artifact, :expired, job: locked_job, locked: locked_job.pipeline.locked) } diff --git a/ee/spec/services/ee/ci/job_artifacts/destroy_batch_service_spec.rb b/ee/spec/services/ee/ci/job_artifacts/destroy_batch_service_spec.rb index 1f1888edaff9dbea0c2a650c31851cd1a6d8b726..458e5364c825b25c0eebcf1ec2957d265725992d 100644 --- a/ee/spec/services/ee/ci/job_artifacts/destroy_batch_service_spec.rb +++ b/ee/spec/services/ee/ci/job_artifacts/destroy_batch_service_spec.rb @@ -13,12 +13,26 @@ let_it_be(:artifact) { create(:ci_job_artifact) } let_it_be(:security_scan) { create(:security_scan, build: artifact.job) } let_it_be(:security_finding) { create(:security_finding, scan: security_scan) } + let_it_be(:event_data) { { job_ids: [artifact.job_id] } } - it 'destroys all expired artifacts' do + it 'destroys all expired artifacts', :sidekiq_inline do expect { subject }.to change { Ci::JobArtifact.count }.by(-1) .and change { Security::Finding.count }.from(1).to(0) end + it 'publishes Ci::JobArtifactsDeletedEvent' do + event = double(:event) + + expect(Ci::JobArtifactsDeletedEvent) + .to receive(:new) + .with(data: event_data) + .and_return(event) + + expect(Gitlab::EventStore).to receive(:publish).with(event) + + subject + end + context 'with Geo replication' do let_it_be(:primary) { create(:geo_node, :primary) } let_it_be(:secondary) { create(:geo_node) } diff --git a/ee/spec/workers/security/findings/delete_by_job_id_worker_spec.rb b/ee/spec/workers/security/findings/delete_by_job_id_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..b1cd900bc2f79fbaa5d4c2fb72f087489ea09da0 --- /dev/null +++ b/ee/spec/workers/security/findings/delete_by_job_id_worker_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::Findings::DeleteByJobIdWorker do + let_it_be(:security_scan1) { create(:security_scan) } + let_it_be(:security_scan2) { create(:security_scan) } + let_it_be(:security_finding_to_be_deleted) { create(:security_finding, scan: security_scan1) } + let_it_be(:security_finding_not_to_be_deleted) { create(:security_finding, scan: security_scan2) } + + let(:event) { Ci::PipelineCreatedEvent.new(data: { job_ids: [security_scan1.build_id] }) } + + subject { consume_event(event) } + + def consume_event(event) + described_class.new.perform(event.class.name, event.data) + end + + it 'destroys all expired artifacts' do + expect { subject }.to change { Security::Finding.count }.from(2).to(1) + end +end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 0beb5157ed6b6127006302a001b168abf575e5ab..d60ec8c29582d2adb4c2e2866a62c883a0cd3f77 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -15,6 +15,7 @@ before do stub_container_registry_config(enabled: true) stub_container_registry_tags(repository: :any, tags: []) + allow(Gitlab::EventStore).to receive(:publish) end shared_examples 'deleting the project' do @@ -42,7 +43,7 @@ end it 'does not publish an event' do - expect(Gitlab::EventStore).not_to receive(:publish) + expect(Gitlab::EventStore).not_to receive(:publish).with(event_type(Projects::ProjectDeletedEvent)) destroy_project(project, user, {}) end