Merge branch '351524_purge_security_findings_periodically' into 'master'

Implement retention period for `security_findings` records

See merge request gitlab-org/gitlab!81423

config/feature_flags/ops/purge_stale_security_findings.yml

+---
+name: purge_stale_security_findings
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/81423
+rollout_issue_url:
+milestone: '14.9'
+type: ops
+group: group::threat insights
+default_enabled: false

config/initializers/1_settings.rb

@@ -745,6 +745,9 @@ Gitlab.ee do
   Settings.cron_jobs['security_orchestration_policy_rule_schedule_worker'] ||= Settingslogic.new({})
   Settings.cron_jobs['security_orchestration_policy_rule_schedule_worker']['cron'] ||= '*/15 * * * *'
   Settings.cron_jobs['security_orchestration_policy_rule_schedule_worker']['job_class'] = 'Security::OrchestrationPolicyRuleScheduleWorker'
+  Settings.cron_jobs['security_findings_cleanup_worker'] ||= Settingslogic.new({})
+  Settings.cron_jobs['security_findings_cleanup_worker']['cron'] ||= '0 */4 * * 6,0'
+  Settings.cron_jobs['security_findings_cleanup_worker']['job_class'] = 'Security::Findings::CleanupWorker'
   Settings.cron_jobs['app_sec_dast_profile_schedule_worker'] ||= Settingslogic.new({})
   Settings.cron_jobs['app_sec_dast_profile_schedule_worker']['cron'] ||= '7-59/15 * * * *'
   Settings.cron_jobs['app_sec_dast_profile_schedule_worker']['job_class'] = 'AppSec::Dast::ProfileScheduleWorker'

ee/app/models/security/finding.rb

@@ -29,6 +29,7 @@ module Security
     scope :by_severity_levels, -> (severity_levels) { where(severity: severity_levels) }
     scope :by_confidence_levels, -> (confidence_levels) { where(confidence: confidence_levels) }
     scope :by_report_types, -> (report_types) { joins(:scan).merge(Scan.by_scan_types(report_types)) }
