Merge branch 'many-dbs-validate-database-config' into 'master'

Strong validate used config names in `database.yml` and normalize legacy config

See merge request gitlab-org/gitlab!67877

config/application.rb

@@ -31,9 +31,18 @@ module Gitlab
     require_dependency Rails.root.join('lib/gitlab/middleware/handle_malformed_strings')
     require_dependency Rails.root.join('lib/gitlab/middleware/rack_multipart_tempfile_factory')
     require_dependency Rails.root.join('lib/gitlab/runtime')
+    require_dependency Rails.root.join('lib/gitlab/patch/legacy_database_config')
 
     config.autoloader = :zeitwerk
 
+    # To be removed in 15.0
+    # This preload is needed to convert legacy `database.yml`
+    # from `production: adapter: postgresql`
+    # into a `production: main: adapter: postgresql`
+    unless Gitlab::Utils.to_boolean(ENV['SKIP_DATABASE_CONFIG_VALIDATION'], default: false)
+      config.class.prepend(::Gitlab::Patch::LegacyDatabaseConfig)
+    end
+
     # Settings in config/environments/* take precedence over those specified here.
     # Application configuration should go into files in config/initializers
     # -- all .rb files in that directory are automatically loaded.

config/database.yml.postgresql

@@ -2,56 +2,60 @@
 # PRODUCTION
 #
 production:
-  adapter: postgresql
-  encoding: unicode
-  database: gitlabhq_production
-  username: git
-  password: "secure password"
-  host: localhost
-  # load_balancing:
-  #   hosts:
-  #     - host1.example.com
-  #     - host2.example.com
-  #   discover:
-  #     nameserver: 1.2.3.4
-  #     port: 8600
-  #     record: secondary.postgresql.service.consul
-  #     interval: 300
+  main:
+    adapter: postgresql
+    encoding: unicode
+    database: gitlabhq_production
+    username: git
+    password: "secure password"
+    host: localhost
+    # load_balancing:
+    #   hosts:
+    #     - host1.example.com
+    #     - host2.example.com
+    #   discover:
+    #     nameserver: 1.2.3.4
+    #     port: 8600
+    #     record: secondary.postgresql.service.consul
+    #     interval: 300
 
 #
 # Development specific
 #
 development:
-  adapter: postgresql
-  encoding: unicode
-  database: gitlabhq_development
-  username: postgres
-  password: "secure password"
-  host: localhost
-  variables:
-    statement_timeout: 15s
+  main:
+    adapter: postgresql
+    encoding: unicode
+    database: gitlabhq_development
+    username: postgres
+    password: "secure password"
+    host: localhost
+    variables:
+      statement_timeout: 15s
 
 #
 # Staging specific
 #
 staging:
-  adapter: postgresql
-  encoding: unicode
-  database: gitlabhq_staging
-  username: git
-  password: "secure password"
-  host: localhost
+  main:
+    adapter: postgresql
+    encoding: unicode
+    database: gitlabhq_staging
+    username: git
+    password: "secure password"
+    host: localhost
 
 # Warning: The database defined as "test" will be erased and
 # re-generated from your development database when you run "rake".
 # Do not set this db to the same as development or production.
 test: &test
-  adapter: postgresql
-  encoding: unicode
-  database: gitlabhq_test
-  username: postgres
-  password:
-  host: localhost
-  prepared_statements: false
-  variables:
-    statement_timeout: 15s
+  main:
+    adapter: postgresql
+    encoding: unicode
+    database: gitlabhq_test
+    username: postgres
+    password:
+    host: localhost
+    prepared_statements: false
+    variables:
+      statement_timeout: 15s

config/initializers/validate_database_config.rb

+# frozen_string_literal: true
+
+if Gitlab::Utils.to_boolean(ENV['SKIP_DATABASE_CONFIG_VALIDATION'], default: false)
+  return
+end
+
+if Rails.application.config.uses_legacy_database_config
+  warn "WARNING: This installation of GitLab uses a deprecated syntax for 'config/database.yml'. " \
+    "The support for this syntax will be removed in 15.0. " \
+    "More information can be found here: https://gitlab.com/gitlab-org/gitlab/-/issues/338182"
+end
+
+if configurations = ActiveRecord::Base.configurations.configurations
+  if configurations.first.name != Gitlab::Database::MAIN_DATABASE_NAME
+    raise "ERROR: This installation of GitLab uses unsupported 'config/database.yml'. " \
+      "The `main:` database needs to be defined as a first configuration item instead of `#{configurations.first.name}`."
+  end
+
+  rejected_config_names = configurations.map(&:name).to_set - Gitlab::Database::DATABASE_NAMES
+  if rejected_config_names.any?
+    raise "ERROR: This installation of GitLab uses unsupported database names " \
+      "in 'config/database.yml': #{rejected_config_names.to_a.join(", ")}. The only supported ones are " \
+      "#{Gitlab::Database::DATABASE_NAMES.join(", ")}."
+  end
+
+  replicas_config_names = configurations.select(&:replica?).map(&:name)
+  if replicas_config_names.any?
+    raise "ERROR: This installation of GitLab uses unsupported database configuration " \
+      "with 'replica: true' parameter in 'config/database.yml' for: #{replicas_config_names.join(", ")}"
+  end
+end

