Merge branch '11126-fix-repository-size-check' into 'master'

Use quarantine size to check push size against repository size limit

Closes #11126

See merge request gitlab-org/gitlab-ee!13460

ee/changelogs/unreleased/11126-fix-repository-size-check.yml

+---
+title: Use quarantine size to check push size against repository size limit
+merge_request: 13460
+author:
+type: fixed

ee/lib/ee/gitlab/git_access.rb

@@ -89,15 +89,45 @@ module EE
         # clear stale lock files.
         project.repository.clean_stale_repository_files
 
-        push_size_in_bytes = 0
+        # Use #check_repository_disk_size to get correct push size whenever a lot of changes
+        # gets pushed at the same time containing the same blobs. This is only
+        # doable if GIT_OBJECT_DIRECTORY_RELATIVE env var is set and happens
+        # when git push comes from CLI (not via UI and API).
+        #
+        # Fallback to determining push size using the changes_list so we can still
+        # determine the push size if env var isn't set (e.g. changes are made
+        # via UI and API).
+        if check_quarantine_size?
+          check_repository_disk_size
+        else
+          check_changes_size
+        end
+      end
+
+      def check_quarantine_size?
+        git_env = ::Gitlab::Git::HookEnv.all(repository.gl_repository)
+
+        git_env['GIT_OBJECT_DIRECTORY_RELATIVE'].present? && ::Feature.enabled?(:quarantine_push_size_check, default_enabled: true)
+      end
+
+      def check_repository_disk_size
+        check_size_against_limit(project.repository.object_directory_size)
+      end
+
+      def check_changes_size
+        changes_size = 0
 
         changes_list.each do |change|
-          push_size_in_bytes += repository.new_blobs(change[:newrev]).sum(&:size) # rubocop: disable CodeReuse/ActiveRecord
+          changes_size += repository.new_blobs(change[:newrev]).sum(&:size) # rubocop: disable CodeReuse/ActiveRecord
 
-          if project.changes_will_exceed_size_limit?(push_size_in_bytes)
-            raise ::Gitlab::GitAccess::UnauthorizedError, ::Gitlab::RepositorySizeError.new(project).new_changes_error
+          check_size_against_limit(changes_size)
         end
       end
+
+      def check_size_against_limit(size)
+        if project.changes_will_exceed_size_limit?(size)
+          raise ::Gitlab::GitAccess::UnauthorizedError, ::Gitlab::RepositorySizeError.new(project).new_changes_error
+        end
       end
 
       def check_size_limit?

ee/spec/lib/gitlab/git_access_spec.rb

@@ -181,18 +181,13 @@ describe Gitlab::GitAccess do
       # Delete branch so Repository#new_blobs can return results
       repository.delete_branch('2-mb-file')
       repository.delete_branch('wip')
-    end
-
-    context 'when repository size is over limit' do
-      before do
-        allow(project).to receive(:repository_and_lfs_size).and_return(2.megabytes)
 
-        project.update_attribute(:repository_size_limit, 1.megabytes)
+      allow(project).to receive(:repository_and_lfs_size).and_return(repository_size)
+      project.update_attribute(:repository_size_limit, repository_size_limit)
     end
 
+    shared_examples_for 'a push to repository over the limit' do
       it 'rejects the push' do
-        expect(repository.new_blobs(sha_with_smallest_changes)).to be_present
-
         expect do
           push_changes("#{Gitlab::Git::BLANK_SHA} #{sha_with_smallest_changes} refs/heads/master")
         end.to raise_error(described_class::UnauthorizedError, /Your push has been rejected/)
@@ -207,13 +202,7 @@ describe Gitlab::GitAccess do
       end
     end
 
-    context 'when repository size is below the limit' do
-      before do
-        allow(project).to receive(:repository_and_lfs_size).and_return(1.megabyte)
-
-        project.update_attribute(:repository_size_limit, 2.megabytes)
-      end
-
+    shared_examples_for 'a push to repository below the limit' do
       context 'when trying to authenticate the user' do
         it 'does not raise an error' do
           expect { push_changes }.not_to raise_error
@@ -229,6 +218,21 @@ describe Gitlab::GitAccess do
           end.not_to raise_error
         end
       end
+    end
+
+    shared_examples_for 'a push to repository using git-rev-list for checking against repository size limit' do
+      context 'when repository size is over limit' do
+        let(:repository_size) { 2.megabytes }
+        let(:repository_size_limit) { 1.megabyte }
+
+        it_behaves_like 'a push to repository over the limit'
+      end
+
+      context 'when repository size is below the limit' do
+        let(:repository_size) { 1.megabyte }
+        let(:repository_size_limit) { 2.megabytes }
+
+        it_behaves_like 'a push to repository below the limit'
 
         context 'when new change exceeds the limit' do
           it 'rejects the push' do
@@ -240,7 +244,7 @@ describe Gitlab::GitAccess do
           end
         end
 
-      context 'when new change does not exceeds the limit' do
+        context 'when new change does not exceed the limit' do
           it 'accepts the push' do
             expect(repository.new_blobs(sha_with_smallest_changes)).to be_present
 
@@ -252,6 +256,71 @@ describe Gitlab::GitAccess do
       end
     end
 
+    context 'when GIT_OBJECT_DIRECTORY_RELATIVE env var is set' do
+      before do
+        allow(Gitlab::Git::HookEnv)
+          .to receive(:all)
+          .with(repository.gl_repository)
+          .and_return({ 'GIT_OBJECT_DIRECTORY_RELATIVE' => 'objects' })
+      end
+
+      context 'when quarantine_push_size_check feature is enabled (default)' do
+        let(:object_directory_size) { 1.megabyte }
+
+        before do
+          # Stub the object directory size to "simulate" quarantine size
+          allow(repository)
+            .to receive(:object_directory_size)
+            .and_return(object_directory_size)
+        end
+
+        context 'when repository size is over limit' do
+          let(:repository_size) { 2.megabytes }
+          let(:repository_size_limit) { 1.megabyte }
+
+          it_behaves_like 'a push to repository over the limit'
+        end
+
+        context 'when repository size is below the limit' do
+          let(:repository_size) { 1.megabyte }
+          let(:repository_size_limit) { 2.megabytes }
+
+          it_behaves_like 'a push to repository below the limit'
+
+          context 'when object directory (quarantine) size exceeds the limit' do
+            let(:object_directory_size) { 2.megabytes }
+
+            it 'rejects the push' do
+              expect do
+                push_changes("#{Gitlab::Git::BLANK_SHA} #{sha_with_2_mb_file} refs/heads/my_branch_2")
+              end.to raise_error(described_class::UnauthorizedError, /Your push to this repository would cause it to exceed the size limit/)
+            end
+          end
+
+          context 'when object directory (quarantine) size does not exceed the limit' do
+            it 'accepts the push' do
+              expect do
+                push_changes("#{Gitlab::Git::BLANK_SHA} #{sha_with_smallest_changes} refs/heads/my_branch_3")
+              end.not_to raise_error
+            end
+          end
+        end
+      end
+
+      context 'when quarantine_push_size_check feature is disabled' do
+        before do
+          stub_feature_flags(quarantine_push_size_check: false)
+        end
+
+        it_behaves_like 'a push to repository using git-rev-list for checking against repository size limit'
+      end
+    end
+
+    context 'when GIT_OBJECT_DIRECTORY_RELATIVE env var is not set' do
+      it_behaves_like 'a push to repository using git-rev-list for checking against repository size limit'
+    end
+  end
+
   describe 'Geo' do
     let(:actor) { :geo }