Merge branch 'pks-find-remote-root-ref-inmemory-remote' into 'master'

Use in-memory remotes to find remote root refs [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!60583

Gemfile

@@ -481,7 +481,7 @@ end
 gem 'spamcheck', '~> 0.0.5'
 
 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 13.11.0.pre.rc1'
+gem 'gitaly', '~> 13.12.0.pre.rc1'
 
 gem 'grpc', '~> 1.30.2'
 

Gemfile.lock

@@ -439,7 +439,7 @@ GEM
       rails (>= 3.2.0)
     git (1.7.0)
       rchardet (~> 1.8)
-    gitaly (13.11.0.pre.rc1)
+    gitaly (13.12.0.pre.rc1)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab (4.16.1)
@@ -1446,7 +1446,7 @@ DEPENDENCIES
   gettext (~> 3.3)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 13.11.0.pre.rc1)
+  gitaly (~> 13.12.0.pre.rc1)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-dangerfiles (~> 2.0.0)

config/feature_flags/development/find_remote_root_refs_inmemory.yml

+---
+name: find_remote_root_refs_inmemory
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60583
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/329664
+milestone: '13.12'
+type: development
+group: group::gitaly
+default_enabled: true

ee/app/models/ee/project.rb

@@ -707,8 +707,8 @@ module EE
     end
 
     # Update the default branch querying the remote to determine its HEAD
-    def update_root_ref(remote_name)
-      root_ref = repository.find_remote_root_ref(remote_name)
+    def update_root_ref(remote, remote_url, authorization)
+      root_ref = repository.find_remote_root_ref(remote, remote_url, authorization)
       change_head(root_ref) if root_ref.present?
     end
 

ee/app/services/geo/repository_sync_service.rb

@@ -50,9 +50,16 @@ module Geo
     end
 
     def update_root_ref
-      # Find the remote root ref, using a JWT header for authentication
-      repository.with_config(jwt_authentication_header) do
-        project.update_root_ref(GEO_REMOTE_NAME)
+      if Feature.enabled?(:find_remote_root_refs_inmemory, default_enabled: :yaml)
+        authorization = ::Gitlab::Geo::RepoSyncRequest.new(
+          scope: repository.full_path
+        ).authorization
+
+        project.update_root_ref(GEO_REMOTE_NAME, remote_url, authorization)
+      else
+        repository_with_config(jwt_authentication_header) do
+          project.update_root_ref(GEO_REMOTE_NAME, remote_url)
+        end
       end
     end
 

ee/spec/models/project_spec.rb

@@ -2147,11 +2147,13 @@ RSpec.describe Project do
 
   describe '#update_root_ref' do
     let(:project) { create(:project, :repository) }
+    let(:url) { 'http://git.example.com/remote-repo.git' }
+    let(:auth) { 'Basic secret' }
 
     it 'updates the default branch when HEAD has changed' do
       stub_find_remote_root_ref(project, ref: 'feature')
 
-      expect { project.update_root_ref('origin') }
+      expect { project.update_root_ref('origin', url, auth) }
         .to change { project.default_branch }
         .from('master')
         .to('feature')
@@ -2162,7 +2164,7 @@ RSpec.describe Project do
 
       expect(project).to receive(:change_head).with('master').and_call_original
 
-      project.update_root_ref('origin')
+      project.update_root_ref('origin', url, auth)
 
       # For good measure, expunge the root ref cache and reload.
       project.repository.expire_all_method_caches
@@ -2172,14 +2174,14 @@ RSpec.describe Project do
     it 'does not update the default branch when HEAD does not exist' do
       stub_find_remote_root_ref(project, ref: 'foo')
 
-      expect { project.update_root_ref('origin') }
+      expect { project.update_root_ref('origin', url, auth) }
         .not_to change { project.default_branch }
     end
 
     def stub_find_remote_root_ref(project, ref:)
       allow(project.repository)
         .to receive(:find_remote_root_ref)
-        .with('origin')
+        .with('origin', url, auth)
         .and_return(ref)
     end
   end

ee/spec/services/geo/repository_sync_service_spec.rb

