diff --git a/ee/changelogs/unreleased/11126-fix-repository-size-check.yml b/ee/changelogs/unreleased/11126-fix-repository-size-check.yml new file mode 100644 index 0000000000000000000000000000000000000000..c8b596c35af45161d9a758b7f85c4e2eaf00fb5c --- /dev/null +++ b/ee/changelogs/unreleased/11126-fix-repository-size-check.yml @@ -0,0 +1,5 @@ +--- +title: Use quarantine size to check push size against repository size limit +merge_request: 13460 +author: +type: fixed diff --git a/ee/lib/ee/gitlab/git_access.rb b/ee/lib/ee/gitlab/git_access.rb index cc1dfae98ff9dc1439e04859bd3f340b17ec52b4..6e4d6391dd80fdd286d69b2f10953e4b8d0c263d 100644 --- a/ee/lib/ee/gitlab/git_access.rb +++ b/ee/lib/ee/gitlab/git_access.rb @@ -89,14 +89,44 @@ def check_push_size! # 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 + + check_size_against_limit(changes_size) + end + end - if project.changes_will_exceed_size_limit?(push_size_in_bytes) - raise ::Gitlab::GitAccess::UnauthorizedError, ::Gitlab::RepositorySizeError.new(project).new_changes_error - 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 diff --git a/ee/spec/lib/gitlab/git_access_spec.rb b/ee/spec/lib/gitlab/git_access_spec.rb index 5d6e05fc7f528557fc80566ae85e4b81a2cd729b..0f3d6c47c6d637e5d61e564b120a6938d44e0719 100644 --- a/ee/spec/lib/gitlab/git_access_spec.rb +++ b/ee/spec/lib/gitlab/git_access_spec.rb @@ -181,18 +181,13 @@ # 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) - end + 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 @@ 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,27 +218,107 @@ end.not_to raise_error end end + end - context 'when new change exceeds the limit' do - it 'rejects the push' do - expect(repository.new_blobs(sha_with_2_mb_file)).to be_present + 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 } - 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/) + 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 + expect(repository.new_blobs(sha_with_2_mb_file)).to be_present + + 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 new change does not exceed the limit' do + it 'accepts 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/my_branch_3") + end.not_to raise_error + end end end + end - context 'when new change does not exceeds the limit' do - it 'accepts the push' do - expect(repository.new_blobs(sha_with_smallest_changes)).to be_present + 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 - expect do - push_changes("#{Gitlab::Git::BLANK_SHA} #{sha_with_smallest_changes} refs/heads/my_branch_3") - end.not_to raise_error + 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