+    scope :by_scan, -> (scans) { where(scan: scans) }
     scope :undismissed, -> do
       where('NOT EXISTS (?)',
             Scan.select(1)

ee/app/models/security/scan.rb

@@ -4,6 +4,8 @@ module Security
   class Scan < ApplicationRecord
     include CreatedAtFilterable
 
+    STALE_AFTER = 90.days
+
     self.table_name = 'security_scans'
 
     validates :build_id, presence: true
@@ -46,6 +48,7 @@ module Security
     scope :latest_successful, -> { latest.succeeded }
     scope :by_build_ids, ->(build_ids) { where(build_id: build_ids) }
     scope :without_errors, -> { where("jsonb_array_length(COALESCE(info->'errors', '[]'::jsonb)) = 0") }
+    scope :stale, -> { succeeded.or(preparation_failed).where('created_at < ?', STALE_AFTER.ago) }
 
     delegate :name, to: :build
 

ee/app/services/security/findings/cleanup_service.rb

+# frozen_string_literal: true
+
+module Security
+  module Findings
+    class CleanupService
+      MAX_STALE_SCANS_SIZE = 50_000
+      BATCH_DELETE_SIZE = 10_000
+
+      class << self
+        def delete_stale_records
+          Security::Scan.stale.limit(MAX_STALE_SCANS_SIZE).then(&method(:execute))
+        end
+
+        def delete_by_build_ids(build_ids)
+          Security::Scan.by_build_ids(build_ids).then(&method(:execute))
+        end
+
+        def execute(security_scans)
+          new(security_scans).execute
+        end
+      end
+
+      def initialize(security_scans)
+        @security_scans = security_scans
+      end
+
+      def execute
+        security_scans.in_batches(&method(:purge)) # rubocop:disable Cop/InBatches (`each_batch` does not let us set a global limit of records to be batched)
+      end
+
+      private
+
+      attr_reader :security_scans
+
+      def purge(scan_batch)
+        delete_findings_for(scan_batch)
+
+        scan_batch.update_all(status: :purged)
+      end
+
+      def delete_findings_for(scan_batch)
+        Security::Finding.by_scan(scan_batch).each_batch(of: BATCH_DELETE_SIZE) do |finding_batch|
+          finding_batch.delete_all
+        end
+      end
+    end
+  end
+end

ee/app/workers/all_queues.yml

@@ -390,6 +390,15 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: cronjob:security_findings_cleanup
+  :worker_name: Security::Findings::CleanupWorker
+  :feature_category: :vulnerability_management
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: cronjob:security_orchestration_policy_rule_schedule
   :worker_name: Security::OrchestrationPolicyRuleScheduleWorker
   :feature_category: :security_orchestration

ee/app/workers/security/findings/cleanup_worker.rb

+# frozen_string_literal: true
+
+module Security
+  module Findings
+    class CleanupWorker
+      include ApplicationWorker
+      include CronjobQueue # rubocop: disable Scalability/CronWorkerContext
+
+      feature_category :vulnerability_management
+      data_consistency :always
+
+      idempotent!
+
+      def perform
+        return unless Feature.enabled?(:purge_stale_security_findings, type: :ops, default_enabled: :yaml)
+
+        ::Security::Findings::CleanupService.delete_stale_records
+      end
+    end
+  end
+end

ee/app/workers/security/findings/delete_by_job_id_worker.rb

@@ -14,7 +14,7 @@ module Security
       idempotent!
 
       def handle_event(event)
-        ::Security::Finding.by_build_ids(event.data[:job_ids]).delete_all
+        ::Security::Findings::CleanupService.delete_by_build_ids(event.data[:job_ids])
       end
     end
   end

ee/spec/models/security/scan_spec.rb

@@ -248,6 +248,25 @@ RSpec.describe Security::Scan do
     it { is_expected.to match_array([latest_scan]) }
   end
 
+  describe '.stale' do
+    let!(:stale_succeeded_scan) { create(:security_scan, status: :succeeded, created_at: 91.days.ago) }
+    let!(:stale_failed_scan) { create(:security_scan, status: :preparation_failed, created_at: 91.days.ago) }
+
+    subject { described_class.stale }
+
+    before do
+      create(:security_scan, status: :succeeded)
+      create(:security_scan, status: :preparation_failed)
+      create(:security_scan, status: :created, created_at: 91.days.ago)
+      create(:security_scan, status: :job_failed, created_at: 91.days.ago)
+      create(:security_scan, status: :report_error, created_at: 91.days.ago)
+      create(:security_scan, status: :preparing, created_at: 91.days.ago)
+      create(:security_scan, status: :purged, created_at: 91.days.ago)
+    end
+
+    it { is_expected.to match_array([stale_succeeded_scan, stale_failed_scan]) }
+  end
+
   describe '#report_findings' do
     let(:artifact) { create(:ee_ci_job_artifact, :dast) }
     let(:scan) { create(:security_scan, build: artifact.job) }

ee/spec/services/security/findings/cleanup_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::Findings::CleanupService do
+  describe 'class interface' do
+    let(:batchable_relation) { instance_double(Security::Scan.all.class) }
+    let(:mock_service_object) { instance_double(described_class, execute: true) }
+
+    describe '.delete_stale_records' do
+      let(:mock_relation) { instance_double(Security::Scan.all.class, limit: batchable_relation) }
+
+      subject(:delete_stale_records) { described_class.delete_stale_records }
+
+      before do
+        allow(Security::Scan).to receive(:stale).and_return(mock_relation)
+        allow(described_class).to receive(:new).and_return(mock_service_object)
+      end
+
+      it 'instantiates the service class with stale scans' do
+        delete_stale_records
+
+        expect(mock_relation).to have_received(:limit).with(50_000)
+        expect(described_class).to have_received(:new).with(batchable_relation)
+        expect(mock_service_object).to have_received(:execute)
+      end
+    end
+
+    describe '.delete_by_build_ids' do
+      let(:build_ids) { [1, 2] }
+
+      subject(:delete_by_build_ids) { described_class.delete_by_build_ids(build_ids) }
+
+      before do
+        allow(Security::Scan).to receive(:by_build_ids).and_return(batchable_relation)
+        allow(described_class).to receive(:new).and_return(mock_service_object)
+      end
+
+      it 'instantiates the service class with scans by given build ids' do
+        delete_by_build_ids
+
+        expect(Security::Scan).to have_received(:by_build_ids).with(build_ids)
+        expect(described_class).to have_received(:new).with(batchable_relation)
+        expect(mock_service_object).to have_received(:execute)
+      end
+    end
+  end
+
+  describe '#execute' do
+    let(:security_scan) { create(:security_scan) }
+    let(:relation) { Security::Scan.where(id: security_scan.id) }
+    let(:service_object) { described_class.new(relation) }
+
+    subject(:cleanup_findings) { service_object.execute }
+
+    before do
+      create_list(:security_finding, 2, scan: security_scan)
+    end
+
+    it 'deletes the findings of the given security scan object and marks the scan as purged' do
+      expect { cleanup_findings }.to change { security_scan.findings.count }.from(2).to(0)
+                                 .and change { security_scan.reload.status }.to('purged')
+    end
+  end
+end

ee/spec/workers/security/findings/cleanup_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::Findings::CleanupWorker do
+  describe '#perform' do
+    subject(:perform) { described_class.new.perform }
+
+    before do
+      allow(::Security::Findings::CleanupService).to receive(:delete_stale_records)
+
+      stub_feature_flags(purge_stale_security_findings: feature_enabled?)
+    end
+
+    context 'when the `purge_stale_security_findings` feature is disabled' do
+      let(:feature_enabled?) { false }
+
+      it 'does not initiate the cleanup job' do
+        perform
+
+        expect(::Security::Findings::CleanupService).not_to have_received(:delete_stale_records)
+      end
+    end
+
+    context 'when the `purge_stale_security_findings` feature is enabled' do
+      let(:feature_enabled?) { true }
+
+      it 'initiates the cleanup job' do
+        perform
+
+        expect(::Security::Findings::CleanupService).to have_received(:delete_stale_records)
+      end
+    end
+  end
+end

ee/spec/workers/security/findings/delete_by_job_id_worker_spec.rb

@@ -3,20 +3,18 @@
 require 'spec_helper'
 
 RSpec.describe Security::Findings::DeleteByJobIdWorker do
-  let_it_be(:security_scan1) { create(:security_scan) }
-  let_it_be(:security_scan2) { create(:security_scan) }
-  let_it_be(:security_finding_to_be_deleted) { create(:security_finding, scan: security_scan1) }
-  let_it_be(:security_finding_not_to_be_deleted) { create(:security_finding, scan: security_scan2) }
+  let(:job_ids) { [1, 2, 3] }
+  let(:event) { Ci::PipelineCreatedEvent.new(data: { job_ids: job_ids }) }
 
-  let(:event) { Ci::PipelineCreatedEvent.new(data: { job_ids: [security_scan1.build_id] }) }
+  subject(:consume_event) { described_class.new.perform(event.class.name, event.data) }
 
-  subject { consume_event(event) }
-
-  def consume_event(event)
-    described_class.new.perform(event.class.name, event.data)
+  before do
+    allow(::Security::Findings::CleanupService).to receive(:delete_by_build_ids)
   end
 
-  it 'destroys all expired artifacts' do
-    expect { subject }.to change { Security::Finding.count }.from(2).to(1)
+  it 'initiates the cleanup by build ids' do
+    consume_event
+
+    expect(::Security::Findings::CleanupService).to have_received(:delete_by_build_ids).with(job_ids)
   end
 end