Merge branch '301159-add-local-storage' into 'master'

Added local_store to gitlab.yml file

See merge request gitlab-org/gitlab!55470

app/models/project.rb

@@ -1814,7 +1814,7 @@ class Project < ApplicationRecord
   # TODO: remove this method https://gitlab.com/gitlab-org/gitlab/-/issues/320775
   # rubocop: disable CodeReuse/ServiceClass
   def legacy_remove_pages
-    return unless Feature.enabled?(:pages_update_legacy_storage, default_enabled: true)
+    return unless ::Settings.pages.local_store.enabled
 
     # Projects with a missing namespace cannot have their pages removed
     return unless namespace

app/services/pages/delete_service.rb

@@ -9,7 +9,7 @@ module Pages
       DestroyPagesDeploymentsWorker.perform_async(project.id)
 
       # TODO: remove this call https://gitlab.com/gitlab-org/gitlab/-/issues/320775
-      PagesRemoveWorker.perform_async(project.id) if Feature.enabled?(:pages_update_legacy_storage, default_enabled: true)
+      PagesRemoveWorker.perform_async(project.id) if ::Settings.pages.local_store.enabled
     end
   end
 end

app/services/projects/update_pages_configuration_service.rb

@@ -11,7 +11,7 @@ module Projects
     end
 
     def execute
-      return success unless Feature.enabled?(:pages_update_legacy_storage, default_enabled: true)
+      return success unless ::Settings.pages.local_store.enabled
 
       # If the pages were never deployed, we can't write out the config, as the
       # directory would not exist.

app/services/projects/update_pages_service.rb

@@ -83,7 +83,7 @@ module Projects
     def deploy_to_legacy_storage(artifacts_path)
       # path today used by one project can later be used by another
       # so we can't really scope this feature flag by project or group
-      return unless Feature.enabled?(:pages_update_legacy_storage, default_enabled: true)
+      return unless ::Settings.pages.local_store.enabled
 
       # Create temporary directory in which we will extract the artifacts
       make_secure_tmp_dir(tmp_path) do |tmp_path|

app/workers/pages_update_configuration_worker.rb

@@ -7,7 +7,7 @@ class PagesUpdateConfigurationWorker
   feature_category :pages
 
   def self.perform_async(*args)
-    return unless Feature.enabled?(:pages_update_legacy_storage, default_enabled: true)
+    return unless ::Settings.pages.local_store.enabled
 
     super(*args)
   end

changelogs/unreleased/301159-add-local-storage.yml

+---
+title: Added local_store to Pages settings in gitlab.yml file
+merge_request: 55470
+author:
+type: added

config/gitlab.yml.example

@@ -408,6 +408,10 @@ production: &base
         aws_access_key_id: AWS_ACCESS_KEY_ID
         aws_secret_access_key: AWS_SECRET_ACCESS_KEY
         region: us-east-1
+    local_store:
+      enabled: true
+      # The location where pages are stored (default: shared/pages).
+      # path: shared/pages
 
   ## Mattermost
   ## For enabling Add to Mattermost button
@@ -1395,6 +1399,9 @@ test:
         aws_access_key_id: AWS_ACCESS_KEY_ID
         aws_secret_access_key: AWS_SECRET_ACCESS_KEY
         region: us-east-1
+    local_store:
+      enabled: true
+      path: tmp/tests/pages
   repositories:
     storages:
       default:

config/initializers/1_settings.rb

@@ -310,6 +310,9 @@ Settings.pages['secret_file'] ||= Rails.root.join('.gitlab_pages_secret')
 # this will allow us to easier migrate existing instances with NFS
 Settings.pages['storage_path']      = Settings.pages['path']
 Settings.pages['object_store']      = ObjectStoreSettings.legacy_parse(Settings.pages['object_store'])
+Settings.pages['local_store'] ||= Settingslogic.new({})
+Settings.pages['local_store']['path'] = Settings.absolute(Settings.pages['local_store']['path'] || File.join(Settings.shared['path'], "pages"))
+Settings.pages['local_store']['enabled'] = true if Settings.pages['local_store']['enabled'].nil?
 
 #
 # GitLab documentation

config/initializers/pages_storage_check.rb

