Merge branch 'skip-migrations-softly' into 'master'

Implement migration skipping when `RestrictGitlabSchema` is used

See merge request gitlab-org/gitlab!83121

lib/gitlab/database.rb

@@ -236,13 +236,16 @@ module Gitlab
     # This does not look at literal connection names, but rather compares
     # models that are holders for a given db_config_name
     def self.gitlab_schemas_for_connection(connection)
-      connection_name = self.db_config_name(connection)
-      primary_model = self.database_base_models.fetch(connection_name)
-
-      self.schemas_to_base_models
-        .select { |_, models| models.include?(primary_model) }
-        .keys
-        .map!(&:to_sym)
+      db_name = self.db_config_name(connection)
+      primary_model = self.database_base_models.fetch(db_name.to_sym)
+
+      self.schemas_to_base_models.select do |_, child_models|
+        child_models.any? do |child_model|
+          child_model == primary_model || \
+            # The model might indicate a child connection, ensure that this is enclosed in a `db_config`
+            self.database_base_models[self.db_config_share_with(child_model.connection_db_config)] == primary_model
+        end
+      end.keys.map!(&:to_sym)
     end
 
     def self.db_config_for_connection(connection)

lib/gitlab/database/migration_helpers/restrict_gitlab_schema.rb

@@ -6,8 +6,6 @@ module Gitlab
       module RestrictGitlabSchema
         extend ActiveSupport::Concern
 
-        MigrationSkippedError = Class.new(StandardError)
-
         included do
           class_attribute :allowed_gitlab_schemas
         end
@@ -25,11 +23,8 @@ module Gitlab
 
         def migrate(direction)
           if unmatched_schemas.any?
-            # TODO: Today skipping migration would raise an exception.
-            # Ideally, skipped migration should be ignored (not loaded), or softly ignored.
-            # Read more in: https://gitlab.com/gitlab-org/gitlab/-/issues/355014
-            raise MigrationSkippedError, "Current migration is skipped since it modifies "\
-              "'#{self.class.allowed_gitlab_schemas}' which is outside of '#{allowed_schemas_for_connection}'"
+            migration_skipped
+            return
           end
 
           Gitlab::Database::QueryAnalyzer.instance.within([validator_class]) do
@@ -41,6 +36,11 @@ module Gitlab
 
         private
 
+        def migration_skipped
+          say "Current migration is skipped since it modifies "\
+            "'#{self.class.allowed_gitlab_schemas}' which is outside of '#{allowed_schemas_for_connection}'"
+        end
+
         def validator_class
           Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas
         end

lib/tasks/gitlab/db/validate_config.rake

@@ -85,9 +85,13 @@ namespace :gitlab do
         warnings.unshift("Database config validation failure:")
 
         # Warn (for now) by default in production environment
-        if Gitlab::Utils.to_boolean(ENV['GITLAB_VALIDATE_DATABASE_CONFIG'], default: Gitlab.dev_or_test_env?)
+        if Gitlab::Utils.to_boolean(ENV['GITLAB_VALIDATE_DATABASE_CONFIG'], default: true)
+          warnings << "Use `export GITLAB_VALIDATE_DATABASE_CONFIG=0` to ignore this validation."
+
           raise warnings.join("\n")
         else
+          warnings << "Use `export GITLAB_VALIDATE_DATABASE_CONFIG=1` to enforce this validation."
+
           warn warnings.join("\n")
         end
       end

spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb

@@ -496,11 +496,16 @@ RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_a
       Gitlab::Database.database_base_models.each do |db_config_name, model|
         context "for db_config_name=#{db_config_name}" do
           around do |example|
+            verbose_was = ActiveRecord::Migration.verbose
+            ActiveRecord::Migration.verbose = false
+
             with_reestablished_active_record_base do
               reconfigure_db_connection(model: ActiveRecord::Base, config_model: model)
 
               example.run
             end
+          ensure
+            ActiveRecord::Migration.verbose = verbose_was
           end
 
           before do
@@ -543,8 +548,15 @@ RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_a
                   expect { ignore_error(Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas::DDLNotAllowedError) { migration_class.migrate(:down) } }.not_to raise_error
 
                 when :skipped
