Allow Pages to define a storage-specific connection

Consolidated object storage settings were introduced to:

1. Simplify configuration settings in Workhorse and Rails by using a
single credential.
2. Ensure credentials used by Workhorse and Rails remained consistent.
3. Avoid configuration spew by preventing admins from configuring 10
different credentials for 10 different object types.

If any storage-specific settings were configured, consolidated object
storage settings would not take effect.

However, in the case of GitLab Pages, which does not use Workhorse
acceleration, we can relax this requirement. Normally in the Helm Chart,
we use an internal service name, such as `gitlab-minio-svc:9000`. But
with Pages, we need to provide a public FQDN, such as
`minio.example.com`.

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/284604

changelogs/unreleased/sh-allow-pages-to-use-storage-specific-settings.yml

+---
+title: Allow Pages to define a storage-specific connection
+merge_request: 48098
+author:
+type: changed

config/object_store_settings.rb

@@ -3,6 +3,13 @@ class ObjectStoreSettings
   SUPPORTED_TYPES = %w(artifacts external_diffs lfs uploads packages dependency_proxy terraform_state pages).freeze
   ALLOWED_OBJECT_STORE_OVERRIDES = %w(bucket enabled proxy_download).freeze
 
+  # To ensure the one Workhorse credential matches the Rails config, we
+  # enforce consolidated settings on those accelerated
+  # endpoints. Technically dependency_proxy and terraform_state fall
+  # into this category, but they will likely be handled by Workhorse in
+  # the future.
+  WORKHORSE_ACCELERATED_TYPES = SUPPORTED_TYPES - %w(pages)
+
   # pages may be enabled but use legacy disk storage
   # we don't need to raise an error in that case
   ALLOWED_INCOMPLETE_TYPES = %w(pages).freeze
@@ -123,6 +130,10 @@ class ObjectStoreSettings
         missing_bucket_for(store_type)
       end
 
+      # If a storage type such as Pages defines its own connection and does not
+      # use Workhorse acceleration, we allow it to override the consolidated form.
+      next if allowed_storage_specific_settings?(store_type, section)
+
       # Map bucket (external name) -> remote_directory (internal representation)
       target_config['remote_directory'] = target_config.delete('bucket')
       target_config['consolidated_settings'] = true
@@ -139,7 +150,7 @@ class ObjectStoreSettings
     return false unless settings.dig('object_store', 'enabled')
     return false unless settings.dig('object_store', 'connection').present?
 
-    SUPPORTED_TYPES.each do |store|
+    WORKHORSE_ACCELERATED_TYPES.each do |store|
       # to_h is needed because something strange happens to
       # Settingslogic#dig when stub_storage_settings is run in tests:
       #
@@ -168,4 +179,15 @@ class ObjectStoreSettings
       raise message
     end
   end
+
+  def allowed_storage_specific_settings?(store_type, section)
+    return false if WORKHORSE_ACCELERATED_TYPES.include?(store_type)
+
+    has_object_store_configured?(section)
+  end
+
+  def has_object_store_configured?(section)
+    # Omnibus defaults to an empty hash for connection
+    section.dig('object_store', 'enabled') && section.dig('object_store', 'connection').present?
+  end
 end

spec/config/object_store_settings_spec.rb

@@ -59,6 +59,7 @@ RSpec.describe ObjectStoreSettings do
         expect(settings.artifacts['object_store']['background_upload']).to be false
         expect(settings.artifacts['object_store']['proxy_download']).to be false
         expect(settings.artifacts['object_store']['remote_directory']).to eq('artifacts')
+        expect(settings.artifacts['object_store']['consolidated_settings']).to be true
 
         expect(settings.lfs['enabled']).to be true
         expect(settings.lfs['object_store']['enabled']).to be true
@@ -67,15 +68,18 @@ RSpec.describe ObjectStoreSettings do
         expect(settings.lfs['object_store']['background_upload']).to be false
         expect(settings.lfs['object_store']['proxy_download']).to be true
         expect(settings.lfs['object_store']['remote_directory']).to eq('lfs-objects')
+        expect(settings.lfs['object_store']['consolidated_settings']).to be true
 
         expect(settings.pages['enabled']).to be true
         expect(settings.pages['object_store']['enabled']).to be true
         expect(settings.pages['object_store']['connection']).to eq(connection)
         expect(settings.pages['object_store']['remote_directory']).to eq('pages')
+        expect(settings.pages['object_store']['consolidated_settings']).to be true
 
         expect(settings.external_diffs['enabled']).to be false
         expect(settings.external_diffs['object_store']['enabled']).to be false
         expect(settings.external_diffs['object_store']['remote_directory']).to eq('external_diffs')
+        expect(settings.external_diffs['object_store']['consolidated_settings']).to be true
       end
 
       it 'raises an error when a bucket is missing' do
@@ -90,6 +94,31 @@ RSpec.describe ObjectStoreSettings do
         expect { subject }.not_to raise_error
       end
 
+      it 'allows pages to define its own connection' do
+        pages_connection = { 'provider' => 'Google', 'google_application_default' => true }
+        config['pages'] = {
+          'enabled' => true,
+          'object_store' => {
+            'enabled' => true,
+            'connection' => pages_connection
+          }
+        }
+
+        expect { subject }.not_to raise_error
+
+        described_class::WORKHORSE_ACCELERATED_TYPES.each do |object_type|
+          section = settings.try(object_type)
+
+          next unless section
+
+          expect(section['object_store']['connection']).to eq(connection)
+          expect(section['object_store']['consolidated_settings']).to be true
+        end
+
+        expect(settings.pages['object_store']['connection']).to eq(pages_connection)
+        expect(settings.pages['object_store']['consolidated_settings']).to be_falsey
+      end
+
       context 'with legacy config' do
         let(:legacy_settings) do
           {