+# frozen_string_literal: true
+
+# This is to make sure at least one storage strategy for Pages is enabled.
+
+pages = Settings.pages
+
+return unless pages['enabled'] && pages['local_store']
+
+local_store_enabled = Gitlab::Utils.to_boolean(pages['local_store']['enabled'])
+object_store_enabled = Gitlab::Utils.to_boolean(pages['object_store']['enabled'])
+
+if !local_store_enabled && !object_store_enabled
+  raise "Please enable at least one of the two Pages storage strategy (local_store or object_store) in your config/gitlab.yml."
+end

lib/gitlab/pages/settings.rb

@@ -6,12 +6,22 @@ module Gitlab
       DiskAccessDenied = Class.new(StandardError)
 
       def path
-        if ::Gitlab::Runtime.web_server? && !::Gitlab::Runtime.test_suite?
-          raise DiskAccessDenied
-        end
+        ::Gitlab::ErrorTracking.track_exception(DiskAccessDenied.new) if disk_access_denied?
 
         super
       end
+
+      def local_store
+        @local_store ||= ::Gitlab::Pages::Stores::LocalStore.new(super)
+      end
+
+      private
+
+      def disk_access_denied?
+        return true unless ::Settings.pages.local_store&.enabled
+
+        ::Gitlab::Runtime.web_server? && !::Gitlab::Runtime.test_suite?
+      end
     end
   end
 end

lib/gitlab/pages/stores/local_store.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Pages
+    module Stores
+      class LocalStore < ::SimpleDelegator
+        def enabled
+          return false unless Feature.enabled?(:pages_update_legacy_storage, default_enabled: true)
+
+          super
+        end
+      end
+    end
+  end
+end

lib/gitlab/pages_transfer.rb

@@ -12,7 +12,7 @@ module Gitlab
     class Async
       METHODS.each do |meth|
         define_method meth do |*args|
-          next unless Feature.enabled?(:pages_update_legacy_storage, default_enabled: true)
+          next unless Settings.pages.local_store.enabled
 
           PagesTransferWorker.perform_async(meth, args)
         end
@@ -21,7 +21,7 @@ module Gitlab
 
     METHODS.each do |meth|
       define_method meth do |*args|
-        next unless Feature.enabled?(:pages_update_legacy_storage, default_enabled: true)
+        next unless Settings.pages.local_store.enabled
 
         super(*args)
       end

lib/gitlab/utils.rb

@@ -99,6 +99,8 @@ module Gitlab
     end
 
     def to_boolean(value, default: nil)
+      value = value.to_s if [0, 1].include?(value)
+
       return value if [true, false].include?(value)
       return true if value =~ /^(true|t|yes|y|1|on)$/i
       return false if value =~ /^(false|f|no|n|0|off)$/i

spec/initializers/pages_storage_check_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'pages storage check' do
+  let(:main_error_message) { "Please enable at least one of the two Pages storage strategy (local_store or object_store) in your config/gitlab.yml." }
+
+  subject(:initializer) { load Rails.root.join('config/initializers/pages_storage_check.rb') }
+
+  context 'when local store does not exist yet' do
+    before do
+      Settings.pages['local_store'] = nil
+    end
+
+    it { is_expected.to be_truthy }
+  end
+
+  context 'when pages is not enabled' do
+    before do
+      Settings.pages['enabled'] = false
+    end
+
+    it { is_expected.to be_truthy }
+  end
+
+  context 'when pages is enabled' do
+    before do
+      Settings.pages['enabled'] = true
+      Settings.pages['local_store'] = Settingslogic.new({})
+    end
+
+    context 'when pages object storage is not enabled' do
+      before do
+        Settings.pages['object_store']['enabled'] = false
+      end
+
+      context 'when pages local storage is not enabled' do
+        it 'raises an exception' do
+          Settings.pages['local_store']['enabled'] = false
+
+          expect { subject }.to raise_error(main_error_message)
+        end
+      end
+
+      context 'when pages local storage is enabled' do
+        it 'is true' do
+          Settings.pages['local_store']['enabled'] = true
+
+          expect(subject).to be_truthy
+        end
+      end
+    end
+
+    context 'when pages object storage is enabled' do
+      before do
+        Settings.pages['object_store']['enabled'] = true
+      end
+
+      context 'when pages local storage is not enabled' do
+        it 'is true' do
+          Settings.pages['local_store']['enabled'] = false
+
+          expect(subject).to be_truthy
+        end
+      end
+
+      context 'when pages local storage is enabled' do
+        it 'is true' do
+          Settings.pages['local_store']['enabled'] = true
+
+          expect(subject).to be_truthy
+        end
+      end
+    end
+
+    context 'when using integers instead of booleans' do
+      it 'is true' do
+        Settings.pages['local_store']['enabled'] = 1
+        Settings.pages['object_store']['enabled'] = 0
+
+        expect(subject).to be_truthy
+      end
+    end
+
+    context 'when both enabled attributes are not set' do
+      it 'raises an exception' do
+        Settings.pages['local_store']['enabled'] = nil
+        Settings.pages['object_store']['enabled'] = nil
+
+        expect { subject }.to raise_error(main_error_message)
+      end
+    end
+  end
+end

