From 1a231d0e573fc5e3a6597c01514bebe0c841f18c Mon Sep 17 00:00:00 2001 From: Subashis Date: Mon, 7 Feb 2022 11:03:08 -0700 Subject: [PATCH] Move findings deletion into a worker - Add event for artifacts deletion - Add subscriber - Add worker to delete realted findings - Update related specs Changelog: changed MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80096 EE: true --- app/events/ci/job_artifacts_deleted_event.rb | 20 ++++++++++ config/sidekiq_queues.yml | 2 + .../ci/job_artifacts/destroy_batch_service.rb | 22 +++++------ ee/app/workers/all_queues.yml | 9 +++++ .../findings/delete_by_job_id_worker.rb | 21 +++++++++++ ee/lib/ee/gitlab/event_store.rb | 1 + ee/spec/lib/gitlab/event_store_spec.rb | 3 +- .../destroy_all_expired_service_spec.rb | 37 +++++++++---------- .../destroy_batch_service_spec.rb | 16 +++++++- .../findings/delete_by_job_id_worker_spec.rb | 22 +++++++++++ .../services/projects/destroy_service_spec.rb | 3 +- 11 files changed, 120 insertions(+), 36 deletions(-) create mode 100644 app/events/ci/job_artifacts_deleted_event.rb create mode 100644 ee/app/workers/security/findings/delete_by_job_id_worker.rb create mode 100644 ee/spec/workers/security/findings/delete_by_job_id_worker_spec.rb 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 00000000000000..2972342cae6969 --- /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 c24a72db454e8b..401471d02d9f6b 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 d6ab9f0bfd1451..852a8719a86a23 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 f068e3c7eabbb1..c91c630329742a 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 00000000000000..fe295a1c8b2dae --- /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 1d29fc4b3e29de..7389a88bcd5da7 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 ae0fa2c0cb8171..684ad264c4d5d6 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 4c1386a7c23b1b..b48bd783f20707 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 1f1888edaff9db..458e5364c825b2 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 00000000000000..b1cd900bc2f79f --- /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 0beb5157ed6b61..d60ec8c29582d2 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 -- GitLab