Commit 16ca6914 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'jej/fix-slow-lfs-object-check' into 'master'

Only check LFS integrity for first branch in push

Closes #41141

See merge request gitlab-org/gitlab-ce!17098
parents 02d9f54f c88fe70f
---
title: Only check LFS integrity for first ref in a push to avoid timeout
merge_request: 17098
author:
type: performance
...@@ -16,11 +16,11 @@ module Gitlab ...@@ -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".' lfs_objects_missing: 'LFS objects are missing. Ensure LFS is properly set up or try a manual "git lfs push --all".'
}.freeze }.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( def initialize(
change, user_access:, project:, skip_authorization: false, change, user_access:, project:, skip_authorization: false,
protocol: skip_lfs_integrity_check: false, protocol:
) )
@oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref) @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@branch_name = Gitlab::Git.branch_name(@ref) @branch_name = Gitlab::Git.branch_name(@ref)
...@@ -28,6 +28,7 @@ module Gitlab ...@@ -28,6 +28,7 @@ module Gitlab
@user_access = user_access @user_access = user_access
@project = project @project = project
@skip_authorization = skip_authorization @skip_authorization = skip_authorization
@skip_lfs_integrity_check = skip_lfs_integrity_check
@protocol = protocol @protocol = protocol
end end
...@@ -37,7 +38,7 @@ module Gitlab ...@@ -37,7 +38,7 @@ module Gitlab
push_checks push_checks
branch_checks branch_checks
tag_checks tag_checks
lfs_objects_exist_check lfs_objects_exist_check unless skip_lfs_integrity_check
commits_check unless skip_commits_check commits_check unless skip_commits_check
true true
......
...@@ -238,19 +238,22 @@ module Gitlab ...@@ -238,19 +238,22 @@ module Gitlab
changes_list = Gitlab::ChangesList.new(changes) changes_list = Gitlab::ChangesList.new(changes)
# Iterate over all changes to find if user allowed all of them to be applied # 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 # If user does not have access to make at least one change, cancel all
# push by allowing the exception to bubble up # 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
end end
def check_single_change_access(change) def check_single_change_access(change, skip_lfs_integrity_check: false)
Checks::ChangeAccess.new( Checks::ChangeAccess.new(
change, change,
user_access: user_access, user_access: user_access,
project: project, project: project,
skip_authorization: deploy_key?, skip_authorization: deploy_key?,
skip_lfs_integrity_check: skip_lfs_integrity_check,
protocol: protocol protocol: protocol
).exec ).exec
end end
......
...@@ -13,7 +13,7 @@ module Gitlab ...@@ -13,7 +13,7 @@ module Gitlab
authentication_abilities.include?(:download_code) && user_access.can_do_action?(:download_wiki_code) authentication_abilities.include?(:download_code) && user_access.can_do_action?(:download_wiki_code)
end end
def check_single_change_access(change) def check_single_change_access(change, _options = {})
unless user_access.can_do_action?(:create_wiki) unless user_access.can_do_action?(:create_wiki)
raise UnauthorizedError, ERROR_MESSAGES[:write_to_wiki] raise UnauthorizedError, ERROR_MESSAGES[:write_to_wiki]
end end
......
...@@ -18,8 +18,9 @@ describe Gitlab::GitAccess do ...@@ -18,8 +18,9 @@ describe Gitlab::GitAccess do
redirected_path: redirected_path) redirected_path: redirected_path)
end end
let(:push_access_check) { access.check('git-receive-pack', '_any') } let(:changes) { '_any' }
let(:pull_access_check) { access.check('git-upload-pack', '_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 describe '#check with single protocols allowed' do
def disable_protocol(protocol) def disable_protocol(protocol)
...@@ -646,6 +647,20 @@ describe Gitlab::GitAccess do ...@@ -646,6 +647,20 @@ describe Gitlab::GitAccess do
end end
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 describe '#check_push_access!' do
before do before do
merge_into_protected_branch merge_into_protected_branch
......
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