Merge branch '341796_add_status_column_to_security_scans_table' into 'master'

Introduce status column for the security_scans table

See merge request gitlab-org/gitlab!71944

db/migrate/20211007113136_add_status_column_to_security_scans_table.rb

+# frozen_string_literal: true
+
+class AddStatusColumnToSecurityScansTable < Gitlab::Database::Migration[1.0]
+  def change
+    add_column :security_scans, :status, :integer, limit: 1, default: 0, null: false
+  end
+end

db/post_migrate/20211007155221_schedule_populate_status_column_of_security_scans.rb

+# frozen_string_literal: true
+
+class SchedulePopulateStatusColumnOfSecurityScans < Gitlab::Database::Migration[1.0]
+  MIGRATION = 'PopulateStatusColumnOfSecurityScans'
+  BATCH_SIZE = 10_000
+  DELAY_INTERVAL = 2.minutes
+
+  disable_ddl_transaction!
+
+  def up
+    return unless Gitlab.ee?
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      define_batchable_model('security_scans'),
+      MIGRATION,
+      DELAY_INTERVAL,
+      batch_size: BATCH_SIZE
+    )
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema_migrations/20211007113136

+7abcc243cd02a4eba77ea39cbb1b1f2de74d85e55055def9ae02c4fdeaba3d9a
\ No newline at end of file

db/schema_migrations/20211007155221

+115427979cd7ecfc14bf4f663a9afd5abc6d481d08fafc13ca7e6a8cac9ba20c
\ No newline at end of file

db/structure.sql

@@ -19054,7 +19054,8 @@ CREATE TABLE security_scans (
     info jsonb DEFAULT '{}'::jsonb NOT NULL,
     project_id bigint,
     pipeline_id bigint,
-    latest boolean DEFAULT true NOT NULL
+    latest boolean DEFAULT true NOT NULL,
+    status smallint DEFAULT 0 NOT NULL
 );
 
 CREATE SEQUENCE security_scans_id_seq

ee/app/models/security/scan.rb

@@ -27,6 +27,8 @@ module Security
       cluster_image_scanning: 8
     }
 
+    enum status: { created: 0, succeeded: 1, failed: 2 }
+
     scope :by_scan_types, -> (scan_types) { where(scan_type: sanitize_scan_types(scan_types)) }
 
     scope :scoped_project, -> { where('security_scans.project_id = projects.id') }

ee/app/services/security/store_scan_service.rb

@@ -19,9 +19,9 @@ module Security
     end
 
     def execute
-      set_security_scan_non_latest! if artifact.job.retried?
+      set_security_scan_non_latest! if job.retried?
 
-      return deduplicate if security_scan.has_errors? || !security_scan.latest?
+      return deduplicate if security_scan.has_errors? || !security_scan.latest? || !security_scan.succeeded?
 
       store_findings
     end
@@ -29,7 +29,7 @@ module Security
     private
 
     attr_reader :artifact, :known_keys, :deduplicate
-    delegate :project, to: :artifact, private: true
+    delegate :project, :job, to: :artifact, private: true
 
     def security_report
       @security_report ||= artifact.security_report.tap do |report|
@@ -42,8 +42,9 @@ module Security
     end
 
     def security_scan
-      @security_scan ||= Security::Scan.safe_find_or_create_by!(build: artifact.job, scan_type: artifact.file_type) do |scan|
+      @security_scan ||= Security::Scan.safe_find_or_create_by!(build: job, scan_type: artifact.file_type) do |scan|
         scan.processing_errors = security_report.errors.map(&:stringify_keys) if security_report.errored?
+        scan.status = job.success? ? :succeeded : :failed
       end
     end
 

ee/lib/ee/gitlab/background_migration/populate_status_column_of_security_scans.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module BackgroundMigration
+      # Backgroung migration to populate the latest column of `security_scans` records
+      module PopulateStatusColumnOfSecurityScans
+        UPDATE_BATCH_SIZE = 500
+        UPDATE_SQL = <<~SQL
+          UPDATE
+            security_scans
+          SET
+            status = (
+              CASE
+                WHEN ci_builds.status = 'success' THEN 1
+                ELSE 2
+              END
+            )
+          FROM
+            ci_builds
+          WHERE
+            ci_builds.id = security_scans.build_id AND
+            security_scans.id BETWEEN %<start_id>d AND %<end_id>d
+        SQL
+
+        class SecurityScan < ActiveRecord::Base
+          include EachBatch
+
+          scope :in_range, -> (start_id, end_id) { where(id: (start_id..end_id)) }
+        end
+
+        def perform(start_id, end_id)
+          log_info('Migration has been started', start_id: start_id, end_id: end_id)
+
+          SecurityScan.in_range(start_id, end_id).each_batch(of: UPDATE_BATCH_SIZE) do |relation|
+            batch_start, batch_end = relation.pluck("MIN(id), MAX(id)").first
+
+            update_batch(batch_start, batch_end)
+          end
+
+          log_info('Migration has been finished', start_id: start_id, end_id: end_id)
+        end
+
+        private
+
+        delegate :connection, to: ActiveRecord::Base, private: true
+        delegate :execute, :quote, to: :connection, private: true
+
+        def update_batch(batch_start, batch_end)
+          sql = format(UPDATE_SQL, start_id: quote(batch_start), end_id: quote(batch_end))
+
+          result = ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/340017') do
+            execute(sql)
+          end
+
+          log_info('Records have been updated', count: result.cmd_tuples)
+        end
+
+        def log_info(message, extra = {})
+          log_payload = extra.merge(migrator: self.class.name, message: message)
+
+          ::Gitlab::BackgroundMigration::Logger.info(**log_payload)
+        end
+      end
+    end
+  end
+end