-                  expect { migration_class.migrate(:up) }.to raise_error(Gitlab::Database::MigrationHelpers::RestrictGitlabSchema::MigrationSkippedError)
-                  expect { migration_class.migrate(:down) }.to raise_error(Gitlab::Database::MigrationHelpers::RestrictGitlabSchema::MigrationSkippedError)
+                  expect_next_instance_of(migration_class) do |migration_object|
+                    expect(migration_object).to receive(:migration_skipped).and_call_original
+                    expect(migration_object).not_to receive(:up)
+                    expect(migration_object).not_to receive(:down)
+                    expect(migration_object).not_to receive(:change)
+                  end
+
+                  migration_class.migrate(:up)
+                  migration_class.migrate(:down)
                 end
               end
             end

spec/lib/gitlab/database_spec.rb

@@ -265,6 +265,47 @@ RSpec.describe Gitlab::Database do
         expect(described_class.gitlab_schemas_for_connection(ActiveRecord::Base.connection)).to include(:gitlab_ci, :gitlab_shared)
       end
     end
+
+    context "when there's CI connection", :request_store do
+      before do
+        skip_if_multiple_databases_not_setup
+
+        # FF due to lib/gitlab/database/load_balancing/configuration.rb:92
+        # Requires usage of `:request_store`
+        stub_feature_flags(force_no_sharing_primary_model: true)
+      end
+
+      context 'when CI uses database_tasks: false does indicate that ci: is subset of main:' do
+        before do
+          allow(Ci::ApplicationRecord.connection_db_config).to receive(:database_tasks?).and_return(false)
+        end
+
+        it 'does return gitlab_ci when accessing via main: connection' do
+          expect(described_class.gitlab_schemas_for_connection(Project.connection)).to include(:gitlab_ci, :gitlab_main, :gitlab_shared)
+        end
+
+        it 'does not return gitlab_main when accessing via ci: connection' do
+          expect(described_class.gitlab_schemas_for_connection(Ci::Build.connection)).to include(:gitlab_ci, :gitlab_shared)
+          expect(described_class.gitlab_schemas_for_connection(Ci::Build.connection)).not_to include(:gitlab_main)
+        end
+      end
+
+      context 'when CI uses database_tasks: true does indicate that ci: has own database' do
+        before do
+          allow(Ci::ApplicationRecord.connection_db_config).to receive(:database_tasks?).and_return(true)
+        end
+
+        it 'does not return gitlab_ci when accessing via main: connection' do
+          expect(described_class.gitlab_schemas_for_connection(Project.connection)).to include(:gitlab_main, :gitlab_shared)
+          expect(described_class.gitlab_schemas_for_connection(Project.connection)).not_to include(:gitlab_ci)
+        end
+
+        it 'does not return gitlab_main when accessing via ci: connection' do
+          expect(described_class.gitlab_schemas_for_connection(Ci::Build.connection)).to include(:gitlab_ci, :gitlab_shared)
+          expect(described_class.gitlab_schemas_for_connection(Ci::Build.connection)).not_to include(:gitlab_main)
+        end
+      end
+    end
   end
 
   describe '#true_value' do

spec/tasks/gitlab/db/validate_config_rake_spec.rb

@@ -72,18 +72,23 @@ RSpec.describe 'gitlab:db:validate_config', :silence_stdout do
         expect { run_rake_task('gitlab:db:validate_config') }.to raise_error(match)
       end
 
-      it 'to stderr instead of exception for production' do
+      it 'for production' do
         allow(Gitlab).to receive(:dev_or_test_env?).and_return(false)
 
-        expect { run_rake_task('gitlab:db:validate_config') }.to output(match).to_stderr
+        expect { run_rake_task('gitlab:db:validate_config') }.to raise_error(match)
       end
 
-      it 'if GITLAB_VALIDATE_DATABASE_CONFIG is set' do
+      it 'if GITLAB_VALIDATE_DATABASE_CONFIG=1' do
         stub_env('GITLAB_VALIDATE_DATABASE_CONFIG', '1')
-        allow(Gitlab).to receive(:dev_or_test_env?).and_return(false)
 
         expect { run_rake_task('gitlab:db:validate_config') }.to raise_error(match)
       end
+
+      it 'to stderr if GITLAB_VALIDATE_DATABASE_CONFIG=0' do
+        stub_env('GITLAB_VALIDATE_DATABASE_CONFIG', '0')
+
+        expect { run_rake_task('gitlab:db:validate_config') }.to output(match).to_stderr
+      end
     end
 
     context 'when only main: is specified' do