Merge branch 'update_repo_storage_checksum' into 'master'

Leverage repository checksums when moving a repository

See merge request gitlab-org/gitlab!26334

app/services/projects/update_repository_storage_service.rb

@@ -4,6 +4,7 @@ module Projects
   class UpdateRepositoryStorageService < BaseService
     include Gitlab::ShellAdapter
 
+    Error = Class.new(StandardError)
     RepositoryAlreadyMoved = Class.new(StandardError)
 
     def initialize(project)
@@ -17,37 +18,45 @@ module Projects
       # exception.
       raise RepositoryAlreadyMoved if project.repository_storage == new_repository_storage_key
 
-      if mirror_repositories(new_repository_storage_key)
-        mark_old_paths_for_archive
+      mirror_repositories(new_repository_storage_key)
 
-        project.update(repository_storage: new_repository_storage_key, repository_read_only: false)
-        project.leave_pool_repository
-        project.track_project_repository
+      mark_old_paths_for_archive
 
-        enqueue_housekeeping
-      else
-        project.update(repository_read_only: false)
-      end
+      project.update(repository_storage: new_repository_storage_key, repository_read_only: false)
+      project.leave_pool_repository
+      project.track_project_repository
+
+      enqueue_housekeeping
+
+      success
+
+    rescue Error => e
+      project.update(repository_read_only: false)
+
+      Gitlab::ErrorTracking.track_exception(e, project_path: project.full_path)
+
+      error(s_("UpdateRepositoryStorage|Error moving repository storage for %{project_full_path} - %{message}") % { project_full_path: project.full_path, message: e.message })
     end
 
     private
 
     def mirror_repositories(new_repository_storage_key)
-      result = mirror_repository(new_repository_storage_key)
+      mirror_repository(new_repository_storage_key)
 
       if project.wiki.repository_exists?
-        result &&= mirror_repository(new_repository_storage_key, type: Gitlab::GlRepository::WIKI)
+        mirror_repository(new_repository_storage_key, type: Gitlab::GlRepository::WIKI)
       end
-
-      result
     end
 
     def mirror_repository(new_storage_key, type: Gitlab::GlRepository::PROJECT)
-      return false unless wait_for_pushes(type)
+      unless wait_for_pushes(type)
+        raise Error, s_('UpdateRepositoryStorage|Timeout waiting for %{type} repository pushes') % { type: type.name }
+      end
 
       repository = type.repository_for(project)
       full_path = repository.full_path
       raw_repository = repository.raw
+      checksum = repository.checksum
 
       # Initialize a git repository on the target path
       gitlab_shell.create_repository(new_storage_key, raw_repository.relative_path, full_path)
@@ -56,7 +65,15 @@ module Projects
                                                    raw_repository.gl_repository,
                                                    full_path)
 
-      new_repository.fetch_repository_as_mirror(raw_repository)
+      unless new_repository.fetch_repository_as_mirror(raw_repository)
+        raise Error, s_('UpdateRepositoryStorage|Failed to fetch %{type} repository as mirror') % { type: type.name }
+      end
+
+      new_checksum = new_repository.checksum
+
+      if checksum != new_checksum
+        raise Error, s_('UpdateRepositoryStorage|Failed to verify %{type} repository checksum from %{old} to %{new}') % { type: type.name, old: checksum, new: new_checksum }
+      end
     end
 
     def mark_old_paths_for_archive

changelogs/unreleased/update_repo_storage_checksum.yml

+---
+title: Ensure checksums match when updating repository storage
+merge_request: 26334
+author:
+type: changed

ee/app/services/ee/projects/update_repository_storage_service.rb

@@ -7,13 +7,11 @@ module EE
 
       override :mirror_repositories
       def mirror_repositories(new_repository_storage_key)
-        result = super
+        super
 
         if project.design_repository.exists?
-          result &&= mirror_repository(new_repository_storage_key, type: Gitlab::GlRepository::DESIGN)
+          mirror_repository(new_repository_storage_key, type: Gitlab::GlRepository::DESIGN)
         end
-
-        result
       end
 
       override :mark_old_paths_for_archive

locale/gitlab.pot

@@ -21221,6 +21221,18 @@ msgstr ""
 msgid "UpdateProject|Project could not be updated!"
 msgstr ""
 
