diff --git a/app/models/concerns/has_repository.rb b/app/models/concerns/has_repository.rb index c6f93f14b1d60d8dbe0e6b868e0d68ae2cb46256..770777b88e7fae699169a963fbeb1331cfa89d51 100644 --- a/app/models/concerns/has_repository.rb +++ b/app/models/concerns/has_repository.rb @@ -47,6 +47,10 @@ def branch_exists?(branch) repository.branch_exists?(branch) end + def ref_exists?(ref) + repository.ref_exists?(ref) + end + def commit_by(oid:) repository.commit_by(oid: oid) end diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 9643457f5f5d98922d76316c530a8e652d9ff010..67271909e6acd9445de33a52389d0541c1f51193 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -197,7 +197,29 @@ def same_source_and_target_project? end def source_branch_exists? - source_branch.blank? || source_project.branch_exists?(source_branch) + source_branch.blank? || source_project.branch_exists?(source_branch) || ref_exists_in_gitaly? + end + + def ref_exists_in_gitaly? + return false unless Feature.enabled?(:pull_ref_directly_from_gitaly, project) + + # The following check acts as a fallback if there is a mismatch between + # the cache and repository state. If the branch ref does not exist in the cache + # then we validate the repository state with Gitaly to avoid an inconsistent response. + exists = source_project.ref_exists?("refs/heads/#{source_branch}") + + return false unless exists + + # Only log when we find a ref that exists in Gitaly but was not found elsewhere + Gitlab::AppJsonLogger.info( + class: self.class.to_s, + method: __method__, + project_id: project.id, + source_branch: source_branch, + **Gitlab::ApplicationContext.current + ) + + true end def target_branch_exists? diff --git a/config/feature_flags/gitlab_com_derisk/pull_ref_directly_from_gitaly.yml b/config/feature_flags/gitlab_com_derisk/pull_ref_directly_from_gitaly.yml new file mode 100644 index 0000000000000000000000000000000000000000..4c6044077cae3ada80eed82c5235a65fae18c65a --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/pull_ref_directly_from_gitaly.yml @@ -0,0 +1,9 @@ +--- +name: pull_ref_directly_from_gitaly +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/348565 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188492 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/536824 +milestone: '18.0' +group: group::source code +type: gitlab_com_derisk +default_enabled: false diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index b1ef229c02227f3795dc16836dfa57e98a3ce144..df784a3b9ea00a9ff0db76dd9171ace1caa39f32 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -43,10 +43,9 @@ def tag_exists? end def merge_request_ref_exists? - strong_memoize(:merge_request_ref_exists) do - MergeRequest.merge_request_ref?(origin_ref) && - project.repository.ref_exists?(origin_ref) - end + return check_merge_request_ref if Feature.enabled?(:pull_ref_directly_from_gitaly, project) + + strong_memoize(:merge_request_ref_exists) { check_merge_request_ref } end def ref @@ -166,6 +165,10 @@ def full_git_ref_name_unavailable? def gitlab_org_project? project.full_path == 'gitlab-org/gitlab' end + + def check_merge_request_ref + MergeRequest.merge_request_ref?(origin_ref) && project.repository.ref_exists?(origin_ref) + end end end end diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index d1b3de4c41827b082e7aa2312fc2aae291663188..fd94bc72202a67c832f3b95fbcf115eed082a225 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -126,6 +126,7 @@ subject { command.merge_request_ref_exists? } let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:origin_ref) { merge_request.source_branch } context 'for existing merge request ref' do let(:origin_ref) { merge_request.ref_path } @@ -134,10 +135,26 @@ end context 'for branch ref' do - let(:origin_ref) { merge_request.source_branch } - it { is_expected.to eq(false) } end + + it 'does not memoize the result' do + expect(command).to receive(:check_merge_request_ref).twice + + 2.times { command.merge_request_ref_exists? } + end + + context 'when pull_ref_directly_from_gitaly feature flag is disabled' do + before do + stub_feature_flags(pull_ref_directly_from_gitaly: false) + end + + it 'memoizes the result' do + expect(command).to receive(:check_merge_request_ref).once + + 2.times { command.merge_request_ref_exists? } + end + end end describe '#ref' do diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 1c7ba90442a04384915dfc9d790e5662aee87e99..ebc94af25753a7fd79d80f9baf110dc661d30533 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -14,6 +14,7 @@ let(:issue) { create(:issue, project: project, title: 'A bug', confidential: issue_confidential) } let(:description) { nil } let(:source_branch) { 'feature' } + let(:ref_path) { "refs/heads/#{source_branch}" } let(:target_branch) { 'master' } let(:milestone_id) { nil } let(:label_ids) { [] } @@ -133,6 +134,8 @@ def stub_compare before do project.add_reporter(user) + + allow(source_project).to receive(:ref_exists?).with(ref_path).and_return(false) end it 'assigns force_remove_source_branch' do @@ -543,6 +546,7 @@ def stub_compare before do allow(project).to receive(:branch_exists?).with(target_branch).and_return(true) allow(project).to receive(:branch_exists?).with(source_branch).and_return(false) + allow(project).to receive(:ref_exists?).with(ref_path).and_return(false) end it_behaves_like 'forbids the merge request from being created' do @@ -550,6 +554,61 @@ def stub_compare end end + context 'when a source branch ref exists in Gitaly but not in the ref cache' do + let(:log_arguments) do + { + class: 'MergeRequests::BuildService', + method: :ref_exists_in_gitaly?, + project_id: project.id, + source_branch: source_branch + } + end + + before do + allow(service).to receive(:source_project).and_return(project) + allow(project).to receive(:branch_exists?).with(target_branch).and_return(true) + allow(project).to receive(:branch_exists?).with(source_branch).and_return(false) + end + + context 'when the ref exists in Gitaly' do + before do + allow(project).to receive(:ref_exists?).with(ref_path).and_return(true) + end + + it 'logs info' do + expect(project).to receive(:ref_exists?).with(ref_path) + expect(Gitlab::AppJsonLogger).to receive(:info).with(hash_including(**log_arguments)) + + service.execute + end + end + + context 'when the ref does not exist in Gitaly' do + before do + allow(project).to receive(:ref_exists?).with(ref_path).and_return(false) + end + + it 'does not log info' do + expect(project).to receive(:ref_exists?).with(ref_path) + expect(Gitlab::AppJsonLogger).not_to receive(:info).with(hash_including(**log_arguments)) + + service.execute + end + end + + context 'and the pull_ref_directly_from_gitaly FF is disabled' do + before do + stub_feature_flags(pull_ref_directly_from_gitaly: false) + end + + it 'does not check for refs in Gitaly' do + expect(project).not_to receive(:ref_exists?).with(ref_path) + + service.execute + end + end + end + context 'target branch does not exist' do before do allow(project).to receive(:branch_exists?).with(target_branch).and_return(false) @@ -563,6 +622,7 @@ def stub_compare context 'both source and target branches do not exist' do before do + allow(project).to receive(:ref_exists?).with(ref_path).and_return(false) allow(project).to receive(:branch_exists?).and_return(false) end diff --git a/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb b/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb index a127451fdb68a9a194a261087b208528f577d35b..070864d673b17e1aab7662360ec4b271fb9d87cf 100644 --- a/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb @@ -139,6 +139,21 @@ it { expect(container.repo_exists?).to be(true) } end + describe '#ref_exists?' do + let(:ref_path) { 'refs/heads/master' } + let(:non_existent_ref_path) { 'refs/heads/foo' } + + before do + allow_next_instance_of(Gitlab::GitalyClient::RefService) do |ref_service| + allow(ref_service).to receive(:ref_exists?).with(ref_path).and_return(true) + allow(ref_service).to receive(:ref_exists?).with(non_existent_ref_path).and_return(false) + end + end + + it { expect(container.ref_exists?(ref_path)).to be(true) } + it { expect(container.ref_exists?(non_existent_ref_path)).to be(false) } + end + describe '#root_ref' do let(:root_ref) { container.repository.root_ref }