lib/gitlab/database.rb

@@ -2,6 +2,9 @@
 
 module Gitlab
   module Database
+    DATABASE_NAMES = %w[main ci].freeze
+
+    MAIN_DATABASE_NAME = 'main'
     CI_DATABASE_NAME = 'ci'
 
     # This constant is used when renaming tables concurrently.

lib/gitlab/patch/legacy_database_config.rb

+# frozen_string_literal: true
+
+# The purpose of this code is to transform legacy `database.yml`
+# into a `database.yml` containing `main:` as a name of a first database
+#
+# This should be removed once all places using legacy `database.yml`
+# are fixed. The likely moment to remove this check is the %14.0.
+#
+# This converts the following syntax:
+#
+# production:
+#   adapter: postgresql
+#   database: gitlabhq_production
+#   username: git
+#   password: "secure password"
+#   host: localhost
+#
+# Into:
+#
+# production:
+#   main:
+#     adapter: postgresql
+#     database: gitlabhq_production
+#     username: git
+#     password: "secure password"
+#     host: localhost
+#
+
+module Gitlab
+  module Patch
+    module LegacyDatabaseConfig
+      extend ActiveSupport::Concern
+
+      prepended do
+        attr_reader :uses_legacy_database_config
+      end
+
+      def database_configuration
+        @uses_legacy_database_config = false # rubocop:disable Gitlab/ModuleWithInstanceVariables
+
+        super.to_h do |env, configs|
+          # This check is taken from Rails where the transformation
+          # of a flat database.yml is done into `primary:`
+          # https://github.com/rails/rails/blob/v6.1.4/activerecord/lib/active_record/database_configurations.rb#L169
+          if configs.is_a?(Hash) && !configs.all? { |_, v| v.is_a?(Hash) }
+            configs = { "main" => configs }
+
+            @uses_legacy_database_config = true # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          end
+
+          [env, configs]
+        end
+      end
+    end
+  end
+end

spec/initializers/validate_database_config_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'validate database config' do
+  include RakeHelpers
+  include StubENV
+
+  let(:rails_configuration) { Rails::Application::Configuration.new(Rails.root) }
+  let(:ar_configurations) { ActiveRecord::DatabaseConfigurations.new(rails_configuration.database_configuration) }
+
+  subject do
+    load Rails.root.join('config/initializers/validate_database_config.rb')
+  end
+
+  before do
+    # The `AS::ConfigurationFile` calls `read` in `def initialize`
+    # thus we cannot use `expect_next_instance_of`
+    # rubocop:disable RSpec/AnyInstanceOf
+    expect_any_instance_of(ActiveSupport::ConfigurationFile)
+      .to receive(:read).with(Rails.root.join('config/database.yml')).and_return(database_yml)
+    # rubocop:enable RSpec/AnyInstanceOf
+
+    allow(Rails.application).to receive(:config).and_return(rails_configuration)
+    allow(ActiveRecord::Base).to receive(:configurations).and_return(ar_configurations)
+  end
+
+  shared_examples 'with SKIP_DATABASE_CONFIG_VALIDATION=true' do
+    before do
+      stub_env('SKIP_DATABASE_CONFIG_VALIDATION', 'true')
+    end
+
+    it 'does not raise exception' do
+      expect { subject }.not_to raise_error
+    end
+  end
+
+  context 'when config/database.yml is valid' do
+    context 'uses legacy syntax' do
+      let(:database_yml) do
+        <<-EOS
+          production:
+            adapter: postgresql
+            encoding: unicode
+            database: gitlabhq_production
+            username: git
+            password: "secure password"
+            host: localhost
+        EOS
+      end
+
+      it 'validates configuration with a warning' do
+        expect(main_object).to receive(:warn).with /uses a deprecated syntax for/
+
+        expect { subject }.not_to raise_error
+      end
+
+      it_behaves_like 'with SKIP_DATABASE_CONFIG_VALIDATION=true'
+    end
+
+    context 'uses new syntax' do
+      let(:database_yml) do
+        <<-EOS
+          production:
+            main:
+              adapter: postgresql
+              encoding: unicode
+              database: gitlabhq_production
+              username: git
+              password: "secure password"
+              host: localhost
+        EOS
+      end
+
+      it 'validates configuration without errors and warnings' do
+        expect(main_object).not_to receive(:warn)
+
+        expect { subject }.not_to raise_error
+      end
+    end
+  end
+
+  context 'when config/database.yml is invalid' do
+    context 'uses unknown connection name' do
+      let(:database_yml) do
+        <<-EOS
+          production:
+            main:
+              adapter: postgresql
+              encoding: unicode
+              database: gitlabhq_production
+              username: git
+              password: "secure password"
+              host: localhost
+
+            another:
+              adapter: postgresql
+              encoding: unicode
+              database: gitlabhq_production
+              username: git
+              password: "secure password"
+              host: localhost
+        EOS
+      end
+
+      it 'raises exception' do
+        expect { subject }.to raise_error /This installation of GitLab uses unsupported database names/
+      end
+
+      it_behaves_like 'with SKIP_DATABASE_CONFIG_VALIDATION=true'
+    end
+
+    context 'uses replica configuration' do
+      let(:database_yml) do
+        <<-EOS
+          production:
+            main:
+              adapter: postgresql
+              encoding: unicode
+              database: gitlabhq_production
+              username: git
+              password: "secure password"
+              host: localhost
+              replica: true
+        EOS
+      end
+
+      it 'raises exception' do
+        expect { subject }.to raise_error /with 'replica: true' parameter in/
+      end
+
+      it_behaves_like 'with SKIP_DATABASE_CONFIG_VALIDATION=true'
+    end
+
+    context 'main is not a first entry' do
+      let(:database_yml) do
+        <<-EOS
+          production:
+            ci:
+              adapter: postgresql
+              encoding: unicode
+              database: gitlabhq_production_ci
+              username: git
+              password: "secure password"
+              host: localhost
+              replica: true
+
+            main:
+              adapter: postgresql
+              encoding: unicode
+              database: gitlabhq_production
+              username: git
+              password: "secure password"
+              host: localhost
+              replica: true
+        EOS
+      end
+
+      it 'raises exception' do
+        expect { subject }.to raise_error /The `main:` database needs to be defined as a first configuration item/
+      end
+
+      it_behaves_like 'with SKIP_DATABASE_CONFIG_VALIDATION=true'
+    end
+  end
+end

