Merge branch '333414-add-columns-from-ci-builds' into 'master'

Denormalize `ci_builds` columns into `security_scans`

See merge request gitlab-org/gitlab!66963

db/migrate/20210722151951_add_columns_to_security_scans.rb

+# frozen_string_literal: true
+
+class AddColumnsToSecurityScans < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    with_lock_retries do
+      add_column :security_scans, :project_id, :bigint
+      add_column :security_scans, :pipeline_id, :bigint
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :security_scans, :project_id, :bigint
+      remove_column :security_scans, :pipeline_id, :bigint
+    end
+  end
+end

db/migrate/20210728174349_add_fk_to_security_scans_columns.rb

+# frozen_string_literal: true
+
+class AddFkToSecurityScansColumns < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :security_scans, :project_id
+    add_concurrent_foreign_key :security_scans, :projects, column: :project_id, on_delete: :cascade
+
+    add_concurrent_index :security_scans, :pipeline_id
+  end
+
+  def down
+    remove_foreign_key :security_scans, column: :project_id
+    remove_concurrent_index_by_name :security_scans, name: 'index_security_scans_on_project_id'
+
+    remove_concurrent_index_by_name :security_scans, name: 'index_security_scans_on_pipeline_id'
+  end
+end

db/post_migrate/20210811214811_schedule_copy_ci_builds_columns_to_security_scans.rb

+# frozen_string_literal: true
+
+class ScheduleCopyCiBuildsColumnsToSecurityScans < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  INTERVAL = 2.minutes.to_i
+  BATCH_SIZE = 5_000
+  MIGRATION = 'CopyCiBuildsColumnsToSecurityScans'
+
+  disable_ddl_transaction!
+
+  class SecurityScan < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'security_scans'
+  end
+
+  def up
+    SecurityScan.reset_column_information
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      SecurityScan,
+      MIGRATION,
+      INTERVAL,
+      batch_size: BATCH_SIZE,
+      track_jobs: true
+    )
+  end
+
+  def down
+    # noop
+  end
+end

db/schema_migrations/20210722151951

+7289fb2a65c1210a352991fae7fac0c8e1129a33c166d0dad6f2aed98cb672a6
\ No newline at end of file

db/schema_migrations/20210728174349

+3a56c903333f13e9e3d39e5b65a3b70fdcfbf967cdac8bff348dfb71c0fde520
\ No newline at end of file

db/schema_migrations/20210811214811

+9e66aa8fc5e2a32ce0857f7ef77e906424bdf86c49643dfc71ed1a2e353b2095
\ No newline at end of file

db/structure.sql

@@ -18133,7 +18133,9 @@ CREATE TABLE security_scans (
     updated_at timestamp with time zone NOT NULL,
     build_id bigint NOT NULL,
     scan_type smallint NOT NULL,
-    info jsonb DEFAULT '{}'::jsonb NOT NULL
+    info jsonb DEFAULT '{}'::jsonb NOT NULL,
+    project_id bigint,
+    pipeline_id bigint
 );
 
 CREATE SEQUENCE security_scans_id_seq
