Commit ec34b2d0 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'dm-gpg-signature-performance' into 'master'

Only create commit GPG signature when necessary

See merge request !13561
parents 72d5165b ba7251fe
...@@ -383,6 +383,6 @@ class Commit ...@@ -383,6 +383,6 @@ class Commit
end end
def gpg_commit def gpg_commit
@gpg_commit ||= Gitlab::Gpg::Commit.new(self) @gpg_commit ||= Gitlab::Gpg::Commit.for_commit(self)
end end
end end
...@@ -18,4 +18,8 @@ class GpgSignature < ActiveRecord::Base ...@@ -18,4 +18,8 @@ class GpgSignature < ActiveRecord::Base
def commit def commit
project.commit(commit_sha) project.commit(commit_sha)
end end
def gpg_commit
Gitlab::Gpg::Commit.new(project, commit_sha)
end
end end
...@@ -90,8 +90,19 @@ class GitPushService < BaseService ...@@ -90,8 +90,19 @@ class GitPushService < BaseService
end end
def update_signatures def update_signatures
@push_commits.each do |commit| commit_shas = @push_commits.last(PROCESS_COMMIT_LIMIT).map(&:sha)
CreateGpgSignatureWorker.perform_async(commit.sha, @project.id)
return if commit_shas.empty?
shas_with_cached_signatures = GpgSignature.where(commit_sha: commit_shas).pluck(:commit_sha)
commit_shas -= shas_with_cached_signatures
return if commit_shas.empty?
commit_shas = Gitlab::Git::Commit.shas_with_signatures(project.repository, commit_shas)
commit_shas.each do |sha|
CreateGpgSignatureWorker.perform_async(sha, project.id)
end end
end end
......
...@@ -4,13 +4,9 @@ class CreateGpgSignatureWorker ...@@ -4,13 +4,9 @@ class CreateGpgSignatureWorker
def perform(commit_sha, project_id) def perform(commit_sha, project_id)
project = Project.find_by(id: project_id) project = Project.find_by(id: project_id)
return unless project return unless project
commit = project.commit(commit_sha) # This calculates and caches the signature in the database
Gitlab::Gpg::Commit.new(project, commit_sha).signature
return unless commit
commit.signature
end end
end end
module ActiveRecord
class PredicateBuilder
class ArrayHandler
module TypeCasting
def call(attribute, value)
# This is necessary because by default ActiveRecord does not respect
# custom type definitions (like our `ShaAttribute`) when providing an
# array in `where`, like in `where(commit_sha: [sha1, sha2, sha3])`.
model = attribute.relation&.engine
type = model.user_provided_columns[attribute.name] if model
value = value.map { |value| type.type_cast_for_database(value) } if type
super(attribute, value)
end
end
prepend TypeCasting
end
end
end
...@@ -210,6 +210,16 @@ module Gitlab ...@@ -210,6 +210,16 @@ module Gitlab
@rugged_sort_types.fetch(sort_type, Rugged::SORT_NONE) @rugged_sort_types.fetch(sort_type, Rugged::SORT_NONE)
end end
def shas_with_signatures(repository, shas)
shas.select do |sha|
begin
Rugged::Commit.extract_signature(repository.rugged, sha)
rescue Rugged::OdbError
false
end
end
end
end end
def initialize(repository, raw_commit, head = nil) def initialize(repository, raw_commit, head = nil)
...@@ -335,15 +345,6 @@ module Gitlab ...@@ -335,15 +345,6 @@ module Gitlab
parent_ids.map { |oid| self.class.find(@repository, oid) }.compact parent_ids.map { |oid| self.class.find(@repository, oid) }.compact
end end
# Get the gpg signature of this commit.
#
# Ex.
# commit.signature(repo)
#
def signature(repo)
Rugged::Commit.extract_signature(repo.rugged, sha)
end
def stats def stats
Gitlab::Git::CommitStats.new(self) Gitlab::Git::CommitStats.new(self)
end end
......
module Gitlab module Gitlab
module Gpg module Gpg
class Commit class Commit
attr_reader :commit def self.for_commit(commit)
new(commit.project, commit.sha)
end
def initialize(commit) def initialize(project, sha)
@commit = commit @project = project
@sha = sha
@signature_text, @signed_text = commit.raw.signature(commit.project.repository) @signature_text, @signed_text =
begin
Rugged::Commit.extract_signature(project.repository.rugged, sha)
rescue Rugged::OdbError
nil
end
end end
def has_signature? def has_signature?
...@@ -16,18 +24,20 @@ module Gitlab ...@@ -16,18 +24,20 @@ module Gitlab
def signature def signature
return unless has_signature? return unless has_signature?
cached_signature = GpgSignature.find_by(commit_sha: commit.sha) return @signature if @signature
return cached_signature if cached_signature.present?
using_keychain do |gpg_key| cached_signature = GpgSignature.find_by(commit_sha: @sha)
create_cached_signature!(gpg_key) return @signature = cached_signature if cached_signature.present?
end
@signature = create_cached_signature!
end end
def update_signature!(cached_signature) def update_signature!(cached_signature)
using_keychain do |gpg_key| using_keychain do |gpg_key|
cached_signature.update_attributes!(attributes(gpg_key)) cached_signature.update_attributes!(attributes(gpg_key))
end end
@signature = cached_signature
end end
private private
...@@ -55,16 +65,18 @@ module Gitlab ...@@ -55,16 +65,18 @@ module Gitlab
end end
end end
def create_cached_signature!(gpg_key) def create_cached_signature!
GpgSignature.create!(attributes(gpg_key)) using_keychain do |gpg_key|
GpgSignature.create!(attributes(gpg_key))
end
end end
def attributes(gpg_key) def attributes(gpg_key)
user_infos = user_infos(gpg_key) user_infos = user_infos(gpg_key)
{ {
commit_sha: commit.sha, commit_sha: @sha,
project: commit.project, project: @project,
gpg_key: gpg_key, gpg_key: gpg_key,
gpg_key_primary_keyid: gpg_key&.primary_keyid || verified_signature.fingerprint, gpg_key_primary_keyid: gpg_key&.primary_keyid || verified_signature.fingerprint,
gpg_key_user_name: user_infos[:name], gpg_key_user_name: user_infos[:name],
......
...@@ -10,9 +10,7 @@ module Gitlab ...@@ -10,9 +10,7 @@ module Gitlab
.select(:id, :commit_sha, :project_id) .select(:id, :commit_sha, :project_id)
.where('gpg_key_id IS NULL OR valid_signature = ?', false) .where('gpg_key_id IS NULL OR valid_signature = ?', false)
.where(gpg_key_primary_keyid: @gpg_key.primary_keyid) .where(gpg_key_primary_keyid: @gpg_key.primary_keyid)
.find_each do |gpg_signature| .find_each { |sig| sig.gpg_commit.update_signature!(sig) }
Gitlab::Gpg::Commit.new(gpg_signature.commit).update_signature!(gpg_signature)
end
end end
end end
end end
......
require 'rails_helper' require 'rails_helper'
RSpec.describe Gitlab::Gpg::Commit do describe Gitlab::Gpg::Commit do
describe '#signature' do describe '#signature' do
let!(:project) { create :project, :repository, path: 'sample-project' } let!(:project) { create :project, :repository, path: 'sample-project' }
let!(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33' } let!(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33' }
context 'unisgned commit' do context 'unsigned commit' do
it 'returns nil' do it 'returns nil' do
expect(described_class.new(project.commit).signature).to be_nil expect(described_class.new(project, commit_sha).signature).to be_nil
end end
end end
...@@ -16,18 +16,19 @@ RSpec.describe Gitlab::Gpg::Commit do ...@@ -16,18 +16,19 @@ RSpec.describe Gitlab::Gpg::Commit do
create :gpg_key, key: GpgHelpers::User1.public_key, user: create(:user, email: GpgHelpers::User1.emails.first) create :gpg_key, key: GpgHelpers::User1.public_key, user: create(:user, email: GpgHelpers::User1.emails.first)
end end
let!(:commit) do before do
raw_commit = double(:raw_commit, signature: [ allow(Rugged::Commit).to receive(:extract_signature)
GpgHelpers::User1.signed_commit_signature, .with(Rugged::Repository, commit_sha)
GpgHelpers::User1.signed_commit_base_data .and_return(
], sha: commit_sha) [
allow(raw_commit).to receive :save! GpgHelpers::User1.signed_commit_signature,
GpgHelpers::User1.signed_commit_base_data
create :commit, git_commit: raw_commit, project: project ]
)
end end
it 'returns a valid signature' do it 'returns a valid signature' do
expect(described_class.new(commit).signature).to have_attributes( expect(described_class.new(project, commit_sha).signature).to have_attributes(
commit_sha: commit_sha, commit_sha: commit_sha,
project: project, project: project,
gpg_key: gpg_key, gpg_key: gpg_key,
...@@ -39,7 +40,7 @@ RSpec.describe Gitlab::Gpg::Commit do ...@@ -39,7 +40,7 @@ RSpec.describe Gitlab::Gpg::Commit do
end end
it 'returns the cached signature on second call' do it 'returns the cached signature on second call' do
gpg_commit = described_class.new(commit) gpg_commit = described_class.new(project, commit_sha)
expect(gpg_commit).to receive(:using_keychain).and_call_original expect(gpg_commit).to receive(:using_keychain).and_call_original
gpg_commit.signature gpg_commit.signature
...@@ -53,18 +54,19 @@ RSpec.describe Gitlab::Gpg::Commit do ...@@ -53,18 +54,19 @@ RSpec.describe Gitlab::Gpg::Commit do
context 'known but unverified public key' do context 'known but unverified public key' do
let!(:gpg_key) { create :gpg_key, key: GpgHelpers::User1.public_key } let!(:gpg_key) { create :gpg_key, key: GpgHelpers::User1.public_key }
let!(:commit) do before do
raw_commit = double(:raw_commit, signature: [ allow(Rugged::Commit).to receive(:extract_signature)
GpgHelpers::User1.signed_commit_signature, .with(Rugged::Repository, commit_sha)
GpgHelpers::User1.signed_commit_base_data .and_return(
], sha: commit_sha) [
allow(raw_commit).to receive :save! GpgHelpers::User1.signed_commit_signature,
GpgHelpers::User1.signed_commit_base_data
create :commit, git_commit: raw_commit, project: project ]
)
end end
it 'returns an invalid signature' do it 'returns an invalid signature' do
expect(described_class.new(commit).signature).to have_attributes( expect(described_class.new(project, commit_sha).signature).to have_attributes(
commit_sha: commit_sha, commit_sha: commit_sha,
project: project, project: project,
gpg_key: gpg_key, gpg_key: gpg_key,
...@@ -76,7 +78,7 @@ RSpec.describe Gitlab::Gpg::Commit do ...@@ -76,7 +78,7 @@ RSpec.describe Gitlab::Gpg::Commit do
end end
it 'returns the cached signature on second call' do it 'returns the cached signature on second call' do
gpg_commit = described_class.new(commit) gpg_commit = described_class.new(project, commit_sha)
expect(gpg_commit).to receive(:using_keychain).and_call_original expect(gpg_commit).to receive(:using_keychain).and_call_original
gpg_commit.signature gpg_commit.signature
...@@ -88,20 +90,19 @@ RSpec.describe Gitlab::Gpg::Commit do ...@@ -88,20 +90,19 @@ RSpec.describe Gitlab::Gpg::Commit do
end end
context 'unknown public key' do context 'unknown public key' do
let!(:commit) do before do
raw_commit = double(:raw_commit, signature: [ allow(Rugged::Commit).to receive(:extract_signature)
GpgHelpers::User1.signed_commit_signature, .with(Rugged::Repository, commit_sha)
GpgHelpers::User1.signed_commit_base_data .and_return(
], sha: commit_sha) [
allow(raw_commit).to receive :save! GpgHelpers::User1.signed_commit_signature,
GpgHelpers::User1.signed_commit_base_data
create :commit, ]
git_commit: raw_commit, )
project: project
end end
it 'returns an invalid signature' do it 'returns an invalid signature' do
expect(described_class.new(commit).signature).to have_attributes( expect(described_class.new(project, commit_sha).signature).to have_attributes(
commit_sha: commit_sha, commit_sha: commit_sha,
project: project, project: project,
gpg_key: nil, gpg_key: nil,
...@@ -113,7 +114,7 @@ RSpec.describe Gitlab::Gpg::Commit do ...@@ -113,7 +114,7 @@ RSpec.describe Gitlab::Gpg::Commit do
end end
it 'returns the cached signature on second call' do it 'returns the cached signature on second call' do
gpg_commit = described_class.new(commit) gpg_commit = described_class.new(project, commit_sha)
expect(gpg_commit).to receive(:using_keychain).and_call_original expect(gpg_commit).to receive(:using_keychain).and_call_original
gpg_commit.signature gpg_commit.signature
......
...@@ -4,23 +4,16 @@ RSpec.describe Gitlab::Gpg::InvalidGpgSignatureUpdater do ...@@ -4,23 +4,16 @@ RSpec.describe Gitlab::Gpg::InvalidGpgSignatureUpdater do
describe '#run' do describe '#run' do
let!(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33' } let!(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33' }
let!(:project) { create :project, :repository, path: 'sample-project' } let!(:project) { create :project, :repository, path: 'sample-project' }
let!(:raw_commit) do
raw_commit = double(:raw_commit, signature: [
GpgHelpers::User1.signed_commit_signature,
GpgHelpers::User1.signed_commit_base_data
], sha: commit_sha)
allow(raw_commit).to receive :save!
raw_commit
end
let!(:commit) do
create :commit, git_commit: raw_commit, project: project
end
before do before do
allow_any_instance_of(Project).to receive(:commit).and_return(commit) allow(Rugged::Commit).to receive(:extract_signature)
.with(Rugged::Repository, commit_sha)
.and_return(
[
GpgHelpers::User1.signed_commit_signature,
GpgHelpers::User1.signed_commit_base_data
]
)
end end
context 'gpg signature did have an associated gpg key which was removed later' do context 'gpg signature did have an associated gpg key which was removed later' do
......
...@@ -688,10 +688,38 @@ describe GitPushService, services: true do ...@@ -688,10 +688,38 @@ describe GitPushService, services: true do
) )
end end
it 'calls CreateGpgSignatureWorker.perform_async for each commit' do context 'when the commit has a signature' do
expect(CreateGpgSignatureWorker).to receive(:perform_async).with(sample_commit.id, project.id) context 'when the signature is already cached' do
before do
create(:gpg_signature, commit_sha: sample_commit.id)
end
execute_service(project, user, oldrev, newrev, ref) it 'does not queue a CreateGpgSignatureWorker' do
expect(CreateGpgSignatureWorker).not_to receive(:perform_async).with(sample_commit.id, project.id)
execute_service(project, user, oldrev, newrev, ref)
end
end
context 'when the signature is not yet cached' do
it 'queues a CreateGpgSignatureWorker' do
expect(CreateGpgSignatureWorker).to receive(:perform_async).with(sample_commit.id, project.id)
execute_service(project, user, oldrev, newrev, ref)
end
end
end
context 'when the commit does not have a signature' do
before do
allow(Gitlab::Git::Commit).to receive(:shas_with_signatures).with(project.repository, [sample_commit.id]).and_return([])
end
it 'does not queue a CreateGpgSignatureWorker' do
expect(CreateGpgSignatureWorker).not_to receive(:perform_async).with(sample_commit.id, project.id)
execute_service(project, user, oldrev, newrev, ref)
end
end end
end end
......
require 'spec_helper' require 'spec_helper'
describe CreateGpgSignatureWorker do describe CreateGpgSignatureWorker do
let(:project) { create(:project, :repository) }
context 'when GpgKey is found' do context 'when GpgKey is found' do
it 'calls Commit#signature' do let(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33' }
commit_sha = '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33'
project = create :project
commit = instance_double(Commit)
allow(Project).to receive(:find_by).with(id: project.id).and_return(project) it 'calls Gitlab::Gpg::Commit#signature' do
allow(project).to receive(:commit).with(commit_sha).and_return(commit) expect(Gitlab::Gpg::Commit).to receive(:new).with(project, commit_sha).and_call_original
expect(commit).to receive(:signature) expect_any_instance_of(Gitlab::Gpg::Commit).to receive(:signature)
described_class.new.perform(commit_sha, project.id) described_class.new.perform(commit_sha, project.id)
end end
end end
context 'when Commit is not found' do context 'when Commit is not found' do
let(:nonexisting_commit_sha) { 'bogus' } let(:nonexisting_commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a34' }
let(:project) { create :project }
it 'does not raise errors' do it 'does not raise errors' do
expect { described_class.new.perform(nonexisting_commit_sha, project.id) }.not_to raise_error expect { described_class.new.perform(nonexisting_commit_sha, project.id) }.not_to raise_error
end end
it 'does not call Commit#signature' do
expect_any_instance_of(Commit).not_to receive(:signature)
described_class.new.perform(nonexisting_commit_sha, project.id)
end
end end
context 'when Project is not found' do context 'when Project is not found' do
...@@ -38,8 +30,8 @@ describe CreateGpgSignatureWorker do ...@@ -38,8 +30,8 @@ describe CreateGpgSignatureWorker do
expect { described_class.new.perform(anything, nonexisting_project_id) }.not_to raise_error expect { described_class.new.perform(anything, nonexisting_project_id) }.not_to raise_error
end end
it 'does not call Commit#signature' do it 'does not call Gitlab::Gpg::Commit#signature' do
expect_any_instance_of(Commit).not_to receive(:signature) expect_any_instance_of(Gitlab::Gpg::Commit).not_to receive(:signature)
described_class.new.perform(anything, nonexisting_project_id) described_class.new.perform(anything, nonexisting_project_id)
end end
......
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