From 2f69784ac2915d0a67087e39b70d48ec6fc07141 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Wed, 17 Apr 2024 08:03:05 -0700
Subject: [PATCH] Allow incomplete bucket for Secure Files in object storage
 config

This restores the changes made in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131819 that was
reverted in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/149479.  If
`ci_secure_files` were configured in a storage-specific settings, this
would previously disable consolidated storage settings.

This change can now go in now that
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/149480 has been
merged. That merge request relaxes the constraints when consolidated
settings can be used so that consolidated settings will still function
if `ci_secure_files` is set in storage-specific settings with the same
`connection`.

This commit applies the suggestion in
https://gitlab.com/gitlab-org/gitlab/-/issues/414673#note_1546146600
to add the `ci_secure_files` config to `SUPPORTED_TYPES` and
`ALLOWED_INCOMPLETE_TYPES` so that the consolidated storage config can
be used, but not produce an error if the bucket configuration is
missing.

This change resolves the issues that caused the original change to be
reverted https://gitlab.com/gitlab-org/gitlab/-/merge_requests/122905+
and allows us to add support for consolidated object storage without
introducing a breaking change.

Changelog: fixed
---
 config/object_store_settings.rb           |  7 +++---
 spec/config/object_store_settings_spec.rb | 30 +++++++++++++++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/config/object_store_settings.rb b/config/object_store_settings.rb
index b17fb540ecc560..9375daff41c563 100644
--- a/config/object_store_settings.rb
+++ b/config/object_store_settings.rb
@@ -2,7 +2,8 @@
 
 # Set default values for object_store settings
 class ObjectStoreSettings
-  SUPPORTED_TYPES = %w[artifacts external_diffs lfs uploads packages dependency_proxy terraform_state pages].freeze
+  SUPPORTED_TYPES = %w[artifacts external_diffs lfs uploads packages dependency_proxy terraform_state pages
+    ci_secure_files].freeze
   ALLOWED_OBJECT_STORE_OVERRIDES = %w[bucket enabled proxy_download cdn].freeze
 
   # To ensure the one Workhorse credential matches the Rails config, we
@@ -12,9 +13,9 @@ class ObjectStoreSettings
   # the future.
   WORKHORSE_ACCELERATED_TYPES = SUPPORTED_TYPES - %w[pages]
 
-  # pages may be enabled but use legacy disk storage
+  # pages and ci_secure_files 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
+  ALLOWED_INCOMPLETE_TYPES = %w[pages ci_secure_files].freeze
 
   attr_accessor :settings
 
diff --git a/spec/config/object_store_settings_spec.rb b/spec/config/object_store_settings_spec.rb
index f9b3cda2c11854..53a44fc2559907 100644
--- a/spec/config/object_store_settings_spec.rb
+++ b/spec/config/object_store_settings_spec.rb
@@ -25,6 +25,7 @@
           'artifacts' => { 'enabled' => true },
           'external_diffs' => { 'enabled' => false },
           'pages' => { 'enabled' => true },
+          'ci_secure_files' => { 'enabled' => true },
           'object_store' => {
             'enabled' => true,
             'connection' => connection,
@@ -193,6 +194,13 @@
         expect(settings.pages['object_store']).to eq(nil)
       end
 
+      it 'does not raise error if ci_secure_files config is missing' do
+        config['object_store']['objects'].delete('ci_secure_files')
+
+        expect { subject }.not_to raise_error
+        expect(settings.ci_secure_files['object_store']).to eq(nil)
+      end
+
       context 'GitLab Pages' do
         let(:pages_connection) { { 'provider' => 'Google', 'google_application_default' => true } }
 
@@ -240,6 +248,28 @@
         end
       end
 
+      context 'when object storage is disabled for ci_secure_files with no bucket' do
+        before do
+          config['ci_secure_files'] = {
+            'enabled' => true,
+            'object_store' => {}
+          }
+          config['object_store']['objects']['ci_secure_files'] = {
+            'enabled' => false
+          }
+        end
+
+        it 'does not enable consolidated settings for ci_secure_files' do
+          subject
+
+          expect(settings.ci_secure_files['enabled']).to be true
+          expect(settings.ci_secure_files['object_store']['remote_directory']).to be_nil
+          expect(settings.ci_secure_files['object_store']['bucket_prefix']).to be_nil
+          expect(settings.ci_secure_files['object_store']['enabled']).to be_falsey
+          expect(settings.ci_secure_files['object_store']['consolidated_settings']).to be_falsey
+        end
+      end
+
       context 'with legacy config' do
         let(:legacy_settings) do
           {
-- 
GitLab