Merge branch 'sh-allow-storage-specific-objectstore-disable' into 'master'

Don't require a bucket if object storage is disabled for a type

See merge request gitlab-org/gitlab!52750

config/object_store_settings.rb

@@ -78,7 +78,7 @@ class ObjectStoreSettings
   #   "background_upload" => false,
   #   "proxy_download" => false,
   #   "remote_directory" => "artifacts"
-  #  }
+  # }
   #
   # Settings.lfs['object_store'] = {
   #   "enabled" => true,
@@ -97,7 +97,7 @@ class ObjectStoreSettings
   #   "background_upload" => false,
   #   "proxy_download" => true,
   #   "remote_directory" => "lfs-objects"
-  #  }
+  # }
   #
   # Note that with the common config:
   # 1. Only one object store credentials can now be used. This is
@@ -124,9 +124,9 @@ class ObjectStoreSettings
       target_config = common_config.merge(overrides.slice(*ALLOWED_OBJECT_STORE_OVERRIDES))
       section = settings.try(store_type)
 
-      next unless section
+      next unless uses_object_storage?(section)
 
-      if section['enabled'] && target_config['bucket'].blank?
+      if target_config['bucket'].blank?
         missing_bucket_for(store_type)
         next
       end
@@ -140,10 +140,26 @@ class ObjectStoreSettings
       target_config['consolidated_settings'] = true
       section['object_store'] = target_config
     end
+
+    settings
   end
 
   private
 
+  # Admins can selectively disable object storage for a specific type. If
+  # this hasn't been set, we assume that the consolidated settings
+  # should be used.
+  def uses_object_storage?(section)
+    # Use to_h to avoid https://gitlab.com/gitlab-org/gitlab/-/issues/286873
+    section = section.to_h
+
+    enabled_globally = section.fetch('enabled', false)
+    object_store_settings = section.fetch('object_store', {})
+    os_enabled = object_store_settings.fetch('enabled', true)
+
+    enabled_globally && os_enabled
+  end
+
   # We only can use the common object storage settings if:
   # 1. The common settings are defined
   # 2. The legacy settings are not defined
@@ -152,8 +168,9 @@ class ObjectStoreSettings
     return false unless settings.dig('object_store', 'connection').present?
 
     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:
+      # to_h is needed because we define `default` as a Gitaly storage name
+      # in stub_storage_settings. This causes Settingslogic to redefine Hash#default,
+      # which causes Hash#dig to fail when the key doesn't exist: https://gitlab.com/gitlab-org/gitlab/-/issues/286873
       #
       # (byebug) section.dig
       # *** ArgumentError Exception: wrong number of arguments (given 0, expected 1+)

spec/config/object_store_settings_spec.rb

@@ -49,6 +49,20 @@ RSpec.describe ObjectStoreSettings do
         }
       end
 
+      shared_examples 'consolidated settings for objects accelerated by Workhorse' do
+        it 'consolidates active object storage settings' do
+          described_class::WORKHORSE_ACCELERATED_TYPES.each do |object_type|
+            # Use to_h to avoid https://gitlab.com/gitlab-org/gitlab/-/issues/286873
+            section = subject.try(object_type).to_h
+
+            next unless section.dig('object_store', 'enabled')
+
+            expect(section['object_store']['connection']).to eq(connection)
+            expect(section['object_store']['consolidated_settings']).to be true
+          end
+        end
+      end
+
       it 'sets correct default values' do
         subject
 
@@ -77,9 +91,7 @@ RSpec.describe ObjectStoreSettings do
         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
+        expect(settings.external_diffs['object_store']).to be_nil
       end
 
       it 'raises an error when a bucket is missing' do
@@ -95,29 +107,49 @@ RSpec.describe ObjectStoreSettings do
         expect(settings.pages['object_store']).to eq(nil)
       end
 
-      it 'allows pages to define its own connection' do
-        pages_connection = { 'provider' => 'Google', 'google_application_default' => true }
-        config['pages'] = {
-          'enabled' => true,
-          'object_store' => {
+      context 'GitLab Pages' do
+        let(:pages_connection) { { 'provider' => 'Google', 'google_application_default' => true } }
+
+        before do
+          config['pages'] = {
             'enabled' => true,
-            'connection' => pages_connection
+            'object_store' => {
+              'enabled' => true,
+              'connection' => pages_connection
+            }
           }
-        }
+        end
 
-        expect { subject }.not_to raise_error
+        it_behaves_like 'consolidated settings for objects accelerated by Workhorse'
 
-        described_class::WORKHORSE_ACCELERATED_TYPES.each do |object_type|
-          section = settings.try(object_type)
+        it 'allows pages to define its own connection' do
+          expect { subject }.not_to raise_error
 
-          next unless section
+          expect(settings.pages['object_store']['connection']).to eq(pages_connection)
+          expect(settings.pages['object_store']['consolidated_settings']).to be_falsey
+        end
+      end
 
-          expect(section['object_store']['connection']).to eq(connection)
-          expect(section['object_store']['consolidated_settings']).to be true
+      context 'when object storage is selectively disabled for artifacts' do
+        before do
+          config['artifacts'] = {
+            'enabled' => true,
+            'object_store' => {
+              'enabled' => false
+            }
+          }
         end
 
-        expect(settings.pages['object_store']['connection']).to eq(pages_connection)
-        expect(settings.pages['object_store']['consolidated_settings']).to be_falsey
+        it_behaves_like 'consolidated settings for objects accelerated by Workhorse'
+
+        it 'does not enable consolidated settings for artifacts' do
+          subject
+
+          expect(settings.artifacts['enabled']).to be true
+          expect(settings.artifacts['object_store']['remote_directory']).to be_nil
+          expect(settings.artifacts['object_store']['enabled']).to be_falsey
+          expect(settings.artifacts['object_store']['consolidated_settings']).to be_falsey
+        end
       end
 
       context 'with legacy config' do