Commit 40c61acb authored by Sean McGivern's avatar Sean McGivern

Merge branch 'feature/fetch-commit-signs-from-gitaly-in-batch' into 'master'

Fetch commit signatures from Gitaly in batches

Closes gitaly#1046

See merge request gitlab-org/gitlab-ce!17456
parents a1e58280 03f3350f
...@@ -411,7 +411,7 @@ group :ed25519 do ...@@ -411,7 +411,7 @@ group :ed25519 do
end end
# Gitaly GRPC client # Gitaly GRPC client
gem 'gitaly-proto', '~> 0.85.0', require: 'gitaly' gem 'gitaly-proto', '~> 0.87.0', require: 'gitaly'
# Locked until https://github.com/google/protobuf/issues/4210 is closed # Locked until https://github.com/google/protobuf/issues/4210 is closed
gem 'google-protobuf', '= 3.5.1' gem 'google-protobuf', '= 3.5.1'
......
...@@ -285,7 +285,7 @@ GEM ...@@ -285,7 +285,7 @@ GEM
po_to_json (>= 1.0.0) po_to_json (>= 1.0.0)
rails (>= 3.2.0) rails (>= 3.2.0)
gherkin-ruby (0.3.2) gherkin-ruby (0.3.2)
gitaly-proto (0.85.0) gitaly-proto (0.87.0)
google-protobuf (~> 3.1) google-protobuf (~> 3.1)
grpc (~> 1.0) grpc (~> 1.0)
github-linguist (5.3.3) github-linguist (5.3.3)
...@@ -1057,7 +1057,7 @@ DEPENDENCIES ...@@ -1057,7 +1057,7 @@ DEPENDENCIES
gettext (~> 3.2.2) gettext (~> 3.2.2)
gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.2.0) gettext_i18n_rails_js (~> 1.2.0)
gitaly-proto (~> 0.85.0) gitaly-proto (~> 0.87.0)
github-linguist (~> 5.3.3) github-linguist (~> 5.3.3)
gitlab-flowdock-git-hook (~> 1.0.1) gitlab-flowdock-git-hook (~> 1.0.1)
gitlab-markup (~> 1.6.2) gitlab-markup (~> 1.6.2)
......
...@@ -14,37 +14,31 @@ class Projects::CommitsController < Projects::ApplicationController ...@@ -14,37 +14,31 @@ class Projects::CommitsController < Projects::ApplicationController
@merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened
.find_by(source_project: @project, source_branch: @ref, target_branch: @repository.root_ref) .find_by(source_project: @project, source_branch: @ref, target_branch: @repository.root_ref)
# https://gitlab.com/gitlab-org/gitaly/issues/931 respond_to do |format|
Gitlab::GitalyClient.allow_n_plus_1_calls do format.html
respond_to do |format| format.atom { render layout: 'xml.atom' }
format.html
format.atom { render layout: 'xml.atom' }
format.json do format.json do
pager_json( pager_json(
'projects/commits/_commits', 'projects/commits/_commits',
@commits.size, @commits.size,
project: @project, project: @project,
ref: @ref) ref: @ref)
end
end end
end end
end end
def signatures def signatures
# https://gitlab.com/gitlab-org/gitaly/issues/931 respond_to do |format|
Gitlab::GitalyClient.allow_n_plus_1_calls do format.json do
respond_to do |format| render json: {
format.json do signatures: @commits.select(&:has_signature?).map do |commit|
render json: { {
signatures: @commits.select(&:has_signature?).map do |commit| commit_sha: commit.sha,
{ html: view_to_html_string('projects/commit/_signature', signature: commit.signature)
commit_sha: commit.sha, }
html: view_to_html_string('projects/commit/_signature', signature: commit.signature) end
} }
end
}
end
end end
end end
end end
......
...@@ -19,6 +19,7 @@ class Commit ...@@ -19,6 +19,7 @@ class Commit
attr_accessor :project, :author attr_accessor :project, :author
attr_accessor :redacted_description_html attr_accessor :redacted_description_html
attr_accessor :redacted_title_html attr_accessor :redacted_title_html
attr_reader :gpg_commit
DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines] DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines]
...@@ -110,6 +111,7 @@ class Commit ...@@ -110,6 +111,7 @@ class Commit
@raw = raw_commit @raw = raw_commit
@project = project @project = project
@statuses = {} @statuses = {}
@gpg_commit = Gitlab::Gpg::Commit.new(self) if project
end end
def id def id
...@@ -452,8 +454,4 @@ class Commit ...@@ -452,8 +454,4 @@ class Commit
def merged_merge_request_no_cache(user) def merged_merge_request_no_cache(user)
MergeRequestsFinder.new(user, project_id: project.id).find_by(merge_commit_sha: id) if merge_commit? MergeRequestsFinder.new(user, project_id: project.id).find_by(merge_commit_sha: id) if merge_commit?
end end
def gpg_commit
@gpg_commit ||= Gitlab::Gpg::Commit.new(self)
end
end end
...@@ -250,6 +250,45 @@ module Gitlab ...@@ -250,6 +250,45 @@ module Gitlab
end end
end end
def extract_signature_lazily(repository, commit_id)
BatchLoader.for({ repository: repository, commit_id: commit_id }).batch do |items, loader|
items_by_repo = items.group_by { |i| i[:repository] }
items_by_repo.each do |repo, items|
commit_ids = items.map { |i| i[:commit_id] }
signatures = batch_signature_extraction(repository, commit_ids)
signatures.each do |commit_sha, signature_data|
loader.call({ repository: repository, commit_id: commit_sha }, signature_data)
end
end
end
end
def batch_signature_extraction(repository, commit_ids)
repository.gitaly_migrate(:extract_commit_signature_in_batch) do |is_enabled|
if is_enabled
gitaly_batch_signature_extraction(repository, commit_ids)
else
rugged_batch_signature_extraction(repository, commit_ids)
end
end
end
def gitaly_batch_signature_extraction(repository, commit_ids)
repository.gitaly_commit_client.get_commit_signatures(commit_ids)
end
def rugged_batch_signature_extraction(repository, commit_ids)
commit_ids.each_with_object({}) do |commit_id, signatures|
signature_data = rugged_extract_signature(repository, commit_id)
next unless signature_data
signatures[commit_id] = signature_data
end
end
def rugged_extract_signature(repository, commit_id) def rugged_extract_signature(repository, commit_id)
begin begin
Rugged::Commit.extract_signature(repository.rugged, commit_id) Rugged::Commit.extract_signature(repository.rugged, commit_id)
......
...@@ -319,6 +319,23 @@ module Gitlab ...@@ -319,6 +319,23 @@ module Gitlab
[signature, signed_text] [signature, signed_text]
end end
def get_commit_signatures(commit_ids)
request = Gitaly::GetCommitSignaturesRequest.new(repository: @gitaly_repo, commit_ids: commit_ids)
response = GitalyClient.call(@repository.storage, :commit_service, :get_commit_signatures, request)
signatures = Hash.new { |h, k| h[k] = [''.b, ''.b] }
current_commit_id = nil
response.each do |message|
current_commit_id = message.commit_id if message.commit_id.present?
signatures[current_commit_id].first << message.signature
signatures[current_commit_id].last << message.signed_text
end
signatures
end
private private
def call_commit_diff(request_params, options = {}) def call_commit_diff(request_params, options = {})
......
module Gitlab module Gitlab
module Gpg module Gpg
class Commit class Commit
include Gitlab::Utils::StrongMemoize
def initialize(commit) def initialize(commit)
@commit = commit @commit = commit
repo = commit.project.repository.raw_repository repo = commit.project.repository.raw_repository
@signature_text, @signed_text = Gitlab::Git::Commit.extract_signature(repo, commit.sha) @signature_data = Gitlab::Git::Commit.extract_signature_lazily(repo, commit.sha || commit.id)
end
def signature_text
strong_memoize(:signature_text) do
@signature_data&.itself && @signature_data[0]
end
end
def signed_text
strong_memoize(:signed_text) do
@signature_data&.itself && @signature_data[1]
end
end end
def has_signature? def has_signature?
!!(@signature_text && @signed_text) !!(signature_text && signed_text)
end end
def signature def signature
...@@ -53,7 +67,7 @@ module Gitlab ...@@ -53,7 +67,7 @@ module Gitlab
end end
def verified_signature def verified_signature
@verified_signature ||= GPGME::Crypto.new.verify(@signature_text, signed_text: @signed_text) do |verified_signature| @verified_signature ||= GPGME::Crypto.new.verify(signature_text, signed_text: signed_text) do |verified_signature|
break verified_signature break verified_signature
end end
end end
......
...@@ -393,81 +393,111 @@ describe Gitlab::Git::Commit, seed_helper: true do ...@@ -393,81 +393,111 @@ describe Gitlab::Git::Commit, seed_helper: true do
end end
end end
describe '.extract_signature' do shared_examples 'extracting commit signature' do
subject { described_class.extract_signature(repository, commit_id) } context 'when the commit is signed' do
let(:commit_id) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' }
shared_examples '.extract_signature' do
context 'when the commit is signed' do it 'returns signature and signed text' do
let(:commit_id) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } signature, signed_text = subject
it 'returns signature and signed text' do expected_signature = <<~SIGNATURE
signature, signed_text = subject -----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
expected_signature = <<~SIGNATURE Comment: GPGTools - https://gpgtools.org
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.22 (Darwin) iQEcBAABCgAGBQJTDvaZAAoJEGJ8X1ifRn8XfvYIAMuB0yrbTGo1BnOSoDfyrjb0
Comment: GPGTools - https://gpgtools.org Kw2EyUzvXYL72B63HMdJ+/0tlSDC6zONF3fc+bBD8z+WjQMTbwFNMRbSSy2rKEh+
mdRybOP3xBIMGgEph0/kmWln39nmFQBsPRbZBWoU10VfI/ieJdEOgOphszgryRar
TyS73dLBGE9y9NIININVaNISet9D9QeXFqc761CGjh4YIghvPpi+YihMWapGka6v
hgKhX+hc5rj+7IEE0CXmlbYR8OYvAbAArc5vJD7UTxAY4Z7/l9d6Ydt9GQ25khfy
ANFgltYzlR6evLFmDjssiP/mx/ZMN91AL0ueJ9nNGv411Mu2CUW+tDCaQf35mdc=
=j51i
-----END PGP SIGNATURE-----
SIGNATURE
expect(signature).to eq(expected_signature.chomp)
expect(signature).to be_a_binary_string
expected_signed_text = <<~SIGNED_TEXT
tree 22bfa2fbd217df24731f43ff43a4a0f8db759dae
parent ae73cb07c9eeaf35924a10f713b364d32b2dd34f
author Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> 1393489561 +0200
committer Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> 1393489561 +0200
Feature added
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
SIGNED_TEXT
expect(signed_text).to eq(expected_signed_text)
expect(signed_text).to be_a_binary_string
end
end
iQEcBAABCgAGBQJTDvaZAAoJEGJ8X1ifRn8XfvYIAMuB0yrbTGo1BnOSoDfyrjb0 context 'when the commit has no signature' do
Kw2EyUzvXYL72B63HMdJ+/0tlSDC6zONF3fc+bBD8z+WjQMTbwFNMRbSSy2rKEh+ let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
mdRybOP3xBIMGgEph0/kmWln39nmFQBsPRbZBWoU10VfI/ieJdEOgOphszgryRar
TyS73dLBGE9y9NIININVaNISet9D9QeXFqc761CGjh4YIghvPpi+YihMWapGka6v
hgKhX+hc5rj+7IEE0CXmlbYR8OYvAbAArc5vJD7UTxAY4Z7/l9d6Ydt9GQ25khfy
ANFgltYzlR6evLFmDjssiP/mx/ZMN91AL0ueJ9nNGv411Mu2CUW+tDCaQf35mdc=
=j51i
-----END PGP SIGNATURE-----
SIGNATURE
expect(signature).to eq(expected_signature.chomp) it 'returns nil' do
expect(signature).to be_a_binary_string expect(subject).to be_nil
end
end
expected_signed_text = <<~SIGNED_TEXT context 'when the commit cannot be found' do
tree 22bfa2fbd217df24731f43ff43a4a0f8db759dae let(:commit_id) { Gitlab::Git::BLANK_SHA }
parent ae73cb07c9eeaf35924a10f713b364d32b2dd34f
author Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> 1393489561 +0200
committer Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> 1393489561 +0200
Feature added it 'returns nil' do
expect(subject).to be_nil
end
end
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> context 'when the commit ID is invalid' do
SIGNED_TEXT let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e' }
expect(signed_text).to eq(expected_signed_text) it 'raises ArgumentError' do
expect(signed_text).to be_a_binary_string expect { subject }.to raise_error(ArgumentError)
end
end end
end
end
context 'when the commit has no signature' do describe '.extract_signature_lazily' do
let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' } shared_examples 'loading signatures in batch once' do
it 'fetches signatures in batch once' do
it 'returns nil' do commit_ids = %w[0b4bc9a49b562e85de7cc9e834518ea6828729b9 4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6]
expect(subject).to be_nil signatures = commit_ids.map do |commit_id|
described_class.extract_signature_lazily(repository, commit_id)
end end
end
context 'when the commit cannot be found' do expect(described_class).to receive(:batch_signature_extraction)
let(:commit_id) { Gitlab::Git::BLANK_SHA } .with(repository, commit_ids)
.once
.and_return({})
it 'returns nil' do 2.times { signatures.each(&:itself) }
expect(subject).to be_nil
end
end end
end
context 'when the commit ID is invalid' do subject { described_class.extract_signature_lazily(repository, commit_id).itself }
let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e' }
it 'raises ArgumentError' do context 'with Gitaly extract_commit_signature_in_batch feature enabled' do
expect { subject }.to raise_error(ArgumentError) it_behaves_like 'extracting commit signature'
end it_behaves_like 'loading signatures in batch once'
end end
context 'with Gitaly extract_commit_signature_in_batch feature disabled', :disable_gitaly do
it_behaves_like 'extracting commit signature'
it_behaves_like 'loading signatures in batch once'
end end
end
describe '.extract_signature' do
subject { described_class.extract_signature(repository, commit_id) }
context 'with gitaly' do context 'with gitaly' do
it_behaves_like '.extract_signature' it_behaves_like 'extracting commit signature'
end end
context 'without gitaly', :skip_gitaly_mock do context 'without gitaly', :disable_gitaly do
it_behaves_like '.extract_signature' it_behaves_like 'extracting commit signature'
end end
end end
end end
......
...@@ -38,7 +38,7 @@ describe Gitlab::Gpg::Commit do ...@@ -38,7 +38,7 @@ describe Gitlab::Gpg::Commit do
end end
before do before do
allow(Gitlab::Git::Commit).to receive(:extract_signature) allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
.with(Gitlab::Git::Repository, commit_sha) .with(Gitlab::Git::Repository, commit_sha)
.and_return( .and_return(
[ [
...@@ -101,7 +101,7 @@ describe Gitlab::Gpg::Commit do ...@@ -101,7 +101,7 @@ describe Gitlab::Gpg::Commit do
end end
before do before do
allow(Gitlab::Git::Commit).to receive(:extract_signature) allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
.with(Gitlab::Git::Repository, commit_sha) .with(Gitlab::Git::Repository, commit_sha)
.and_return( .and_return(
[ [
...@@ -140,7 +140,7 @@ describe Gitlab::Gpg::Commit do ...@@ -140,7 +140,7 @@ describe Gitlab::Gpg::Commit do
end end
before do before do
allow(Gitlab::Git::Commit).to receive(:extract_signature) allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
.with(Gitlab::Git::Repository, commit_sha) .with(Gitlab::Git::Repository, commit_sha)
.and_return( .and_return(
[ [
...@@ -175,7 +175,7 @@ describe Gitlab::Gpg::Commit do ...@@ -175,7 +175,7 @@ describe Gitlab::Gpg::Commit do
end end
before do before do
allow(Gitlab::Git::Commit).to receive(:extract_signature) allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
.with(Gitlab::Git::Repository, commit_sha) .with(Gitlab::Git::Repository, commit_sha)
.and_return( .and_return(
[ [
...@@ -211,7 +211,7 @@ describe Gitlab::Gpg::Commit do ...@@ -211,7 +211,7 @@ describe Gitlab::Gpg::Commit do
end end
before do before do
allow(Gitlab::Git::Commit).to receive(:extract_signature) allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
.with(Gitlab::Git::Repository, commit_sha) .with(Gitlab::Git::Repository, commit_sha)
.and_return( .and_return(
[ [
...@@ -241,7 +241,7 @@ describe Gitlab::Gpg::Commit do ...@@ -241,7 +241,7 @@ describe Gitlab::Gpg::Commit do
let!(:commit) { create :commit, project: project, sha: commit_sha } let!(:commit) { create :commit, project: project, sha: commit_sha }
before do before do
allow(Gitlab::Git::Commit).to receive(:extract_signature) allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
.with(Gitlab::Git::Repository, commit_sha) .with(Gitlab::Git::Repository, commit_sha)
.and_return( .and_return(
[ [
......
...@@ -26,7 +26,7 @@ RSpec.describe Gitlab::Gpg::InvalidGpgSignatureUpdater do ...@@ -26,7 +26,7 @@ RSpec.describe Gitlab::Gpg::InvalidGpgSignatureUpdater do
before do before do
allow_any_instance_of(Project).to receive(:commit).and_return(commit) allow_any_instance_of(Project).to receive(:commit).and_return(commit)
allow(Gitlab::Git::Commit).to receive(:extract_signature) allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
.with(Gitlab::Git::Repository, commit_sha) .with(Gitlab::Git::Repository, commit_sha)
.and_return(signature) .and_return(signature)
end end
......
...@@ -15,8 +15,8 @@ describe MergeRequests::BuildService do ...@@ -15,8 +15,8 @@ describe MergeRequests::BuildService do
let(:target_branch) { 'master' } let(:target_branch) { 'master' }
let(:merge_request) { service.execute } let(:merge_request) { service.execute }
let(:compare) { double(:compare, commits: commits) } let(:compare) { double(:compare, commits: commits) }
let(:commit_1) { double(:commit_1, safe_message: "Initial commit\n\nCreate the app") } let(:commit_1) { double(:commit_1, sha: 'f00ba7', safe_message: "Initial commit\n\nCreate the app") }
let(:commit_2) { double(:commit_2, safe_message: 'This is a bad commit message!') } let(:commit_2) { double(:commit_2, sha: 'f00ba7', safe_message: 'This is a bad commit message!') }
let(:commits) { nil } let(:commits) { nil }
let(:service) do let(:service) do
......
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