From 2abbe24399204d250a3ba390ebabb24812247176 Mon Sep 17 00:00:00 2001 From: ngala Date: Wed, 21 Aug 2024 22:25:10 +0530 Subject: [PATCH 1/5] Pages unique domain url update Related: https://gitlab.com/gitlab-org/gitlab/-/issues/441215 Changelog: fixed --- app/models/project_setting.rb | 4 +++ lib/gitlab/pages.rb | 12 ++++++-- lib/gitlab/pages/random_domain.rb | 32 ++++++--------------- spec/lib/gitlab/pages/random_domain_spec.rb | 17 +++++------ spec/lib/gitlab/pages_spec.rb | 14 +++++++++ 5 files changed, 43 insertions(+), 36 deletions(-) diff --git a/app/models/project_setting.rb b/app/models/project_setting.rb index f15923ce672469..4f97ad070ee4df 100644 --- a/app/models/project_setting.rb +++ b/app/models/project_setting.rb @@ -54,6 +54,10 @@ class ProjectSetting < ApplicationRecord Feature.enabled?(:legacy_open_source_license_available, type: :ops) end + def self.unique_domain_exists?(domain) + where(pages_unique_domain: domain).exists? + end + def squash_enabled_by_default? %w[always default_on].include?(squash_option) end diff --git a/lib/gitlab/pages.rb b/lib/gitlab/pages.rb index defe054d631261..07ab2635979975 100644 --- a/lib/gitlab/pages.rb +++ b/lib/gitlab/pages.rb @@ -35,9 +35,15 @@ def add_unique_domain_to(project) return if project.project_setting.pages_unique_domain_in_database.present? project.project_setting.pages_unique_domain_enabled = true - project.project_setting.pages_unique_domain = Gitlab::Pages::RandomDomain.generate( - project_path: project.path, - namespace_path: project.parent.full_path) + + # Generate and assign a unique domain + loop do + pages_unique_domain = Gitlab::Pages::RandomDomain.generate(project_path: project.path) + unless ProjectSetting.unique_domain_exists?(pages_unique_domain) + project.project_setting.pages_unique_domain = pages_unique_domain + break + end + end end def multiple_versions_enabled_for?(project) diff --git a/lib/gitlab/pages/random_domain.rb b/lib/gitlab/pages/random_domain.rb index 8aa7611c91042a..6d71639d3af396 100644 --- a/lib/gitlab/pages/random_domain.rb +++ b/lib/gitlab/pages/random_domain.rb @@ -3,41 +3,27 @@ module Gitlab module Pages class RandomDomain - PROJECT_PATH_LIMIT = 48 - SUBDOMAIN_LABEL_LIMIT = 63 + PROJECT_PATH_LIMIT = 56 - def self.generate(project_path:, namespace_path:) - new(project_path: project_path, namespace_path: namespace_path).generate + def self.generate(project_path:) + new(project_path: project_path).generate end - def initialize(project_path:, namespace_path:) + def initialize(project_path:) @project_path = project_path - @namespace_path = namespace_path end # Subdomains have a limit of 63 bytes (https://www.freesoft.org/CIE/RFC/1035/9.htm) # For this reason we're limiting each part of the unique subdomain # - # The domain is made up of 3 parts, like: projectpath-namespacepath-randomstring - # - project path: between 1 and 48 chars - # - namespace path: when the project path has less than 48 chars, - # the namespace full path will be used to fill the value up to 48 chars - # - random hexadecimal: to ensure a random value, the domain is then filled - # with a random hexadecimal value to complete 63 chars + # The domain is made up of 2 parts, like: projectpath-randomstring + # - project path: between 1 and 56 chars + # - random hexadecimal: to ensure a random value of length 6 def generate domain = project_path.byteslice(0, PROJECT_PATH_LIMIT) - # if the project_path has less than PROJECT_PATH_LIMIT chars, - # fill the domain with the parent full_path up to 48 chars like: - # projectpath-namespacepath - if domain.length < PROJECT_PATH_LIMIT - namespace_size = PROJECT_PATH_LIMIT - domain.length - 1 - domain.concat('-', namespace_path.byteslice(0, namespace_size)) - end - - # Complete the domain with random hexadecimal values util it is 63 chars long # PS.: SecureRandom.hex return an string twice the size passed as argument. - domain.concat('-', SecureRandom.hex(SUBDOMAIN_LABEL_LIMIT - domain.length - 1)) + domain.concat('-', SecureRandom.hex(3)) # Slugify ensures the format and size (63 chars) of the given string Gitlab::Utils.slugify(domain) @@ -45,7 +31,7 @@ def generate private - attr_reader :project_path, :namespace_path + attr_reader :project_path end end end diff --git a/spec/lib/gitlab/pages/random_domain_spec.rb b/spec/lib/gitlab/pages/random_domain_spec.rb index 978412bb72cca8..697f3360a42a33 100644 --- a/spec/lib/gitlab/pages/random_domain_spec.rb +++ b/spec/lib/gitlab/pages/random_domain_spec.rb @@ -3,10 +3,8 @@ require 'spec_helper' RSpec.describe Gitlab::Pages::RandomDomain, feature_category: :pages do - let(:namespace_path) { 'namespace' } - subject(:generator) do - described_class.new(project_path: project_path, namespace_path: namespace_path) + described_class.new(project_path: project_path) end RSpec.shared_examples 'random domain' do |domain| @@ -14,31 +12,30 @@ expect(SecureRandom) .to receive(:hex) .and_wrap_original do |_, size, _| - ('h' * size) + ('h' * size * 2) end generated = generator.generate expect(generated).to eq(domain) - expect(generated.length).to eq(63) end end context 'when project path is less than 48 chars' do let(:project_path) { 'p' } - it_behaves_like 'random domain', 'p-namespace-hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh' + it_behaves_like 'random domain', 'p-hhhhhh' end context 'when project path is close to 48 chars' do - let(:project_path) { 'p' * 45 } + let(:project_path) { 'p' * 56 } - it_behaves_like 'random domain', 'ppppppppppppppppppppppppppppppppppppppppppppp-na-hhhhhhhhhhhhhh' + it_behaves_like 'random domain', 'pppppppppppppppppppppppppppppppppppppppppppppppppppppppp-hhhhhh' end context 'when project path is larger than 48 chars' do - let(:project_path) { 'p' * 49 } + let(:project_path) { 'p' * 57 } - it_behaves_like 'random domain', 'pppppppppppppppppppppppppppppppppppppppppppppppp-hhhhhhhhhhhhhh' + it_behaves_like 'random domain', 'pppppppppppppppppppppppppppppppppppppppppppppppppppppppp-hhhhhh' end end diff --git a/spec/lib/gitlab/pages_spec.rb b/spec/lib/gitlab/pages_spec.rb index c20956788ac5df..d4aed97ae7a383 100644 --- a/spec/lib/gitlab/pages_spec.rb +++ b/spec/lib/gitlab/pages_spec.rb @@ -131,6 +131,20 @@ expect(project.project_setting.pages_unique_domain).to eq('unique-domain') end end + + context 'when a unique domain is already in use and needs to generate a new one' do + it 'generates a different unique domain if the original is already taken' do + allow(Gitlab::Pages::RandomDomain).to receive(:generate).and_return('existing-domain', 'new-unique-domain') + + # Simulate the existing domain being in use + create(:project_setting, pages_unique_domain: 'existing-domain') + + described_class.add_unique_domain_to(project) + + expect(project.project_setting.pages_unique_domain_enabled).to eq(true) + expect(project.project_setting.pages_unique_domain).to eq('new-unique-domain') + end + end end end end -- GitLab From 6966627e921ff312f1062cf8b5a46ee16bb66d20 Mon Sep 17 00:00:00 2001 From: ngala Date: Fri, 23 Aug 2024 20:59:00 +0530 Subject: [PATCH 2/5] address review comment --- app/models/project_setting.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/project_setting.rb b/app/models/project_setting.rb index 4f97ad070ee4df..ac25c3f3c2f204 100644 --- a/app/models/project_setting.rb +++ b/app/models/project_setting.rb @@ -54,6 +54,7 @@ class ProjectSetting < ApplicationRecord Feature.enabled?(:legacy_open_source_license_available, type: :ops) end + # Checks if a given domain is already assigned to any existing project def self.unique_domain_exists?(domain) where(pages_unique_domain: domain).exists? end -- GitLab From 516cbfd33022b1be6088fa25ff86ba4a215c6c8c Mon Sep 17 00:00:00 2001 From: ngala Date: Tue, 27 Aug 2024 15:27:40 +0530 Subject: [PATCH 3/5] Loop max 10 times and raise error --- app/services/projects/update_service.rb | 4 ++-- lib/gitlab/pages.rb | 7 +++++++ spec/lib/gitlab/pages_spec.rb | 14 ++++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index a0748015e4834c..d3ad4f0c26b515 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -39,10 +39,10 @@ def execute else update_failed! end - rescue ValidationError => e - error(e.message) rescue ApiError => e error(e.message, status: :api_error) + rescue ValidationError, StandardError => e + error(e.message) end def run_auto_devops_pipeline? diff --git a/lib/gitlab/pages.rb b/lib/gitlab/pages.rb index 07ab2635979975..ba10a459bf8dcd 100644 --- a/lib/gitlab/pages.rb +++ b/lib/gitlab/pages.rb @@ -37,12 +37,19 @@ def add_unique_domain_to(project) project.project_setting.pages_unique_domain_enabled = true # Generate and assign a unique domain + max_attempts = 10 + attempts = 0 + loop do + attempts += 1 pages_unique_domain = Gitlab::Pages::RandomDomain.generate(project_path: project.path) + unless ProjectSetting.unique_domain_exists?(pages_unique_domain) project.project_setting.pages_unique_domain = pages_unique_domain break end + + raise StandardError, "Failed to generate a unique domain" if attempts >= max_attempts end end diff --git a/spec/lib/gitlab/pages_spec.rb b/spec/lib/gitlab/pages_spec.rb index d4aed97ae7a383..a659413a2500b0 100644 --- a/spec/lib/gitlab/pages_spec.rb +++ b/spec/lib/gitlab/pages_spec.rb @@ -145,6 +145,20 @@ expect(project.project_setting.pages_unique_domain).to eq('new-unique-domain') end end + + context 'when generated 10 unique domains are already in use' do + it 'raises an error' do + allow(Gitlab::Pages::RandomDomain).to receive(:generate).and_return('existing-domain') + + # Simulate the existing domain being in use + create(:project_setting, pages_unique_domain: 'existing-domain') + + expect { described_class.add_unique_domain_to(project) } + .to raise_error(StandardError, 'Failed to generate a unique domain') + + expect(project.project_setting.pages_unique_domain).to be_nil + end + end end end end -- GitLab From 276758fe061741d98832228933be6671879a34a8 Mon Sep 17 00:00:00 2001 From: ngala Date: Tue, 27 Aug 2024 18:38:30 +0530 Subject: [PATCH 4/5] apply review comment --- app/services/projects/create_service.rb | 2 +- app/services/projects/update_service.rb | 2 +- lib/gitlab/pages.rb | 34 +++++++++++++------------ spec/lib/gitlab/pages_spec.rb | 6 +++-- 4 files changed, 24 insertions(+), 20 deletions(-) diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 844c4e8574d8c3..f2c2873295b0ce 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -92,7 +92,7 @@ def execute rescue ImportSourceDisabledError => e @project.errors.add(:import_source_disabled, e.message) if @project fail(error: e.message) - rescue StandardError => e + rescue Gitlab::Pages::UniqueDomainGenerationFailure, StandardError => e @project.errors.add(:base, e.message) if @project fail(error: e.message) end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index d3ad4f0c26b515..93821f016edd64 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -41,7 +41,7 @@ def execute end rescue ApiError => e error(e.message, status: :api_error) - rescue ValidationError, StandardError => e + rescue ValidationError, Gitlab::Pages::UniqueDomainGenerationFailure => e error(e.message) end diff --git a/lib/gitlab/pages.rb b/lib/gitlab/pages.rb index ba10a459bf8dcd..928248c1235249 100644 --- a/lib/gitlab/pages.rb +++ b/lib/gitlab/pages.rb @@ -9,6 +9,12 @@ module Pages include JwtAuthenticatable + class UniqueDomainGenerationFailure < StandardError + def initialize(msg = "Can't generate unique domain for GitLab Pages") + super(msg) + end + end + class << self def verify_api_request(request_headers) decode_jwt(request_headers[INTERNAL_API_REQUEST_HEADER], issuer: 'gitlab-pages') @@ -35,22 +41,7 @@ def add_unique_domain_to(project) return if project.project_setting.pages_unique_domain_in_database.present? project.project_setting.pages_unique_domain_enabled = true - - # Generate and assign a unique domain - max_attempts = 10 - attempts = 0 - - loop do - attempts += 1 - pages_unique_domain = Gitlab::Pages::RandomDomain.generate(project_path: project.path) - - unless ProjectSetting.unique_domain_exists?(pages_unique_domain) - project.project_setting.pages_unique_domain = pages_unique_domain - break - end - - raise StandardError, "Failed to generate a unique domain" if attempts >= max_attempts - end + project.project_setting.pages_unique_domain = generate_unique_domain(project) end def multiple_versions_enabled_for?(project) @@ -60,6 +51,17 @@ def multiple_versions_enabled_for?(project) project.licensed_feature_available?(:pages_multiple_versions) && project.project_setting.pages_multiple_versions_enabled end + + private + + def generate_unique_domain(project) + 10.times do + pages_unique_domain = Gitlab::Pages::RandomDomain.generate(project_path: project.path) + return pages_unique_domain unless ProjectSetting.unique_domain_exists?(pages_unique_domain) + end + + raise UniqueDomainGenerationFailure + end end end end diff --git a/spec/lib/gitlab/pages_spec.rb b/spec/lib/gitlab/pages_spec.rb index a659413a2500b0..928e5a6c461169 100644 --- a/spec/lib/gitlab/pages_spec.rb +++ b/spec/lib/gitlab/pages_spec.rb @@ -153,8 +153,10 @@ # Simulate the existing domain being in use create(:project_setting, pages_unique_domain: 'existing-domain') - expect { described_class.add_unique_domain_to(project) } - .to raise_error(StandardError, 'Failed to generate a unique domain') + expect { described_class.add_unique_domain_to(project) }.to raise_error( + described_class::UniqueDomainGenerationFailure, + "Can't generate unique domain for GitLab Pages" + ) expect(project.project_setting.pages_unique_domain).to be_nil end -- GitLab From 905c587d4a5becf00e6cbbdf0c93a161ffbc5550 Mon Sep 17 00:00:00 2001 From: Naman Jagdish Gala Date: Wed, 28 Aug 2024 16:38:07 +0000 Subject: [PATCH 5/5] Address review comment --- app/services/projects/create_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index f2c2873295b0ce..844c4e8574d8c3 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -92,7 +92,7 @@ def execute rescue ImportSourceDisabledError => e @project.errors.add(:import_source_disabled, e.message) if @project fail(error: e.message) - rescue Gitlab::Pages::UniqueDomainGenerationFailure, StandardError => e + rescue StandardError => e @project.errors.add(:base, e.message) if @project fail(error: e.message) end -- GitLab