ee/spec/lib/ee/gitlab/background_migration/populate_status_column_of_security_scans_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::PopulateStatusColumnOfSecurityScans, schema: 20211007155221 do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:pipelines) { table(:ci_pipelines) }
+  let(:builds) { table(:ci_builds) }
+  let(:security_scans) { table(:security_scans) }
+
+  let(:namespace) { namespaces.create!(name: 'foo', path: 'bar') }
+  let(:project) { projects.create!(namespace_id: namespace.id) }
+  let(:pipeline) { pipelines.create!(project_id: project.id, ref: 'master', sha: 'adf43c3a', status: 'success') }
+  let(:ci_build_1) { builds.create!(commit_id: pipeline.id, retried: false, type: 'Ci::Build', status: 'success') }
+  let(:ci_build_2) { builds.create!(commit_id: pipeline.id, retried: false, type: 'Ci::Build', status: 'failed') }
+
+  let!(:security_scan_1) { security_scans.create!(build_id: ci_build_1.id, scan_type: 1) }
+  let!(:security_scan_2) { security_scans.create!(build_id: ci_build_2.id, scan_type: 1) }
+
+  describe '#perform' do
+    subject(:migrate) { described_class.new.perform(security_scan_1.id, security_scan_2.id) }
+
+    it 'changes the status of the security_scan records' do
+      expect { migrate }.to change { security_scan_1.reload.status }.to(1)
+                        .and change { security_scan_2.reload.status }.to(2)
+    end
+  end
+end

ee/spec/services/security/store_scan_service_spec.rb

@@ -7,6 +7,10 @@ RSpec.describe Security::StoreScanService do
 
   let(:known_keys) { Set.new }
 
+  before do
+    artifact.job.update!(status: :success)
+  end
+
   describe '.execute' do
     let(:mock_service_object) { instance_double(described_class, execute: true) }
 
@@ -57,6 +61,10 @@ RSpec.describe Security::StoreScanService do
       known_keys.add(finding_key)
     end
 
+    it 'creates a succeeded security scan' do
+      expect { store_scan }.to change { Security::Scan.succeeded.count }.by(1)
+    end
+
     context 'when the `vulnerability_finding_signatures` licensed feature is available' do
       before do
         stub_licensed_features(vulnerability_finding_signatures: true)
@@ -96,6 +104,18 @@ RSpec.describe Security::StoreScanService do
       end
     end
 
+    context 'when the report is produced by a failed job' do
+      before do
+        artifact.job.update!(status: :failed)
+      end
+
+      it 'does not call the `Security::StoreFindingsMetadataService` and sets the security scan as failed' do
+        expect { store_scan }.to change { Security::Scan.failed.count }.by(1)
+
+        expect(Security::StoreFindingsMetadataService).not_to have_received(:execute)
+      end
+    end
+
     context 'when the report is produced by a retried job' do
       before do
         artifact.job.update!(retried: true)
@@ -120,7 +140,7 @@ RSpec.describe Security::StoreScanService do
       end
 
       context 'when the security scan already exists for the artifact' do
-        let_it_be(:security_scan) { create(:security_scan, build: artifact.job, scan_type: :sast) }
+        let_it_be(:security_scan) { create(:security_scan, build: artifact.job, scan_type: :sast, status: :succeeded) }
         let_it_be(:unique_security_finding) do
           create(:security_finding,
                  scan: security_scan,

lib/gitlab/background_migration/populate_status_column_of_security_scans.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    class PopulateStatusColumnOfSecurityScans # rubocop:disable Style/Documentation
+      def perform(_start_id, _end_id)
+        # no-op
+      end
+    end
+  end
+end
+
+Gitlab::BackgroundMigration::PopulateStatusColumnOfSecurityScans.prepend_mod

spec/migrations/schedule_populate_status_column_of_security_scans_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe SchedulePopulateStatusColumnOfSecurityScans do
+  before do
+    allow(Gitlab).to receive(:ee?).and_return(ee?)
+    stub_const("#{described_class.name}::BATCH_SIZE", 1)
+  end
+
+  context 'when the Gitlab instance is CE' do
+    let(:ee?) { false }
+
+    it 'does not run the migration' do
+      expect { migrate! }.not_to change { BackgroundMigrationWorker.jobs.size }
+    end
+  end
+
+  context 'when the Gitlab instance is EE' do
+    let(:ee?) { true }
+    let(:namespaces) { table(:namespaces) }
+    let(:projects) { table(:projects) }
+    let(:pipelines) { table(:ci_pipelines) }
+    let(:builds) { table(:ci_builds) }
+    let(:security_scans) { table(:security_scans) }
+
+    let(:namespace) { namespaces.create!(name: "foo", path: "bar") }
+    let(:project) { projects.create!(namespace_id: namespace.id) }
+    let(:pipeline) { pipelines.create!(project_id: project.id, ref: 'master', sha: 'adf43c3a', status: 'success') }
+    let(:ci_build) { builds.create!(commit_id: pipeline.id, retried: false, type: 'Ci::Build') }
+
+    let!(:security_scan_1) { security_scans.create!(build_id: ci_build.id, scan_type: 1) }
+    let!(:security_scan_2) { security_scans.create!(build_id: ci_build.id, scan_type: 2) }
+
+    around do |example|
+      freeze_time { Sidekiq::Testing.fake! { example.run } }
+    end
+
+    it 'schedules the background jobs', :aggregate_failures do
+      migrate!
+
+      expect(BackgroundMigrationWorker.jobs.size).to be(2)
+      expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, security_scan_1.id, security_scan_1.id)
+      expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, security_scan_2.id, security_scan_2.id)
+    end
+  end
+end