From 5848c1d2ee56f333a5068424fcd53f4eec67a9a4 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 28 Oct 2019 17:43:54 +0100 Subject: [PATCH] WIP: Only call out to gpg from sidekiq --- .../projects/commits_controller.rb | 10 ++++-- app/services/git/branch_hooks_service.rb | 8 +---- app/workers/create_gpg_signature_worker.rb | 2 +- lib/gitlab/gpg.rb | 33 +++++++++++++++++-- lib/gitlab/gpg/commit.rb | 25 ++++++-------- lib/gitlab/gpg/commit_signature_loader.rb | 30 +++++++++++++++++ 6 files changed, 79 insertions(+), 29 deletions(-) create mode 100644 lib/gitlab/gpg/commit_signature_loader.rb diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index 15bb35dd0be1e5..1a171bb569bced 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require "base64" +require 'base64' class Projects::CommitsController < Projects::ApplicationController include ExtractsPath @@ -40,15 +40,19 @@ def show # rubocop: enable CodeReuse/ActiveRecord def signatures + commit_loader = Gitlab::Gpg::CommitSignatureLoader.new(@commits, project) + commit_loader.schedule_loading! + respond_to do |format| format.json do render json: { - signatures: @commits.select(&:has_signature?).map do |commit| + signatures: commit_loader.with_loaded_signatures.map do |commit| { commit_sha: commit.sha, html: view_to_html_string('projects/commit/_signature', signature: commit.signature) } - end + end, + missing_signatures: commit_loader.commits_missing_signatures.any? } end end diff --git a/app/services/git/branch_hooks_service.rb b/app/services/git/branch_hooks_service.rb index 69f1f9eb31fe34..3581a8e0d4e515 100644 --- a/app/services/git/branch_hooks_service.rb +++ b/app/services/git/branch_hooks_service.rb @@ -104,13 +104,7 @@ def enqueue_process_commit_messages end def enqueue_update_gpg_signatures - unsigned = GpgSignature.unsigned_commit_shas(limited_commits.map(&:sha)) - return if unsigned.empty? - - signable = Gitlab::Git::Commit.shas_with_signatures(project.repository, unsigned) - return if signable.empty? - - CreateGpgSignatureWorker.perform_async(signable, project.id) + Gitlab::Gpg::CommitSignatureLoader.new(limited_commits, project).schedule_loading! end # It's not sufficient to just check for a blank SHA as it's possible for the diff --git a/app/workers/create_gpg_signature_worker.rb b/app/workers/create_gpg_signature_worker.rb index fc36a2adccdd9b..79befc4f071674 100644 --- a/app/workers/create_gpg_signature_worker.rb +++ b/app/workers/create_gpg_signature_worker.rb @@ -22,7 +22,7 @@ def perform(commit_shas, project_id) # This calculates and caches the signature in the database commits.each do |commit| - Gitlab::Gpg::Commit.new(commit).signature + Gitlab::Gpg::Commit.new(commit).create_cached_signature! rescue => e Rails.logger.error("Failed to create signature for commit #{commit.id}. Error: #{e.message}") # rubocop:disable Gitlab/RailsLogger end diff --git a/lib/gitlab/gpg.rb b/lib/gitlab/gpg.rb index 32f61b1d65c67b..2fdc524887cb35 100644 --- a/lib/gitlab/gpg.rb +++ b/lib/gitlab/gpg.rb @@ -4,6 +4,9 @@ module Gitlab module Gpg extend self + CleanupFailed = Class.new(StandardError) + CLEANUP_ATTEMPTS = 5 + MUTEX = Mutex.new module CurrentKeyChain @@ -94,16 +97,40 @@ def optimistic_using_tmp_keychain previous_dir = current_home_dir tmp_dir = Dir.mktmpdir GPGME::Engine.home_dir = tmp_dir + yield ensure - # Ignore any errors when removing the tmp directory, as we may run into a + GPGME::Engine.home_dir = previous_dir + + begin + cleanup_tmp_dir(tmp_dir) + rescue CleanupError + # This means we left a GPG-agent process hanging, need to see how often + # this happens with the number of attempts at removal. + Gitlab::Sentry.track_exception(e, issue_url: 'https://gitlab.com/gitlab-org/gitlab/issues/20918') + end + end + + def cleanup_tmp_dir(tmp_dir) + # Retry when removing the tmp directory failed, as we may run into a # race condition: # The `gpg-agent` agent process may clean up some files as well while # `FileUtils.remove_entry` is iterating the directory and removing all # its contained files and directories recursively, which could raise an # error. - FileUtils.remove_entry(tmp_dir, true) - GPGME::Engine.home_dir = previous_dir + # Failing to remove the tmp directory could leave the `gpg-agent` process + # running forever. + + tries = 0 + + while tries < CLEANUP_ATTEMPTS && File.exist?(tmp_dir) + tries += 1 + FileUtils.remove_entry(tmp_dir, true) + # Give GPG-agent a moment to clean up after itself + sleep 0.1 if File.exist?(tmp_dir) + end + + raise CleanupError, "Could not remove temp gpg homedir: #{tmp_dir}" if File.exist?(tmp_dir) end end end diff --git a/lib/gitlab/gpg/commit.rb b/lib/gitlab/gpg/commit.rb index dc71d0b427a28d..2f57e47676c596 100644 --- a/lib/gitlab/gpg/commit.rb +++ b/lib/gitlab/gpg/commit.rb @@ -33,12 +33,7 @@ def has_signature? def signature return unless has_signature? - return @signature if @signature - - cached_signature = lazy_signature&.itself - return @signature = cached_signature if cached_signature.present? - - @signature = create_cached_signature! + @signature ||= lazy_signature&.itself end def update_signature!(cached_signature) @@ -48,6 +43,15 @@ def update_signature!(cached_signature) end end + def create_cached_signature! + using_keychain do |gpg_key| + attributes = attributes(gpg_key) + break GpgSignature.new(attributes) if Gitlab::Database.read_only? + + GpgSignature.safe_create!(attributes) + end + end + private def lazy_signature @@ -95,15 +99,6 @@ def gpgme_signature nil end - def create_cached_signature! - using_keychain do |gpg_key| - attributes = attributes(gpg_key) - break GpgSignature.new(attributes) if Gitlab::Database.read_only? - - GpgSignature.safe_create!(attributes) - end - end - def attributes(gpg_key) user_infos = user_infos(gpg_key) verification_status = verification_status(gpg_key) diff --git a/lib/gitlab/gpg/commit_signature_loader.rb b/lib/gitlab/gpg/commit_signature_loader.rb new file mode 100644 index 00000000000000..af229319b6c515 --- /dev/null +++ b/lib/gitlab/gpg/commit_signature_loader.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Gitlab + module Gpg + class CommitSignatureLoader + def initialize(commits, project) + @commits = commits + @project = project + end + + def schedule_loading! + return if commits_missing_signatures.empty? + + CreateGpgSignatureWorker.perform_async(commits_missing_signatures.map(&:sha), project.id) + end + + def with_loaded_signatures + @commits_loaded_signatures ||= commits.select { |commit| commit.signature.present? } + end + + def commits_missing_signatures + @commits_missing_signatures ||= commits.select { |commit| commit.has_signature? && commit.signature.nil? } + end + + private + + attr_reader :commits, :project + end + end +end -- GitLab