@@ -25160,6 +25162,10 @@ CREATE INDEX index_security_scans_on_created_at ON security_scans USING btree (c
 
 CREATE INDEX index_security_scans_on_date_created_at_and_id ON security_scans USING btree (date(timezone('UTC'::text, created_at)), id);
 
+CREATE INDEX index_security_scans_on_pipeline_id ON security_scans USING btree (pipeline_id);
+
+CREATE INDEX index_security_scans_on_project_id ON security_scans USING btree (project_id);
+
 CREATE INDEX index_self_managed_prometheus_alert_events_on_environment_id ON self_managed_prometheus_alert_events USING btree (environment_id);
 
 CREATE INDEX index_sent_notifications_on_noteable_type_noteable_id ON sent_notifications USING btree (noteable_id) WHERE ((noteable_type)::text = 'Issue'::text);
@@ -26691,6 +26697,9 @@ ALTER TABLE ONLY label_links
 ALTER TABLE ONLY project_group_links
     ADD CONSTRAINT fk_daa8cee94c FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY security_scans
+    ADD CONSTRAINT fk_dbc89265b9 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY epics
     ADD CONSTRAINT fk_dccd3f98fc FOREIGN KEY (assignee_id) REFERENCES users(id) ON DELETE SET NULL;
 

ee/app/models/security/scan.rb

@@ -42,8 +42,17 @@ module Security
 
     delegate :project, :name, to: :build
 
+    before_save :ensure_project_id_pipeline_id
+
     def has_errors?
       info&.fetch('errors', []).present?
     end
+
+    private
+
+    def ensure_project_id_pipeline_id
+      self.project_id ||= build.project_id
+      self.pipeline_id ||= build.commit_id
+    end
   end
 end

ee/spec/models/security/scan_spec.rb

@@ -108,4 +108,14 @@ RSpec.describe Security::Scan do
   end
 
   it_behaves_like 'having unique enum values'
+
+  it 'sets `project_id` and `pipeline_id` before save' do
+    scan = create(:security_scan)
+    scan.update_columns(project_id: nil, pipeline_id: nil)
+
+    scan.save!
+
+    expect(scan.project_id).to eq(scan.build.project_id)
+    expect(scan.pipeline_id).to eq(scan.build.commit_id)
+  end
 end

lib/gitlab/background_migration/copy_ci_builds_columns_to_security_scans.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class CopyCiBuildsColumnsToSecurityScans
+      extend ::Gitlab::Utils::Override
+
+      UPDATE_BATCH_SIZE = 500
+
+      def perform(start_id, stop_id)
+        (start_id..stop_id).step(UPDATE_BATCH_SIZE).each do |offset|
+          batch_start = offset
+          batch_stop = offset + UPDATE_BATCH_SIZE - 1
+
+          ActiveRecord::Base.connection.execute <<~SQL
+            UPDATE
+              security_scans
+            SET
+              project_id = ci_builds.project_id,
+              pipeline_id = ci_builds.commit_id
+            FROM ci_builds
+            WHERE ci_builds.type='Ci::Build' 
+              AND ci_builds.id=security_scans.build_id 
+              AND security_scans.id BETWEEN #{Integer(batch_start)} AND #{Integer(batch_stop)}
+          SQL
+        end
+
+        mark_job_as_succeeded(start_id, stop_id)
+      rescue StandardError => error
+        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error)
+      end
+    end
+
+    private
+
+    def mark_job_as_succeeded(*arguments)
+      Gitlab::Database::BackgroundMigrationJob.mark_all_as_succeeded(
+        'CopyCiBuildsColumnsToSecurityScans',
+        arguments
+      )
+    end
+  end
+end

spec/db/schema_spec.rb

@@ -86,7 +86,8 @@ RSpec.describe 'Database schema' do
     users: %w[color_scheme_id created_by_id theme_id email_opted_in_source_id],
     users_star_projects: %w[user_id],
     vulnerability_identifiers: %w[external_id],
-    vulnerability_scanners: %w[external_id]
+    vulnerability_scanners: %w[external_id],
+    security_scans: %w[pipeline_id] # foreign key is not added as ci_pipeline table will be moved into different db soon
   }.with_indifferent_access.freeze
 
   context 'for table' do

