diff --git a/changelogs/unreleased/jej-fix-slow-lfs-object-check.yml b/changelogs/unreleased/jej-fix-slow-lfs-object-check.yml new file mode 100644 index 00000000000..09112fba85e --- /dev/null +++ b/changelogs/unreleased/jej-fix-slow-lfs-object-check.yml @@ -0,0 +1,5 @@ +--- +title: Only check LFS integrity for first ref in a push to avoid timeout +merge_request: 17098 +author: +type: performance diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index d75e73dac10..521680b8708 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -16,11 +16,11 @@ module Gitlab lfs_objects_missing: 'LFS objects are missing. Ensure LFS is properly set up or try a manual "git lfs push --all".' }.freeze - attr_reader :user_access, :project, :skip_authorization, :protocol, :oldrev, :newrev, :ref, :branch_name, :tag_name + attr_reader :user_access, :project, :skip_authorization, :skip_lfs_integrity_check, :protocol, :oldrev, :newrev, :ref, :branch_name, :tag_name def initialize( change, user_access:, project:, skip_authorization: false, - protocol: + skip_lfs_integrity_check: false, protocol: ) @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref) @branch_name = Gitlab::Git.branch_name(@ref) @@ -28,6 +28,7 @@ module Gitlab @user_access = user_access @project = project @skip_authorization = skip_authorization + @skip_lfs_integrity_check = skip_lfs_integrity_check @protocol = protocol end @@ -37,7 +38,7 @@ module Gitlab push_checks branch_checks tag_checks - lfs_objects_exist_check + lfs_objects_exist_check unless skip_lfs_integrity_check commits_check unless skip_commits_check true diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 8ec3386184a..9ec3858b493 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -238,19 +238,22 @@ module Gitlab changes_list = Gitlab::ChangesList.new(changes) # Iterate over all changes to find if user allowed all of them to be applied - changes_list.each do |change| + changes_list.each.with_index do |change, index| + first_change = index == 0 + # If user does not have access to make at least one change, cancel all # push by allowing the exception to bubble up - check_single_change_access(change) + check_single_change_access(change, skip_lfs_integrity_check: !first_change) end end - def check_single_change_access(change) + def check_single_change_access(change, skip_lfs_integrity_check: false) Checks::ChangeAccess.new( change, user_access: user_access, project: project, skip_authorization: deploy_key?, + skip_lfs_integrity_check: skip_lfs_integrity_check, protocol: protocol ).exec end diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index 1c9477e84b2..84d6e1490c3 100644 --- a/lib/gitlab/git_access_wiki.rb +++ b/lib/gitlab/git_access_wiki.rb @@ -13,7 +13,7 @@ module Gitlab authentication_abilities.include?(:download_code) && user_access.can_do_action?(:download_wiki_code) end - def check_single_change_access(change) + def check_single_change_access(change, _options = {}) unless user_access.can_do_action?(:create_wiki) raise UnauthorizedError, ERROR_MESSAGES[:write_to_wiki] end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 3c3697e7aa9..19d3f55501e 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -18,8 +18,9 @@ describe Gitlab::GitAccess do redirected_path: redirected_path) end - let(:push_access_check) { access.check('git-receive-pack', '_any') } - let(:pull_access_check) { access.check('git-upload-pack', '_any') } + let(:changes) { '_any' } + let(:push_access_check) { access.check('git-receive-pack', changes) } + let(:pull_access_check) { access.check('git-upload-pack', changes) } describe '#check with single protocols allowed' do def disable_protocol(protocol) @@ -646,6 +647,20 @@ describe Gitlab::GitAccess do end end + describe 'check LFS integrity' do + let(:changes) { ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature'] } + + before do + project.add_developer(user) + end + + it 'checks LFS integrity only for first change' do + expect_any_instance_of(Gitlab::Checks::LfsIntegrity).to receive(:objects_missing?).exactly(1).times + + push_access_check + end + end + describe '#check_push_access!' do before do merge_into_protected_branch