+msgid "UpdateRepositoryStorage|Error moving repository storage for %{project_full_path} - %{message}"
+msgstr ""
+
+msgid "UpdateRepositoryStorage|Failed to fetch %{type} repository as mirror"
+msgstr ""
+
+msgid "UpdateRepositoryStorage|Failed to verify %{type} repository checksum from %{old} to %{new}"
+msgstr ""
+
+msgid "UpdateRepositoryStorage|Timeout waiting for %{type} repository pushes"
+msgstr ""
+
 msgid "Updated"
 msgstr ""
 

spec/services/projects/fork_service_spec.rb

@@ -315,6 +315,7 @@ describe Projects::ForkService do
       # Stub everything required to move a project to a Gitaly shard that does not exist
       stub_storage_settings('test_second_storage' => { 'path' => 'tmp/tests/second_storage' })
       allow_any_instance_of(Gitlab::Git::Repository).to receive(:fetch_repository_as_mirror).and_return(true)
+      allow_any_instance_of(Gitlab::Git::Repository).to receive(:checksum).and_return(::Gitlab::Git::BLANK_SHA)
 
       Projects::UpdateRepositoryStorageService.new(project).execute('test_second_storage')
       fork_after_move = fork_project(project)

spec/services/projects/update_repository_storage_service_spec.rb

@@ -16,6 +16,15 @@ describe Projects::UpdateRepositoryStorageService do
 
     context 'without wiki and design repository' do
       let(:project) { create(:project, :repository, repository_read_only: true, wiki_enabled: false) }
+      let!(:checksum) { project.repository.checksum }
+      let(:project_repository_double) { double(:repository) }
+
+      before do
+        allow(Gitlab::Git::Repository).to receive(:new).and_call_original
+        allow(Gitlab::Git::Repository).to receive(:new)
+          .with('test_second_storage', project.repository.raw.relative_path, project.repository.gl_repository, project.repository.full_path)
+          .and_return(project_repository_double)
+      end
 
       context 'when the move succeeds' do
         it 'moves the repository to the new storage and unmarks the repository as read only' do
@@ -23,10 +32,14 @@ describe Projects::UpdateRepositoryStorageService do
             project.repository.path_to_repo
           end
 
-          expect_any_instance_of(Gitlab::Git::Repository).to receive(:fetch_repository_as_mirror)
+          expect(project_repository_double).to receive(:fetch_repository_as_mirror)
             .with(project.repository.raw).and_return(true)
+          expect(project_repository_double).to receive(:checksum)
+            .and_return(checksum)
+
+          result = subject.execute('test_second_storage')
 
-          subject.execute('test_second_storage')
+          expect(result[:status]).to eq(:success)
           expect(project).not_to be_repository_read_only
           expect(project.repository_storage).to eq('test_second_storage')
           expect(gitlab_shell.repository_exists?('default', old_path)).to be(false)
@@ -44,16 +57,50 @@ describe Projects::UpdateRepositoryStorageService do
 
       context 'when the move fails' do
         it 'unmarks the repository as read-only without updating the repository storage' do
-          expect_any_instance_of(Gitlab::Git::Repository).to receive(:fetch_repository_as_mirror)
+          expect(project_repository_double).to receive(:fetch_repository_as_mirror)
             .with(project.repository.raw).and_return(false)
           expect(GitlabShellWorker).not_to receive(:perform_async)
 
-          subject.execute('test_second_storage')
+          result = subject.execute('test_second_storage')
 
+          expect(result[:status]).to eq(:error)
           expect(project).not_to be_repository_read_only
           expect(project.repository_storage).to eq('default')
         end
       end
+
+      context 'when the checksum does not match' do
+        it 'unmarks the repository as read-only without updating the repository storage' do
+          expect(project_repository_double).to receive(:fetch_repository_as_mirror)
+            .with(project.repository.raw).and_return(true)
+          expect(project_repository_double).to receive(:checksum)
+            .and_return('not matching checksum')
+          expect(GitlabShellWorker).not_to receive(:perform_async)
+
+          result = subject.execute('test_second_storage')
+
+          expect(result[:status]).to eq(:error)
+          expect(project).not_to be_repository_read_only
+          expect(project.repository_storage).to eq('default')
+        end
+      end
+
+      context 'when a object pool was joined' do
+        let!(:pool) { create(:pool_repository, :ready, source_project: project) }
+
+        it 'leaves the pool' do
+          expect(project_repository_double).to receive(:fetch_repository_as_mirror)
+            .with(project.repository.raw).and_return(true)
+          expect(project_repository_double).to receive(:checksum)
+            .and_return(checksum)
+
+          result = subject.execute('test_second_storage')
+
+          expect(result[:status]).to eq(:success)
+          expect(project.repository_storage).to eq('test_second_storage')
+          expect(project.reload_pool_repository).to be_nil
+        end
+      end
     end
 
     context 'with wiki repository' do
@@ -66,18 +113,5 @@ describe Projects::UpdateRepositoryStorageService do
         end
       end
     end
-
-    context 'when a object pool was joined' do
-      let(:project) { create(:project, :repository, wiki_enabled: false, repository_read_only: true) }
-      let(:pool) { create(:pool_repository, :ready, source_project: project) }
-
-      it 'leaves the pool' do
-        allow_any_instance_of(Gitlab::Git::Repository).to receive(:fetch_repository_as_mirror).and_return(true)
-
-        subject.execute('test_second_storage')
-
-        expect(project.reload_pool_repository).to be_nil
-      end
-    end
   end
 end

spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb

@@ -2,7 +2,10 @@
 
 RSpec.shared_examples 'moves repository to another storage' do |repository_type|
   let(:project_repository_double) { double(:repository) }
+  let!(:project_repository_checksum) { project.repository.checksum }
+
   let(:repository_double) { double(:repository) }
+  let(:repository_checksum) { repository.checksum }
 
   before do
     # Default stub for non-specified params
@@ -19,15 +22,16 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
   context 'when the move succeeds', :clean_gitlab_redis_shared_state do
     before do
-      allow(project_repository_double)
-        .to receive(:fetch_repository_as_mirror)
+      allow(project_repository_double).to receive(:fetch_repository_as_mirror)
         .with(project.repository.raw)
         .and_return(true)
+      allow(project_repository_double).to receive(:checksum)
+        .and_return(project_repository_checksum)
 
-      allow(repository_double)
-        .to receive(:fetch_repository_as_mirror)
-        .with(repository.raw)
-        .and_return(true)
+      allow(repository_double).to receive(:fetch_repository_as_mirror)
+        .with(repository.raw).and_return(true)
+      allow(repository_double).to receive(:checksum)
+        .and_return(repository_checksum)
     end
 
     it "moves the project and its #{repository_type} repository to the new storage and unmarks the repository as read only" do
@@ -37,8 +41,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
       old_repository_path = repository.full_path
 
-      subject.execute('test_second_storage')
+      result = subject.execute('test_second_storage')
 
+      expect(result[:status]).to eq(:success)
       expect(project).not_to be_repository_read_only
       expect(project.repository_storage).to eq('test_second_storage')
       expect(gitlab_shell.repository_exists?('default', old_project_repository_path)).to be(false)
@@ -87,13 +92,38 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
     it 'unmarks the repository as read-only without updating the repository storage' do
       allow(project_repository_double).to receive(:fetch_repository_as_mirror)
         .with(project.repository.raw).and_return(true)
+      allow(project_repository_double).to receive(:checksum)
+        .and_return(project_repository_checksum)
       allow(repository_double).to receive(:fetch_repository_as_mirror)
         .with(repository.raw).and_return(false)
 
       expect(GitlabShellWorker).not_to receive(:perform_async)
 
-      subject.execute('test_second_storage')
+      result = subject.execute('test_second_storage')
+
+      expect(result[:status]).to eq(:error)
+      expect(project).not_to be_repository_read_only
+      expect(project.repository_storage).to eq('default')
+    end
+  end
+
+  context "when the checksum of the #{repository_type} repository does not match" do
+    it 'unmarks the repository as read-only without updating the repository storage' do
+      allow(project_repository_double).to receive(:fetch_repository_as_mirror)
+        .with(project.repository.raw).and_return(true)
+      allow(project_repository_double).to receive(:checksum)
+        .and_return(project_repository_checksum)
+
+      allow(repository_double).to receive(:fetch_repository_as_mirror)
+        .with(repository.raw).and_return(true)
+      allow(repository_double).to receive(:checksum)
+        .and_return('not matching checksum')
+
+      expect(GitlabShellWorker).not_to receive(:perform_async)
+
+      result = subject.execute('test_second_storage')
 
+      expect(result[:status]).to eq(:error)
       expect(project).not_to be_repository_read_only
       expect(project.repository_storage).to eq('default')
     end