spec/lib/gitlab/background_migration/copy_ci_builds_columns_to_security_scans_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::CopyCiBuildsColumnsToSecurityScans, schema: 20210728174349 do
+  let(:migration) { described_class.new }
+
+  let_it_be(:namespaces) { table(:namespaces) }
+  let_it_be(:projects) { table(:projects) }
+  let_it_be(:ci_pipelines) { table(:ci_pipelines) }
+  let_it_be(:ci_builds) { table(:ci_builds) }
+  let_it_be(:security_scans) { table(:security_scans) }
+
+  let!(:namespace) { namespaces.create!(name: 'namespace', path: 'namespace') }
+  let!(:project1) { projects.create!(namespace_id: namespace.id) }
+  let!(:project2) { projects.create!(namespace_id: namespace.id) }
+  let!(:pipeline1) { ci_pipelines.create!(status: "success")}
+  let!(:pipeline2) { ci_pipelines.create!(status: "success")}
+
+  let!(:build1) { ci_builds.create!(commit_id: pipeline1.id, type: 'Ci::Build', project_id: project1.id) }
+  let!(:build2) { ci_builds.create!(commit_id: pipeline2.id, type: 'Ci::Build', project_id: project2.id) }
+  let!(:build3) { ci_builds.create!(commit_id: pipeline1.id, type: 'Ci::Build', project_id: project1.id) }
+
+  let!(:scan1) { security_scans.create!(build_id: build1.id, scan_type: 1) }
+  let!(:scan2) { security_scans.create!(build_id: build2.id, scan_type: 1) }
+  let!(:scan3) { security_scans.create!(build_id: build3.id, scan_type: 1) }
+
+  subject { migration.perform(scan1.id, scan2.id) }
+
+  before do
+    stub_const("#{described_class}::UPDATE_BATCH_SIZE", 2)
+  end
+
+  it 'copies `project_id`, `commit_id` from `ci_builds` to `security_scans`', :aggregate_failures do
+    expect(migration).to receive(:mark_job_as_succeeded).with(scan1.id, scan2.id)
+
+    subject
+
+    scan1.reload
+    expect(scan1.project_id).to eq(project1.id)
+    expect(scan1.pipeline_id).to eq(pipeline1.id)
+
+    scan2.reload
+    expect(scan2.project_id).to eq(project2.id)
+    expect(scan2.pipeline_id).to eq(pipeline2.id)
+
+    scan3.reload
+    expect(scan3.project_id).to be_nil
+    expect(scan3.pipeline_id).to be_nil
+  end
+end

spec/migrations/schedule_copy_ci_builds_columns_to_security_scans_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe ScheduleCopyCiBuildsColumnsToSecurityScans do
+  let_it_be(:namespaces) { table(:namespaces) }
+  let_it_be(:projects) { table(:projects) }
+  let_it_be(:ci_pipelines) { table(:ci_pipelines) }
+  let_it_be(:ci_builds) { table(:ci_builds) }
+  let_it_be(:security_scans) { table(:security_scans) }
+
+  let!(:namespace) { namespaces.create!(name: 'namespace', path: 'namespace') }
+  let!(:project) { projects.create!(namespace_id: namespace.id) }
+  let!(:pipeline) { ci_pipelines.create!(status: "success")}
+
+  let!(:build1) { ci_builds.create!(commit_id: pipeline.id, type: 'Ci::Build', project_id: project.id) }
+  let!(:build2) { ci_builds.create!(commit_id: pipeline.id, type: 'Ci::Build', project_id: project.id) }
+  let!(:build3) { ci_builds.create!(commit_id: pipeline.id, type: 'Ci::Build', project_id: project.id) }
+
+  let!(:scan1) { security_scans.create!(build_id: build1.id, scan_type: 1) }
+  let!(:scan2) { security_scans.create!(build_id: build2.id, scan_type: 1) }
+  let!(:scan3) { security_scans.create!(build_id: build3.id, scan_type: 1) }
+
+  before do
+    stub_const("#{described_class}::BATCH_SIZE", 2)
+    allow_next_instance_of(Gitlab::BackgroundMigration::CopyCiBuildsColumnsToSecurityScans) do |instance|
+      allow(instance).to receive(:mark_job_as_succeeded)
+    end
+  end
+
+  around do |example|
+    freeze_time { Sidekiq::Testing.fake! { example.run } }
+  end
+
+  it 'schedules background migrations', :aggregate_failures do
+    migrate!
+
+    expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+    expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, scan1.id, scan2.id)
+    expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, scan3.id, scan3.id)
+  end
+end