@@ -36,7 +36,7 @@ RSpec.describe Geo::RepositorySyncService, :geo do
 
       allow_any_instance_of(Repository)
         .to receive(:find_remote_root_ref)
-        .with('geo')
+        .with('geo', url_to_repo, anything)
         .and_return('master')
 
       allow_any_instance_of(Geo::ProjectHousekeepingService).to receive(:execute)
@@ -48,7 +48,6 @@ RSpec.describe Geo::RepositorySyncService, :geo do
     it 'fetches project repository with JWT credentials' do
       expect(repository).to receive(:with_config)
         .with("http.#{url_to_repo}.extraHeader" => anything)
-        .twice
         .and_call_original
 
       expect(repository).to receive(:fetch_as_mirror)
@@ -238,7 +237,7 @@ RSpec.describe Geo::RepositorySyncService, :geo do
             before do
               allow(project.repository)
                 .to receive(:find_remote_root_ref)
-                .with('geo')
+                .with('geo', url_to_repo, anything)
                 .and_return('feature')
             end
 
@@ -251,7 +250,6 @@ RSpec.describe Geo::RepositorySyncService, :geo do
             it 'updates the default branch with JWT credentials' do
               expect(repository).to receive(:with_config)
                 .with("http.#{url_to_repo}.extraHeader" => anything)
-                .twice
                 .and_call_original
 
               expect(project).to receive(:change_head).with('feature').once
@@ -264,7 +262,7 @@ RSpec.describe Geo::RepositorySyncService, :geo do
             before do
               allow(project.repository)
                 .to receive(:find_remote_root_ref)
-                .with('geo')
+                .with('geo', url_to_repo, anything)
                 .and_return(project.default_branch)
             end
 
@@ -277,7 +275,6 @@ RSpec.describe Geo::RepositorySyncService, :geo do
             it 'updates the default branch with JWT credentials' do
               expect(repository).to receive(:with_config)
                 .with("http.#{url_to_repo}.extraHeader" => anything)
-                .twice
                 .and_call_original
 
               expect(project).to receive(:change_head).with('master').once

lib/gitlab/git/repository.rb

@@ -700,11 +700,11 @@ module Gitlab
         end
       end
 
-      def find_remote_root_ref(remote_name)
-        return unless remote_name.present?
+      def find_remote_root_ref(remote_name, remote_url, authorization = nil)
+        return unless remote_name.present? && remote_url.present?
 
         wrapped_gitaly_errors do
-          gitaly_remote_client.find_remote_root_ref(remote_name)
+          gitaly_remote_client.find_remote_root_ref(remote_name, remote_url, authorization)
         end
       end
 

lib/gitlab/gitaly_client/remote_service.rb

@@ -43,11 +43,20 @@ module Gitlab
         GitalyClient.call(@storage, :remote_service, :remove_remote, request, timeout: GitalyClient.long_timeout).result
       end
 
-      def find_remote_root_ref(remote_name)
-        request = Gitaly::FindRemoteRootRefRequest.new(
-          repository: @gitaly_repo,
-          remote: remote_name
-        )
+      # The remote_name parameter is deprecated and will be removed soon.
+      def find_remote_root_ref(remote_name, remote_url, authorization)
+        request = if Feature.enabled?(:find_remote_root_refs_inmemory, default_enabled: :yaml)
+                    Gitaly::FindRemoteRootRefRequest.new(
+                      repository: @gitaly_repo,
+                      remote_url: remote_url,
+                      http_authorization_header: authorization
+                    )
+                  else
+                    Gitaly::FindRemoteRootRefRequest.new(
+                      repository: @gitaly_repo,
+                      remote: remote_name
+                    )
+                  end
 
         response = GitalyClient.call(@storage, :remote_service,
                                      :find_remote_root_ref, request, timeout: GitalyClient.medium_timeout)

spec/lib/gitlab/git/repository_spec.rb

@@ -604,29 +604,29 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
       expect_any_instance_of(Gitlab::GitalyClient::RemoteService)
         .to receive(:find_remote_root_ref).and_call_original
 