spec/lib/gitlab/pages/settings_spec.rb

@@ -3,11 +3,11 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Pages::Settings do
+  let(:settings) { double(path: 'the path', local_store: 'local store') }
+
   describe '#path' do
     subject { described_class.new(settings).path }
 
-    let(:settings) { double(path: 'the path') }
-
     it { is_expected.to eq('the path') }
 
     context 'when running under a web server outside of test mode' do
@@ -16,9 +16,43 @@ RSpec.describe Gitlab::Pages::Settings do
         allow(::Gitlab::Runtime).to receive(:web_server?).and_return(true)
       end
 
-      it 'raises a DiskAccessDenied exception' do
-        expect { subject }.to raise_error(described_class::DiskAccessDenied)
+      it 'logs a DiskAccessDenied error' do
+        expect(Gitlab::ErrorTracking).to receive(:track_exception).with(
+          instance_of(described_class::DiskAccessDenied)
+        )
+
+        subject
+      end
+    end
+
+    context 'when local_store settings does not exist yet' do
+      before do
+        allow(Settings.pages).to receive(:local_store).and_return(nil)
       end
+
+      it { is_expected.to eq('the path') }
+    end
+
+    context 'when local store exists but legacy storage is disabled' do
+      before do
+        allow(Settings.pages.local_store).to receive(:enabled).and_return(false)
+      end
+
+      it 'logs a DiskAccessDenied error' do
+        expect(Gitlab::ErrorTracking).to receive(:track_exception).with(
+          instance_of(described_class::DiskAccessDenied)
+        )
+
+        subject
+      end
+    end
+  end
+
+  describe '#local_store' do
+    subject(:local_store) { described_class.new(settings).local_store }
+
+    it 'is an instance of Gitlab::Pages::Stores::LocalStore' do
+      expect(local_store).to be_a(Gitlab::Pages::Stores::LocalStore)
     end
   end
 end

spec/lib/gitlab/pages/stores/local_store_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Pages::Stores::LocalStore do
+  describe '#enabled' do
+    let(:local_store) { double(enabled: true) }
+
+    subject(:local_store_enabled) { described_class.new(local_store).enabled }
+
+    context 'when the pages_update_legacy_storage FF is disabled' do
+      before do
+        stub_feature_flags(pages_update_legacy_storage: false)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when the pages_update_legacy_storage FF is enabled' do
+      it 'is equal to the original value' do
+        expect(local_store_enabled).to eq(local_store.enabled)
+      end
+    end
+  end
+end

spec/lib/gitlab/pages_transfer_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe Gitlab::PagesTransfer do
       end
 
       it 'does nothing if legacy storage is disabled' do
-        stub_feature_flags(pages_update_legacy_storage: false)
+        allow(Settings.pages.local_store).to receive(:enabled).and_return(false)
 
         described_class::METHODS.each do |meth|
           expect(PagesTransferWorker)
@@ -72,7 +72,7 @@ RSpec.describe Gitlab::PagesTransfer do
     end
 
     it 'does nothing if legacy storage is disabled' do
-      stub_feature_flags(pages_update_legacy_storage: false)
+      allow(Settings.pages.local_store).to receive(:enabled).and_return(false)
 
       subject.public_send(meth, *args)
 

spec/lib/gitlab/utils_spec.rb