spec/lib/gitlab/patch/legacy_database_config_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Patch::LegacyDatabaseConfig do
+  it 'module is included' do
+    expect(Rails::Application::Configuration).to include(described_class)
+  end
+
+  describe 'config/database.yml' do
+    let(:configuration) { Rails::Application::Configuration.new(Rails.root) }
+
+    before do
+      # The `AS::ConfigurationFile` calls `read` in `def initialize`
+      # thus we cannot use `expect_next_instance_of`
+      # rubocop:disable RSpec/AnyInstanceOf
+      expect_any_instance_of(ActiveSupport::ConfigurationFile)
+        .to receive(:read).with(Rails.root.join('config/database.yml')).and_return(database_yml)
+      # rubocop:enable RSpec/AnyInstanceOf
+    end
+
+    shared_examples 'hash containing main: connection name' do
+      it 'returns a hash containing only main:' do
+        database_configuration = configuration.database_configuration
+
+        expect(database_configuration).to match(
+          "production" => { "main" => a_hash_including("adapter") },
+          "development" => { "main" => a_hash_including("adapter" => "postgresql") },
+          "test" => { "main" => a_hash_including("adapter" => "postgresql") }
+        )
+      end
+    end
+
+    context 'when a new syntax is used' do
+      let(:database_yml) do
+        <<-EOS
+          production:
+            main:
+              adapter: postgresql
+              encoding: unicode
+              database: gitlabhq_production
+              username: git
+              password: "secure password"
+              host: localhost
+
+          development:
+            main:
+              adapter: postgresql
+              encoding: unicode
+              database: gitlabhq_development
+              username: postgres
+              password: "secure password"
+              host: localhost
+              variables:
+                statement_timeout: 15s
+
+          test: &test
+            main:
+              adapter: postgresql
+              encoding: unicode
+              database: gitlabhq_test
+              username: postgres
+              password:
+              host: localhost
+              prepared_statements: false
+              variables:
+                statement_timeout: 15s
+        EOS
+      end
+
+      include_examples 'hash containing main: connection name'
+
+      it 'configuration is not legacy one' do
+        configuration.database_configuration
+
+        expect(configuration.uses_legacy_database_config).to eq(false)
+      end
+    end
+
+    context 'when a legacy syntax is used' do
+      let(:database_yml) do
+        <<-EOS
+          production:
+            adapter: postgresql
+            encoding: unicode
+            database: gitlabhq_production
+            username: git
+            password: "secure password"
+            host: localhost
+
+          development:
+            adapter: postgresql
+            encoding: unicode
+            database: gitlabhq_development
+            username: postgres
+            password: "secure password"
+            host: localhost
+            variables:
+              statement_timeout: 15s
+
+          test: &test
+            adapter: postgresql
+            encoding: unicode
+            database: gitlabhq_test
+            username: postgres
+            password:
+            host: localhost
+            prepared_statements: false
+            variables:
+              statement_timeout: 15s
+        EOS
+      end
+
+      include_examples 'hash containing main: connection name'
+
+      it 'configuration is legacy' do
+        configuration.database_configuration
+
+        expect(configuration.uses_legacy_database_config).to eq(true)
+      end
+    end
+  end
+end