From dbb3899c45782dc9c979dea5bea17824e36fdf7a Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Mon, 30 Jun 2025 13:05:39 +0300 Subject: [PATCH 01/71] Add scan ingestion when all sec jobs completed --- ee/app/models/ee/ci/build.rb | 26 ++++++++++++++++++++++++++ ee/app/models/ee/ci/pipeline.rb | 32 +++++++++++++++++++++++++------- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 68bb62d6bfa1af..80b21a76002973 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -73,6 +73,24 @@ module Build ::Ci::Minutes::UpdateBuildMinutesService.new(build.project, nil).execute(build) end end + + after_transition any => [:success, :failed, :canceled] do |build| + if build.security_job? + build.run_after_commit do + if !::Security::Scan.exists?(build_id: build.id) && + build.pipeline.completed_security_jobs? + + if build.pipeline.could_store_security_reports? + ::Security::StoreScansWorker.perform_async(build.pipeline.id) + ::Security::ProcessScanEventsWorker.perform_async(build.pipeline.id) + else + ::Sbom::ScheduleIngestReportsService.new(build.pipeline).execute + ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: build.pipeline.id) + end + end + end + end + end end end @@ -203,6 +221,14 @@ def pages end strong_memoize_attr :pages + def security_job? + (::EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types & (reports_file_types || [])).any? + end + + def reports_file_types + metadata.config_options.dig(:artifacts, :reports)&.keys&.map(&:to_s) + end + private def expand_pages_variables diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index 48acf7741e4bac..dfad54ef9faa4c 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -74,13 +74,15 @@ def self.latest_limited_pipeline_ids_per_source(pipelines, sha) end after_transition any => ::Ci::Pipeline.completed_with_manual_statuses do |pipeline| - pipeline.run_after_commit do - if pipeline.can_store_security_reports? - ::Security::StoreScansWorker.perform_async(pipeline.id) - ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) - else - ::Sbom::ScheduleIngestReportsService.new(pipeline).execute - ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) + unless pipeline.has_security_scans? + pipeline.run_after_commit do + if pipeline.can_store_security_reports? + ::Security::StoreScansWorker.perform_async(pipeline.id) + ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) + else + ::Sbom::ScheduleIngestReportsService.new(pipeline).execute + ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) + end end end end @@ -267,6 +269,10 @@ def can_store_security_reports? project.can_store_security_reports? && has_security_reports? end + def could_store_security_reports? + project.can_store_security_reports? && has_security_jobs? + end + # We want all the `security_findings` records for a particular pipeline to be stored in # the same partition, therefore, we check if the pipeline already has a `security_scan`. # @@ -334,6 +340,18 @@ def has_security_reports? complete_or_manual_and_has_reports?(::Ci::JobArtifact.with_file_types(security_and_license_scanning_file_types)) end + def has_security_jobs? + builds.select(&:security_job?).present? + end + + def has_security_scans? + builds.select(&:security_job?).any? { |build| ::Security::Scan.exists?(build_id: build.id) } + end + + def completed_security_jobs? + builds.select(&:security_job?).all? { |build| build.status.in?(::Ci::Build::COMPLETED_STATUSES) } + end + def has_all_security_policies_reports? can_store_security_reports? && can_ingest_sbom_reports? end -- GitLab From a04f1bcca5507c332a2530cbeae5ba687f512652 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 19 Jun 2025 15:37:00 +0300 Subject: [PATCH 02/71] Ingest security scans when security job completed --- ee/app/models/ee/ci/build.rb | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 80b21a76002973..850f5aa1b2ba12 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -75,18 +75,15 @@ module Build end after_transition any => [:success, :failed, :canceled] do |build| - if build.security_job? + reports = build.metadata.config_options.dig(:artifacts, :reports) + if reports && ::EE::Enums::Ci::JobArtifact.security_report_file_types.any? { |type| reports.key?(type.to_sym) } build.run_after_commit do - if !::Security::Scan.exists?(build_id: build.id) && - build.pipeline.completed_security_jobs? - - if build.pipeline.could_store_security_reports? - ::Security::StoreScansWorker.perform_async(build.pipeline.id) - ::Security::ProcessScanEventsWorker.perform_async(build.pipeline.id) - else - ::Sbom::ScheduleIngestReportsService.new(build.pipeline).execute - ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: build.pipeline.id) - end + if build.pipeline.can_store_security_reports? + ::Security::StoreScansWorker.perform_async(build.pipeline.id) + ::Security::ProcessScanEventsWorker.perform_async(build.pipeline.id) + else + ::Sbom::ScheduleIngestReportsService.new(build.pipeline).execute + ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: build.pipeline.id) end end end -- GitLab From c383292f5406946131621bc851aaf991854fd10f Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 1 Jul 2025 10:24:44 +0000 Subject: [PATCH 03/71] Use any instead of select..present? for earlier response --- ee/app/models/ee/ci/pipeline.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index dfad54ef9faa4c..e5df7a8add8fa3 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -341,7 +341,7 @@ def has_security_reports? end def has_security_jobs? - builds.select(&:security_job?).present? + builds.any?(&:security_job?) end def has_security_scans? -- GitLab From 2d64c90cb7eaf2ef17f13979486297421c6a479d Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 1 Jul 2025 17:52:01 +0300 Subject: [PATCH 04/71] Add all sec jobs completed condition --- ee/app/models/ee/ci/build.rb | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 850f5aa1b2ba12..80b21a76002973 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -75,15 +75,18 @@ module Build end after_transition any => [:success, :failed, :canceled] do |build| - reports = build.metadata.config_options.dig(:artifacts, :reports) - if reports && ::EE::Enums::Ci::JobArtifact.security_report_file_types.any? { |type| reports.key?(type.to_sym) } + if build.security_job? build.run_after_commit do - if build.pipeline.can_store_security_reports? - ::Security::StoreScansWorker.perform_async(build.pipeline.id) - ::Security::ProcessScanEventsWorker.perform_async(build.pipeline.id) - else - ::Sbom::ScheduleIngestReportsService.new(build.pipeline).execute - ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: build.pipeline.id) + if !::Security::Scan.exists?(build_id: build.id) && + build.pipeline.completed_security_jobs? + + if build.pipeline.could_store_security_reports? + ::Security::StoreScansWorker.perform_async(build.pipeline.id) + ::Security::ProcessScanEventsWorker.perform_async(build.pipeline.id) + else + ::Sbom::ScheduleIngestReportsService.new(build.pipeline).execute + ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: build.pipeline.id) + end end end end -- GitLab From d3a79b063ce7daf8193307a75bcfe4dab6ef0130 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 2 Jul 2025 12:02:22 +0300 Subject: [PATCH 05/71] Add ingest scan reports event --- .../ci/job_security_scan_completed_event.rb | 15 +++++++++ ee/app/models/ee/ci/build.rb | 18 +++------- ee/app/models/ee/ci/pipeline.rb | 26 +++++++-------- .../security/scans/ingest_reports_worker.rb | 33 +++++++++++++++++++ ee/lib/ee/gitlab/event_store.rb | 1 + 5 files changed, 67 insertions(+), 26 deletions(-) create mode 100644 ee/app/events/ci/job_security_scan_completed_event.rb create mode 100644 ee/app/workers/security/scans/ingest_reports_worker.rb diff --git a/ee/app/events/ci/job_security_scan_completed_event.rb b/ee/app/events/ci/job_security_scan_completed_event.rb new file mode 100644 index 00000000000000..17d6194475f5aa --- /dev/null +++ b/ee/app/events/ci/job_security_scan_completed_event.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Ci + class JobSecurityScanCompletedEvent < ::Gitlab::EventStore::Event + def schema + { + 'type' => 'object', + 'required' => ['job_id'], + 'properties' => { + 'job_id' => { 'type' => 'integer' } + } + } + end + end +end diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 80b21a76002973..f0cee1dff23fd3 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -75,19 +75,11 @@ module Build end after_transition any => [:success, :failed, :canceled] do |build| - if build.security_job? - build.run_after_commit do - if !::Security::Scan.exists?(build_id: build.id) && - build.pipeline.completed_security_jobs? - - if build.pipeline.could_store_security_reports? - ::Security::StoreScansWorker.perform_async(build.pipeline.id) - ::Security::ProcessScanEventsWorker.perform_async(build.pipeline.id) - else - ::Sbom::ScheduleIngestReportsService.new(build.pipeline).execute - ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: build.pipeline.id) - end - end + build.run_after_commit do + if build.security_job? && !::Security::Scan.exists?(build_id: build.id) + Gitlab::EventStore.publish( + Ci::JobSecurityScanCompletedEvent.new(data: { job_id: buidl.id }) + ) end end end diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index e5df7a8add8fa3..fa3468c1743b2c 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -73,19 +73,19 @@ def self.latest_limited_pipeline_ids_per_source(pipelines, sha) ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_polling(pipeline_id: pipeline.id) end - after_transition any => ::Ci::Pipeline.completed_with_manual_statuses do |pipeline| - unless pipeline.has_security_scans? - pipeline.run_after_commit do - if pipeline.can_store_security_reports? - ::Security::StoreScansWorker.perform_async(pipeline.id) - ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) - else - ::Sbom::ScheduleIngestReportsService.new(pipeline).execute - ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) - end - end - end - end + # after_transition any => ::Ci::Pipeline.completed_with_manual_statuses do |pipeline| + # unless pipeline.has_security_scans? + # pipeline.run_after_commit do + # if pipeline.can_store_security_reports? + # ::Security::StoreScansWorker.perform_async(pipeline.id) + # ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) + # else + # ::Sbom::ScheduleIngestReportsService.new(pipeline).execute + # ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) + # end + # end + # end + # end after_transition any => ::Ci::Pipeline.completed_with_manual_statuses do |pipeline| pipeline.run_after_commit do diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb new file mode 100644 index 00000000000000..5538e84fa9bd6d --- /dev/null +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Security + module Scans + class IngestReportsWorker + include ApplicationWorker + include Gitlab::EventStore::Subscriber + + feature_category :vulnerability_management + urgency :low + worker_resource_boundary :cpu + data_consistency :sticky + sidekiq_options retry: 3 + + defer_on_database_health_signal :gitlab_sec, [:vulnerability_occurences], 1.minute + idempotent! + + def handle_event(event) + build = ::Ci::Build.find_by_id(event.data[:job_id]) + + # return unless build.pipeline.completed_security_jobs? + + if build.pipeline.could_store_security_reports? + ::Security::StoreScansWorker.perform_async(build.pipeline.id) + ::Security::ProcessScanEventsWorker.perform_async(build.pipeline.id) + else + ::Sbom::ScheduleIngestReportsService.new(build.pipeline).execute + ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: build.pipeline.id) + end + end + end + end +end diff --git a/ee/lib/ee/gitlab/event_store.rb b/ee/lib/ee/gitlab/event_store.rb index f9707b5cbe1444..d5ac078d2d3d21 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 ::Security::Scans::PurgeByJobIdWorker, to: ::Ci::JobArtifactsDeletedEvent + store.subscribe ::Security::Scans::IngestReportsWorker, to: ::Ci::JobSecurityScanCompletedEvent store.subscribe ::Geo::CreateRepositoryUpdatedEventWorker, to: ::Repositories::KeepAroundRefsCreatedEvent, if: ->(_) { ::Gitlab::Geo.primary? } -- GitLab From 3f6c4da609bc37b676f4560a8980558c6c218ef9 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 2 Jul 2025 12:19:45 +0300 Subject: [PATCH 06/71] Update sidekiq queues configuration --- config/sidekiq_queues.yml | 2 ++ ee/app/workers/all_queues.yml | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 2d39a2ca5b0145..7cdf800fcb63cc 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -1009,6 +1009,8 @@ - 1 - - security_scans - 2 +- - security_scans_ingest_reports + - 1 - - security_scans_purge_by_job_id - 1 - - security_secret_detection_gitlab_token_verification diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 4a7bb91eff3c9f..67c5d0f70fa760 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -3814,6 +3814,16 @@ :idempotent: true :tags: [] :queue_namespace: +- :name: security_scans_ingest_reports + :worker_name: Security::Scans::IngestReportsWorker + :feature_category: :vulnerability_management + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :cpu + :weight: 1 + :idempotent: true + :tags: [] + :queue_namespace: - :name: security_scans_purge_by_job_id :worker_name: Security::Scans::PurgeByJobIdWorker :feature_category: :vulnerability_management -- GitLab From c949715deda3d866773e55fa81de6c5c43b53699 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Fri, 4 Jul 2025 15:42:13 +0300 Subject: [PATCH 07/71] Add ingest reports service --- ee/app/models/ee/ci/build.rb | 6 +-- ee/app/models/ee/ci/pipeline.rb | 26 +++++------ .../security/scans/ingest_reports_service.rb | 43 +++++++++++++++++++ .../security/scans/ingest_reports_worker.rb | 15 +++---- ee/app/workers/security/store_scans_worker.rb | 2 +- 5 files changed, 67 insertions(+), 25 deletions(-) create mode 100644 ee/app/services/security/scans/ingest_reports_service.rb diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index f0cee1dff23fd3..d8685e4e934f8f 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -76,9 +76,9 @@ module Build after_transition any => [:success, :failed, :canceled] do |build| build.run_after_commit do - if build.security_job? && !::Security::Scan.exists?(build_id: build.id) - Gitlab::EventStore.publish( - Ci::JobSecurityScanCompletedEvent.new(data: { job_id: buidl.id }) + if build.security_job? + ::Gitlab::EventStore.publish( + ::Ci::JobSecurityScanCompletedEvent.new(data: { job_id: build.id }) ) end end diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index fa3468c1743b2c..bfddc535bfa2ec 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -74,15 +74,13 @@ def self.latest_limited_pipeline_ids_per_source(pipelines, sha) end # after_transition any => ::Ci::Pipeline.completed_with_manual_statuses do |pipeline| - # unless pipeline.has_security_scans? - # pipeline.run_after_commit do - # if pipeline.can_store_security_reports? - # ::Security::StoreScansWorker.perform_async(pipeline.id) - # ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) - # else - # ::Sbom::ScheduleIngestReportsService.new(pipeline).execute - # ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) - # end + # pipeline.run_after_commit do + # if pipeline.can_store_security_reports? + # ::Security::StoreScansWorker.perform_async(pipeline.id) + # ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) + # else + # ::Sbom::ScheduleIngestReportsService.new(pipeline).execute + # ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) # end # end # end @@ -340,12 +338,14 @@ def has_security_reports? complete_or_manual_and_has_reports?(::Ci::JobArtifact.with_file_types(security_and_license_scanning_file_types)) end - def has_security_jobs? - builds.any?(&:security_job?) + def has_security_reports_ready? + security_and_license_scanning_file_types = EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types | %w[license_scanning] + + has_reports?(::Ci::JobArtifact.with_file_types(security_and_license_scanning_file_types)) end - def has_security_scans? - builds.select(&:security_job?).any? { |build| ::Security::Scan.exists?(build_id: build.id) } + def has_security_jobs? + builds.any?(&:security_job?) end def completed_security_jobs? diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb new file mode 100644 index 00000000000000..5d78627e24b575 --- /dev/null +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module Security + module Scans + class IngestReportsService + def initialize(pipeline) + @pipeline = pipeline + end + + def execute + if pipeline.could_store_security_reports? + ensure_security_reports_available! + ::Security::StoreScansWorker.perform_async(pipeline.id) + ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) + else + ::Sbom::ScheduleIngestReportsService.new(pipeline).execute + ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) + end + end + + private + + attr_reader :pipeline + + def ensure_security_reports_available! + retries = 0 + max_retries = 5 + + loop do + return if pipeline.has_security_reports_ready? + + if retries >= max_retries + raise(StandardError, + "Security reports not available after #{max_retries} retries for pipeline #{pipeline.id}") + end + + sleep(60) + retries += 1 + end + end + end + end +end diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index 5538e84fa9bd6d..c07b3d23fc114c 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -5,6 +5,7 @@ module Scans class IngestReportsWorker include ApplicationWorker include Gitlab::EventStore::Subscriber + include Gitlab::ExclusiveLeaseHelpers feature_category :vulnerability_management urgency :low @@ -18,15 +19,13 @@ class IngestReportsWorker def handle_event(event) build = ::Ci::Build.find_by_id(event.data[:job_id]) - # return unless build.pipeline.completed_security_jobs? - - if build.pipeline.could_store_security_reports? - ::Security::StoreScansWorker.perform_async(build.pipeline.id) - ::Security::ProcessScanEventsWorker.perform_async(build.pipeline.id) - else - ::Sbom::ScheduleIngestReportsService.new(build.pipeline).execute - ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: build.pipeline.id) + completed_security_jobs = false + in_lock("scans:ingest_reports:#{build.pipeline_id}", ttl: 10.minutes, retries: 3) do + completed_security_jobs = build.pipeline.completed_security_jobs? end + return unless completed_security_jobs && !build.security_scans.exists? + + ::Security::Scans::IngestReportsService.new(build.pipeline).execute end end end diff --git a/ee/app/workers/security/store_scans_worker.rb b/ee/app/workers/security/store_scans_worker.rb index 292a5bf962ae7f..d9e2b7282cb20a 100644 --- a/ee/app/workers/security/store_scans_worker.rb +++ b/ee/app/workers/security/store_scans_worker.rb @@ -14,7 +14,7 @@ class StoreScansWorker # rubocop:disable Scalability/IdempotentWorker def perform(pipeline_id) ::Ci::Pipeline.find_by_id(pipeline_id).try do |pipeline| - break unless pipeline.can_store_security_reports? + break unless pipeline.has_security_reports_ready? Security::StoreScansService.execute(pipeline) end -- GitLab From 6a6ca44ea1d1d76c820a6ce5bccc52374d43bdf7 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Sun, 6 Jul 2025 16:05:22 +0300 Subject: [PATCH 08/71] Add spec for IngestReportsWorker --- .../security/scans/ingest_reports_service.rb | 4 ++ .../security/scans/ingest_reports_worker.rb | 3 +- .../scans/ingest_reports_worker_spec.rb | 48 +++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 ee/spec/workers/security/scans/ingest_reports_worker_spec.rb diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index 5d78627e24b575..4ee27a90b54142 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -3,6 +3,10 @@ module Security module Scans class IngestReportsService + def self.execute(pipeline) + new(pipeline).execute + end + def initialize(pipeline) @pipeline = pipeline end diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index c07b3d23fc114c..afd6bad9fd2f9d 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -23,9 +23,10 @@ def handle_event(event) in_lock("scans:ingest_reports:#{build.pipeline_id}", ttl: 10.minutes, retries: 3) do completed_security_jobs = build.pipeline.completed_security_jobs? end + return unless completed_security_jobs && !build.security_scans.exists? - ::Security::Scans::IngestReportsService.new(build.pipeline).execute + ::Security::Scans::IngestReportsService.execute(build.pipeline) end end end diff --git a/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb new file mode 100644 index 00000000000000..8f520c7b05099c --- /dev/null +++ b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::Scans::IngestReportsWorker, feature_category: :vulnerability_management do + let(:pipeline) { create(:ci_pipeline) } + let(:job) { create(:ci_build, :sast, pipeline: pipeline) } + let(:event) { ::Ci::JobSecurityScanCompletedEvent.new(data: { job_id: job.id }) } + + subject(:handle_event) { consume_event(subscriber: described_class, event: event) } + + before do + allow(::Security::Scans::IngestReportsService).to receive(:execute) + end + + it_behaves_like 'subscribes to event' + + describe '#handle_event' do + context 'when the job is not completed' do + it 'handle_event returns without calling service' do + handle_event + + expect(::Security::Scans::IngestReportsService).not_to have_received(:execute) + end + end + + context 'when the job has completed' do + let(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } + + it 'handle_event returns without calling service' do + handle_event + + expect(::Security::Scans::IngestReportsService).to have_received(:execute).with(pipeline) + end + end + + context 'when security scan already exists' do + let(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } + let!(:scan) { create(:security_scan, :latest_successful, scan_type: :sast, build: job) } + + it 'handle_event returns without calling service' do + handle_event + + expect(::Security::Scans::IngestReportsService).not_to have_received(:execute) + end + end + end +end -- GitLab From 0a1a1623cb590bf5418db9c55f79c4dccfdf9ee1 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Sun, 6 Jul 2025 19:16:55 +0300 Subject: [PATCH 09/71] Add spec for IngestReportsService --- .../security/scans/ingest_reports_service.rb | 4 +- .../security/scans/ingest_reports_worker.rb | 2 +- .../scans/ingest_reports_service_spec.rb | 75 +++++++++++++++++++ 3 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 ee/spec/services/security/scans/ingest_reports_service_spec.rb diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index 4ee27a90b54142..9a097ec2557e75 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -28,7 +28,7 @@ def execute def ensure_security_reports_available! retries = 0 - max_retries = 5 + max_retries = 3 loop do return if pipeline.has_security_reports_ready? @@ -38,7 +38,7 @@ def ensure_security_reports_available! "Security reports not available after #{max_retries} retries for pipeline #{pipeline.id}") end - sleep(60) + sleep(10) retries += 1 end end diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index afd6bad9fd2f9d..3aa96e37c6d992 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -20,7 +20,7 @@ def handle_event(event) build = ::Ci::Build.find_by_id(event.data[:job_id]) completed_security_jobs = false - in_lock("scans:ingest_reports:#{build.pipeline_id}", ttl: 10.minutes, retries: 3) do + in_lock("scans:ingest_reports:#{build.pipeline_id}", ttl: 1.minute, retries: 3) do completed_security_jobs = build.pipeline.completed_security_jobs? end diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb new file mode 100644 index 00000000000000..63fbd54d368776 --- /dev/null +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::Scans::IngestReportsService, feature_category: :vulnerability_management do + let_it_be(:user) { create(:user) } + let_it_be(:pipeline) { create(:ci_pipeline, user: user) } + + describe '.execute' do + let(:mock_service_object) { instance_double(described_class, execute: true) } + + subject(:execute) { described_class.execute(pipeline) } + + before do + allow(described_class).to receive(:new).with(pipeline).and_return(mock_service_object) + end + + it 'delegates the call to an instance of `Security::Scans::IngestReportsService`' do + execute + + expect(described_class).to have_received(:new).with(pipeline) + expect(mock_service_object).to have_received(:execute) + end + end + + describe '#execute' do + let(:service_object) { described_class.new(pipeline) } + let(:mock_sbom_ingestion_service) { instance_double(::Sbom::ScheduleIngestReportsService, execute: nil) } + + subject(:ingest_security_scans) { service_object.execute } + + before do + allow(::Security::StoreScansWorker).to receive(:perform_async) + allow(pipeline).to receive_messages( + could_store_security_reports?: could_store_security_reports, + has_security_reports_ready?: true + ) + allow(::Sbom::ScheduleIngestReportsService).to receive(:new) + .with(pipeline).and_return(mock_sbom_ingestion_service) + end + + context 'when the security scans can be stored for the pipeline' do + let(:could_store_security_reports) { true } + + it 'schedules store security scans job' do + ingest_security_scans + + expect(::Security::StoreScansWorker).to have_received(:perform_async).with(pipeline.id) + end + + it 'does not try to ingest the SBOM reports' do + ingest_security_scans + + expect(::Sbom::ScheduleIngestReportsService).not_to have_received(:new) + end + end + + context 'when the security scans can not be stored for the pipeline' do + let(:could_store_security_reports) { false } + + it 'does not schedule store security scans job' do + ingest_security_scans + + expect(::Security::StoreScansWorker).not_to have_received(:perform_async) + end + + it 'tries to ingest sbom reports' do + ingest_security_scans + + expect(::Sbom::ScheduleIngestReportsService).to have_received(:new).with(pipeline) + expect(mock_sbom_ingestion_service).to have_received(:execute) + end + end + end +end -- GitLab From 1fb424819bc94953d8fe7eace6c2a2ed836d5eeb Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 8 Jul 2025 12:47:55 +0300 Subject: [PATCH 10/71] Add tests for in_lock --- .../security/scans/ingest_reports_worker.rb | 5 +++- .../scans/ingest_reports_worker_spec.rb | 25 ++++++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index 3aa96e37c6d992..fab1ee9596a98a 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -7,6 +7,9 @@ class IngestReportsWorker include Gitlab::EventStore::Subscriber include Gitlab::ExclusiveLeaseHelpers + LEASE_RETRIES = 3 + LEASE_TRY_AFTER = 10.seconds + feature_category :vulnerability_management urgency :low worker_resource_boundary :cpu @@ -20,7 +23,7 @@ def handle_event(event) build = ::Ci::Build.find_by_id(event.data[:job_id]) completed_security_jobs = false - in_lock("scans:ingest_reports:#{build.pipeline_id}", ttl: 1.minute, retries: 3) do + in_lock("scans:ingest_reports:#{build.pipeline_id}", retries: LEASE_RETRIES, sleep_sec: LEASE_TRY_AFTER) do completed_security_jobs = build.pipeline.completed_security_jobs? end diff --git a/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb index 8f520c7b05099c..d258bae183840b 100644 --- a/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb +++ b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb @@ -15,7 +15,7 @@ it_behaves_like 'subscribes to event' - describe '#handle_event' do + describe '.handle_event' do context 'when the job is not completed' do it 'handle_event returns without calling service' do handle_event @@ -44,5 +44,28 @@ expect(::Security::Scans::IngestReportsService).not_to have_received(:execute) end end + + context 'when lease is taken' do + include ExclusiveLeaseHelpers + + let(:lease_key) { "scans:ingest_reports:#{pipeline.id}" } + let(:other_pipeline) { create(:ci_pipeline) } + let(:other_job) { create(:ci_build, :sast, pipeline: other_pipeline) } + let(:other_event) { ::Ci::JobSecurityScanCompletedEvent.new(data: { job_id: other_job.id }) } + + before do + # Speed up retries to avoid long-running tests + stub_const("#{described_class}::LEASE_TRY_AFTER", 0.01) + stub_exclusive_lease_taken(lease_key) + end + + it 'does not permit parallel execution on the same pipeline' do + expect { handle_event }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + end + + it 'allows parallel execution on different pipelines' do + expect { consume_event(subscriber: described_class, event: other_event) }.not_to raise_error + end + end end end -- GitLab From 18e23f56004b271d4f06cd8a31b608f270540a7c Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 8 Jul 2025 15:47:03 +0300 Subject: [PATCH 11/71] Add test for JobSecurityScanCompletedEvent publisher --- ee/spec/models/ci/build_spec.rb | 63 +++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/ee/spec/models/ci/build_spec.rb b/ee/spec/models/ci/build_spec.rb index fe23214e32bc97..76d5a8d0b7fb78 100644 --- a/ee/spec/models/ci/build_spec.rb +++ b/ee/spec/models/ci/build_spec.rb @@ -1265,4 +1265,67 @@ end end end + + describe 'JobSecurityScanCompletedEvent publishing' do + let(:build) { create(:ci_build, :running, pipeline: pipeline) } + + shared_examples 'publishes JobSecurityScanCompletedEvent' do + it 'publishes the event with correct job_id when transitioning to the target state' do + expect(::Gitlab::EventStore).to receive(:publish) do |event| + expect(event).to be_an_instance_of(::Ci::JobSecurityScanCompletedEvent) + expect(event.data).to eq({ "job_id" => build.id }) + end + + build.public_send(transition_method) + end + end + + shared_examples 'does not publish JobSecurityScanCompletedEvent' do + it 'does not publish the event when transitioning to the target state' do + expect(::Gitlab::EventStore).not_to receive(:publish) + + build.public_send(transition_method) + end + end + + context 'when build is a security job' do + before do + allow(build).to receive(:security_job?).and_return(true) + end + + where(:transition_method, :transition_description) do + [ + [:success, 'success'], + [:drop, 'failed'], + [:cancel, 'canceled'] + ] + end + + with_them do + context "when transitioning to #{params[:transition_description]}" do + it_behaves_like 'publishes JobSecurityScanCompletedEvent' + end + end + end + + context 'when build is not a security job' do + before do + allow(build).to receive(:security_job?).and_return(false) + end + + where(:transition_method, :transition_description) do + [ + [:success, 'success'], + [:drop, 'failed'], + [:cancel, 'canceled'] + ] + end + + with_them do + context "when transitioning to #{params[:transition_description]}" do + it_behaves_like 'does not publish JobSecurityScanCompletedEvent' + end + end + end + end end -- GitLab From baae9c255f1d4be769addfee62c71c4b85c5718b Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 8 Jul 2025 17:27:56 +0300 Subject: [PATCH 12/71] Add FF definition --- .../ingest_sec_reports_when_sec_jobs_completed.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 ee/config/feature_flags/beta/ingest_sec_reports_when_sec_jobs_completed.yml diff --git a/ee/config/feature_flags/beta/ingest_sec_reports_when_sec_jobs_completed.yml b/ee/config/feature_flags/beta/ingest_sec_reports_when_sec_jobs_completed.yml new file mode 100644 index 00000000000000..dd4e67590c9036 --- /dev/null +++ b/ee/config/feature_flags/beta/ingest_sec_reports_when_sec_jobs_completed.yml @@ -0,0 +1,10 @@ +--- +name: ingest_sec_reports_when_sec_jobs_completed +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/513326 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195012 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/554222 +milestone: '18.2' +group: group::security infrastructure +type: beta +default_enabled: false -- GitLab From 0af4b8539e0971d4030a77394da0238c84144613 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 8 Jul 2025 18:35:12 +0300 Subject: [PATCH 13/71] Fix pipeline spec by disabling FF --- ee/app/models/ee/ci/pipeline.rb | 24 +++++++++++++----------- ee/spec/models/ci/pipeline_spec.rb | 3 +++ 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index bfddc535bfa2ec..d77f836f4c0e30 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -73,17 +73,19 @@ def self.latest_limited_pipeline_ids_per_source(pipelines, sha) ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_polling(pipeline_id: pipeline.id) end - # after_transition any => ::Ci::Pipeline.completed_with_manual_statuses do |pipeline| - # pipeline.run_after_commit do - # if pipeline.can_store_security_reports? - # ::Security::StoreScansWorker.perform_async(pipeline.id) - # ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) - # else - # ::Sbom::ScheduleIngestReportsService.new(pipeline).execute - # ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) - # end - # end - # end + after_transition any => ::Ci::Pipeline.completed_with_manual_statuses do |pipeline| + if ::Feature.disabled?(:ingest_sec_reports_when_sec_jobs_completed, pipeline.project) + pipeline.run_after_commit do + if pipeline.can_store_security_reports? + ::Security::StoreScansWorker.perform_async(pipeline.id) + ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) + else + ::Sbom::ScheduleIngestReportsService.new(pipeline).execute + ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) + end + end + end + end after_transition any => ::Ci::Pipeline.completed_with_manual_statuses do |pipeline| pipeline.run_after_commit do diff --git a/ee/spec/models/ci/pipeline_spec.rb b/ee/spec/models/ci/pipeline_spec.rb index 43844931e05c62..36d807c80b3d1f 100644 --- a/ee/spec/models/ci/pipeline_spec.rb +++ b/ee/spec/models/ci/pipeline_spec.rb @@ -215,6 +215,7 @@ let(:cache_key) { Ci::CompareSecurityReportsService.transition_cache_key(pipeline_id: pipeline_id) } before do + stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: false) allow(redis_spy).to receive(:ttl).and_return(10) # to allow event tracking Redis call end @@ -254,6 +255,7 @@ subject(:transition_pipeline) { pipeline.update!(status_event: transition) } before do + stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: false) allow(::Security::StoreScansWorker).to receive(:perform_async) allow(pipeline).to receive(:can_store_security_reports?).and_return(can_store_security_reports) end @@ -346,6 +348,7 @@ subject(:transition_pipeline) { pipeline.update!(status_event: transition) } before do + stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: false) allow(::Sbom::ScheduleIngestReportsService).to receive(:new).with(pipeline).and_return(sbom_ingestion_scheduler) end -- GitLab From d96b61ddd6c318b0c59e8df74656d053ec603cf5 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 8 Jul 2025 22:44:08 +0300 Subject: [PATCH 14/71] Add FF to build state transition --- ee/app/models/ee/ci/build.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index d8685e4e934f8f..ecbd3498cf6455 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -75,11 +75,13 @@ module Build end after_transition any => [:success, :failed, :canceled] do |build| - build.run_after_commit do - if build.security_job? - ::Gitlab::EventStore.publish( - ::Ci::JobSecurityScanCompletedEvent.new(data: { job_id: build.id }) - ) + if ::Feature.enabled?(:ingest_sec_reports_when_sec_jobs_completed, build.project) + build.run_after_commit do + if build.security_job? + ::Gitlab::EventStore.publish( + ::Ci::JobSecurityScanCompletedEvent.new(data: { job_id: build.id }) + ) + end end end end -- GitLab From 3a3eae7de5821a076450a5a43af34394f0697c4b Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 9 Jul 2025 08:42:11 +0300 Subject: [PATCH 15/71] Remove sidekiq retry options from IngestReportsWorker --- ee/app/workers/security/scans/ingest_reports_worker.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index fab1ee9596a98a..23f1b22c93211d 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -14,7 +14,6 @@ class IngestReportsWorker urgency :low worker_resource_boundary :cpu data_consistency :sticky - sidekiq_options retry: 3 defer_on_database_health_signal :gitlab_sec, [:vulnerability_occurences], 1.minute idempotent! -- GitLab From a11d947c0c9b8f5b31d302636c910e065774bcbe Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 9 Jul 2025 09:31:45 +0300 Subject: [PATCH 16/71] Fix StoreScansWorker Rspec --- ee/spec/workers/security/store_scans_worker_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/spec/workers/security/store_scans_worker_spec.rb b/ee/spec/workers/security/store_scans_worker_spec.rb index 41f7815230fc12..b5ebf5e4113e2e 100644 --- a/ee/spec/workers/security/store_scans_worker_spec.rb +++ b/ee/spec/workers/security/store_scans_worker_spec.rb @@ -13,12 +13,12 @@ before do allow(Security::StoreScansService).to receive(:execute) allow_next_found_instance_of(Ci::Pipeline) do |record| - allow(record).to receive(:can_store_security_reports?).and_return(can_store_security_reports) + allow(record).to receive(:has_security_reports_ready?).and_return(has_security_reports_ready) end end context 'when security reports can not be stored for the pipeline' do - let(:can_store_security_reports) { false } + let(:has_security_reports_ready) { false } it 'does not call `Security::StoreScansService`' do run_worker @@ -28,7 +28,7 @@ end context 'when security reports can be stored for the pipeline' do - let(:can_store_security_reports) { true } + let(:has_security_reports_ready) { true } it 'calls `Security::StoreScansService`' do run_worker -- GitLab From 0632913e9a7596a4fc3d90953db4f71b926ff59a Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 9 Jul 2025 10:04:30 +0300 Subject: [PATCH 17/71] Add Ci::JobSecurityScanCompletedEvent to event_store_spec expected events --- ee/spec/lib/ee/gitlab/event_store_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/ee/spec/lib/ee/gitlab/event_store_spec.rb b/ee/spec/lib/ee/gitlab/event_store_spec.rb index d23052e5d52d71..eff64061cddd43 100644 --- a/ee/spec/lib/ee/gitlab/event_store_spec.rb +++ b/ee/spec/lib/ee/gitlab/event_store_spec.rb @@ -13,6 +13,7 @@ Ai::ActiveContext::Code::SaasInitialIndexingEvent, ::Ci::JobArtifactsDeletedEvent, ::Ci::PipelineCreatedEvent, + ::Ci::JobSecurityScanCompletedEvent, ::Repositories::KeepAroundRefsCreatedEvent, ::MergeRequests::ApprovedEvent, ::MergeRequests::MergedEvent, -- GitLab From 8f5319518a234672ce27808c4f6d2ebaec9f4cd4 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 9 Jul 2025 10:05:39 +0300 Subject: [PATCH 18/71] Remove unneeded check ensure_security_reports_available --- .../security/scans/ingest_reports_service.rb | 18 ------------------ .../scans/ingest_reports_service_spec.rb | 3 +-- 2 files changed, 1 insertion(+), 20 deletions(-) diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index 9a097ec2557e75..05f3f97a9e7afe 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -13,7 +13,6 @@ def initialize(pipeline) def execute if pipeline.could_store_security_reports? - ensure_security_reports_available! ::Security::StoreScansWorker.perform_async(pipeline.id) ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) else @@ -25,23 +24,6 @@ def execute private attr_reader :pipeline - - def ensure_security_reports_available! - retries = 0 - max_retries = 3 - - loop do - return if pipeline.has_security_reports_ready? - - if retries >= max_retries - raise(StandardError, - "Security reports not available after #{max_retries} retries for pipeline #{pipeline.id}") - end - - sleep(10) - retries += 1 - end - end end end end diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index 63fbd54d368776..1452c940d6095b 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -32,8 +32,7 @@ before do allow(::Security::StoreScansWorker).to receive(:perform_async) allow(pipeline).to receive_messages( - could_store_security_reports?: could_store_security_reports, - has_security_reports_ready?: true + could_store_security_reports?: could_store_security_reports ) allow(::Sbom::ScheduleIngestReportsService).to receive(:new) .with(pipeline).and_return(mock_sbom_ingestion_service) -- GitLab From 5b4fb9f901ab792bc7288fe19bac98c2116436c8 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 9 Jul 2025 07:55:55 +0000 Subject: [PATCH 19/71] Check that build was found --- ee/app/workers/security/scans/ingest_reports_worker.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index 23f1b22c93211d..ffd11c36e60610 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -20,6 +20,7 @@ class IngestReportsWorker def handle_event(event) build = ::Ci::Build.find_by_id(event.data[:job_id]) + return unless build completed_security_jobs = false in_lock("scans:ingest_reports:#{build.pipeline_id}", retries: LEASE_RETRIES, sleep_sec: LEASE_TRY_AFTER) do -- GitLab From a9cd9f70cf98c09ed15b62e2d2d20935071e466b Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 9 Jul 2025 07:58:49 +0000 Subject: [PATCH 20/71] Fix N+1 issue in completed_security_jobs --- ee/app/models/ee/ci/pipeline.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index d77f836f4c0e30..0b7913be1e5034 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -351,7 +351,10 @@ def has_security_jobs? end def completed_security_jobs? - builds.select(&:security_job?).all? { |build| build.status.in?(::Ci::Build::COMPLETED_STATUSES) } + security_builds = builds.select(&:security_job?) + return true if security_builds.empty? + + security_builds.all? { |build| build.status.in?(::Ci::Build::COMPLETED_STATUSES) } end def has_all_security_policies_reports? -- GitLab From d64bb46ee899bfa965c71d13a579e2138de7d9f1 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 9 Jul 2025 11:46:51 +0300 Subject: [PATCH 21/71] Add license_scanning to security_job? method --- ee/app/models/ee/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index ecbd3498cf6455..35363ea263630c 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -216,7 +216,7 @@ def pages strong_memoize_attr :pages def security_job? - (::EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types & (reports_file_types || [])).any? + ((::EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types | %w[license_scanning]) & (reports_file_types || [])).any? end def reports_file_types -- GitLab From 5004972ebe447f93732fbc5d1527491d1e81e107 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 9 Jul 2025 12:45:47 +0300 Subject: [PATCH 22/71] Fix trailing white spaces --- ee/app/models/ee/ci/pipeline.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index 0b7913be1e5034..56522cc62e7be7 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -353,7 +353,7 @@ def has_security_jobs? def completed_security_jobs? security_builds = builds.select(&:security_job?) return true if security_builds.empty? - + security_builds.all? { |build| build.status.in?(::Ci::Build::COMPLETED_STATUSES) } end -- GitLab From f3f7aa028e863b41baef9ca2a8b406ee81081ebb Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 9 Jul 2025 13:26:13 +0300 Subject: [PATCH 23/71] Protect IngestReportsService with lock --- ee/app/workers/security/scans/ingest_reports_worker.rb | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index ffd11c36e60610..4e3d0f13abfb31 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -22,14 +22,11 @@ def handle_event(event) build = ::Ci::Build.find_by_id(event.data[:job_id]) return unless build - completed_security_jobs = false in_lock("scans:ingest_reports:#{build.pipeline_id}", retries: LEASE_RETRIES, sleep_sec: LEASE_TRY_AFTER) do - completed_security_jobs = build.pipeline.completed_security_jobs? + if build.pipeline.completed_security_jobs? && !build.security_scans.exists? + ::Security::Scans::IngestReportsService.execute(build.pipeline) + end end - - return unless completed_security_jobs && !build.security_scans.exists? - - ::Security::Scans::IngestReportsService.execute(build.pipeline) end end end -- GitLab From 669ab54d704fd8c783514e105ef98966adb60411 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 9 Jul 2025 21:11:11 +0000 Subject: [PATCH 24/71] Simplify check that all security jobs completed --- ee/app/models/ee/ci/pipeline.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index 56522cc62e7be7..d3d014ffc645bc 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -352,9 +352,7 @@ def has_security_jobs? def completed_security_jobs? security_builds = builds.select(&:security_job?) - return true if security_builds.empty? - - security_builds.all? { |build| build.status.in?(::Ci::Build::COMPLETED_STATUSES) } + security_builds.all?(&:complete?) end def has_all_security_policies_reports? -- GitLab From a5edb30917efb5012f8b9025833344afb9c1bda8 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 10 Jul 2025 00:21:48 +0300 Subject: [PATCH 25/71] Use completed_statuses in status transition --- ee/app/models/ee/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 35363ea263630c..ababef1bd97a82 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -74,7 +74,7 @@ module Build end end - after_transition any => [:success, :failed, :canceled] do |build| + after_transition any => ::Ci::Build.completed_statuses do |build| if ::Feature.enabled?(:ingest_sec_reports_when_sec_jobs_completed, build.project) build.run_after_commit do if build.security_job? -- GitLab From b8ed39f3a88f0098a4f1f8dd51debddf58ed6649 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Mon, 14 Jul 2025 09:25:05 +0300 Subject: [PATCH 26/71] Add ceche key to check if reports already ingested --- ee/app/models/ee/ci/pipeline.rb | 8 +++++++ .../security/scans/ingest_reports_service.rb | 10 ++++++++ .../security/scans/ingest_reports_worker.rb | 4 +--- .../scans/ingest_reports_service_spec.rb | 23 +++++++++++-------- .../scans/ingest_reports_worker_spec.rb | 11 --------- 5 files changed, 33 insertions(+), 23 deletions(-) diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index d3d014ffc645bc..b033d9366c4373 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -355,6 +355,14 @@ def completed_security_jobs? security_builds.all?(&:complete?) end + def scans_cache_key + sha = builds.select(&:security_job?) + .map(&:updated_at) + .join('|') + .then { |value| Digest::SHA512.hexdigest(value) } + "security:report:ingest:#{sha}" + end + def has_all_security_policies_reports? can_store_security_reports? && can_ingest_sbom_reports? end diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index 05f3f97a9e7afe..eb01cb2fc2d9f6 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -3,6 +3,8 @@ module Security module Scans class IngestReportsService + TTL_REPORT_INGESTION = 1.hour + def self.execute(pipeline) new(pipeline).execute end @@ -12,6 +14,8 @@ def initialize(pipeline) end def execute + return if already_ingested? + if pipeline.could_store_security_reports? ::Security::StoreScansWorker.perform_async(pipeline.id) ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) @@ -24,6 +28,12 @@ def execute private attr_reader :pipeline + + def already_ingested? + ::Gitlab::Redis::SharedState.with do |redis| + !redis.set(@pipeline.scans_cache_key, 'OK', nx: true, ex: TTL_REPORT_INGESTION) + end + end end end end diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index 4e3d0f13abfb31..6d5c1e67248edb 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -23,9 +23,7 @@ def handle_event(event) return unless build in_lock("scans:ingest_reports:#{build.pipeline_id}", retries: LEASE_RETRIES, sleep_sec: LEASE_TRY_AFTER) do - if build.pipeline.completed_security_jobs? && !build.security_scans.exists? - ::Security::Scans::IngestReportsService.execute(build.pipeline) - end + ::Security::Scans::IngestReportsService.execute(build.pipeline) if build.pipeline.completed_security_jobs? end end end diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index 1452c940d6095b..0367a695735215 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -4,9 +4,9 @@ RSpec.describe Security::Scans::IngestReportsService, feature_category: :vulnerability_management do let_it_be(:user) { create(:user) } - let_it_be(:pipeline) { create(:ci_pipeline, user: user) } describe '.execute' do + let(:pipeline) { create(:ci_pipeline, user: user) } let(:mock_service_object) { instance_double(described_class, execute: true) } subject(:execute) { described_class.execute(pipeline) } @@ -40,32 +40,37 @@ context 'when the security scans can be stored for the pipeline' do let(:could_store_security_reports) { true } + let_it_be(:pipeline) { create(:ci_pipeline, user: user) } + let_it_be(:job) do + create(:ci_build, :sast, pipeline: pipeline, status: 'success', updated_at: "2025-07-13 13:29:04 UTC") + end - it 'schedules store security scans job' do + it 'schedules store security scans job and does not ingest the SBOM reports' do ingest_security_scans expect(::Security::StoreScansWorker).to have_received(:perform_async).with(pipeline.id) + expect(::Sbom::ScheduleIngestReportsService).not_to have_received(:new) end - it 'does not try to ingest the SBOM reports' do + it 'already ingested' do ingest_security_scans + expect(::Security::StoreScansWorker).not_to have_received(:perform_async) expect(::Sbom::ScheduleIngestReportsService).not_to have_received(:new) end end context 'when the security scans can not be stored for the pipeline' do let(:could_store_security_reports) { false } - - it 'does not schedule store security scans job' do - ingest_security_scans - - expect(::Security::StoreScansWorker).not_to have_received(:perform_async) + let_it_be(:pipeline) { create(:ci_pipeline, user: user) } + let_it_be(:job) do + create(:ci_build, :sast, pipeline: pipeline, status: 'success', updated_at: "2025-07-13 14:29:04 UTC") end - it 'tries to ingest sbom reports' do + it 'does not schedule store security scans job and to ingests sbom reports' do ingest_security_scans + expect(::Security::StoreScansWorker).not_to have_received(:perform_async) expect(::Sbom::ScheduleIngestReportsService).to have_received(:new).with(pipeline) expect(mock_sbom_ingestion_service).to have_received(:execute) end diff --git a/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb index d258bae183840b..f273e0f8f1b36e 100644 --- a/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb +++ b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb @@ -34,17 +34,6 @@ end end - context 'when security scan already exists' do - let(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } - let!(:scan) { create(:security_scan, :latest_successful, scan_type: :sast, build: job) } - - it 'handle_event returns without calling service' do - handle_event - - expect(::Security::Scans::IngestReportsService).not_to have_received(:execute) - end - end - context 'when lease is taken' do include ExclusiveLeaseHelpers -- GitLab From 477d14d854b42c5ed878c5a55adfd31ede2d12fd Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 15 Jul 2025 15:33:40 +0300 Subject: [PATCH 27/71] Move reports_file_types to private --- ee/app/models/ee/ci/build.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index ababef1bd97a82..1c586afbfbd664 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -219,12 +219,12 @@ def security_job? ((::EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types | %w[license_scanning]) & (reports_file_types || [])).any? end + private + def reports_file_types metadata.config_options.dig(:artifacts, :reports)&.keys&.map(&:to_s) end - private - def expand_pages_variables pages_config .slice(:path_prefix, :expire_in) -- GitLab From ba1f62d6adfa52a60b63e444db6f2eabe3a5e13a Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 15 Jul 2025 15:36:31 +0300 Subject: [PATCH 28/71] Add strong_memoize_attr for security_job? --- ee/app/models/ee/ci/build.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 1c586afbfbd664..afe592c623f099 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -216,14 +216,21 @@ def pages strong_memoize_attr :pages def security_job? - ((::EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types | %w[license_scanning]) & (reports_file_types || [])).any? + return false if reports_file_types.empty? + + security_types = ::EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types | %w[license_scanning] + common_types = security_types & reports_file_types + + common_types.any? end + strong_memoize_attr :security_job? private def reports_file_types metadata.config_options.dig(:artifacts, :reports)&.keys&.map(&:to_s) end + strong_memoize_attr :reports_file_types def expand_pages_variables pages_config -- GitLab From 319bc6d2a2c7f8201db8cdfbfcc4e1a89ce4647a Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 15 Jul 2025 18:57:55 +0300 Subject: [PATCH 29/71] Fix security_job failure --- ee/app/models/ee/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index afe592c623f099..e29ee49b6fe420 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -216,7 +216,7 @@ def pages strong_memoize_attr :pages def security_job? - return false if reports_file_types.empty? + return false unless reports_file_types&.any? security_types = ::EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types | %w[license_scanning] common_types = security_types & reports_file_types -- GitLab From 4edf1bc8d605dc07e26008c292b9320f565a30b8 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 16 Jul 2025 09:34:21 +0300 Subject: [PATCH 30/71] Fix reports_file_types failure --- ee/app/models/ee/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index e29ee49b6fe420..15a20e15d11295 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -228,7 +228,7 @@ def security_job? private def reports_file_types - metadata.config_options.dig(:artifacts, :reports)&.keys&.map(&:to_s) + metadata.config_options&.dig(:artifacts, :reports)&.keys&.map(&:to_s) end strong_memoize_attr :reports_file_types -- GitLab From 43c40d17e49e4b05765e8c71bc8b6a1f643f896d Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 16 Jul 2025 11:20:56 +0300 Subject: [PATCH 31/71] Update extra_update_queries due to added transition --- spec/services/ci/cancel_pipeline_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/ci/cancel_pipeline_service_spec.rb b/spec/services/ci/cancel_pipeline_service_spec.rb index c33150aebcf6be..5732aa59ab6575 100644 --- a/spec/services/ci/cancel_pipeline_service_spec.rb +++ b/spec/services/ci/cancel_pipeline_service_spec.rb @@ -210,7 +210,7 @@ described_class.new(pipeline: pipeline2, current_user: current_user).force_execute end - extra_update_queries = 4 # transition ... => :canceled, queue pop + extra_update_queries = 5 # transition ... => :canceled, queue pop extra_generic_commit_status_validation_queries = 2 # name_uniqueness_across_types expect(control2.count) -- GitLab From 434c421e3629034812d7ad351ecd8912a7b9e1bc Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 16 Jul 2025 12:15:17 +0000 Subject: [PATCH 32/71] Add project_id to cache key so entries related to the same project are stored on the same cluster --- ee/app/workers/security/scans/ingest_reports_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index 6d5c1e67248edb..e014751897f738 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -22,7 +22,7 @@ def handle_event(event) build = ::Ci::Build.find_by_id(event.data[:job_id]) return unless build - in_lock("scans:ingest_reports:#{build.pipeline_id}", retries: LEASE_RETRIES, sleep_sec: LEASE_TRY_AFTER) do + in_lock("scans:{#{build.project_id}}:ingest_reports:#{build.pipeline_id}", retries: LEASE_RETRIES, sleep_sec: LEASE_TRY_AFTER) do ::Security::Scans::IngestReportsService.execute(build.pipeline) if build.pipeline.completed_security_jobs? end end -- GitLab From 8841abea912812baa189185aee7c32821f32f0b5 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 16 Jul 2025 19:34:27 +0300 Subject: [PATCH 33/71] Add new enum all_security_report_file_types --- ee/app/models/concerns/ee/enums/ci/job_artifact.rb | 6 ++++++ ee/app/models/ee/ci/build.rb | 2 +- ee/app/models/ee/ci/pipeline.rb | 4 ++-- ee/app/workers/security/scans/ingest_reports_worker.rb | 3 ++- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/ee/app/models/concerns/ee/enums/ci/job_artifact.rb b/ee/app/models/concerns/ee/enums/ci/job_artifact.rb index b4feff68025066..46c9e9226264a9 100644 --- a/ee/app/models/concerns/ee/enums/ci/job_artifact.rb +++ b/ee/app/models/concerns/ee/enums/ci/job_artifact.rb @@ -11,6 +11,8 @@ module JobArtifact SECURITY_REPORT_AND_CYCLONEDX_REPORT_FILE_TYPES = (SECURITY_REPORT_FILE_TYPES | %w[cyclonedx]).freeze + SECURITY_ALL_REPORT_FILE_TYPES = (SECURITY_REPORT_AND_CYCLONEDX_REPORT_FILE_TYPES | %w[license_scanning]).freeze + EE_REPORT_FILE_TYPES = { license_scanning: %w[license_scanning].freeze, dependency_list: %w[dependency_scanning].freeze, @@ -34,6 +36,10 @@ def self.security_report_and_cyclonedx_report_file_types SECURITY_REPORT_AND_CYCLONEDX_REPORT_FILE_TYPES end + def self.all_security_report_file_types + SECURITY_ALL_REPORT_FILE_TYPES + end + def self.ee_report_file_types EE_REPORT_FILE_TYPES end diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 15a20e15d11295..c178048426f3c0 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -218,7 +218,7 @@ def pages def security_job? return false unless reports_file_types&.any? - security_types = ::EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types | %w[license_scanning] + security_types = ::EE::Enums::Ci::JobArtifact.all_security_report_file_types common_types = security_types & reports_file_types common_types.any? diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index b033d9366c4373..fe5027d7ede1c2 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -335,13 +335,13 @@ def security_scans_created_at end def has_security_reports? - security_and_license_scanning_file_types = EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types | %w[license_scanning] + security_and_license_scanning_file_types = EE::Enums::Ci::JobArtifact.all_security_report_file_types complete_or_manual_and_has_reports?(::Ci::JobArtifact.with_file_types(security_and_license_scanning_file_types)) end def has_security_reports_ready? - security_and_license_scanning_file_types = EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types | %w[license_scanning] + security_and_license_scanning_file_types = EE::Enums::Ci::JobArtifact.all_security_report_file_types has_reports?(::Ci::JobArtifact.with_file_types(security_and_license_scanning_file_types)) end diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index e014751897f738..929a30f6336f62 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -22,7 +22,8 @@ def handle_event(event) build = ::Ci::Build.find_by_id(event.data[:job_id]) return unless build - in_lock("scans:{#{build.project_id}}:ingest_reports:#{build.pipeline_id}", retries: LEASE_RETRIES, sleep_sec: LEASE_TRY_AFTER) do + in_lock("scans:{#{build.project_id}}:ingest_reports:#{build.pipeline_id}", + retries: LEASE_RETRIES, sleep_sec: LEASE_TRY_AFTER) do ::Security::Scans::IngestReportsService.execute(build.pipeline) if build.pipeline.completed_security_jobs? end end -- GitLab From 9e0ac667c338c45a6384bc68ad2e6068c15ffdad Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 16 Jul 2025 23:29:25 +0300 Subject: [PATCH 34/71] Use options instead of metadata --- app/models/ci/build.rb | 11 ++++++++++- ee/app/models/ee/ci/build.rb | 7 +------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8e36c676f0c7c4..fc995a0e4bd630 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -956,8 +956,17 @@ def supported_runner?(features) end end + def reports_definitions + options.dig(:artifacts, :reports) + end + + def reports_file_types + reports_definitions&.keys + end + strong_memoize_attr :reports_file_types + def publishes_artifacts_reports? - options&.dig(:artifacts, :reports)&.any? + reports_definitions&.any? end def supports_artifacts_exclude? diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index c178048426f3c0..4cfab0dbaeac12 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -216,7 +216,7 @@ def pages strong_memoize_attr :pages def security_job? - return false unless reports_file_types&.any? + return false unless reports_file_types&.map(&:to_s)&.any? security_types = ::EE::Enums::Ci::JobArtifact.all_security_report_file_types common_types = security_types & reports_file_types @@ -227,11 +227,6 @@ def security_job? private - def reports_file_types - metadata.config_options&.dig(:artifacts, :reports)&.keys&.map(&:to_s) - end - strong_memoize_attr :reports_file_types - def expand_pages_variables pages_config .slice(:path_prefix, :expire_in) -- GitLab From e33ce8e32d2c0ed6cb9fb96b67c28732e1fa244a Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 17 Jul 2025 01:31:48 +0300 Subject: [PATCH 35/71] Fix security_job? method --- ee/app/models/ee/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 4cfab0dbaeac12..21bc713aabde46 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -219,7 +219,7 @@ def security_job? return false unless reports_file_types&.map(&:to_s)&.any? security_types = ::EE::Enums::Ci::JobArtifact.all_security_report_file_types - common_types = security_types & reports_file_types + common_types = security_types & reports_file_types.map(&:to_s) common_types.any? end -- GitLab From 5a26b60a13719f31a31b53db7fe2fe45968a2cde Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 17 Jul 2025 12:48:49 +0300 Subject: [PATCH 36/71] Move in_lock ingest report worker to service --- .../security/scans/ingest_reports_service.rb | 20 ++++---- .../security/scans/ingest_reports_worker.rb | 9 +--- .../scans/ingest_reports_service_spec.rb | 48 ++++++++++++++++--- .../scans/ingest_reports_worker_spec.rb | 23 --------- 4 files changed, 55 insertions(+), 45 deletions(-) diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index eb01cb2fc2d9f6..48d473c57ed8ba 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -3,6 +3,8 @@ module Security module Scans class IngestReportsService + include Gitlab::ExclusiveLeaseHelpers + TTL_REPORT_INGESTION = 1.hour def self.execute(pipeline) @@ -14,14 +16,16 @@ def initialize(pipeline) end def execute - return if already_ingested? - - if pipeline.could_store_security_reports? - ::Security::StoreScansWorker.perform_async(pipeline.id) - ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) - else - ::Sbom::ScheduleIngestReportsService.new(pipeline).execute - ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) + in_lock("scans:#{@pipeline.project_id}:ingest_reports:#{@pipeline.id}", retries: 0) do + unless already_ingested? + if pipeline.could_store_security_reports? + ::Security::StoreScansWorker.perform_async(pipeline.id) + ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) + else + ::Sbom::ScheduleIngestReportsService.new(pipeline).execute + ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) + end + end end end diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index 929a30f6336f62..4ce4dfea4cf5bf 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -5,10 +5,6 @@ module Scans class IngestReportsWorker include ApplicationWorker include Gitlab::EventStore::Subscriber - include Gitlab::ExclusiveLeaseHelpers - - LEASE_RETRIES = 3 - LEASE_TRY_AFTER = 10.seconds feature_category :vulnerability_management urgency :low @@ -22,10 +18,7 @@ def handle_event(event) build = ::Ci::Build.find_by_id(event.data[:job_id]) return unless build - in_lock("scans:{#{build.project_id}}:ingest_reports:#{build.pipeline_id}", - retries: LEASE_RETRIES, sleep_sec: LEASE_TRY_AFTER) do - ::Security::Scans::IngestReportsService.execute(build.pipeline) if build.pipeline.completed_security_jobs? - end + ::Security::Scans::IngestReportsService.execute(build.pipeline) if build.pipeline.completed_security_jobs? end end end diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index 0367a695735215..d08dd8d7ff4f63 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -40,9 +40,13 @@ context 'when the security scans can be stored for the pipeline' do let(:could_store_security_reports) { true } - let_it_be(:pipeline) { create(:ci_pipeline, user: user) } - let_it_be(:job) do - create(:ci_build, :sast, pipeline: pipeline, status: 'success', updated_at: "2025-07-13 13:29:04 UTC") + let(:pipeline) { create(:ci_pipeline, user: user) } + let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } + + before do + Gitlab::Redis::SharedState.with do |redis| + redis.del(pipeline.scans_cache_key) + end end it 'schedules store security scans job and does not ingest the SBOM reports' do @@ -53,6 +57,9 @@ end it 'already ingested' do + ::Gitlab::Redis::SharedState.with do |redis| + !redis.set(pipeline.scans_cache_key, 'OK', nx: true, ex: described_class::TTL_REPORT_INGESTION) + end ingest_security_scans expect(::Security::StoreScansWorker).not_to have_received(:perform_async) @@ -62,9 +69,13 @@ context 'when the security scans can not be stored for the pipeline' do let(:could_store_security_reports) { false } - let_it_be(:pipeline) { create(:ci_pipeline, user: user) } - let_it_be(:job) do - create(:ci_build, :sast, pipeline: pipeline, status: 'success', updated_at: "2025-07-13 14:29:04 UTC") + let(:pipeline) { create(:ci_pipeline, user: user) } + let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } + + before do + Gitlab::Redis::SharedState.with do |redis| + redis.del(pipeline.scans_cache_key) + end end it 'does not schedule store security scans job and to ingests sbom reports' do @@ -75,5 +86,30 @@ expect(mock_sbom_ingestion_service).to have_received(:execute) end end + + context 'when lease is taken' do + include ExclusiveLeaseHelpers + let(:could_store_security_reports) { true } + let(:pipeline) { create(:ci_pipeline, user: user) } + let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } + let(:lease_key) { "scans:#{pipeline.project_id}:ingest_reports:#{pipeline.id}" } + let(:other_pipeline) { create(:ci_pipeline) } + let(:other_job) { create(:ci_build, :sast, pipeline: other_pipeline) } + + before do + stub_exclusive_lease_taken(lease_key) + allow(other_pipeline).to receive_messages( + could_store_security_reports?: could_store_security_reports + ) + end + + it 'does not permit parallel execution on the same pipeline' do + expect { ingest_security_scans }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + end + + it 'allows parallel execution on different pipelines' do + expect { described_class.new(other_pipeline).execute }.not_to raise_error + end + end end end diff --git a/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb index f273e0f8f1b36e..e8024e980a65ed 100644 --- a/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb +++ b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb @@ -33,28 +33,5 @@ expect(::Security::Scans::IngestReportsService).to have_received(:execute).with(pipeline) end end - - context 'when lease is taken' do - include ExclusiveLeaseHelpers - - let(:lease_key) { "scans:ingest_reports:#{pipeline.id}" } - let(:other_pipeline) { create(:ci_pipeline) } - let(:other_job) { create(:ci_build, :sast, pipeline: other_pipeline) } - let(:other_event) { ::Ci::JobSecurityScanCompletedEvent.new(data: { job_id: other_job.id }) } - - before do - # Speed up retries to avoid long-running tests - stub_const("#{described_class}::LEASE_TRY_AFTER", 0.01) - stub_exclusive_lease_taken(lease_key) - end - - it 'does not permit parallel execution on the same pipeline' do - expect { handle_event }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) - end - - it 'allows parallel execution on different pipelines' do - expect { consume_event(subscriber: described_class, event: other_event) }.not_to raise_error - end - end end end -- GitLab From d1b37e98803f4ea528498cd820f7c8e9d666026b Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 17 Jul 2025 13:19:41 +0300 Subject: [PATCH 37/71] Move scans_cache_key to IngestReportsService --- ee/app/models/ee/ci/pipeline.rb | 8 -------- .../services/security/scans/ingest_reports_service.rb | 11 ++++++++++- .../security/scans/ingest_reports_service_spec.rb | 6 +++--- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index fe5027d7ede1c2..da4ad4473eff33 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -355,14 +355,6 @@ def completed_security_jobs? security_builds.all?(&:complete?) end - def scans_cache_key - sha = builds.select(&:security_job?) - .map(&:updated_at) - .join('|') - .then { |value| Digest::SHA512.hexdigest(value) } - "security:report:ingest:#{sha}" - end - def has_all_security_policies_reports? can_store_security_reports? && can_ingest_sbom_reports? end diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index 48d473c57ed8ba..b0fd4d045046c0 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -29,13 +29,22 @@ def execute end end + def scans_cache_key + sha = @pipeline.latest_builds.select(&:security_job?) + .sort_by(&:id) + .map { |build| "#{build.id}:#{build.updated_at}" } + .join('|') + .then { |value| Digest::SHA512.hexdigest(value) } + "security:report:ingest:#{sha}" + end + private attr_reader :pipeline def already_ingested? ::Gitlab::Redis::SharedState.with do |redis| - !redis.set(@pipeline.scans_cache_key, 'OK', nx: true, ex: TTL_REPORT_INGESTION) + !redis.set(scans_cache_key, 'OK', nx: true, ex: TTL_REPORT_INGESTION) end end end diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index d08dd8d7ff4f63..31bcce79bb380f 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -45,7 +45,7 @@ before do Gitlab::Redis::SharedState.with do |redis| - redis.del(pipeline.scans_cache_key) + redis.del(service_object.scans_cache_key) end end @@ -58,7 +58,7 @@ it 'already ingested' do ::Gitlab::Redis::SharedState.with do |redis| - !redis.set(pipeline.scans_cache_key, 'OK', nx: true, ex: described_class::TTL_REPORT_INGESTION) + !redis.set(service_object.scans_cache_key, 'OK', nx: true, ex: described_class::TTL_REPORT_INGESTION) end ingest_security_scans @@ -74,7 +74,7 @@ before do Gitlab::Redis::SharedState.with do |redis| - redis.del(pipeline.scans_cache_key) + redis.del(service_object.scans_cache_key) end end -- GitLab From b854b7eaf89681cb2945cf0cb88357298b8c1768 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 17 Jul 2025 23:56:45 +0300 Subject: [PATCH 38/71] Add test for could_store_security_reports? --- ee/spec/models/ci/pipeline_spec.rb | 43 ++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/ee/spec/models/ci/pipeline_spec.rb b/ee/spec/models/ci/pipeline_spec.rb index 36d807c80b3d1f..326d1af9d59a35 100644 --- a/ee/spec/models/ci/pipeline_spec.rb +++ b/ee/spec/models/ci/pipeline_spec.rb @@ -1007,6 +1007,49 @@ end end + describe '#could_store_security_reports?', feature_category: :vulnerability_management do + subject { pipeline.could_store_security_reports? } + + let(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project) } + + context 'when the security reports can not be stored for the project' do + before do + allow(project).to receive(:can_store_security_reports?).and_return(false) + end + + context 'when the pipeline does not have security jobs' do + it { is_expected.to be_falsy } + end + + context 'when the pipeline has security jobs' do + before do + create(:ci_build, :success, :sast, pipeline: pipeline, project: project) + end + + it { is_expected.to be_falsy } + end + end + + context 'when the security reports can be stored for the project' do + before do + allow(project).to receive(:can_store_security_reports?).and_return(true) + end + + context 'when the pipeline does not have security jobs' do + it { is_expected.to be_falsy } + end + + context 'when the pipeline has security jobs' do + before do + stage = create(:ci_stage) + create(:ee_ci_build, :sast, pipeline: pipeline, project: project, ci_stage: stage) + end + + it { is_expected.to be_truthy } + end + end + end + describe '#has_depdency_scanning_reports?', feature_category: :security_policy_management do let_it_be(:pipeline) do create(:ee_ci_pipeline, :success, :with_dependency_scanning_report) -- GitLab From 43d0984fb7d4ee75e1409ad16c0e8f726c2ea4a4 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Sun, 20 Jul 2025 16:42:45 +0300 Subject: [PATCH 39/71] Add method to ensure that reports are ready to ingest --- .../security/scans/ingest_reports_service.rb | 21 ++++++++++++++++++- .../scans/ingest_reports_service_spec.rb | 2 ++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index b0fd4d045046c0..caa2c4122ced01 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -6,6 +6,8 @@ class IngestReportsService include Gitlab::ExclusiveLeaseHelpers TTL_REPORT_INGESTION = 1.hour + ENSURE_REPORTS_RETRIES = 6 + ENSURE_REPORTS_SLEEP = 10.seconds def self.execute(pipeline) new(pipeline).execute @@ -19,6 +21,7 @@ def execute in_lock("scans:#{@pipeline.project_id}:ingest_reports:#{@pipeline.id}", retries: 0) do unless already_ingested? if pipeline.could_store_security_reports? + ensure_security_reports_available! ::Security::StoreScansWorker.perform_async(pipeline.id) ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) else @@ -30,7 +33,7 @@ def execute end def scans_cache_key - sha = @pipeline.latest_builds.select(&:security_job?) + sha = pipeline.latest_builds.select(&:security_job?) .sort_by(&:id) .map { |build| "#{build.id}:#{build.updated_at}" } .join('|') @@ -47,6 +50,22 @@ def already_ingested? !redis.set(scans_cache_key, 'OK', nx: true, ex: TTL_REPORT_INGESTION) end end + + def ensure_security_reports_available! + retries = 0 + + loop do + return if pipeline.has_security_reports_ready? + + if retries >= ENSURE_REPORTS_RETRIES + raise(StandardError, + "Security reports not available after #{ENSURE_REPORTS_RETRIES} retries for pipeline #{pipeline.id}") + end + + sleep(ENSURE_REPORTS_SLEEP) + retries += 1 + end + end end end end diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index 31bcce79bb380f..15bd9b7c085452 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -47,6 +47,7 @@ Gitlab::Redis::SharedState.with do |redis| redis.del(service_object.scans_cache_key) end + allow(pipeline).to receive(:has_security_reports_ready?).and_return(true) end it 'schedules store security scans job and does not ingest the SBOM reports' do @@ -101,6 +102,7 @@ allow(other_pipeline).to receive_messages( could_store_security_reports?: could_store_security_reports ) + allow(other_pipeline).to receive(:has_security_reports_ready?).and_return(true) end it 'does not permit parallel execution on the same pipeline' do -- GitLab From 87814a9177b313cfa0e865b2333cc36fc0250cd6 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Mon, 21 Jul 2025 08:31:19 +0300 Subject: [PATCH 40/71] Add test for has_security_reports_ready? --- ee/spec/models/ci/pipeline_spec.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/ee/spec/models/ci/pipeline_spec.rb b/ee/spec/models/ci/pipeline_spec.rb index 326d1af9d59a35..1dee1d1871254f 100644 --- a/ee/spec/models/ci/pipeline_spec.rb +++ b/ee/spec/models/ci/pipeline_spec.rb @@ -1104,6 +1104,23 @@ end end + describe '#has_security_reports_ready?', feature_category: :vulnerability_management do + let_it_be(:pipeline) { create(:ci_pipeline, :running) } + let_it_be(:build_sast) { create(:ci_build, :success, name: 'sast', pipeline: pipeline) } + + subject(:has_security_reports_ready?) { pipeline.has_security_reports_ready? } + + context 'when the pipeline does not have security scan reports' do + it { is_expected.to be_falsey } + end + + context 'when the pipeline does have security scan reports' do + let_it_be(:artifact_sast) { create(:ee_ci_job_artifact, :sast, job: build_sast) } + + it { is_expected.to be_truthy } + end + end + describe '#opened_merge_requests_with_head_sha' do let_it_be(:non_head_sha) { OpenSSL::Digest.hexdigest('SHA256', 'foo') } let_it_be(:merge_request) do -- GitLab From 9f66b010041a6266b8b2b92ca36ec000b638a15d Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 22 Jul 2025 15:44:57 +0300 Subject: [PATCH 41/71] Move scans_cache_key to private --- ee/app/services/security/scans/ingest_reports_service.rb | 8 ++++---- .../security/scans/ingest_reports_service_spec.rb | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index caa2c4122ced01..91fb80300d81c0 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -32,6 +32,10 @@ def execute end end + private + + attr_reader :pipeline + def scans_cache_key sha = pipeline.latest_builds.select(&:security_job?) .sort_by(&:id) @@ -41,10 +45,6 @@ def scans_cache_key "security:report:ingest:#{sha}" end - private - - attr_reader :pipeline - def already_ingested? ::Gitlab::Redis::SharedState.with do |redis| !redis.set(scans_cache_key, 'OK', nx: true, ex: TTL_REPORT_INGESTION) diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index 15bd9b7c085452..7dbe2ab956672a 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -45,7 +45,7 @@ before do Gitlab::Redis::SharedState.with do |redis| - redis.del(service_object.scans_cache_key) + redis.del(service_object.send(:scans_cache_key)) end allow(pipeline).to receive(:has_security_reports_ready?).and_return(true) end @@ -59,7 +59,7 @@ it 'already ingested' do ::Gitlab::Redis::SharedState.with do |redis| - !redis.set(service_object.scans_cache_key, 'OK', nx: true, ex: described_class::TTL_REPORT_INGESTION) + !redis.set(service_object.send(:scans_cache_key), 'OK', nx: true, ex: described_class::TTL_REPORT_INGESTION) end ingest_security_scans @@ -75,7 +75,7 @@ before do Gitlab::Redis::SharedState.with do |redis| - redis.del(service_object.scans_cache_key) + redis.del(service_object.send(:scans_cache_key)) end end -- GitLab From 31c3bf6684e01620e5858d3bd2d334e817430844 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 22 Jul 2025 17:12:02 +0300 Subject: [PATCH 42/71] Add to context for when reports aren't available --- .../scans/ingest_reports_service_spec.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index 7dbe2ab956672a..c6bea087b314d9 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -38,6 +38,23 @@ .with(pipeline).and_return(mock_sbom_ingestion_service) end + context 'when reports are not available' do + let(:could_store_security_reports) { true } + let(:pipeline) { create(:ci_pipeline, user: user) } + let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } + + before do + stub_const("Security::Scans::IngestReportsService::ENSURE_REPORTS_SLEEP", 0.01) + Gitlab::Redis::SharedState.with do |redis| + redis.del(service_object.send(:scans_cache_key)) + end + end + + it 'does not ingest and raises error' do + expect { ingest_security_scans }.to raise_error(StandardError) + end + end + context 'when the security scans can be stored for the pipeline' do let(:could_store_security_reports) { true } let(:pipeline) { create(:ci_pipeline, user: user) } -- GitLab From 168f1c3da910c9e8c72c0393d4ecfc2e768a5e9d Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 23 Jul 2025 15:11:31 +0300 Subject: [PATCH 43/71] Updated completed_security_jobs? to ingore manual non-blocking jobs --- ee/app/models/ee/ci/pipeline.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index da4ad4473eff33..64591b0d0b292e 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -352,7 +352,8 @@ def has_security_jobs? def completed_security_jobs? security_builds = builds.select(&:security_job?) - security_builds.all?(&:complete?) + blocking_manual_jobs = security_builds.select(&:manual?).reject(&:allow_failure?) + security_builds.reject(&:manual?).all?(&:complete?) && blocking_manual_jobs.empty? end def has_all_security_policies_reports? -- GitLab From e13736fd7eb588908d53cd60a7ce7f7f50d1c0b0 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Fri, 25 Jul 2025 15:26:48 +0300 Subject: [PATCH 44/71] Remove method could_store_security_reports? and ensure_security_reports_available --- ee/app/models/ee/ci/pipeline.rb | 14 ----- .../security/scans/ingest_reports_service.rb | 19 +----- ee/app/workers/security/store_scans_worker.rb | 2 +- ee/spec/models/ci/pipeline_spec.rb | 60 ------------------- .../scans/ingest_reports_service_spec.rb | 6 +- .../security/store_scans_worker_spec.rb | 7 ++- 6 files changed, 10 insertions(+), 98 deletions(-) diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index 64591b0d0b292e..8cb8b279d933f5 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -269,10 +269,6 @@ def can_store_security_reports? project.can_store_security_reports? && has_security_reports? end - def could_store_security_reports? - project.can_store_security_reports? && has_security_jobs? - end - # We want all the `security_findings` records for a particular pipeline to be stored in # the same partition, therefore, we check if the pipeline already has a `security_scan`. # @@ -340,16 +336,6 @@ def has_security_reports? complete_or_manual_and_has_reports?(::Ci::JobArtifact.with_file_types(security_and_license_scanning_file_types)) end - def has_security_reports_ready? - security_and_license_scanning_file_types = EE::Enums::Ci::JobArtifact.all_security_report_file_types - - has_reports?(::Ci::JobArtifact.with_file_types(security_and_license_scanning_file_types)) - end - - def has_security_jobs? - builds.any?(&:security_job?) - end - def completed_security_jobs? security_builds = builds.select(&:security_job?) blocking_manual_jobs = security_builds.select(&:manual?).reject(&:allow_failure?) diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index 91fb80300d81c0..4e20b02e6f9b5a 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -20,8 +20,7 @@ def initialize(pipeline) def execute in_lock("scans:#{@pipeline.project_id}:ingest_reports:#{@pipeline.id}", retries: 0) do unless already_ingested? - if pipeline.could_store_security_reports? - ensure_security_reports_available! + if pipeline.project.can_store_security_reports? ::Security::StoreScansWorker.perform_async(pipeline.id) ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) else @@ -50,22 +49,6 @@ def already_ingested? !redis.set(scans_cache_key, 'OK', nx: true, ex: TTL_REPORT_INGESTION) end end - - def ensure_security_reports_available! - retries = 0 - - loop do - return if pipeline.has_security_reports_ready? - - if retries >= ENSURE_REPORTS_RETRIES - raise(StandardError, - "Security reports not available after #{ENSURE_REPORTS_RETRIES} retries for pipeline #{pipeline.id}") - end - - sleep(ENSURE_REPORTS_SLEEP) - retries += 1 - end - end end end end diff --git a/ee/app/workers/security/store_scans_worker.rb b/ee/app/workers/security/store_scans_worker.rb index d9e2b7282cb20a..0d66b0c00a634b 100644 --- a/ee/app/workers/security/store_scans_worker.rb +++ b/ee/app/workers/security/store_scans_worker.rb @@ -14,7 +14,7 @@ class StoreScansWorker # rubocop:disable Scalability/IdempotentWorker def perform(pipeline_id) ::Ci::Pipeline.find_by_id(pipeline_id).try do |pipeline| - break unless pipeline.has_security_reports_ready? + break unless pipeline.project.can_store_security_reports? Security::StoreScansService.execute(pipeline) end diff --git a/ee/spec/models/ci/pipeline_spec.rb b/ee/spec/models/ci/pipeline_spec.rb index 1dee1d1871254f..36d807c80b3d1f 100644 --- a/ee/spec/models/ci/pipeline_spec.rb +++ b/ee/spec/models/ci/pipeline_spec.rb @@ -1007,49 +1007,6 @@ end end - describe '#could_store_security_reports?', feature_category: :vulnerability_management do - subject { pipeline.could_store_security_reports? } - - let(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project) } - - context 'when the security reports can not be stored for the project' do - before do - allow(project).to receive(:can_store_security_reports?).and_return(false) - end - - context 'when the pipeline does not have security jobs' do - it { is_expected.to be_falsy } - end - - context 'when the pipeline has security jobs' do - before do - create(:ci_build, :success, :sast, pipeline: pipeline, project: project) - end - - it { is_expected.to be_falsy } - end - end - - context 'when the security reports can be stored for the project' do - before do - allow(project).to receive(:can_store_security_reports?).and_return(true) - end - - context 'when the pipeline does not have security jobs' do - it { is_expected.to be_falsy } - end - - context 'when the pipeline has security jobs' do - before do - stage = create(:ci_stage) - create(:ee_ci_build, :sast, pipeline: pipeline, project: project, ci_stage: stage) - end - - it { is_expected.to be_truthy } - end - end - end - describe '#has_depdency_scanning_reports?', feature_category: :security_policy_management do let_it_be(:pipeline) do create(:ee_ci_pipeline, :success, :with_dependency_scanning_report) @@ -1104,23 +1061,6 @@ end end - describe '#has_security_reports_ready?', feature_category: :vulnerability_management do - let_it_be(:pipeline) { create(:ci_pipeline, :running) } - let_it_be(:build_sast) { create(:ci_build, :success, name: 'sast', pipeline: pipeline) } - - subject(:has_security_reports_ready?) { pipeline.has_security_reports_ready? } - - context 'when the pipeline does not have security scan reports' do - it { is_expected.to be_falsey } - end - - context 'when the pipeline does have security scan reports' do - let_it_be(:artifact_sast) { create(:ee_ci_job_artifact, :sast, job: build_sast) } - - it { is_expected.to be_truthy } - end - end - describe '#opened_merge_requests_with_head_sha' do let_it_be(:non_head_sha) { OpenSSL::Digest.hexdigest('SHA256', 'foo') } let_it_be(:merge_request) do diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index c6bea087b314d9..78db8a35723d61 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -64,7 +64,8 @@ Gitlab::Redis::SharedState.with do |redis| redis.del(service_object.send(:scans_cache_key)) end - allow(pipeline).to receive(:has_security_reports_ready?).and_return(true) + allow(pipeline).to receive_message_chain(:project, + :can_store_security_reports?).and_return(true) end it 'schedules store security scans job and does not ingest the SBOM reports' do @@ -119,7 +120,8 @@ allow(other_pipeline).to receive_messages( could_store_security_reports?: could_store_security_reports ) - allow(other_pipeline).to receive(:has_security_reports_ready?).and_return(true) + allow(pipeline).to receive_message_chain(:project, + :can_store_security_reports?).and_return(true) end it 'does not permit parallel execution on the same pipeline' do diff --git a/ee/spec/workers/security/store_scans_worker_spec.rb b/ee/spec/workers/security/store_scans_worker_spec.rb index b5ebf5e4113e2e..d0513d33c377a8 100644 --- a/ee/spec/workers/security/store_scans_worker_spec.rb +++ b/ee/spec/workers/security/store_scans_worker_spec.rb @@ -13,12 +13,13 @@ before do allow(Security::StoreScansService).to receive(:execute) allow_next_found_instance_of(Ci::Pipeline) do |record| - allow(record).to receive(:has_security_reports_ready?).and_return(has_security_reports_ready) + allow(record).to receive_message_chain(:project, + :can_store_security_reports?).and_return(can_store_security_reports) end end context 'when security reports can not be stored for the pipeline' do - let(:has_security_reports_ready) { false } + let(:can_store_security_reports) { false } it 'does not call `Security::StoreScansService`' do run_worker @@ -28,7 +29,7 @@ end context 'when security reports can be stored for the pipeline' do - let(:has_security_reports_ready) { true } + let(:can_store_security_reports) { true } it 'calls `Security::StoreScansService`' do run_worker -- GitLab From bd4d595a7947e7d05e953232ad9167440bd28a6c Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Sun, 27 Jul 2025 11:32:14 +0300 Subject: [PATCH 45/71] Remoce test for when reports are not available --- .../security/scans/ingest_reports_service.rb | 2 -- .../scans/ingest_reports_service_spec.rb | 29 ++----------------- 2 files changed, 3 insertions(+), 28 deletions(-) diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index 4e20b02e6f9b5a..4b79531bcb591e 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -6,8 +6,6 @@ class IngestReportsService include Gitlab::ExclusiveLeaseHelpers TTL_REPORT_INGESTION = 1.hour - ENSURE_REPORTS_RETRIES = 6 - ENSURE_REPORTS_SLEEP = 10.seconds def self.execute(pipeline) new(pipeline).execute diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index 78db8a35723d61..99af213a04fa4f 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -31,32 +31,11 @@ before do allow(::Security::StoreScansWorker).to receive(:perform_async) - allow(pipeline).to receive_messages( - could_store_security_reports?: could_store_security_reports - ) allow(::Sbom::ScheduleIngestReportsService).to receive(:new) .with(pipeline).and_return(mock_sbom_ingestion_service) end - context 'when reports are not available' do - let(:could_store_security_reports) { true } - let(:pipeline) { create(:ci_pipeline, user: user) } - let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } - - before do - stub_const("Security::Scans::IngestReportsService::ENSURE_REPORTS_SLEEP", 0.01) - Gitlab::Redis::SharedState.with do |redis| - redis.del(service_object.send(:scans_cache_key)) - end - end - - it 'does not ingest and raises error' do - expect { ingest_security_scans }.to raise_error(StandardError) - end - end - context 'when the security scans can be stored for the pipeline' do - let(:could_store_security_reports) { true } let(:pipeline) { create(:ci_pipeline, user: user) } let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } @@ -87,7 +66,6 @@ end context 'when the security scans can not be stored for the pipeline' do - let(:could_store_security_reports) { false } let(:pipeline) { create(:ci_pipeline, user: user) } let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } @@ -108,7 +86,6 @@ context 'when lease is taken' do include ExclusiveLeaseHelpers - let(:could_store_security_reports) { true } let(:pipeline) { create(:ci_pipeline, user: user) } let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } let(:lease_key) { "scans:#{pipeline.project_id}:ingest_reports:#{pipeline.id}" } @@ -117,11 +94,10 @@ before do stub_exclusive_lease_taken(lease_key) - allow(other_pipeline).to receive_messages( - could_store_security_reports?: could_store_security_reports - ) allow(pipeline).to receive_message_chain(:project, :can_store_security_reports?).and_return(true) + allow(::Sbom::ScheduleIngestReportsService).to receive(:new) + .with(other_pipeline).and_return(mock_sbom_ingestion_service) end it 'does not permit parallel execution on the same pipeline' do @@ -130,6 +106,7 @@ it 'allows parallel execution on different pipelines' do expect { described_class.new(other_pipeline).execute }.not_to raise_error + expect(::Sbom::ScheduleIngestReportsService).to have_received(:new).with(other_pipeline) end end end -- GitLab From 1632994227e24c6451b3558038c3b6125e04a166 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 30 Jul 2025 13:44:34 +0300 Subject: [PATCH 46/71] Fix comments from review --- app/models/ci/build.rb | 18 +++++++++--------- ee/app/models/ee/ci/build.rb | 7 ++----- .../security/scans/ingest_reports_service.rb | 2 ++ .../security/scans/ingest_reports_worker.rb | 2 +- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index fc995a0e4bd630..3d215ab8d8c4fa 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -956,15 +956,6 @@ def supported_runner?(features) end end - def reports_definitions - options.dig(:artifacts, :reports) - end - - def reports_file_types - reports_definitions&.keys - end - strong_memoize_attr :reports_file_types - def publishes_artifacts_reports? reports_definitions&.any? end @@ -1214,6 +1205,15 @@ def run_status_commit_hooks! private + def reports_definitions + options.dig(:artifacts, :reports) + end + + def reports_file_types + reports_definitions&.keys&.map(&:to_s) + end + strong_memoize_attr :reports_file_types + def use_jwt_for_ci_cd_job_token? namespace&.root_ancestor&.namespace_settings&.jwt_ci_cd_job_token_enabled? end diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 21bc713aabde46..9aa711dcb74ee2 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -216,12 +216,9 @@ def pages strong_memoize_attr :pages def security_job? - return false unless reports_file_types&.map(&:to_s)&.any? + return false unless publishes_artifacts_reports? - security_types = ::EE::Enums::Ci::JobArtifact.all_security_report_file_types - common_types = security_types & reports_file_types.map(&:to_s) - - common_types.any? + ::EE::Enums::Ci::JobArtifact.all_security_report_file_types.intersect?(reports_file_types) end strong_memoize_attr :security_job? diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index 4b79531bcb591e..72ae87296f3890 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -16,6 +16,8 @@ def initialize(pipeline) end def execute + return unless @pipeline.completed_security_jobs? + in_lock("scans:#{@pipeline.project_id}:ingest_reports:#{@pipeline.id}", retries: 0) do unless already_ingested? if pipeline.project.can_store_security_reports? diff --git a/ee/app/workers/security/scans/ingest_reports_worker.rb b/ee/app/workers/security/scans/ingest_reports_worker.rb index 4ce4dfea4cf5bf..05a002191dd228 100644 --- a/ee/app/workers/security/scans/ingest_reports_worker.rb +++ b/ee/app/workers/security/scans/ingest_reports_worker.rb @@ -18,7 +18,7 @@ def handle_event(event) build = ::Ci::Build.find_by_id(event.data[:job_id]) return unless build - ::Security::Scans::IngestReportsService.execute(build.pipeline) if build.pipeline.completed_security_jobs? + ::Security::Scans::IngestReportsService.execute(build.pipeline) end end end -- GitLab From 7cfd921c94bf5f41ef4ecc187f9653dc42316fd1 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 30 Jul 2025 15:18:11 +0300 Subject: [PATCH 47/71] Fix IngestReportsWorker spec --- .../scans/ingest_reports_worker_spec.rb | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb index e8024e980a65ed..45c70a70ec6e9b 100644 --- a/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb +++ b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb @@ -16,22 +16,12 @@ it_behaves_like 'subscribes to event' describe '.handle_event' do - context 'when the job is not completed' do - it 'handle_event returns without calling service' do - handle_event + let(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } - expect(::Security::Scans::IngestReportsService).not_to have_received(:execute) - end - end - - context 'when the job has completed' do - let(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } - - it 'handle_event returns without calling service' do - handle_event + it 'handle_event calls service' do + handle_event - expect(::Security::Scans::IngestReportsService).to have_received(:execute).with(pipeline) - end + expect(::Security::Scans::IngestReportsService).to have_received(:execute).with(pipeline) end end end -- GitLab From 9e67349466bc46974e0df830f844a571cf0111fa Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 30 Jul 2025 15:43:35 +0300 Subject: [PATCH 48/71] Add test for when security jobs is still running --- .../scans/ingest_reports_service_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index 99af213a04fa4f..245882e86f5e53 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -35,6 +35,24 @@ .with(pipeline).and_return(mock_sbom_ingestion_service) end + context 'when the security scans not completed' do + let(:pipeline) { create(:ci_pipeline, user: user) } + let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'running') } + + before do + Gitlab::Redis::SharedState.with do |redis| + redis.del(service_object.send(:scans_cache_key)) + end + end + + it 'does not schedule store security scans job and to ingests sbom reports' do + ingest_security_scans + + expect(::Security::StoreScansWorker).not_to have_received(:perform_async) + expect(::Sbom::ScheduleIngestReportsService).not_to have_received(:new) + end + end + context 'when the security scans can be stored for the pipeline' do let(:pipeline) { create(:ci_pipeline, user: user) } let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } -- GitLab From f90670f196720c7d466739085edf4ce96eb3a75a Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 06:17:04 +0000 Subject: [PATCH 49/71] Rename completed_security_jobs? to all_security_jobs_complete? --- ee/app/models/ee/ci/pipeline.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index 8cb8b279d933f5..822d5729141695 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -336,7 +336,7 @@ def has_security_reports? complete_or_manual_and_has_reports?(::Ci::JobArtifact.with_file_types(security_and_license_scanning_file_types)) end - def completed_security_jobs? + def all_security_jobs_complete? security_builds = builds.select(&:security_job?) blocking_manual_jobs = security_builds.select(&:manual?).reject(&:allow_failure?) security_builds.reject(&:manual?).all?(&:complete?) && blocking_manual_jobs.empty? -- GitLab From e6b6a35efc497ae62aa9046ad0b66a09c27e8e81 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 06:17:04 +0000 Subject: [PATCH 50/71] Rename completed_security_jobs? to all_security_jobs_complete? --- ee/app/services/security/scans/ingest_reports_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index 72ae87296f3890..e736096b94c025 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -16,7 +16,7 @@ def initialize(pipeline) end def execute - return unless @pipeline.completed_security_jobs? + return unless @pipeline.all_security_jobs_complete? in_lock("scans:#{@pipeline.project_id}:ingest_reports:#{@pipeline.id}", retries: 0) do unless already_ingested? -- GitLab From 0ab6d25ec676a225c1c13b7fe3d568a9042fa51e Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 06:24:11 +0000 Subject: [PATCH 51/71] Update FF milestone --- .../beta/ingest_sec_reports_when_sec_jobs_completed.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/config/feature_flags/beta/ingest_sec_reports_when_sec_jobs_completed.yml b/ee/config/feature_flags/beta/ingest_sec_reports_when_sec_jobs_completed.yml index dd4e67590c9036..17d12bca58e5a1 100644 --- a/ee/config/feature_flags/beta/ingest_sec_reports_when_sec_jobs_completed.yml +++ b/ee/config/feature_flags/beta/ingest_sec_reports_when_sec_jobs_completed.yml @@ -4,7 +4,7 @@ description: feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/513326 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195012 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/554222 -milestone: '18.2' +milestone: '18.3' group: group::security infrastructure type: beta default_enabled: false -- GitLab From e94aace07342c7f93662ba8c53fd874560babbf6 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 09:28:53 +0300 Subject: [PATCH 52/71] Use pipeline as attribute --- ee/app/services/security/scans/ingest_reports_service.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index e736096b94c025..9ad9480514a3d5 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -16,9 +16,9 @@ def initialize(pipeline) end def execute - return unless @pipeline.all_security_jobs_complete? + return unless pipeline.all_security_jobs_complete? - in_lock("scans:#{@pipeline.project_id}:ingest_reports:#{@pipeline.id}", retries: 0) do + in_lock("scans:#{pipeline.project_id}:ingest_reports:#{pipeline.id}", retries: 0) do unless already_ingested? if pipeline.project.can_store_security_reports? ::Security::StoreScansWorker.perform_async(pipeline.id) -- GitLab From 031b51355dccfb4401ad6e8174bdca9e5e348a6b Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 10:56:56 +0300 Subject: [PATCH 53/71] Remove unneeded semaphore --- .../security/scans/ingest_reports_service.rb | 18 ++++++------- .../scans/ingest_reports_service_spec.rb | 26 ------------------- 2 files changed, 8 insertions(+), 36 deletions(-) diff --git a/ee/app/services/security/scans/ingest_reports_service.rb b/ee/app/services/security/scans/ingest_reports_service.rb index 9ad9480514a3d5..3d8edfe2e45329 100644 --- a/ee/app/services/security/scans/ingest_reports_service.rb +++ b/ee/app/services/security/scans/ingest_reports_service.rb @@ -18,16 +18,14 @@ def initialize(pipeline) def execute return unless pipeline.all_security_jobs_complete? - in_lock("scans:#{pipeline.project_id}:ingest_reports:#{pipeline.id}", retries: 0) do - unless already_ingested? - if pipeline.project.can_store_security_reports? - ::Security::StoreScansWorker.perform_async(pipeline.id) - ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) - else - ::Sbom::ScheduleIngestReportsService.new(pipeline).execute - ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) - end - end + return if already_ingested? + + if pipeline.project.can_store_security_reports? + ::Security::StoreScansWorker.perform_async(pipeline.id) + ::Security::ProcessScanEventsWorker.perform_async(pipeline.id) + else + ::Sbom::ScheduleIngestReportsService.new(pipeline).execute + ::Ci::CompareSecurityReportsService.set_security_mr_widget_to_ready(pipeline_id: pipeline.id) end end diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index 245882e86f5e53..ddf0ebe1450719 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -101,31 +101,5 @@ expect(mock_sbom_ingestion_service).to have_received(:execute) end end - - context 'when lease is taken' do - include ExclusiveLeaseHelpers - let(:pipeline) { create(:ci_pipeline, user: user) } - let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } - let(:lease_key) { "scans:#{pipeline.project_id}:ingest_reports:#{pipeline.id}" } - let(:other_pipeline) { create(:ci_pipeline) } - let(:other_job) { create(:ci_build, :sast, pipeline: other_pipeline) } - - before do - stub_exclusive_lease_taken(lease_key) - allow(pipeline).to receive_message_chain(:project, - :can_store_security_reports?).and_return(true) - allow(::Sbom::ScheduleIngestReportsService).to receive(:new) - .with(other_pipeline).and_return(mock_sbom_ingestion_service) - end - - it 'does not permit parallel execution on the same pipeline' do - expect { ingest_security_scans }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) - end - - it 'allows parallel execution on different pipelines' do - expect { described_class.new(other_pipeline).execute }.not_to raise_error - expect(::Sbom::ScheduleIngestReportsService).to have_received(:new).with(other_pipeline) - end - end end end -- GitLab From 80aabea8715c59d0f2a3c8483b21e813714fdd7d Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 12:54:14 +0300 Subject: [PATCH 54/71] Replace shared example which was only used once --- ee/spec/models/ci/build_spec.rb | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/ee/spec/models/ci/build_spec.rb b/ee/spec/models/ci/build_spec.rb index 76d5a8d0b7fb78..eb0c25bacf00c9 100644 --- a/ee/spec/models/ci/build_spec.rb +++ b/ee/spec/models/ci/build_spec.rb @@ -1269,25 +1269,6 @@ describe 'JobSecurityScanCompletedEvent publishing' do let(:build) { create(:ci_build, :running, pipeline: pipeline) } - shared_examples 'publishes JobSecurityScanCompletedEvent' do - it 'publishes the event with correct job_id when transitioning to the target state' do - expect(::Gitlab::EventStore).to receive(:publish) do |event| - expect(event).to be_an_instance_of(::Ci::JobSecurityScanCompletedEvent) - expect(event.data).to eq({ "job_id" => build.id }) - end - - build.public_send(transition_method) - end - end - - shared_examples 'does not publish JobSecurityScanCompletedEvent' do - it 'does not publish the event when transitioning to the target state' do - expect(::Gitlab::EventStore).not_to receive(:publish) - - build.public_send(transition_method) - end - end - context 'when build is a security job' do before do allow(build).to receive(:security_job?).and_return(true) @@ -1303,7 +1284,13 @@ with_them do context "when transitioning to #{params[:transition_description]}" do - it_behaves_like 'publishes JobSecurityScanCompletedEvent' + it 'publishes JobSecurityScanCompletedEvent' do + expect(::Gitlab::EventStore).to receive(:publish) do |event| + expect(event).to be_an_instance_of(::Ci::JobSecurityScanCompletedEvent) + expect(event.data).to eq({ "job_id" => build.id }) + end + build.public_send(transition_method) + end end end end @@ -1323,7 +1310,11 @@ with_them do context "when transitioning to #{params[:transition_description]}" do - it_behaves_like 'does not publish JobSecurityScanCompletedEvent' + it 'does not publish the JobSecurityScanCompletedEvent' do + expect(::Gitlab::EventStore).not_to receive(:publish) + + build.public_send(transition_method) + end end end end -- GitLab From cd0e469174da62bedf93db29ece501079808e4f3 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 13:12:24 +0300 Subject: [PATCH 55/71] Use :clean_gitlab_redis_shared_state instead of manualy deleting --- .../scans/ingest_reports_service_spec.rb | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index ddf0ebe1450719..d42f1a18f82e7d 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Security::Scans::IngestReportsService, feature_category: :vulnerability_management do +RSpec.describe Security::Scans::IngestReportsService, :clean_gitlab_redis_shared_state, feature_category: :vulnerability_management do let_it_be(:user) { create(:user) } describe '.execute' do @@ -39,12 +39,6 @@ let(:pipeline) { create(:ci_pipeline, user: user) } let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'running') } - before do - Gitlab::Redis::SharedState.with do |redis| - redis.del(service_object.send(:scans_cache_key)) - end - end - it 'does not schedule store security scans job and to ingests sbom reports' do ingest_security_scans @@ -58,9 +52,6 @@ let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } before do - Gitlab::Redis::SharedState.with do |redis| - redis.del(service_object.send(:scans_cache_key)) - end allow(pipeline).to receive_message_chain(:project, :can_store_security_reports?).and_return(true) end @@ -87,12 +78,6 @@ let(:pipeline) { create(:ci_pipeline, user: user) } let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } - before do - Gitlab::Redis::SharedState.with do |redis| - redis.del(service_object.send(:scans_cache_key)) - end - end - it 'does not schedule store security scans job and to ingests sbom reports' do ingest_security_scans -- GitLab From 6b4bcd1f7560d6f2a0304eff41ff7c8fa24b1637 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 13:20:40 +0300 Subject: [PATCH 56/71] Use cache key mock instead of service_object.send --- .../services/security/scans/ingest_reports_service_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index d42f1a18f82e7d..27b042f56e613b 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -50,10 +50,12 @@ context 'when the security scans can be stored for the pipeline' do let(:pipeline) { create(:ci_pipeline, user: user) } let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } + let(:mock_cache_key) { SecureRandom.uuid } before do allow(pipeline).to receive_message_chain(:project, :can_store_security_reports?).and_return(true) + allow(service_object).to receive(:scans_cache_key).and_return(mock_cache_key) end it 'schedules store security scans job and does not ingest the SBOM reports' do @@ -65,7 +67,7 @@ it 'already ingested' do ::Gitlab::Redis::SharedState.with do |redis| - !redis.set(service_object.send(:scans_cache_key), 'OK', nx: true, ex: described_class::TTL_REPORT_INGESTION) + !redis.set(mock_cache_key, 'OK', nx: true, ex: described_class::TTL_REPORT_INGESTION) end ingest_security_scans -- GitLab From a5fec5691a5270910672fd68c4cf5d307a0eff8e Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 13:31:31 +0300 Subject: [PATCH 57/71] Introduce a spec for security_job? --- ee/spec/models/ci/build_spec.rb | 42 +++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/ee/spec/models/ci/build_spec.rb b/ee/spec/models/ci/build_spec.rb index eb0c25bacf00c9..ebfe7eb40c49e0 100644 --- a/ee/spec/models/ci/build_spec.rb +++ b/ee/spec/models/ci/build_spec.rb @@ -372,6 +372,48 @@ end end + describe '#security_job?' do + subject { job.security_job? } + + context 'when build does not publish artifacts reports' do + before do + allow(job).to receive(:publishes_artifacts_reports?).and_return(false) + end + + it { is_expected.to be false } + end + + context 'when build publishes artifacts reports' do + before do + allow(job).to receive(:publishes_artifacts_reports?).and_return(true) + end + + context 'when build has multiple security report artifacts' do + before do + allow(job).to receive(:reports_file_types).and_return(%w[sast dependency_scanning]) + end + + it { is_expected.to be true } + end + + context 'when build has mixed security and non-security report artifacts' do + before do + allow(job).to receive(:reports_file_types).and_return(%w[sast junit]) + end + + it { is_expected.to be true } + end + + context 'when build has no security report artifacts' do + before do + allow(job).to receive(:reports_file_types).and_return(%w[junit coverage]) + end + + it { is_expected.to be false } + end + end + end + describe '#unmerged_security_reports' do subject(:security_reports) { job.unmerged_security_reports } -- GitLab From 82c466989abda0cefdee97dbb8dbfccf9149a03f Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 14:39:49 +0300 Subject: [PATCH 58/71] Add strong_memoize_attr :reports_definitions --- app/models/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 3d215ab8d8c4fa..9f3eb859161045 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -1208,11 +1208,11 @@ def run_status_commit_hooks! def reports_definitions options.dig(:artifacts, :reports) end + strong_memoize_attr :reports_definitions def reports_file_types reports_definitions&.keys&.map(&:to_s) end - strong_memoize_attr :reports_file_types def use_jwt_for_ci_cd_job_token? namespace&.root_ancestor&.namespace_settings&.jwt_ci_cd_job_token_enabled? -- GitLab From d7ef523ea5f6c3492b40f2be7b4eda306cc0830f Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 14:51:44 +0300 Subject: [PATCH 59/71] Use let_it_be instead of let --- .../scans/ingest_reports_service_spec.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index 27b042f56e613b..55ec792013f8fa 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -6,7 +6,7 @@ let_it_be(:user) { create(:user) } describe '.execute' do - let(:pipeline) { create(:ci_pipeline, user: user) } + let_it_be(:pipeline) { create(:ci_pipeline, user: user) } let(:mock_service_object) { instance_double(described_class, execute: true) } subject(:execute) { described_class.execute(pipeline) } @@ -36,8 +36,8 @@ end context 'when the security scans not completed' do - let(:pipeline) { create(:ci_pipeline, user: user) } - let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'running') } + let_it_be(:pipeline) { create(:ci_pipeline, user: user) } + let_it_be(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'running') } it 'does not schedule store security scans job and to ingests sbom reports' do ingest_security_scans @@ -48,9 +48,9 @@ end context 'when the security scans can be stored for the pipeline' do - let(:pipeline) { create(:ci_pipeline, user: user) } - let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } - let(:mock_cache_key) { SecureRandom.uuid } + let_it_be(:pipeline) { create(:ci_pipeline, user: user) } + let_it_be(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } + let_it_be(:mock_cache_key) { SecureRandom.uuid } before do allow(pipeline).to receive_message_chain(:project, @@ -77,8 +77,8 @@ end context 'when the security scans can not be stored for the pipeline' do - let(:pipeline) { create(:ci_pipeline, user: user) } - let!(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } + let_it_be(:pipeline) { create(:ci_pipeline, user: user) } + let_it_be(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } it 'does not schedule store security scans job and to ingests sbom reports' do ingest_security_scans -- GitLab From 92b63972651841646b479cc2da9b803c335630c6 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 14:52:53 +0300 Subject: [PATCH 60/71] Move run_after_commit after security_job? check --- ee/app/models/ee/ci/build.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 9aa711dcb74ee2..77e96316a3a179 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -75,13 +75,12 @@ module Build end after_transition any => ::Ci::Build.completed_statuses do |build| - if ::Feature.enabled?(:ingest_sec_reports_when_sec_jobs_completed, build.project) + if ::Feature.enabled?(:ingest_sec_reports_when_sec_jobs_completed, build.project) && + build.security_job? build.run_after_commit do - if build.security_job? - ::Gitlab::EventStore.publish( - ::Ci::JobSecurityScanCompletedEvent.new(data: { job_id: build.id }) - ) - end + ::Gitlab::EventStore.publish( + ::Ci::JobSecurityScanCompletedEvent.new(data: { job_id: build.id }) + ) end end end -- GitLab From 33a797850aac89047e12cbd0e83a95eed8dc37fb Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 15:49:26 +0300 Subject: [PATCH 61/71] Change FF type to gitlab_com_derisk --- .../ingest_sec_reports_when_sec_jobs_completed.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename ee/config/feature_flags/{beta => gitlab_com_derisk}/ingest_sec_reports_when_sec_jobs_completed.yml (93%) diff --git a/ee/config/feature_flags/beta/ingest_sec_reports_when_sec_jobs_completed.yml b/ee/config/feature_flags/gitlab_com_derisk/ingest_sec_reports_when_sec_jobs_completed.yml similarity index 93% rename from ee/config/feature_flags/beta/ingest_sec_reports_when_sec_jobs_completed.yml rename to ee/config/feature_flags/gitlab_com_derisk/ingest_sec_reports_when_sec_jobs_completed.yml index 17d12bca58e5a1..b67d6ecad2e183 100644 --- a/ee/config/feature_flags/beta/ingest_sec_reports_when_sec_jobs_completed.yml +++ b/ee/config/feature_flags/gitlab_com_derisk/ingest_sec_reports_when_sec_jobs_completed.yml @@ -6,5 +6,5 @@ introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195012 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/554222 milestone: '18.3' group: group::security infrastructure -type: beta +type: gitlab_com_derisk default_enabled: false -- GitLab From a298cdf90730c61dae25b33fbdafc1923bc61b66 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Thu, 31 Jul 2025 19:09:07 +0300 Subject: [PATCH 62/71] Remove license_scanning from security_jobs check method --- ee/app/models/ee/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 77e96316a3a179..67c9da66cbaea9 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -217,7 +217,7 @@ def pages def security_job? return false unless publishes_artifacts_reports? - ::EE::Enums::Ci::JobArtifact.all_security_report_file_types.intersect?(reports_file_types) + ::EE::Enums::Ci::JobArtifact.security_report_and_cyclonedx_report_file_types.intersect?(reports_file_types) end strong_memoize_attr :security_job? -- GitLab From 2788040e67de8aa6c94dbfb2b04856b97b5cd310 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Fri, 1 Aug 2025 07:41:51 +0300 Subject: [PATCH 63/71] Add test for when ingest_sec_reports_when_sec_jobs_completed false --- ee/spec/models/ci/pipeline_spec.rb | 41 ++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/ee/spec/models/ci/pipeline_spec.rb b/ee/spec/models/ci/pipeline_spec.rb index 36d807c80b3d1f..e8d3e17865ac97 100644 --- a/ee/spec/models/ci/pipeline_spec.rb +++ b/ee/spec/models/ci/pipeline_spec.rb @@ -215,30 +215,49 @@ let(:cache_key) { Ci::CompareSecurityReportsService.transition_cache_key(pipeline_id: pipeline_id) } before do - stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: false) allow(redis_spy).to receive(:ttl).and_return(10) # to allow event tracking Redis call end - it "sets the polling redis key for mr security widget when transitioning to: #{transition}" do - expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis_spy).at_least(:once) + context "when ingest_sec_reports_when_sec_jobs_completed set to true" do + before do + stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: true) + end - transition_pipeline + it "sets the polling redis key for mr security widget when transitioning to: #{transition}" do + expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis_spy).at_least(:once) - expect(redis_spy).to have_received(:set).with(cache_key, pipeline_id, ex: kind_of(Integer)) + transition_pipeline + + expect(redis_spy).to have_received(:set).with(cache_key, pipeline_id, ex: kind_of(Integer)) + end end - context 'when the security scans can not be stored for the pipeline' do + context "when ingest_sec_reports_when_sec_jobs_completed set to false" do before do - allow(pipeline).to receive(:can_store_security_reports?).and_return(false) + stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: false) end - it 'deletes the polling cache key' do - expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis_spy).at_least(:twice) + it "sets the polling redis key for mr security widget when transitioning to: #{transition}" do + expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis_spy).at_least(:once) transition_pipeline - expect(redis_spy).to have_received(:set).with(cache_key, pipeline_id, ex: kind_of(Integer)).once - expect(redis_spy).to have_received(:del).with(cache_key).once + expect(redis_spy).to have_received(:set).with(cache_key, pipeline_id, ex: kind_of(Integer)) + end + + context 'when the security scans can not be stored for the pipeline' do + before do + allow(pipeline).to receive(:can_store_security_reports?).and_return(false) + end + + it 'deletes the polling cache key' do + expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis_spy).at_least(:twice) + + transition_pipeline + + expect(redis_spy).to have_received(:set).with(cache_key, pipeline_id, ex: kind_of(Integer)).once + expect(redis_spy).to have_received(:del).with(cache_key).once + end end end end -- GitLab From 9d23408269dff08717c1f5751b66a92331533bff Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 5 Aug 2025 08:38:50 +0300 Subject: [PATCH 64/71] Remove mocking of reports_file_types --- ee/spec/models/ci/build_spec.rb | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-) diff --git a/ee/spec/models/ci/build_spec.rb b/ee/spec/models/ci/build_spec.rb index ebfe7eb40c49e0..b8919a9830830e 100644 --- a/ee/spec/models/ci/build_spec.rb +++ b/ee/spec/models/ci/build_spec.rb @@ -376,38 +376,18 @@ subject { job.security_job? } context 'when build does not publish artifacts reports' do - before do - allow(job).to receive(:publishes_artifacts_reports?).and_return(false) - end - it { is_expected.to be false } end context 'when build publishes artifacts reports' do - before do - allow(job).to receive(:publishes_artifacts_reports?).and_return(true) - end - - context 'when build has multiple security report artifacts' do - before do - allow(job).to receive(:reports_file_types).and_return(%w[sast dependency_scanning]) - end - - it { is_expected.to be true } - end - - context 'when build has mixed security and non-security report artifacts' do - before do - allow(job).to receive(:reports_file_types).and_return(%w[sast junit]) - end + context 'when build has multiple security report artifact' do + let(:job) { create(:ci_build, :sast, pipeline: pipeline) } it { is_expected.to be true } end context 'when build has no security report artifacts' do - before do - allow(job).to receive(:reports_file_types).and_return(%w[junit coverage]) - end + let(:job) { create(:ci_build, :coverage_report_cobertura, pipeline: pipeline) } it { is_expected.to be false } end -- GitLab From 94775d5a3effa2d7a09d8ca6ca3db8d7fd9721d6 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 5 Aug 2025 09:28:35 +0300 Subject: [PATCH 65/71] Add expectation for non publishing events --- ee/spec/models/ci/build_spec.rb | 48 ++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/ee/spec/models/ci/build_spec.rb b/ee/spec/models/ci/build_spec.rb index b8919a9830830e..cfac99ec682531 100644 --- a/ee/spec/models/ci/build_spec.rb +++ b/ee/spec/models/ci/build_spec.rb @@ -1296,22 +1296,44 @@ allow(build).to receive(:security_job?).and_return(true) end - where(:transition_method, :transition_description) do - [ - [:success, 'success'], - [:drop, 'failed'], - [:cancel, 'canceled'] - ] + context "when actions aren't complete statuses" do + where(:transition_method, :transition_description) do + [ + [:process, 'created'], + [:enqueue, 'pending'], + [:run, 'running'] + ] + end + + with_them do + context "when transitioning to #{params[:transition_description]}" do + it 'does not publish the JobSecurityScanCompletedEvent' do + expect(::Gitlab::EventStore).not_to receive(:publish) + + build.public_send(transition_method) + end + end + end end - with_them do - context "when transitioning to #{params[:transition_description]}" do - it 'publishes JobSecurityScanCompletedEvent' do - expect(::Gitlab::EventStore).to receive(:publish) do |event| - expect(event).to be_an_instance_of(::Ci::JobSecurityScanCompletedEvent) - expect(event.data).to eq({ "job_id" => build.id }) + context "when actions are complete statuses" do + where(:transition_method, :transition_description) do + [ + [:success, 'success'], + [:drop, 'failed'], + [:cancel, 'canceled'] + ] + end + + with_them do + context "when transitioning to #{params[:transition_description]}" do + it 'publishes JobSecurityScanCompletedEvent' do + expect(::Gitlab::EventStore).to receive(:publish) do |event| + expect(event).to be_an_instance_of(::Ci::JobSecurityScanCompletedEvent) + expect(event.data).to eq({ "job_id" => build.id }) + end + build.public_send(transition_method) end - build.public_send(transition_method) end end end -- GitLab From 8926001be2d44bf7ae59b76132ebdf556b6f3bdb Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 5 Aug 2025 09:36:54 +0300 Subject: [PATCH 66/71] Remove job overwrite --- ee/spec/workers/security/scans/ingest_reports_worker_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb index 45c70a70ec6e9b..e21ade76a51d90 100644 --- a/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb +++ b/ee/spec/workers/security/scans/ingest_reports_worker_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Security::Scans::IngestReportsWorker, feature_category: :vulnerability_management do let(:pipeline) { create(:ci_pipeline) } - let(:job) { create(:ci_build, :sast, pipeline: pipeline) } + let(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } let(:event) { ::Ci::JobSecurityScanCompletedEvent.new(data: { job_id: job.id }) } subject(:handle_event) { consume_event(subscriber: described_class, event: event) } @@ -16,8 +16,6 @@ it_behaves_like 'subscribes to event' describe '.handle_event' do - let(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } - it 'handle_event calls service' do handle_event -- GitLab From 0fc048f28bdb98a2147f3cdf96379f71e09348b2 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 5 Aug 2025 11:22:30 +0300 Subject: [PATCH 67/71] Add test for when ingest_sec_reports_when_sec_jobs_completed enabled --- ee/spec/models/ci/pipeline_spec.rb | 184 ++++++++++++++++++++++------- 1 file changed, 140 insertions(+), 44 deletions(-) diff --git a/ee/spec/models/ci/pipeline_spec.rb b/ee/spec/models/ci/pipeline_spec.rb index e8d3e17865ac97..f4d7ca0130b0b3 100644 --- a/ee/spec/models/ci/pipeline_spec.rb +++ b/ee/spec/models/ci/pipeline_spec.rb @@ -274,28 +274,59 @@ subject(:transition_pipeline) { pipeline.update!(status_event: transition) } before do - stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: false) allow(::Security::StoreScansWorker).to receive(:perform_async) allow(pipeline).to receive(:can_store_security_reports?).and_return(can_store_security_reports) end - context 'when the security scans can be stored for the pipeline' do - let(:can_store_security_reports) { true } + context 'when ingest_sec_reports_when_sec_jobs_completed flag is disabled' do + before do + stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: false) + end - it 'schedules store security scans job' do - transition_pipeline + context 'when the security scans can be stored for the pipeline' do + let(:can_store_security_reports) { true } - expect(::Security::StoreScansWorker).to have_received(:perform_async).with(pipeline.id) + it 'schedules store security scans job' do + transition_pipeline + + expect(::Security::StoreScansWorker).to have_received(:perform_async).with(pipeline.id) + end + end + + context 'when the security scans can not be stored for the pipeline' do + let(:can_store_security_reports) { false } + + it 'does not schedule store security scans job' do + transition_pipeline + + expect(::Security::StoreScansWorker).not_to have_received(:perform_async) + end end end - context 'when the security scans can not be stored for the pipeline' do - let(:can_store_security_reports) { false } + context 'when ingest_sec_reports_when_sec_jobs_completed flag is enabled' do + before do + stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: true) + end + + context 'when the security scans can be stored for the pipeline' do + let(:can_store_security_reports) { true } - it 'does not schedule store security scans job' do - transition_pipeline + it 'does not schedule store security scans job' do + transition_pipeline - expect(::Security::StoreScansWorker).not_to have_received(:perform_async) + expect(::Security::StoreScansWorker).not_to have_received(:perform_async) + end + end + + context 'when the security scans can not be stored for the pipeline' do + let(:can_store_security_reports) { false } + + it 'does not schedule store security scans job' do + transition_pipeline + + expect(::Security::StoreScansWorker).not_to have_received(:perform_async) + end end end end @@ -367,61 +398,126 @@ subject(:transition_pipeline) { pipeline.update!(status_event: transition) } before do - stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: false) allow(::Sbom::ScheduleIngestReportsService).to receive(:new).with(pipeline).and_return(sbom_ingestion_scheduler) end - shared_examples_for 'ingesting sbom reports' do - context 'when security reports are available' do - before do - allow(pipeline).to receive(:can_store_security_reports?).and_return(true) + context 'when ingest_sec_reports_when_sec_jobs_completed flag is disabled' do + before do + stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: false) + end + + shared_examples_for 'ingesting sbom reports' do + context 'when security reports are available' do + before do + allow(pipeline).to receive(:can_store_security_reports?).and_return(true) + end + + it 'does not try to ingest the SBOM reports' do + transition_pipeline + + expect(::Sbom::ScheduleIngestReportsService).not_to have_received(:new) + end end - it 'does not try to ingest the SBOM reports' do - transition_pipeline + context 'when security reports are not available' do + before do + allow(pipeline).to receive(:can_store_security_reports?).and_return(false) + end + + it 'tries to ingest sbom reports' do + transition_pipeline - expect(::Sbom::ScheduleIngestReportsService).not_to have_received(:new) + expect(::Sbom::ScheduleIngestReportsService).to have_received(:new).with(pipeline) + expect(sbom_ingestion_scheduler).to have_received(:execute) + end end end - context 'when security reports are not available' do - before do - allow(pipeline).to receive(:can_store_security_reports?).and_return(false) + context 'when transitioning to completed or blocked status' do + where(:transition) { %i[succeed drop skip cancel block] } + + with_them do + it_behaves_like 'ingesting sbom reports' end + end - it 'tries to ingest sbom reports' do - transition_pipeline + context 'when transitioning to a non-completed status except block' do + where(:transition) do + %i[ + enqueue + request_resource + prepare + run + delay + ] + end - expect(::Sbom::ScheduleIngestReportsService).to have_received(:new).with(pipeline) - expect(sbom_ingestion_scheduler).to have_received(:execute) + with_them do + it 'does not try to ingest sbom reports' do + transition_pipeline + + expect(::Sbom::ScheduleIngestReportsService).not_to have_received(:new) + end end end end - context 'when transitioning to completed or blocked status' do - where(:transition) { %i[succeed drop skip cancel block] } + context 'when ingest_sec_reports_when_sec_jobs_completed flag is enabled' do + before do + stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: true) + end + + shared_examples_for 'ingesting sbom reports' do + context 'when security reports are available' do + before do + allow(pipeline).to receive(:can_store_security_reports?).and_return(true) + end + + it 'does not try to ingest the SBOM reports' do + transition_pipeline - with_them do - it_behaves_like 'ingesting sbom reports' + expect(::Sbom::ScheduleIngestReportsService).not_to have_received(:new) + end + end + + context 'when security reports are not available' do + before do + allow(pipeline).to receive(:can_store_security_reports?).and_return(false) + end + + it 'tries to ingest sbom reports' do + transition_pipeline + + expect(::Sbom::ScheduleIngestReportsService).not_to have_received(:new) + end + end end - end - context 'when transitioning to a non-completed status except block' do - where(:transition) do - %i[ - enqueue - request_resource - prepare - run - delay - ] + context 'when transitioning to completed or blocked status' do + where(:transition) { %i[succeed drop skip cancel block] } + + with_them do + it_behaves_like 'ingesting sbom reports' + end end - with_them do - it 'does not try to ingest sbom reports' do - transition_pipeline + context 'when transitioning to a non-completed status except block' do + where(:transition) do + %i[ + enqueue + request_resource + prepare + run + delay + ] + end - expect(::Sbom::ScheduleIngestReportsService).not_to have_received(:new) + with_them do + it 'does not try to ingest sbom reports' do + transition_pipeline + + expect(::Sbom::ScheduleIngestReportsService).not_to have_received(:new) + end end end end -- GitLab From 4684146e1bf46d8b4d539a74f1da7a184f4724b3 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 5 Aug 2025 14:23:39 +0300 Subject: [PATCH 68/71] Moved reports_file_types to ee version of build.rb --- app/models/ci/build.rb | 4 ---- ee/app/models/ee/ci/build.rb | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 9f3eb859161045..cfa26db8b8aabe 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -1210,10 +1210,6 @@ def reports_definitions end strong_memoize_attr :reports_definitions - def reports_file_types - reports_definitions&.keys&.map(&:to_s) - end - def use_jwt_for_ci_cd_job_token? namespace&.root_ancestor&.namespace_settings&.jwt_ci_cd_job_token_enabled? end diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 67c9da66cbaea9..de676488cbe376 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -223,6 +223,10 @@ def security_job? private + def reports_file_types + reports_definitions&.keys&.map(&:to_s) + end + def expand_pages_variables pages_config .slice(:path_prefix, :expire_in) -- GitLab From e2ae94273bdabcd497e277daba0c542c0a24d130 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Tue, 5 Aug 2025 14:26:27 +0300 Subject: [PATCH 69/71] Set can_store_security_reports? explicitely to false --- .../services/security/scans/ingest_reports_service_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ee/spec/services/security/scans/ingest_reports_service_spec.rb b/ee/spec/services/security/scans/ingest_reports_service_spec.rb index 55ec792013f8fa..f3307160e1694f 100644 --- a/ee/spec/services/security/scans/ingest_reports_service_spec.rb +++ b/ee/spec/services/security/scans/ingest_reports_service_spec.rb @@ -77,6 +77,11 @@ end context 'when the security scans can not be stored for the pipeline' do + before do + allow(pipeline).to receive_message_chain(:project, + :can_store_security_reports?).and_return(false) + end + let_it_be(:pipeline) { create(:ci_pipeline, user: user) } let_it_be(:job) { create(:ci_build, :sast, pipeline: pipeline, status: 'success') } -- GitLab From e786f0086c4565a48d05f35ae0968dc355a06638 Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 6 Aug 2025 12:58:14 +0300 Subject: [PATCH 70/71] Add test for when ingest_sec_reports_when_sec_jobs_completed enabled --- .../ci/cancel_pipeline_service_spec.rb | 46 +++++++++++++++---- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/spec/services/ci/cancel_pipeline_service_spec.rb b/spec/services/ci/cancel_pipeline_service_spec.rb index 5732aa59ab6575..820f7b37030c3f 100644 --- a/spec/services/ci/cancel_pipeline_service_spec.rb +++ b/spec/services/ci/cancel_pipeline_service_spec.rb @@ -201,20 +201,48 @@ create(:generic_commit_status, :pending, pipeline: pipeline2) end - it 'preloads relations for each build to avoid N+1 queries' do - control1 = ActiveRecord::QueryRecorder.new do - described_class.new(pipeline: pipeline1, current_user: current_user).force_execute + context 'when ingest_sec_reports_when_sec_jobs_completed is enabled' do + before do + stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: true) end - control2 = ActiveRecord::QueryRecorder.new do - described_class.new(pipeline: pipeline2, current_user: current_user).force_execute + it 'preloads relations for each build to avoid N+1 queries' do + control1 = ActiveRecord::QueryRecorder.new do + described_class.new(pipeline: pipeline1, current_user: current_user).force_execute + end + + control2 = ActiveRecord::QueryRecorder.new do + described_class.new(pipeline: pipeline2, current_user: current_user).force_execute + end + + extra_update_queries = 5 # transition ... => :canceled, queue pop + extra_generic_commit_status_validation_queries = 2 # name_uniqueness_across_types + + expect(control2.count) + .to eq(control1.count + extra_update_queries + extra_generic_commit_status_validation_queries) end + end - extra_update_queries = 5 # transition ... => :canceled, queue pop - extra_generic_commit_status_validation_queries = 2 # name_uniqueness_across_types + context 'when ingest_sec_reports_when_sec_jobs_completed is disabled' do + before do + stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: false) + end - expect(control2.count) - .to eq(control1.count + extra_update_queries + extra_generic_commit_status_validation_queries) + it 'preloads relations for each build to avoid N+1 queries' do + control1 = ActiveRecord::QueryRecorder.new do + described_class.new(pipeline: pipeline1, current_user: current_user).force_execute + end + + control2 = ActiveRecord::QueryRecorder.new do + described_class.new(pipeline: pipeline2, current_user: current_user).force_execute + end + + extra_update_queries = 4 # transition ... => :canceled, queue pop + extra_generic_commit_status_validation_queries = 2 # name_uniqueness_across_types + + expect(control2.count) + .to eq(control1.count + extra_update_queries + extra_generic_commit_status_validation_queries) + end end end end -- GitLab From e9f1dc5db7e2886336eb46ac21e51d63dc8df60a Mon Sep 17 00:00:00 2001 From: Schmil Monderer Date: Wed, 6 Aug 2025 17:09:14 +0300 Subject: [PATCH 71/71] Preload build metadata for cancel_pipeline_service --- app/services/ci/cancel_pipeline_service.rb | 10 ++-- .../services/ee/ci/cancel_pipeline_service.rb | 16 +++++++ .../ci/cancel_pipeline_service_spec.rb | 46 ++++--------------- 3 files changed, 32 insertions(+), 40 deletions(-) create mode 100644 ee/app/services/ee/ci/cancel_pipeline_service.rb diff --git a/app/services/ci/cancel_pipeline_service.rb b/app/services/ci/cancel_pipeline_service.rb index a6a75b197d2d1c..493b33eb4167fc 100644 --- a/app/services/ci/cancel_pipeline_service.rb +++ b/app/services/ci/cancel_pipeline_service.rb @@ -94,17 +94,19 @@ def execute_async? def cancel_jobs(jobs) retries = 3 retry_lock(jobs, retries, name: 'ci_pipeline_cancel_running') do |jobs_to_cancel| - preloaded_relations = [:project, :pipeline, :deployment, :taggings] - jobs_to_cancel.find_in_batches do |batch| relation = CommitStatus.id_in(batch) - Preloaders::CommitStatusPreloader.new(relation).execute(preloaded_relations) + Preloaders::CommitStatusPreloader.new(relation).execute(build_preloads) relation.each { |job| cancel_job(job) } end end end + def build_preloads + [:project, :pipeline, :deployment, :taggings] + end + def cancel_job(job) if @auto_canceled_by_pipeline job.auto_canceled_by_id = @auto_canceled_by_pipeline.id @@ -151,3 +153,5 @@ def cancel_children end end end + +Ci::CancelPipelineService.prepend_mod diff --git a/ee/app/services/ee/ci/cancel_pipeline_service.rb b/ee/app/services/ee/ci/cancel_pipeline_service.rb new file mode 100644 index 00000000000000..aa55c1f0f94db7 --- /dev/null +++ b/ee/app/services/ee/ci/cancel_pipeline_service.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module EE + module Ci + module CancelPipelineService + extend ::Gitlab::Utils::Override + + private + + override :build_preloads + def build_preloads + super + [:metadata] + end + end + end +end diff --git a/spec/services/ci/cancel_pipeline_service_spec.rb b/spec/services/ci/cancel_pipeline_service_spec.rb index 820f7b37030c3f..c33150aebcf6be 100644 --- a/spec/services/ci/cancel_pipeline_service_spec.rb +++ b/spec/services/ci/cancel_pipeline_service_spec.rb @@ -201,48 +201,20 @@ create(:generic_commit_status, :pending, pipeline: pipeline2) end - context 'when ingest_sec_reports_when_sec_jobs_completed is enabled' do - before do - stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: true) + it 'preloads relations for each build to avoid N+1 queries' do + control1 = ActiveRecord::QueryRecorder.new do + described_class.new(pipeline: pipeline1, current_user: current_user).force_execute end - it 'preloads relations for each build to avoid N+1 queries' do - control1 = ActiveRecord::QueryRecorder.new do - described_class.new(pipeline: pipeline1, current_user: current_user).force_execute - end - - control2 = ActiveRecord::QueryRecorder.new do - described_class.new(pipeline: pipeline2, current_user: current_user).force_execute - end - - extra_update_queries = 5 # transition ... => :canceled, queue pop - extra_generic_commit_status_validation_queries = 2 # name_uniqueness_across_types - - expect(control2.count) - .to eq(control1.count + extra_update_queries + extra_generic_commit_status_validation_queries) + control2 = ActiveRecord::QueryRecorder.new do + described_class.new(pipeline: pipeline2, current_user: current_user).force_execute end - end - context 'when ingest_sec_reports_when_sec_jobs_completed is disabled' do - before do - stub_feature_flags(ingest_sec_reports_when_sec_jobs_completed: false) - end + extra_update_queries = 4 # transition ... => :canceled, queue pop + extra_generic_commit_status_validation_queries = 2 # name_uniqueness_across_types - it 'preloads relations for each build to avoid N+1 queries' do - control1 = ActiveRecord::QueryRecorder.new do - described_class.new(pipeline: pipeline1, current_user: current_user).force_execute - end - - control2 = ActiveRecord::QueryRecorder.new do - described_class.new(pipeline: pipeline2, current_user: current_user).force_execute - end - - extra_update_queries = 4 # transition ... => :canceled, queue pop - extra_generic_commit_status_validation_queries = 2 # name_uniqueness_across_types - - expect(control2.count) - .to eq(control1.count + extra_update_queries + extra_generic_commit_status_validation_queries) - end + expect(control2.count) + .to eq(control1.count + extra_update_queries + extra_generic_commit_status_validation_queries) end end end -- GitLab