diff --git a/db/post_migrate/20211110143306_add_not_null_constraint_to_security_findings_uuid.rb b/db/post_migrate/20211110143306_add_not_null_constraint_to_security_findings_uuid.rb new file mode 100644 index 0000000000000000000000000000000000000000..bdb8f5cd12097fa7b0db01ec27e8bb44e813d7ee --- /dev/null +++ b/db/post_migrate/20211110143306_add_not_null_constraint_to_security_findings_uuid.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class AddNotNullConstraintToSecurityFindingsUuid < Gitlab::Database::Migration[1.0] + disable_ddl_transaction! + + def up + add_not_null_constraint( + :security_findings, + :uuid, + validate: false + ) + end + + def down + remove_not_null_constraint( + :security_findings, + :uuid + ) + end +end diff --git a/db/post_migrate/20211110151320_add_temporary_index_on_security_findings_uuid.rb b/db/post_migrate/20211110151320_add_temporary_index_on_security_findings_uuid.rb new file mode 100644 index 0000000000000000000000000000000000000000..7bc4af0ec4d728249c7213815c59e0a19e309343 --- /dev/null +++ b/db/post_migrate/20211110151320_add_temporary_index_on_security_findings_uuid.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class AddTemporaryIndexOnSecurityFindingsUuid < Gitlab::Database::Migration[1.0] + disable_ddl_transaction! + + INDEX_NAME = "tmp_index_uuid_is_null" + + def up + add_concurrent_index( + :security_findings, + :id, + where: "uuid IS NULL", + name: INDEX_NAME + ) + end + + def down + remove_concurrent_index_by_name( + :security_findings, + INDEX_NAME + ) + end +end diff --git a/db/post_migrate/20211110151350_schedule_drop_invalid_security_findings.rb b/db/post_migrate/20211110151350_schedule_drop_invalid_security_findings.rb new file mode 100644 index 0000000000000000000000000000000000000000..98e7b2a8a15a2e8bed981ddcb48d5ef1340e3ae0 --- /dev/null +++ b/db/post_migrate/20211110151350_schedule_drop_invalid_security_findings.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class ScheduleDropInvalidSecurityFindings < Gitlab::Database::Migration[1.0] + disable_ddl_transaction! + + MIGRATION = "DropInvalidSecurityFindings" + DELAY_INTERVAL = 2.minutes.to_i + BATCH_SIZE = 100_000 + SUB_BATCH_SIZE = 10_000 + + def up + queue_background_migration_jobs_by_range_at_intervals( + define_batchable_model('security_findings').where(uuid: nil), + MIGRATION, + DELAY_INTERVAL, + batch_size: BATCH_SIZE, + other_job_arguments: [SUB_BATCH_SIZE], + track_jobs: true + ) + end + + def down + # no-op + end +end diff --git a/db/schema_migrations/20211110143306 b/db/schema_migrations/20211110143306 new file mode 100644 index 0000000000000000000000000000000000000000..e1618c07f755741982ff829f5fa3f8563332bcd3 --- /dev/null +++ b/db/schema_migrations/20211110143306 @@ -0,0 +1 @@ +7724e5a2c52be99b1b40c449f25abdc23f279f5b0bdaebcfd897c39d295fda41 \ No newline at end of file diff --git a/db/schema_migrations/20211110151320 b/db/schema_migrations/20211110151320 new file mode 100644 index 0000000000000000000000000000000000000000..91f780811c3050f61ad308497277e13245a6bb30 --- /dev/null +++ b/db/schema_migrations/20211110151320 @@ -0,0 +1 @@ +dab6123f19fb44a1566a8de9c760dedec5548dd64e472a180e7748cd7c93eea9 \ No newline at end of file diff --git a/db/schema_migrations/20211110151350 b/db/schema_migrations/20211110151350 new file mode 100644 index 0000000000000000000000000000000000000000..98d590c26e9c7f42e9bc7b5b2111659907560dac --- /dev/null +++ b/db/schema_migrations/20211110151350 @@ -0,0 +1 @@ +f5e69502e582c5f30ba686f8b668d8f0ce5cf8078b0833d2eda67f5ed97ac074 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index b708e48a111aaf1e8919ee489f3391fb59f6887c..caa1f603df31f322b42543fc316f04aa57b7b3ef 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -22613,6 +22613,9 @@ ALTER TABLE ONLY chat_teams ALTER TABLE vulnerability_scanners ADD CONSTRAINT check_37608c9db5 CHECK ((char_length(vendor) <= 255)) NOT VALID; +ALTER TABLE security_findings + ADD CONSTRAINT check_6c2851a8c9 CHECK ((uuid IS NOT NULL)) NOT VALID; + ALTER TABLE sprints ADD CONSTRAINT check_ccd8a1eae0 CHECK ((start_date IS NOT NULL)) NOT VALID; @@ -27722,6 +27725,8 @@ CREATE UNIQUE INDEX tmp_index_on_tmp_project_id_on_namespaces ON namespaces USIN CREATE INDEX tmp_index_on_vulnerabilities_non_dismissed ON vulnerabilities USING btree (id) WHERE (state <> 2); +CREATE INDEX tmp_index_uuid_is_null ON security_findings USING btree (id) WHERE (uuid IS NULL); + CREATE UNIQUE INDEX uniq_pkgs_deb_grp_architectures_on_distribution_id_and_name ON packages_debian_group_architectures USING btree (distribution_id, name); CREATE UNIQUE INDEX uniq_pkgs_deb_grp_components_on_distribution_id_and_name ON packages_debian_group_components USING btree (distribution_id, name); diff --git a/ee/spec/lib/ee/gitlab/background_migration/populate_uuids_for_security_findings_spec.rb b/ee/spec/lib/ee/gitlab/background_migration/populate_uuids_for_security_findings_spec.rb index 8ba734f54ecdf13c3351e733494e1348a8efe370..821548d09c37c6f62f20ac56927d931f285c5dea 100644 --- a/ee/spec/lib/ee/gitlab/background_migration/populate_uuids_for_security_findings_spec.rb +++ b/ee/spec/lib/ee/gitlab/background_migration/populate_uuids_for_security_findings_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Gitlab::BackgroundMigration::PopulateUuidsForSecurityFindings do +RSpec.describe ::Gitlab::BackgroundMigration::PopulateUuidsForSecurityFindings, schema: 20211108211434 do let(:users) { table(:users) } let(:namespaces) { table(:namespaces) } let(:projects) { table(:projects) } diff --git a/lib/gitlab/background_migration/drop_invalid_security_findings.rb b/lib/gitlab/background_migration/drop_invalid_security_findings.rb new file mode 100644 index 0000000000000000000000000000000000000000..87551bb1b1ec10bfccdf261bff3356cc01006957 --- /dev/null +++ b/lib/gitlab/background_migration/drop_invalid_security_findings.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true +module Gitlab + module BackgroundMigration + # Drop rows from security_findings where the uuid is NULL + class DropInvalidSecurityFindings + # rubocop:disable Style/Documentation + class SecurityFinding < ActiveRecord::Base + include ::EachBatch + self.table_name = 'security_findings' + scope :no_uuid, -> { where(uuid: nil) } + end + # rubocop:enable Style/Documentation + + PAUSE_SECONDS = 0.1 + + def perform(start_id, end_id, sub_batch_size) + ranged_query = SecurityFinding + .where(id: start_id..end_id) + .no_uuid + + ranged_query.each_batch(of: sub_batch_size) do |sub_batch| + first, last = sub_batch.pluck(Arel.sql('min(id), max(id)')).first + + # The query need to be reconstructed because .each_batch modifies the default scope + # See: https://gitlab.com/gitlab-org/gitlab/-/issues/330510 + SecurityFinding.unscoped + .where(id: first..last) + .no_uuid + .delete_all + + sleep PAUSE_SECONDS + end + + mark_job_as_succeeded(start_id, end_id, sub_batch_size) + end + + private + + def mark_job_as_succeeded(*arguments) + Gitlab::Database::BackgroundMigrationJob.mark_all_as_succeeded( + self.class.name.demodulize, + arguments + ) + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/drop_invalid_security_findings_spec.rb b/spec/lib/gitlab/background_migration/drop_invalid_security_findings_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..95fdfd752f66b738b281ae2427c5ce8583582755 --- /dev/null +++ b/spec/lib/gitlab/background_migration/drop_invalid_security_findings_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Gitlab::BackgroundMigration::DropInvalidSecurityFindings, schema: 20211108211434 do + let(:namespace) { table(:namespaces).create!(name: 'user', path: 'user') } + let(:project) { table(:projects).create!(namespace_id: namespace.id) } + + let(:pipelines) { table(:ci_pipelines) } + let!(:pipeline) { pipelines.create!(project_id: project.id) } + + let(:ci_builds) { table(:ci_builds) } + let!(:ci_build) { ci_builds.create! } + + let(:security_scans) { table(:security_scans) } + let!(:security_scan) do + security_scans.create!( + scan_type: 1, + status: 1, + build_id: ci_build.id, + project_id: project.id, + pipeline_id: pipeline.id + ) + end + + let(:vulnerability_scanners) { table(:vulnerability_scanners) } + let!(:vulnerability_scanner) { vulnerability_scanners.create!(project_id: project.id, external_id: 'test 1', name: 'test scanner 1') } + + let(:security_findings) { table(:security_findings) } + let!(:security_finding_without_uuid) do + security_findings.create!( + severity: 1, + confidence: 1, + scan_id: security_scan.id, + scanner_id: vulnerability_scanner.id, + uuid: nil + ) + end + + let!(:security_finding_with_uuid) do + security_findings.create!( + severity: 1, + confidence: 1, + scan_id: security_scan.id, + scanner_id: vulnerability_scanner.id, + uuid: 'bd95c085-71aa-51d7-9bb6-08ae669c262e' + ) + end + + let(:sub_batch_size) { 10_000 } + + subject { described_class.new.perform(security_finding_without_uuid.id, security_finding_with_uuid.id, sub_batch_size) } + + it 'drops Security::Finding objects with no UUID' do + expect { subject }.to change(security_findings, :count).from(2).to(1) + end +end diff --git a/spec/migrations/20211110143306_add_not_null_constraint_to_security_findings_uuid_spec.rb b/spec/migrations/20211110143306_add_not_null_constraint_to_security_findings_uuid_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..946fbf7f568d4be3e3b55e06067bae170aff6c05 --- /dev/null +++ b/spec/migrations/20211110143306_add_not_null_constraint_to_security_findings_uuid_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true +require 'spec_helper' +require_migration! + +RSpec.describe AddNotNullConstraintToSecurityFindingsUuid do + let_it_be(:security_findings) { table(:security_findings) } + let_it_be(:migration) { described_class.new } + + before do + allow(migration).to receive(:transaction_open?).and_return(false) + allow(migration).to receive(:with_lock_retries).and_yield + end + + it 'adds a check constraint' do + constraint = security_findings.connection.check_constraints(:security_findings).find { |constraint| constraint.expression == "uuid IS NOT NULL" } + expect(constraint).to be_nil + + migration.up + + constraint = security_findings.connection.check_constraints(:security_findings).find { |constraint| constraint.expression == "uuid IS NOT NULL" } + expect(constraint).to be_a(ActiveRecord::ConnectionAdapters::CheckConstraintDefinition) + end +end diff --git a/spec/migrations/20211110151350_schedule_drop_invalid_security_findings_spec.rb b/spec/migrations/20211110151350_schedule_drop_invalid_security_findings_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..90debca13e17ecb31417b5e78349db8aad215131 --- /dev/null +++ b/spec/migrations/20211110151350_schedule_drop_invalid_security_findings_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe ScheduleDropInvalidSecurityFindings, :migration, schema: 20211108211434 do + let_it_be(:background_migration_jobs) { table(:background_migration_jobs) } + + let_it_be(:namespace) { table(:namespaces).create!(name: 'user', path: 'user') } + let_it_be(:project) { table(:projects).create!(namespace_id: namespace.id) } + + let_it_be(:pipelines) { table(:ci_pipelines) } + let_it_be(:pipeline) { pipelines.create!(project_id: project.id) } + + let_it_be(:ci_builds) { table(:ci_builds) } + let_it_be(:ci_build) { ci_builds.create! } + + let_it_be(:security_scans) { table(:security_scans) } + let_it_be(:security_scan) do + security_scans.create!( + scan_type: 1, + status: 1, + build_id: ci_build.id, + project_id: project.id, + pipeline_id: pipeline.id + ) + end + + let_it_be(:vulnerability_scanners) { table(:vulnerability_scanners) } + let_it_be(:vulnerability_scanner) { vulnerability_scanners.create!(project_id: project.id, external_id: 'test 1', name: 'test scanner 1') } + + let_it_be(:security_findings) { table(:security_findings) } + let_it_be(:security_finding_without_uuid) do + security_findings.create!( + severity: 1, + confidence: 1, + scan_id: security_scan.id, + scanner_id: vulnerability_scanner.id, + uuid: nil + ) + end + + let_it_be(:security_finding_with_uuid) do + security_findings.create!( + severity: 1, + confidence: 1, + scan_id: security_scan.id, + scanner_id: vulnerability_scanner.id, + uuid: 'bd95c085-71aa-51d7-9bb6-08ae669c262e' + ) + end + + before do + stub_const("#{described_class}::BATCH_SIZE", 1) + stub_const("#{described_class}::SUB_BATCH_SIZE", 1) + end + + around do |example| + freeze_time { Sidekiq::Testing.fake! { example.run } } + end + + it 'schedules background migrations' do + migrate! + + expect(background_migration_jobs.count).to eq(1) + expect(background_migration_jobs.first.arguments).to match_array([security_finding_without_uuid.id, security_finding_without_uuid.id, described_class::SUB_BATCH_SIZE]) + + expect(BackgroundMigrationWorker.jobs.size).to eq(1) + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, security_finding_without_uuid.id, security_finding_without_uuid.id, described_class::SUB_BATCH_SIZE) + end +end