Skip to content

G
gitlab-ce
Commit 9a7fd03c authored by Etienne Baqué's avatar Etienne Baqué
Browse files
Options

Merge branch 'remove-pages-config-updates' into 'master' 

Remove legacy pages config updates

See merge request gitlab-org/gitlab!76918
parents d68ee8eb a1a540c0
Hide whitespace changes
Inline Side-by-side
Showing with 23 additions and 525 deletions
app/models/pages_domain.rb
View file @ 9a7fd03c
......@@ -46,9 +46,6 @@ class PagesDomain < ApplicationRecord
algorithm: 'aes-256-cbc'
after_initialize :set_verification_code
after_create :update_daemon
after_update :update_daemon, if: :saved_change_to_pages_config?
after_destroy :update_daemon
scope :for_project, ->(project) { where(project: project) }
......@@ -233,32 +230,6 @@ class PagesDomain < ApplicationRecord
self.verification_code = SecureRandom.hex(16)
end
# rubocop: disable CodeReuse/ServiceClass
def update_daemon
return if usage_serverless?
return unless pages_deployed?
run_after_commit { PagesUpdateConfigurationWorker.perform_async(project_id) }
end
# rubocop: enable CodeReuse/ServiceClass
def saved_change_to_pages_config?
saved_change_to_project_id? ||
saved_change_to_domain? ||
saved_change_to_certificate? ||
saved_change_to_key? ||
became_enabled? ||
became_disabled?
end
def became_enabled?
enabled_until.present? && !enabled_until_before_last_save.present?
end
def became_disabled?
!enabled_until.present? && enabled_until_before_last_save.present?
end
def validate_matching_key
unless has_matching_key?
self.errors.add(:key, "doesn't match the certificate")
......
app/services/projects/update_pages_configuration_service.rb deleted 100644 → 0
View file @ d68ee8eb
# frozen_string_literal: true
module Projects
class UpdatePagesConfigurationService < BaseService
include Gitlab::Utils::StrongMemoize
attr_reader :project
def initialize(project)
@project = project
end
def execute
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.
# https://gitlab.com/gitlab-org/gitlab/-/issues/235139
return success unless project.pages_deployed?
unless file_equals?(pages_config_file, pages_config_json)
update_file(pages_config_file, pages_config_json)
reload_daemon
end
success
end
private
def pages_config_json
strong_memoize(:pages_config_json) do
pages_config.to_json
end
end
def pages_config
{
domains: pages_domains_config,
https_only: project.pages_https_only?,
id: project.project_id,
access_control: !project.public_pages?
}
end
def pages_domains_config
enabled_pages_domains.map do |domain|
{
domain: domain.domain,
certificate: domain.certificate,
key: domain.key,
https_only: project.pages_https_only? && domain.https?,
id: project.project_id,
access_control: !project.public_pages?
}
end
end
def enabled_pages_domains
if Gitlab::CurrentSettings.pages_domain_verification_enabled?
project.pages_domains.enabled
else
project.pages_domains
end
end
def reload_daemon
# GitLab Pages daemon constantly watches for modification time of `pages.path`
# It reloads configuration when `pages.path` is modified
update_file(pages_update_file, SecureRandom.hex(64))
end
def pages_path
@pages_path ||= project.pages_path
end
def pages_config_file
File.join(pages_path, 'config.json')
end
def pages_update_file
File.join(::Settings.pages.path, '.update')
end
def update_file(file, data)
temp_file = "#{file}.#{SecureRandom.hex(16)}"
File.open(temp_file, 'w') do |f|
f.write(data)
end
FileUtils.move(temp_file, file, force: true)
ensure
# In case if the updating fails
FileUtils.remove(temp_file, force: true)
end
def file_equals?(file, data)
existing_data = read_file(file)
data == existing_data.to_s
end
def read_file(file)
File.open(file, 'r') do |f|
f.read
end
rescue StandardError
nil
end
end
end
app/services/projects/update_service.rb
View file @ 9a7fd03c
......@@ -104,7 +104,6 @@ module Projects
system_hook_service.execute_hooks_for(project, :update)
end
update_pages_config if changing_pages_related_config?
update_pending_builds if runners_settings_toggled?
end
......@@ -112,10 +111,6 @@ module Projects
AfterRenameService.new(project, path_before: project.path_before_last_save, full_path_before: project.full_path_before_last_save)
end
def changing_pages_related_config?
changing_pages_https_only? || changing_pages_access_level?
end
def update_failed!
model_errors = project.errors.full_messages.to_sentence
error_message = model_errors.presence || s_('UpdateProject|Project could not be updated!')
......@@ -143,10 +138,6 @@ module Projects
params.dig(:project_feature_attributes, :wiki_access_level).to_i > ProjectFeature::DISABLED
end
def changing_pages_access_level?
params.dig(:project_feature_attributes, :pages_access_level)
end
def ensure_wiki_exists
return if project.create_wiki
......@@ -154,16 +145,6 @@ module Projects
Gitlab::Metrics.counter(:wiki_can_not_be_created_total, 'Counts the times we failed to create a wiki').increment
end
def update_pages_config
return unless project.pages_deployed?
PagesUpdateConfigurationWorker.perform_async(project.id)
end
def changing_pages_https_only?
project.previous_changes.include?(:pages_https_only)
end
def changing_repository_storage?
new_repository_storage = params[:repository_storage]
......
app/workers/pages_update_configuration_worker.rb
View file @ 9a7fd03c
# frozen_string_literal: true
# TODO: remove this in 14.7 https://gitlab.com/gitlab-org/gitlab/-/issues/348582
class PagesUpdateConfigurationWorker
include ApplicationWorker
data_consistency :always
sidekiq_options retry: 3
sidekiq_options retry: 1
idempotent!
feature_category :pages
def self.perform_async(*args)
return unless ::Settings.pages.local_store.enabled
super(*args)
end
def perform(project_id)
project = Project.find_by_id(project_id)
return unless project
Projects::UpdatePagesConfigurationService.new(project).execute
def perform(_project_id)
# Do nothing
end
end
app/workers/pages_worker.rb
View file @ 9a7fd03c
......@@ -16,14 +16,7 @@ class PagesWorker # rubocop:disable Scalability/IdempotentWorker
def deploy(build_id)
build = Ci::Build.find_by_id(build_id)
update_contents = Projects::UpdatePagesService.new(build.project, build).execute
if update_contents[:status] == :success
Projects::UpdatePagesConfigurationService.new(build.project).execute
end
end
def remove(namespace_path, project_path)
full_path = File.join(Settings.pages.path, namespace_path, project_path)
FileUtils.rm_r(full_path, force: true)
Projects::UpdatePagesService.new(build.project, build).execute
end
end
spec/models/pages_domain_spec.rb
View file @ 9a7fd03c
......@@ -349,129 +349,6 @@ RSpec.describe PagesDomain do
end
end
describe '#update_daemon' do
let_it_be(:project) { create(:project).tap(&:mark_pages_as_deployed) }
context 'when usage is serverless' do
it 'does not call the UpdatePagesConfigurationService' do
expect(PagesUpdateConfigurationWorker).not_to receive(:perform_async)
create(:pages_domain, usage: :serverless)
end
end
it 'runs when the domain is created' do
domain = build(:pages_domain)
expect(domain).to receive(:update_daemon)
domain.save!
end
it 'runs when the domain is destroyed' do
domain = create(:pages_domain)
expect(domain).to receive(:update_daemon)
domain.destroy!
end
it "schedules a PagesUpdateConfigurationWorker" do
expect(PagesUpdateConfigurationWorker).to receive(:perform_async).with(project.id)
create(:pages_domain, project: project)
end
context "when the pages aren't deployed" do
let_it_be(:project) { create(:project).tap(&:mark_pages_as_not_deployed) }
it "does not schedule a PagesUpdateConfigurationWorker" do
expect(PagesUpdateConfigurationWorker).not_to receive(:perform_async).with(project.id)
create(:pages_domain, project: project)
end
end
context 'configuration updates when attributes change' do
let_it_be(:project1) { create(:project) }
let_it_be(:project2) { create(:project) }
let_it_be(:domain) { create(:pages_domain) }
where(:attribute, :old_value, :new_value, :update_expected) do
now = Time.current
future = now + 1.day
:project | nil | :project1 | true
:project | :project1 | :project1 | false
:project | :project1 | :project2 | true
:project | :project1 | nil | true
# domain can't be set to nil
:domain | 'a.com' | 'a.com' | false
:domain | 'a.com' | 'b.com' | true
# verification_code can't be set to nil
:verification_code | 'foo' | 'foo' | false
:verification_code | 'foo' | 'bar' | false
:verified_at | nil | now | false
:verified_at | now | now | false
:verified_at | now | future | false
:verified_at | now | nil | false
:enabled_until | nil | now | true
:enabled_until | now | now | false
:enabled_until | now | future | false
:enabled_until | now | nil | true
end
with_them do
it 'runs if a relevant attribute has changed' do
a = old_value.is_a?(Symbol) ? send(old_value) : old_value
b = new_value.is_a?(Symbol) ? send(new_value) : new_value
domain.update!(attribute => a)
if update_expected
expect(domain).to receive(:update_daemon)
else
expect(domain).not_to receive(:update_daemon)
end
domain.update!(attribute => b)
end
end
context 'TLS configuration' do
let_it_be(:domain_without_tls) { create(:pages_domain, :without_certificate, :without_key) }
let_it_be(:domain) { create(:pages_domain) }
let(:cert1) { domain.certificate }
let(:cert2) { cert1 + ' ' }
let(:key1) { domain.key }
let(:key2) { key1 + ' ' }
it 'updates when added' do
expect(domain_without_tls).to receive(:update_daemon)
domain_without_tls.update!(key: key1, certificate: cert1)
end
it 'updates when changed' do
expect(domain).to receive(:update_daemon)
domain.update!(key: key2, certificate: cert2)
end
it 'updates when removed' do
expect(domain).to receive(:update_daemon)
domain.update!(key: nil, certificate: nil)
end
end
end
end
describe '#user_provided_key' do
subject { domain.user_provided_key }
......
spec/services/projects/update_pages_configuration_service_spec.rb deleted 100644 → 0
View file @ d68ee8eb
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Projects::UpdatePagesConfigurationService do
let(:service) { described_class.new(project) }
describe "#execute" do
subject { service.execute }
context 'when pages are deployed' do
let_it_be(:project) do
create(:project).tap(&:mark_pages_as_deployed)
end
let(:file) { Tempfile.new('pages-test') }
before do
allow(service).to receive(:pages_config_file).and_return(file.path)
end
after do
file.close
file.unlink
end
context 'when configuration changes' do
it 'updates the config and reloads the daemon' do
expect(service).to receive(:update_file).with(file.path, an_instance_of(String))
.and_call_original
allow(service).to receive(:update_file).with(File.join(::Settings.pages.path, '.update'),
an_instance_of(String)).and_call_original
expect(subject).to include(status: :success)
end
it "doesn't update configuration files if updates on legacy storage are disabled" do
allow(Settings.pages.local_store).to receive(:enabled).and_return(false)
expect(service).not_to receive(:update_file)
expect(subject).to include(status: :success)
end
end
context 'when configuration does not change' do
before do
# we set the configuration
service.execute
end
it 'does not update anything' do
expect(service).not_to receive(:update_file)
expect(subject).to include(status: :success)
end
end
end
context 'when pages are not deployed' do
let_it_be(:project) do
create(:project).tap(&:mark_pages_as_not_deployed)
end
it 'returns successfully' do
expect(subject).to eq(status: :success)
end
it 'does not update the config' do
expect(service).not_to receive(:update_file)
subject
end
end
end
end
spec/services/projects/update_service_spec.rb
View file @ 9a7fd03c
......@@ -379,52 +379,6 @@ RSpec.describe Projects::UpdateService do
end
end
shared_examples 'updating pages configuration' do
it 'schedules the `PagesUpdateConfigurationWorker` when pages are deployed' do
project.mark_pages_as_deployed
expect(PagesUpdateConfigurationWorker).to receive(:perform_async).with(project.id)
subject
end
it "does not schedule a job when pages aren't deployed" do
project.mark_pages_as_not_deployed
expect(PagesUpdateConfigurationWorker).not_to receive(:perform_async).with(project.id)
subject
end
end
context 'when updating #pages_https_only', :https_pages_enabled do
subject(:call_service) do
update_project(project, admin, pages_https_only: false)
end
it 'updates the attribute' do
expect { call_service }
.to change { project.pages_https_only? }
.to(false)
end
it_behaves_like 'updating pages configuration'
end
context 'when updating #pages_access_level' do
subject(:call_service) do
update_project(project, admin, project_feature_attributes: { pages_access_level: ProjectFeature::ENABLED })
end
it 'updates the attribute' do
expect { call_service }
.to change { project.project_feature.pages_access_level }
.to(ProjectFeature::ENABLED)
end
it_behaves_like 'updating pages configuration'
end
context 'when updating #emails_disabled' do
it 'updates the attribute for the project owner' do
expect { update_project(project, user, emails_disabled: true) }
......
spec/services/verify_pages_domain_service_spec.rb
View file @ 9a7fd03c
......@@ -269,56 +269,6 @@ RSpec.describe VerifyPagesDomainService do
end
end
context 'pages configuration updates' do
context 'enabling a disabled domain' do
let(:domain) { create(:pages_domain, :disabled) }
it 'schedules an update' do
stub_resolver(domain.domain => domain.verification_code)
expect(domain).to receive(:update_daemon)
service.execute
end
end
context 'verifying an enabled domain' do
let(:domain) { create(:pages_domain) }
it 'schedules an update' do
stub_resolver(domain.domain => domain.verification_code)
expect(domain).not_to receive(:update_daemon)
service.execute
end
end
context 'disabling an expired domain' do
let(:domain) { create(:pages_domain, :expired) }
it 'schedules an update' do
stub_resolver
expect(domain).to receive(:update_daemon)
service.execute
end
end
context 'failing to verify a disabled domain' do
let(:domain) { create(:pages_domain, :disabled) }
it 'does not schedule an update' do
stub_resolver
expect(domain).not_to receive(:update_daemon)
service.execute
end
end
end
context 'no verification code' do
let(:domain) { create(:pages_domain) }
......
spec/workers/every_sidekiq_worker_spec.rb
View file @ 9a7fd03c
......@@ -369,7 +369,7 @@ RSpec.describe 'Every Sidekiq worker' do
'PagesDomainSslRenewalWorker' => 3,
'PagesDomainVerificationWorker' => 3,
'PagesTransferWorker' => 3,
'PagesUpdateConfigurationWorker' => 3,
'PagesUpdateConfigurationWorker' => 1,
'PagesWorker' => 3,
'PersonalAccessTokens::Groups::PolicyWorker' => 3,
'PersonalAccessTokens::Instance::PolicyWorker' => 3,
......
spec/workers/pages_update_configuration_worker_spec.rb
View file @ 9a7fd03c
......@@ -5,59 +5,8 @@ RSpec.describe PagesUpdateConfigurationWorker do
let_it_be(:project) { create(:project) }
describe "#perform" do
it "does not break if the project doesn't exist" do
it "does not break" do
expect { subject.perform(-1) }.not_to raise_error
end
it "calls the correct service" do
expect_next_instance_of(Projects::UpdatePagesConfigurationService, project) do |service|
expect(service).to receive(:execute).and_return({})
end
subject.perform(project.id)
end
it_behaves_like "an idempotent worker" do
let(:job_args) { [project.id] }
let(:pages_dir) { Dir.mktmpdir }
let(:config_path) { File.join(pages_dir, "config.json") }
before do
allow(Project).to receive(:find_by_id).with(project.id).and_return(project)
allow(project).to receive(:pages_path).and_return(pages_dir)
# Make sure _some_ config exists
FileUtils.touch(config_path)
end
after do
FileUtils.remove_entry(pages_dir)
end
it "only updates the config file once" do
described_class.new.perform(project.id)
expect(File.mtime(config_path)).not_to be_nil
expect { subject }.not_to change { File.mtime(config_path) }
end
end
end
describe '#perform_async' do
it "calls the correct service", :sidekiq_inline do
expect_next_instance_of(Projects::UpdatePagesConfigurationService, project) do |service|
expect(service).to receive(:execute).and_return(status: :success)
end
described_class.perform_async(project.id)
end
it "doesn't schedule a worker if updates on legacy storage are disabled", :sidekiq_inline do
allow(Settings.pages.local_store).to receive(:enabled).and_return(false)
expect(Projects::UpdatePagesConfigurationService).not_to receive(:new)
described_class.perform_async(project.id)
end
end
end
spec/workers/pages_worker_spec.rb 0 → 100644
View file @ 9a7fd03c
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe PagesWorker, :sidekiq_inline do
let(:project) { create(:project) }
let(:ci_build) { create(:ci_build, project: project)}
it 'calls UpdatePagesService' do
expect_next_instance_of(Projects::UpdatePagesService, project, ci_build) do |service|
expect(service).to receive(:execute)
end
described_class.perform_async(:deploy, ci_build.id)
end
end
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7