-      expect(repository.find_remote_root_ref('origin')).to eq 'master'
+      expect(repository.find_remote_root_ref('origin', SeedHelper::GITLAB_GIT_TEST_REPO_URL)).to eq 'master'
     end
 
     it 'returns UTF-8' do
-      expect(repository.find_remote_root_ref('origin')).to be_utf8
+      expect(repository.find_remote_root_ref('origin', SeedHelper::GITLAB_GIT_TEST_REPO_URL)).to be_utf8
     end
 
     it 'returns nil when remote name is nil' do
       expect_any_instance_of(Gitlab::GitalyClient::RemoteService)
         .not_to receive(:find_remote_root_ref)
 
-      expect(repository.find_remote_root_ref(nil)).to be_nil
+      expect(repository.find_remote_root_ref(nil, nil)).to be_nil
     end
 
     it 'returns nil when remote name is empty' do
       expect_any_instance_of(Gitlab::GitalyClient::RemoteService)
         .not_to receive(:find_remote_root_ref)
 
-      expect(repository.find_remote_root_ref('')).to be_nil
+      expect(repository.find_remote_root_ref('', '')).to be_nil
     end
 
     it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RemoteService, :find_remote_root_ref do
-      subject { repository.find_remote_root_ref('origin') }
+      subject { repository.find_remote_root_ref('origin', SeedHelper::GITLAB_GIT_TEST_REPO_URL) }
     end
   end
 

spec/lib/gitlab/gitaly_client/remote_service_spec.rb

@@ -35,22 +35,50 @@ RSpec.describe Gitlab::GitalyClient::RemoteService do
   end
 
   describe '#find_remote_root_ref' do
-    it 'sends an find_remote_root_ref message and returns the root ref' do
-      expect_any_instance_of(Gitaly::RemoteService::Stub)
-        .to receive(:find_remote_root_ref)
-        .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
-        .and_return(double(ref: 'master'))
+    let(:remote) { 'origin' }
+    let(:url) { 'http://git.example.com/my-repo.git' }
+    let(:auth) { 'Basic secret' }
+
+    shared_examples 'a find_remote_root_ref call' do
+      it 'sends an find_remote_root_ref message and returns the root ref' do
+        expect_any_instance_of(Gitaly::RemoteService::Stub)
+          .to receive(:find_remote_root_ref)
+          .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+          .with(gitaly_request_with_params(expected_params), kind_of(Hash))
+          .and_return(double(ref: 'master'))
+
+        expect(client.find_remote_root_ref(remote, url, auth)).to eq 'master'
+      end
+
+      it 'ensure ref is a valid UTF-8 string' do
+        expect_any_instance_of(Gitaly::RemoteService::Stub)
+          .to receive(:find_remote_root_ref)
+          .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+          .with(gitaly_request_with_params(expected_params), kind_of(Hash))
+          .and_return(double(ref: "an_invalid_ref_\xE5"))
 
-      expect(client.find_remote_root_ref('origin')).to eq 'master'
+        expect(client.find_remote_root_ref(remote, url, auth)).to eq "an_invalid_ref_å"
+      end
     end
 
-    it 'ensure ref is a valid UTF-8 string' do
-      expect_any_instance_of(Gitaly::RemoteService::Stub)
-        .to receive(:find_remote_root_ref)
-        .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
-        .and_return(double(ref: "an_invalid_ref_\xE5"))
+    context 'with inmemory feature enabled' do
+      before do
+        stub_feature_flags(find_remote_root_refs_inmemory: true)
+      end
+
+      it_behaves_like 'a find_remote_root_ref call' do
+        let(:expected_params) { { remote_url: url, http_authorization_header: auth } }
+      end
+    end
 
-      expect(client.find_remote_root_ref('origin')).to eq "an_invalid_ref_å"
+    context 'with inmemory feature disabled' do
+      before do
+        stub_feature_flags(find_remote_root_refs_inmemory: false)
+      end
+
+      it_behaves_like 'a find_remote_root_ref call' do
+        let(:expected_params) { { remote: remote } }
+      end
     end
   end