From 6e46fa00a5c42cb21f240275106c80362fd2aefd Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 29 May 2019 12:59:17 +0800 Subject: [PATCH 1/5] Check push size based on quarantine size Fallback to using the old approach when env var is not present. --- .../11126-fix-repository-size-check.yml | 5 + ee/lib/ee/gitlab/git_access.rb | 32 ++++- ee/spec/lib/gitlab/git_access_spec.rb | 109 +++++++++++++----- 3 files changed, 112 insertions(+), 34 deletions(-) create mode 100644 ee/changelogs/unreleased/11126-fix-repository-size-check.yml 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 00000000000000..c8b596c35af451 --- /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 cc1dfae98ff9dc..2dd0d72fef4ca6 100644 --- a/ee/lib/ee/gitlab/git_access.rb +++ b/ee/lib/ee/gitlab/git_access.rb @@ -84,20 +84,40 @@ def check_if_license_blocks_changes! def check_push_size! return unless check_size_limit? + git_env = ::Gitlab::Git::HookEnv.all(repository.gl_repository) + + # Use #quarantine_size to get correct push size whenever a lof 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). + push_size = git_env['GIT_OBJECT_DIRECTORY_RELATIVE'].present? ? quarantine_size : changes_size + + if project.changes_will_exceed_size_limit?(push_size) + raise ::Gitlab::GitAccess::UnauthorizedError, ::Gitlab::RepositorySizeError.new(project).new_changes_error + end + end + + def quarantine_size + project.repository.object_directory_size + end + + def changes_size # If there are worktrees with a HEAD pointing to a non-existent object, # calls to `git rev-list --all` will fail in git 2.15+. This should also # clear stale lock files. project.repository.clean_stale_repository_files - push_size_in_bytes = 0 + size_in_bytes = 0 changes_list.each do |change| - push_size_in_bytes += 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 - end + size_in_bytes += repository.new_blobs(change[:newrev]).sum(&:size) # rubocop: disable CodeReuse/ActiveRecord end + + size_in_bytes end def check_size_limit? diff --git a/ee/spec/lib/gitlab/git_access_spec.rb b/ee/spec/lib/gitlab/git_access_spec.rb index 5d6e05fc7f5285..16f312baf9a8bd 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,24 +218,88 @@ 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 + context 'when GIT_OBJECT_DIRECTORY_RELATIVE env var is set' do + let(:object_directory_size) { 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/) + before do + allow(Gitlab::Git::HookEnv) + .to receive(:all) + .with(repository.gl_repository) + .and_return({ 'GIT_OBJECT_DIRECTORY_RELATIVE' => 'objects' }) + + # 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 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 not set' 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_smallest_changes} refs/heads/my_branch_3") - end.not_to raise_error + 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 -- GitLab From 894157ad4b251e145d9ebd248a073dd8cb90e44f Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 29 May 2019 13:32:42 +0800 Subject: [PATCH 2/5] Add feature flag enabled by default --- ee/lib/ee/gitlab/git_access.rb | 12 +++- ee/spec/lib/gitlab/git_access_spec.rb | 100 +++++++++++++++----------- 2 files changed, 67 insertions(+), 45 deletions(-) diff --git a/ee/lib/ee/gitlab/git_access.rb b/ee/lib/ee/gitlab/git_access.rb index 2dd0d72fef4ca6..cc952762d2aa2a 100644 --- a/ee/lib/ee/gitlab/git_access.rb +++ b/ee/lib/ee/gitlab/git_access.rb @@ -84,8 +84,6 @@ def check_if_license_blocks_changes! def check_push_size! return unless check_size_limit? - git_env = ::Gitlab::Git::HookEnv.all(repository.gl_repository) - # Use #quarantine_size to get correct push size whenever a lof 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 @@ -94,13 +92,21 @@ def check_push_size! # 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). - push_size = git_env['GIT_OBJECT_DIRECTORY_RELATIVE'].present? ? quarantine_size : changes_size + push_size = check_quarantine_size? ? quarantine_size : changes_size if project.changes_will_exceed_size_limit?(push_size) raise ::Gitlab::GitAccess::UnauthorizedError, ::Gitlab::RepositorySizeError.new(project).new_changes_error end end + def check_quarantine_size? + strong_memoize(:check_quarantine_size) do + 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 + end + def quarantine_size project.repository.object_directory_size end diff --git a/ee/spec/lib/gitlab/git_access_spec.rb b/ee/spec/lib/gitlab/git_access_spec.rb index 16f312baf9a8bd..0f3d6c47c6d637 100644 --- a/ee/spec/lib/gitlab/git_access_spec.rb +++ b/ee/spec/lib/gitlab/git_access_spec.rb @@ -220,21 +220,7 @@ end end - context 'when GIT_OBJECT_DIRECTORY_RELATIVE env var is set' do - let(:object_directory_size) { 1.megabyte } - - before do - allow(Gitlab::Git::HookEnv) - .to receive(:all) - .with(repository.gl_repository) - .and_return({ 'GIT_OBJECT_DIRECTORY_RELATIVE' => 'objects' }) - - # Stub the object directory size to "simulate" quarantine size - allow(repository) - .to receive(:object_directory_size) - .and_return(object_directory_size) - 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 } @@ -248,18 +234,20 @@ 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 } - + 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 object directory (quarantine) size does not exceed 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 + expect do push_changes("#{Gitlab::Git::BLANK_SHA} #{sha_with_smallest_changes} refs/heads/my_branch_3") end.not_to raise_error @@ -268,40 +256,68 @@ end end - context 'when GIT_OBJECT_DIRECTORY_RELATIVE env var is not set' 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' + 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 repository size is below the limit' do - let(:repository_size) { 1.megabyte } - let(:repository_size_limit) { 2.megabytes } + context 'when quarantine_push_size_check feature is enabled (default)' do + let(:object_directory_size) { 1.megabyte } - it_behaves_like 'a push to repository below the limit' + 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 new change exceeds the limit' do - it 'rejects the push' do - expect(repository.new_blobs(sha_with_2_mb_file)).to be_present + 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/) - end + it_behaves_like 'a push to repository over the limit' 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 + context 'when repository size is below the limit' do + let(:repository_size) { 1.megabyte } + let(:repository_size_limit) { 2.megabytes } - expect do - push_changes("#{Gitlab::Git::BLANK_SHA} #{sha_with_smallest_changes} refs/heads/my_branch_3") - end.not_to raise_error + 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 -- GitLab From 69df9d7bc14a5eb25124aa454edea7f3fe129d1d Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 5 Jun 2019 13:11:08 +0800 Subject: [PATCH 3/5] Abort immediately when limit gets reached --- ee/lib/ee/gitlab/git_access.rb | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/ee/lib/ee/gitlab/git_access.rb b/ee/lib/ee/gitlab/git_access.rb index cc952762d2aa2a..51133b0e622255 100644 --- a/ee/lib/ee/gitlab/git_access.rb +++ b/ee/lib/ee/gitlab/git_access.rb @@ -84,7 +84,7 @@ def check_if_license_blocks_changes! def check_push_size! return unless check_size_limit? - # Use #quarantine_size to get correct push size whenever a lof of changes + # Use #check_quarantine_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). @@ -92,10 +92,10 @@ def check_push_size! # 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). - push_size = check_quarantine_size? ? quarantine_size : changes_size - - if project.changes_will_exceed_size_limit?(push_size) - raise ::Gitlab::GitAccess::UnauthorizedError, ::Gitlab::RepositorySizeError.new(project).new_changes_error + if check_quarantine_size? + check_quarantine_size + else + check_changes_size end end @@ -107,23 +107,29 @@ def check_quarantine_size? end end - def quarantine_size - project.repository.object_directory_size + def check_quarantine_size + check_size_against_limit(project.repository.object_directory_size) end - def changes_size + def check_changes_size # If there are worktrees with a HEAD pointing to a non-existent object, # calls to `git rev-list --all` will fail in git 2.15+. This should also # clear stale lock files. project.repository.clean_stale_repository_files - size_in_bytes = 0 + changes_size = 0 changes_list.each do |change| - 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 - size_in_bytes + 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? -- GitLab From 6fcf1e9d522802360243f05466eada2623ae1112 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 5 Jun 2019 23:15:30 +0800 Subject: [PATCH 4/5] Rename method to check_repository_disk_size Also remove the memoization of `check_quarantine_size?` as it's no longer needed to be memoized. --- ee/lib/ee/gitlab/git_access.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/ee/lib/ee/gitlab/git_access.rb b/ee/lib/ee/gitlab/git_access.rb index 51133b0e622255..7f621f79cba6c0 100644 --- a/ee/lib/ee/gitlab/git_access.rb +++ b/ee/lib/ee/gitlab/git_access.rb @@ -84,7 +84,7 @@ def check_if_license_blocks_changes! def check_push_size! return unless check_size_limit? - # Use #check_quarantine_size to get correct push size whenever a lot of changes + # 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). @@ -93,21 +93,19 @@ def check_push_size! # determine the push size if env var isn't set (e.g. changes are made # via UI and API). if check_quarantine_size? - check_quarantine_size + check_repository_disk_size else check_changes_size end end def check_quarantine_size? - strong_memoize(:check_quarantine_size) do - git_env = ::Gitlab::Git::HookEnv.all(repository.gl_repository) + 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 + git_env['GIT_OBJECT_DIRECTORY_RELATIVE'].present? && ::Feature.enabled?(:quarantine_push_size_check, default_enabled: true) end - def check_quarantine_size + def check_repository_disk_size check_size_against_limit(project.repository.object_directory_size) end -- GitLab From 8010d6bb44be48c2cda0cd62f1ba777abb1d6988 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Thu, 6 Jun 2019 00:20:43 +0800 Subject: [PATCH 5/5] Clean stale repository files every size check --- ee/lib/ee/gitlab/git_access.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ee/lib/ee/gitlab/git_access.rb b/ee/lib/ee/gitlab/git_access.rb index 7f621f79cba6c0..6e4d6391dd80fd 100644 --- a/ee/lib/ee/gitlab/git_access.rb +++ b/ee/lib/ee/gitlab/git_access.rb @@ -84,6 +84,11 @@ def check_if_license_blocks_changes! def check_push_size! return unless check_size_limit? + # If there are worktrees with a HEAD pointing to a non-existent object, + # calls to `git rev-list --all` will fail in git 2.15+. This should also + # clear stale lock files. + project.repository.clean_stale_repository_files + # 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 @@ -110,11 +115,6 @@ def check_repository_disk_size end def check_changes_size - # If there are worktrees with a HEAD pointing to a non-existent object, - # calls to `git rev-list --all` will fail in git 2.15+. This should also - # clear stale lock files. - project.repository.clean_stale_repository_files - changes_size = 0 changes_list.each do |change| -- GitLab