Commit bad9f7d1 authored by Rubén Dávila's avatar Rubén Dávila

Implement usage of new Gitaly RPC to get new blobs from push

Instead of invoking git commands from GitLab we are now invoking the
`list_new_blobs` RPC from Gitaly that will return all the blobs found in
the new revision that's being used for the push operation.

With this change we're also fixing a bug by making push size checks only count the size of newly-pushed files
parent ad606ea6
......@@ -438,7 +438,7 @@ group :ed25519 do
end
# Gitaly GRPC client
gem 'gitaly-proto', '~> 0.109.0', require: 'gitaly'
gem 'gitaly-proto', '~> 0.112.0', require: 'gitaly'
gem 'grpc', '~> 1.11.0'
# Locked until https://github.com/google/protobuf/issues/4210 is closed
......
......@@ -308,7 +308,7 @@ GEM
gettext_i18n_rails (>= 0.7.1)
po_to_json (>= 1.0.0)
rails (>= 3.2.0)
gitaly-proto (0.109.0)
gitaly-proto (0.112.0)
google-protobuf (~> 3.1)
grpc (~> 1.10)
github-linguist (5.3.3)
......@@ -1082,7 +1082,7 @@ DEPENDENCIES
gettext (~> 3.2.2)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
gitaly-proto (~> 0.109.0)
gitaly-proto (~> 0.112.0)
github-linguist (~> 5.3.3)
gitlab-flowdock-git-hook (~> 1.0.1)
gitlab-gollum-lib (~> 4.2)
......
......@@ -311,7 +311,7 @@ GEM
gettext_i18n_rails (>= 0.7.1)
po_to_json (>= 1.0.0)
rails (>= 3.2.0)
gitaly-proto (0.109.0)
gitaly-proto (0.112.0)
google-protobuf (~> 3.1)
grpc (~> 1.10)
github-linguist (5.3.3)
......@@ -1092,7 +1092,7 @@ DEPENDENCIES
gettext (~> 3.2.2)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
gitaly-proto (~> 0.109.0)
gitaly-proto (~> 0.112.0)
github-linguist (~> 5.3.3)
gitlab-flowdock-git-hook (~> 1.0.1)
gitlab-gollum-lib (~> 4.2)
......
---
title: Ensure that push size checks only count the size of newly-pushed files
merge_request: 6767
author:
type: fixed
......@@ -34,16 +34,14 @@ module EE
return if push_rule.nil? || push_rule.max_file_size.zero?
max_file_size = push_rule.max_file_size
raw_changes = project.repository.raw_changes_between(oldrev, newrev)
blobs = project.repository.new_blobs(newrev)
large_file = raw_changes.find do |c|
size_in_mb = ::Gitlab::Utils.bytes_to_megabytes(c.blob_size)
c.operation != :deleted && size_in_mb > max_file_size
large_blob = blobs.find do |blob|
::Gitlab::Utils.bytes_to_megabytes(blob.size) > max_file_size
end
if large_file
raise ::Gitlab::GitAccess::UnauthorizedError, %Q{File "#{large_file.new_path}" is larger than the allowed size of #{max_file_size} MB}
if large_blob
raise ::Gitlab::GitAccess::UnauthorizedError, %Q{File "#{large_blob.path}" is larger than the allowed size of #{max_file_size} MB}
end
end
......
......@@ -262,15 +262,19 @@ describe Gitlab::Checks::ChangeAccess do
context 'max file size rules' do
let(:push_rule) { create(:push_rule, max_file_size: 1) }
# SHA of the 2-mb-file branch
let(:newrev) { 'bf12d2567099e26f59692896f73ac819bae45b00' }
let(:ref) { 'my-branch' }
before do
allow_any_instance_of(::Gitlab::Git::RawDiffChange).to receive(:blob_size).and_return(2.megabytes)
# Delete branch so Repository#new_blobs can return results
project.repository.delete_branch('2-mb-file')
end
it_behaves_like 'check ignored when push rule unlicensed'
it 'returns an error if file exceeds the maximum file size' do
expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "File \"README\" is larger than the allowed size of 1 MB")
expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "File \"file.bin\" is larger than the allowed size of 1 MB")
end
end
......
......@@ -5,6 +5,7 @@ describe Gitlab::GitAccess do
let(:actor) { user }
let(:project) { create(:project, :repository) }
let(:repository) { project.repository }
let(:protocol) { 'web' }
let(:authentication_abilities) { %i[read_project download_code push_code] }
let(:redirected_path) { nil }
......@@ -154,42 +155,44 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
end
describe "max file size check" do
let(:start_sha) { 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660' }
let(:end_sha) { 'c84ff944ff4529a70788a5e9003c2b7feae29047' }
let(:changes) { "#{start_sha} #{end_sha} refs/heads/master" }
let(:start_sha) { ::Gitlab::Git::BLANK_SHA }
# SHA of the 2-mb-file branch
let(:end_sha) { 'bf12d2567099e26f59692896f73ac819bae45b00' }
let(:changes) { "#{start_sha} #{end_sha} refs/heads/my-branch" }
before do
project.add_developer(user)
# Delete branch so Repository#new_blobs can return results
repository.delete_branch('2-mb-file')
end
it "returns false when size is too large" do
project.create_push_rule(max_file_size: 1)
expect(repository.new_blobs(end_sha)).to be_present
expect { push_changes(changes) }.to raise_error(described_class::UnauthorizedError)
end
it "returns true when size is allowed" do
project.create_push_rule(max_file_size: 2)
expect { push_changes(changes) }.not_to raise_error
end
it "returns true when size is nil" do
allow_any_instance_of(Gitlab::Git::Blob).to receive(:size).and_return(nil)
project.create_push_rule(max_file_size: 2)
project.create_push_rule(max_file_size: 3)
expect(repository.new_blobs(end_sha)).to be_present
expect { push_changes(changes) }.not_to raise_error
end
end
end
describe 'repository size restrictions' do
let(:start_sha) { 'cfe32cf61b73a0d5e9f13e774abde7ff789b1660' }
let(:sha_with_2_mb_file) { 'c84ff944ff4529a70788a5e9003c2b7feae29047' }
let(:sha_with_smallest_changes) { '33f3729a45c02fc67d00adb1b8bca394b0e761d9' }
# SHA for the 2-mb-file branch
let(:sha_with_2_mb_file) { 'bf12d2567099e26f59692896f73ac819bae45b00' }
# SHA for the wip branch
let(:sha_with_smallest_changes) { 'b9238ee5bf1d7359dd3b8c89fd76c1c7f8b75aba' }
before do
project.add_developer(user)
# 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
......@@ -200,17 +203,17 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
end
it 'rejects the push' do
expect(repository.new_blobs(sha_with_smallest_changes)).to be_present
expect do
push_changes("#{start_sha} #{sha_with_smallest_changes} refs/heads/master")
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/)
end
context 'when deleting a branch' do
it 'accepts the operation' do
feature_branch_sha = project.commit('feature').id
expect do
push_changes("#{feature_branch_sha} #{::Gitlab::Git::BLANK_SHA} refs/heads/feature")
push_changes("#{sha_with_smallest_changes} #{::Gitlab::Git::BLANK_SHA} refs/heads/feature")
end.not_to raise_error
end
end
......@@ -223,62 +226,33 @@ For more information: #{EE::Gitlab::GeoGitAccess::GEO_SERVER_DOCS_URL}"
project.update_attribute(:repository_size_limit, 2.megabytes)
end
context 'when new change exceeds the limit' do
it 'rejects the push' do
expect do
push_changes("#{start_sha} #{sha_with_2_mb_file} refs/heads/master")
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 exceeds the limit' do
context 'when pushing a new branch' do
it 'accepts the push' do
master_sha = project.commit('master').id
expect do
push_changes("#{start_sha} #{sha_with_smallest_changes} refs/heads/master")
push_changes("#{Gitlab::Git::BLANK_SHA} #{master_sha} refs/heads/my_branch")
end.not_to raise_error
end
end
context 'when a file is modified' do
let(:start_sha) { '281d3a76f31c812dbf48abce82ccf6860adedd81' } # file created
let(:end_sha) { 'c347ca2e140aa667b968e51ed0ffe055501fe4f4' } # file modified
let(:changes) { "#{start_sha} #{end_sha} refs/heads/master" }
before do
# Substract 10_000 bytes in order to demostrate that the 23 KB are not added to the total
allow(project).to receive(:repository_and_lfs_size).and_return(2.megabytes - 10000)
end
it 'just add the difference between the two versions to the total size' do
expect { push_changes(changes) }.not_to raise_error
end
end
context 'when a file is renamed' do
let(:start_sha) { '281d3a76f31c812dbf48abce82ccf6860adedd81' } # file deleted
let(:end_sha) { 'c347ca2e140aa667b968e51ed0ffe055501fe4f4' } # file added with different name
let(:changes) { "#{start_sha} #{end_sha} refs/heads/master" }
before do
allow(project).to receive(:repository_and_lfs_size).and_return(2.megabytes)
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
it 'does not modify the total size given the content is the same' do
expect { push_changes(changes) }.not_to raise_error
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 a file is deleted' do
let(:start_sha) { 'c1acaa58bbcbc3eafe538cb8274ba387047b69f8' } # file deleted
let(:end_sha) { '5937ac0a7beb003549fc5fd26fc247adbce4a52e' } # New changes introduced
let(:changes) { "#{start_sha} #{end_sha} refs/heads/master" }
before do
allow(project).to receive(:repository_and_lfs_size).and_return(2.megabytes)
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
it 'subtracts the size of the deleted file before calculate the new total' do
expect { push_changes(changes) }.not_to raise_error
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
......
module Gitlab
module Checks
class LfsIntegrity
REV_LIST_OBJECT_LIMIT = 2_000
def initialize(project, newrev)
@project = project
@newrev = newrev
......@@ -11,7 +9,8 @@ module Gitlab
def objects_missing?
return false unless @newrev && @project.lfs_enabled?
new_lfs_pointers = Gitlab::Git::LfsChanges.new(@project.repository, @newrev).new_pointers(object_limit: REV_LIST_OBJECT_LIMIT)
new_lfs_pointers = Gitlab::Git::LfsChanges.new(@project.repository, @newrev)
.new_pointers(object_limit: ::Gitlab::Git::Repository::REV_LIST_COMMIT_LIMIT)
return false unless new_lfs_pointers.present?
......
......@@ -19,6 +19,7 @@ module Gitlab
GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE
].freeze
SEARCH_CONTEXT_LINES = 3
REV_LIST_COMMIT_LIMIT = 2_000
# In https://gitlab.com/gitlab-org/gitaly/merge_requests/698
# We copied these two prefixes into gitaly-go, so don't change these
# or things will break! (REBASE_WORKTREE_PREFIX and SQUASH_WORKTREE_PREFIX)
......@@ -380,6 +381,16 @@ module Gitlab
end
end
def new_blobs(newrev)
return [] if newrev == ::Gitlab::Git::BLANK_SHA
strong_memoize("new_blobs_#{newrev}") do
wrapped_gitaly_errors do
gitaly_ref_client.list_new_blobs(newrev, REV_LIST_COMMIT_LIMIT)
end
end
end
def count_commits(options)
options = process_count_commits_options(options.dup)
......
......@@ -272,7 +272,7 @@ module Gitlab
check_single_change_access(change, skip_lfs_integrity_check: !first_change)
if project.size_limit_enabled?
push_size_in_bytes += push_size(change)
push_size_in_bytes += repository.new_blobs(change[:newrev]).sum(&:size)
end
end
......@@ -281,29 +281,6 @@ module Gitlab
end
end
def push_size(change)
old_rev, new_rev = change.values_at(:oldrev, :newrev)
current_changes_size = 0
old_paths = []
repository.raw_changes_between(old_rev, new_rev).each do |c|
case c.operation
when :deleted
current_changes_size -= c.blob_size
when :added
current_changes_size += c.blob_size
when :copied, :modified, :renamed, :type_changed
current_changes_size += c.blob_size
old_paths << [old_rev, c.old_path]
end
end
old_changes_size = Gitlab::Git::Blob.batch_metadata(repository, old_paths).sum(&:size)
current_changes_size - old_changes_size
end
def check_single_change_access(change, skip_lfs_integrity_check: false)
Checks::ChangeAccess.new(
change,
......
......@@ -82,6 +82,23 @@ module Gitlab
commits
end
def list_new_blobs(newrev, limit = 0)
request = Gitaly::ListNewBlobsRequest.new(
repository: @gitaly_repo,
commit_id: newrev,
limit: limit
)
response = GitalyClient
.call(@storage, :ref_service, :list_new_blobs, request, timeout: GitalyClient.medium_timeout)
response.flat_map do |msg|
# Returns an Array of Gitaly::NewBlobObject objects
# Available methods are: #size, #oid and #path
msg.new_blob_objects
end
end
def count_tag_names
tag_names.count
end
......
......@@ -51,7 +51,8 @@ module TestEnv
'add-pdf-text-binary' => '79faa7b',
'add_images_and_changes' => '010d106',
'update-gitlab-shell-v-6-0-1' => '2f61d70',
'update-gitlab-shell-v-6-0-3' => 'de78448'
'update-gitlab-shell-v-6-0-3' => 'de78448',
'2-mb-file' => 'bf12d25'
}.freeze
# gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment