Merge branch 'sh-allow-pages-to-use-storage-specific-settings' into 'master'

Allow Pages to define a storage-specific connection

See merge request gitlab-org/gitlab!48098

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
           {