From 95cc0c4b5b3bcc121800ead4f2f7ee4964702c30 Mon Sep 17 00:00:00 2001 From: Robert May Date: Wed, 28 Oct 2020 16:22:00 +0000 Subject: [PATCH 1/3] Pass environment to check threads This ensures the git environment is passed to the parallel check threads. --- ee/lib/ee/gitlab/checks/push_rule_check.rb | 20 ++++++++++---- .../ee/gitlab/checks/push_rule_check_spec.rb | 27 +++++++++++++++++++ 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/ee/lib/ee/gitlab/checks/push_rule_check.rb b/ee/lib/ee/gitlab/checks/push_rule_check.rb index 441aeafa4a1012..abfa27c89d4481 100644 --- a/ee/lib/ee/gitlab/checks/push_rule_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rule_check.rb @@ -41,18 +41,23 @@ def run_checks_in_sequence! check_file_size! end - # Run the checks in separate threads for performance benefits + # Run the checks in separate threads for performance benefits. + # + # The git hook environment is currently set in the current thread + # in lib/api/internal/base.rb. This needs to be passed into the + # child threads we spawn here. # # @return [Nil] returns nil unless an error is raised # @raise [Gitlab::GitAccess::ForbiddenError] if any check fails def run_checks_in_parallel! + git_env = ::Gitlab::Git::HookEnv.all(project.repository.gl_repository) @threads = [] - parallelize do + parallelize(git_env) do check_tag_or_branch! end - parallelize do + parallelize(git_env) do check_file_size! end @@ -73,8 +78,9 @@ def run_checks_in_parallel! # Runs a block inside a new thread. This thread will # exit immediately upon an exception being raised. # + # @param git_env [Hash] the current git environment # @raise [Gitlab::GitAccess::ForbiddenError] - def parallelize + def parallelize(git_env) @threads << Thread.new do Thread.current.tap do |t| t.name = "push_rule_check" @@ -82,7 +88,11 @@ def parallelize t.report_on_exception = false end - yield + ::Gitlab::WithRequestStore.with_request_store do + ::Gitlab::Git::HookEnv.set(project.repository.gl_repository, git_env) + + yield + end ensure # rubocop: disable Layout/RescueEnsureAlignment ActiveRecord::Base.clear_active_connections! end diff --git a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb index bb860b111a9e1d..00d83a6a99a207 100644 --- a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb +++ b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb @@ -69,4 +69,31 @@ it_behaves_like "push checks" end end + + describe "#parallelize" do + let(:parallelize) do + subject.send(:parallelize, git_env) do + Thread.current[:test_env] = Thread.current[:request_store][:gitlab_git_env] + end + end + + let(:git_env) do + { + "GIT_OBJECT_DIRECTORY_RELATIVE" => "foo", + "GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE" => "bar" + } + end + + before do + subject.instance_variable_set(:@threads, []) + end + + it "returns the git env" do + parallelize.each do |thread| + thread.join + + expect(thread[:test_env]).to eq({ project.repository.gl_repository => git_env }) + end + end + end end -- GitLab From 5a663851edf80c7471f4ac42d81b8d2cbeebb8eb Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 29 Oct 2020 11:38:19 +0000 Subject: [PATCH 2/3] Refactor spec to be more concise --- .../lib/ee/gitlab/checks/push_rule_check_spec.rb | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb index 00d83a6a99a207..5e45c1bf1252c2 100644 --- a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb +++ b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb @@ -71,12 +71,6 @@ end describe "#parallelize" do - let(:parallelize) do - subject.send(:parallelize, git_env) do - Thread.current[:test_env] = Thread.current[:request_store][:gitlab_git_env] - end - end - let(:git_env) do { "GIT_OBJECT_DIRECTORY_RELATIVE" => "foo", @@ -84,15 +78,11 @@ } end - before do + it "makes the git env available to the child thread" do subject.instance_variable_set(:@threads, []) - end - it "returns the git env" do - parallelize.each do |thread| - thread.join - - expect(thread[:test_env]).to eq({ project.repository.gl_repository => git_env }) + subject.send(:parallelize, git_env) do + expect(::Gitlab::Git::HookEnv.all(project.repository.gl_repository)).to eq(git_env) end end end -- GitLab From 26d9d07c5426c325f71293739a6ccddf9aa10944 Mon Sep 17 00:00:00 2001 From: Robert May Date: Fri, 30 Oct 2020 12:10:37 +0000 Subject: [PATCH 3/3] Improve spec coverage of hook env implementation --- .../ee/gitlab/checks/push_rule_check_spec.rb | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb index 5e45c1bf1252c2..053e272c0dc67e 100644 --- a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb +++ b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb @@ -59,31 +59,31 @@ end describe '#validate!' do - it_behaves_like "push checks" + context "parallel push checks" do + it_behaves_like "push checks" - context ":parallel_push_checks feature is disabled" do before do - stub_feature_flags(parallel_push_checks: false) + ::Gitlab::Git::HookEnv.set(project.repository.gl_repository, + "GIT_OBJECT_DIRECTORY_RELATIVE" => "objects", + "GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE" => []) end - it_behaves_like "push checks" - end - end + it "sets the git env correctly for all hooks", :request_store do + expect(Gitaly::Repository).to receive(:new) + .with(a_hash_including(git_object_directory: "objects")) + .and_call_original - describe "#parallelize" do - let(:git_env) do - { - "GIT_OBJECT_DIRECTORY_RELATIVE" => "foo", - "GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE" => "bar" - } + # This push fails because of the commit message check + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError) + end end - it "makes the git env available to the child thread" do - subject.instance_variable_set(:@threads, []) - - subject.send(:parallelize, git_env) do - expect(::Gitlab::Git::HookEnv.all(project.repository.gl_repository)).to eq(git_env) + context ":parallel_push_checks feature is disabled" do + before do + stub_feature_flags(parallel_push_checks: false) end + + it_behaves_like "push checks" end end end -- GitLab