@@ -192,6 +192,7 @@ RSpec.describe Gitlab::Utils do
       expect(to_boolean('YeS')).to be(true)
       expect(to_boolean('t')).to be(true)
       expect(to_boolean('1')).to be(true)
+      expect(to_boolean(1)).to be(true)
       expect(to_boolean('ON')).to be(true)
 
       expect(to_boolean('FaLse')).to be(false)
@@ -199,6 +200,7 @@ RSpec.describe Gitlab::Utils do
       expect(to_boolean('NO')).to be(false)
       expect(to_boolean('n')).to be(false)
       expect(to_boolean('0')).to be(false)
+      expect(to_boolean(0)).to be(false)
       expect(to_boolean('oFF')).to be(false)
     end
 

spec/models/project_spec.rb

@@ -4228,7 +4228,7 @@ RSpec.describe Project, factory_default: :keep do
     end
 
     it 'does nothing if updates on legacy storage are disabled' do
-      stub_feature_flags(pages_update_legacy_storage: false)
+      allow(Settings.pages.local_store).to receive(:enabled).and_return(false)
 
       expect(Gitlab::PagesTransfer).not_to receive(:new)
       expect(PagesWorker).not_to receive(:perform_in)

spec/services/pages/delete_services_spec.rb → spec/services/pages/delete_service_spec.rb

@@ -16,7 +16,10 @@ RSpec.describe Pages::DeleteService do
   it 'deletes published pages', :sidekiq_inline do
     expect(project.pages_deployed?).to be(true)
 
-    expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true
+    expect_next_instance_of(Gitlab::PagesTransfer) do |pages_transfer|
+      expect(pages_transfer).to receive(:rename_project).and_return true
+    end
+
     expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything)
 
     service.execute
@@ -24,11 +27,10 @@ RSpec.describe Pages::DeleteService do
     expect(project.pages_deployed?).to be(false)
   end
 
-  it "doesn't remove anything from the legacy storage if updates on it are disabled", :sidekiq_inline do
-    stub_feature_flags(pages_update_legacy_storage: false)
+  it "doesn't remove anything from the legacy storage", :sidekiq_inline do
+    allow(Settings.pages.local_store).to receive(:enabled).and_return(false)
 
     expect(project.pages_deployed?).to be(true)
-
     expect(PagesWorker).not_to receive(:perform_in)
 
     service.execute
@@ -69,7 +71,9 @@ RSpec.describe Pages::DeleteService do
     expect(project.pages_deployed?).to eq(false)
     expect(project.pages_domains.count).to eq(0)
 
-    expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true
+    expect_next_instance_of(Gitlab::PagesTransfer) do |pages_transfer|
+      expect(pages_transfer).to receive(:rename_project).and_return true
+    end
 
     Sidekiq::Worker.drain_all
   end

spec/services/projects/update_pages_configuration_service_spec.rb

@@ -35,7 +35,7 @@ RSpec.describe Projects::UpdatePagesConfigurationService do
         end
 
         it "doesn't update configuration files if updates on legacy storage are disabled" do
-          stub_feature_flags(pages_update_legacy_storage: false)
+          allow(Settings.pages.local_store).to receive(:enabled).and_return(false)
 
           expect(service).not_to receive(:update_file)
 

spec/services/projects/update_pages_service_spec.rb

@@ -56,7 +56,7 @@ RSpec.describe Projects::UpdatePagesService do
       end
 
       it "doesn't deploy to legacy storage if it's disabled" do
-        stub_feature_flags(pages_update_legacy_storage: false)
+        allow(Settings.pages.local_store).to receive(:enabled).and_return(false)
 
         expect(execute).to eq(:success)
         expect(project.pages_deployed?).to be_truthy

spec/workers/pages_update_configuration_worker_spec.rb

@@ -53,7 +53,7 @@ RSpec.describe PagesUpdateConfigurationWorker do
     end
 
     it "doesn't schedule a worker if updates on legacy storage are disabled", :sidekiq_inline do
-      stub_feature_flags(pages_update_legacy_storage: false)
+      allow(Settings.pages.local_store).to receive(:enabled).and_return(false)
 
       expect(Projects::UpdatePagesConfigurationService).not_to receive(:new)