From c7d3c135d3255af7e113a6056417fde509232519 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sat, 13 Apr 2024 23:58:20 -0700
Subject: [PATCH] Relax constraints when consolidated object storage settings
 is enabled

Previously consolidated object storage settings were disabled if any
section-specific connection were specified. For example, in Omnibus if
`gitlab_rails['artifacts_object_store_connection']` were specified
with some value, this would be a signal to use storage-specific
configurations.

However, this restriction became a problem when
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131819 promoted
`ci_secure_files` to be specified as a consolidated storage
type. Because `gitlab_rails['ci_secure_files_object_store_connection']`
may be specified, this would effectively disable object storage
for all the other types.

Let's relax this constraint by comparing the section-specific config
with the consolidated object config. If the connection settings are
the same, we can still use consolidated settings.

This makes it possible to "promote" `ci_secure_files` from
a storage-specific type to a consolidated storage supported type.

Changelog: changed
---
 config/object_store_settings.rb           | 18 ++++++--
 spec/config/object_store_settings_spec.rb | 50 ++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/config/object_store_settings.rb b/config/object_store_settings.rb
index 666a1967f189ed..9375daff41c563 100644
--- a/config/object_store_settings.rb
+++ b/config/object_store_settings.rb
@@ -195,16 +195,26 @@ def parse!
   # 2. The legacy settings are not defined
   def use_consolidated_settings?
     return false unless settings.dig('object_store', 'enabled')
-    return false unless settings.dig('object_store', 'connection').present?
+
+    connection = settings.dig('object_store', 'connection')
+
+    return false unless connection.present?
 
     WORKHORSE_ACCELERATED_TYPES.each do |store|
       section = settings.try(store)
 
       next unless section
+      next unless section.dig('object_store', 'enabled')
+
+      section_connection = section.dig('object_store', 'connection')
 
-      return false if section.dig('object_store', 'enabled')
-      # Omnibus defaults to an empty hash
-      return false if section.dig('object_store', 'connection').present?
+      # We can use consolidated settings if the main object store
+      # connection matches the section-specific connection. This makes
+      # it possible to automatically use consolidated settings as new
+      # settings (such as ci_secure_files) get promoted to a supported
+      # type. Omnibus defaults to an empty hash for the
+      # section-specific connection.
+      return false if section_connection.present? && section_connection.to_h != connection.to_h
     end
 
     true
diff --git a/spec/config/object_store_settings_spec.rb b/spec/config/object_store_settings_spec.rb
index cc517fea303036..53a44fc2559907 100644
--- a/spec/config/object_store_settings_spec.rb
+++ b/spec/config/object_store_settings_spec.rb
@@ -115,6 +115,51 @@
         expect(settings.lfs['object_store']['bucket_prefix']).to eq('lfs')
       end
 
+      context 'when the same section-specified connection is specified' do
+        before do
+          config['artifacts'] = GitlabSettings::Options.build(
+            {
+              'enabled' => true,
+              'object_store' => {
+                'enabled' => true,
+                'connection' => connection
+              }
+            }
+          )
+        end
+
+        it_behaves_like 'consolidated settings for objects accelerated by Workhorse'
+      end
+
+      context 'when a different section-specified connection is specified' do
+        let(:gcs_connection) { GitlabSettings::Options.build("provider" => "GCS") }
+
+        before do
+          config['artifacts'] = GitlabSettings::Options.build(
+            {
+              'enabled' => true,
+              'object_store' => {
+                'enabled' => true,
+                'connection' => gcs_connection
+              }
+            }
+          )
+        end
+
+        it 'disables consolidated object settings' do
+          expect(settings.artifacts['enabled']).to be true
+          expect(settings.artifacts['object_store']['connection']).to eq(gcs_connection)
+
+          described_class::WORKHORSE_ACCELERATED_TYPES.each do |object_type|
+            section = subject.try(object_type).to_h
+
+            next unless section.dig('object_store', 'enabled')
+
+            expect(section['object_store']['consolidated_settings']).to_be falsey
+          end
+        end
+      end
+
       context 'with Google CDN enabled' do
         let(:cdn_config) do
           {
@@ -231,7 +276,10 @@
             'enabled' => true,
             'remote_directory' => 'some-bucket',
             'direct_upload' => false,
-            'proxy_download' => false
+            'proxy_download' => false,
+            'connection' => {
+              'provider' => 'GCS'
+            }
           }
         end
 
-- 
GitLab