From b6086d0f3e764ebfda3bce235938955304dabf10 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Fri, 5 Aug 2022 11:49:18 +0800 Subject: [PATCH] Implement StreamApprovalAuditEvent worker/service This moves streaming approval audit event to another subscriber to `MergeRequests::ApprovedEvent`. This is behind `async_after_approval` feature flag. --- .../merge_requests/approval_service.rb | 2 +- config/sidekiq_queues.yml | 2 + .../stream_approval_audit_event_service.rb | 18 ++++++ ee/app/workers/all_queues.yml | 9 +++ .../stream_approval_audit_event_worker.rb | 37 ++++++++++++ ee/lib/ee/gitlab/event_store.rb | 1 + .../lib/{ => ee}/gitlab/event_store_spec.rb | 3 +- .../merge_requests/approval_service_spec.rb | 28 ++++----- ...tream_approval_audit_event_service_spec.rb | 27 +++++++++ ...stream_approval_audit_event_worker_spec.rb | 57 +++++++++++++++++++ 10 files changed, 168 insertions(+), 16 deletions(-) create mode 100644 ee/app/services/merge_requests/stream_approval_audit_event_service.rb create mode 100644 ee/app/workers/merge_requests/stream_approval_audit_event_worker.rb rename ee/spec/lib/{ => ee}/gitlab/event_store_spec.rb (81%) create mode 100644 ee/spec/services/merge_requests/stream_approval_audit_event_service_spec.rb create mode 100644 ee/spec/workers/merge_requests/stream_approval_audit_event_worker_spec.rb diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index f26513703055a8..be6ab9050d9309 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -25,9 +25,9 @@ def execute(merge_request) ) else create_event(merge_request) + stream_audit_event(merge_request) end - stream_audit_event(merge_request) create_approval_note(merge_request) mark_pending_todos_as_done(merge_request) execute_approval_hooks(merge_request, current_user) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index b4e2071668e1eb..9c7afc9f1cfca4 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -285,6 +285,8 @@ - 1 - - merge_requests_resolve_todos - 1 +- - merge_requests_stream_approval_audit_event + - 1 - - merge_requests_sync_code_owner_approval_rules - 1 - - merge_requests_update_head_pipeline diff --git a/ee/app/services/merge_requests/stream_approval_audit_event_service.rb b/ee/app/services/merge_requests/stream_approval_audit_event_service.rb new file mode 100644 index 00000000000000..0a62dd6ea3542d --- /dev/null +++ b/ee/app/services/merge_requests/stream_approval_audit_event_service.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module MergeRequests + class StreamApprovalAuditEventService < MergeRequests::BaseService + def execute(merge_request) + audit_context = { + name: 'merge_request_approval_operation', + stream_only: true, + author: current_user, + scope: merge_request.project, + target: merge_request, + message: 'Approved merge request' + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end +end diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 63a41c6ae6ebbe..23230960e30deb 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -1236,6 +1236,15 @@ :weight: 1 :idempotent: false :tags: [] +- :name: merge_requests_stream_approval_audit_event + :worker_name: MergeRequests::StreamApprovalAuditEventWorker + :feature_category: :code_review + :has_external_dependencies: true + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: merge_requests_sync_code_owner_approval_rules :worker_name: MergeRequests::SyncCodeOwnerApprovalRulesWorker :feature_category: :source_code_management diff --git a/ee/app/workers/merge_requests/stream_approval_audit_event_worker.rb b/ee/app/workers/merge_requests/stream_approval_audit_event_worker.rb new file mode 100644 index 00000000000000..0576ca3100e0f9 --- /dev/null +++ b/ee/app/workers/merge_requests/stream_approval_audit_event_worker.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module MergeRequests + class StreamApprovalAuditEventWorker + include Gitlab::EventStore::Subscriber + + data_consistency :always + feature_category :code_review + urgency :low + idempotent! + + # MergeRequests:StreamApprovalAuditEventService makes an external HTTP request + worker_has_external_dependencies! + + def handle_event(event) + current_user_id = event.data[:current_user_id] + merge_request_id = event.data[:merge_request_id] + current_user = User.find_by_id(current_user_id) + + unless current_user + logger.info(structured_payload(message: 'Current user not found.', current_user_id: current_user_id)) + return + end + + merge_request = MergeRequest.find_by_id(merge_request_id) + + unless merge_request + logger.info(structured_payload(message: 'Merge request not found.', merge_request_id: merge_request_id)) + return + end + + ::MergeRequests::StreamApprovalAuditEventService + .new(project: merge_request.project, current_user: current_user) + .execute(merge_request) + end + end +end diff --git a/ee/lib/ee/gitlab/event_store.rb b/ee/lib/ee/gitlab/event_store.rb index 47150fd0390734..42f500d8fa3c80 100644 --- a/ee/lib/ee/gitlab/event_store.rb +++ b/ee/lib/ee/gitlab/event_store.rb @@ -28,6 +28,7 @@ def configure!(store) store.subscribe ::Geo::CreateRepositoryUpdatedEventWorker, to: ::Repositories::KeepAroundRefsCreatedEvent, if: -> (_) { ::Gitlab::Geo.primary? } + store.subscribe ::MergeRequests::StreamApprovalAuditEventWorker, to: ::MergeRequests::ApprovedEvent end end end diff --git a/ee/spec/lib/gitlab/event_store_spec.rb b/ee/spec/lib/ee/gitlab/event_store_spec.rb similarity index 81% rename from ee/spec/lib/gitlab/event_store_spec.rb rename to ee/spec/lib/ee/gitlab/event_store_spec.rb index 480c08354404b9..dfff3a3bb5b312 100644 --- a/ee/spec/lib/gitlab/event_store_spec.rb +++ b/ee/spec/lib/ee/gitlab/event_store_spec.rb @@ -11,7 +11,8 @@ ::Ci::JobArtifactsDeletedEvent, ::Ci::PipelineCreatedEvent, ::Members::MembersAddedEvent, - ::Repositories::KeepAroundRefsCreatedEvent + ::Repositories::KeepAroundRefsCreatedEvent, + ::MergeRequests::ApprovedEvent ) end end diff --git a/ee/spec/services/merge_requests/approval_service_spec.rb b/ee/spec/services/merge_requests/approval_service_spec.rb index f4959961357104..b6a12a7889bf24 100644 --- a/ee/spec/services/merge_requests/approval_service_spec.rb +++ b/ee/spec/services/merge_requests/approval_service_spec.rb @@ -50,20 +50,6 @@ expect(todo.reload).to be_done end - it 'sends the audit streaming event' do - audit_context = { - name: 'merge_request_approval_operation', - stream_only: true, - author: user, - scope: merge_request.project, - target: merge_request, - message: 'Approved merge request' - } - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) - - service.execute(merge_request) - end - context 'with remaining approvals' do it 'fires an approval webhook' do expect(merge_request).to receive(:approvals_left).and_return(5) @@ -116,6 +102,20 @@ service.execute(merge_request) end + it 'sends the audit streaming event' do + audit_context = { + name: 'merge_request_approval_operation', + stream_only: true, + author: user, + scope: merge_request.project, + target: merge_request, + message: 'Approved merge request' + } + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) + + service.execute(merge_request) + end + context 'approvals metrics calculation' do context 'when code_review_analytics project feature is available' do before do diff --git a/ee/spec/services/merge_requests/stream_approval_audit_event_service_spec.rb b/ee/spec/services/merge_requests/stream_approval_audit_event_service_spec.rb new file mode 100644 index 00000000000000..58523183e1e6bc --- /dev/null +++ b/ee/spec/services/merge_requests/stream_approval_audit_event_service_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::StreamApprovalAuditEventService do + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.project } + + subject(:service) { described_class.new(project: project, current_user: user) } + + describe '#execute' do + it 'sends the audit streaming event' do + audit_context = { + name: 'merge_request_approval_operation', + stream_only: true, + author: user, + scope: merge_request.project, + target: merge_request, + message: 'Approved merge request' + } + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context).and_call_original + + service.execute(merge_request) + end + end +end diff --git a/ee/spec/workers/merge_requests/stream_approval_audit_event_worker_spec.rb b/ee/spec/workers/merge_requests/stream_approval_audit_event_worker_spec.rb new file mode 100644 index 00000000000000..645dd85de8ae8a --- /dev/null +++ b/ee/spec/workers/merge_requests/stream_approval_audit_event_worker_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::StreamApprovalAuditEventWorker do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + + let(:data) { { current_user_id: user.id, merge_request_id: merge_request.id } } + let(:approved_event) { MergeRequests::ApprovedEvent.new(data: data) } + + it_behaves_like 'subscribes to event' do + let(:event) { approved_event } + end + + it 'calls MergeRequests::SteamApprovalAuditEventService' do + expect_next_instance_of( + MergeRequests::StreamApprovalAuditEventService, + project: project, current_user: user + ) do |service| + expect(service).to receive(:execute).with(merge_request) + end + + consume_event(subscriber: described_class, event: approved_event) + end + + shared_examples 'when object does not exist' do + it 'logs and does not call MergeRequests::SteamApprovalAuditEventService' do + expect(Sidekiq.logger).to receive(:info).with(hash_including(log_payload)) + expect(MergeRequests::StreamApprovalAuditEventService).not_to receive(:new) + + expect { consume_event(subscriber: described_class, event: approved_event) } + .not_to raise_exception + end + end + + context 'when the user does not exist' do + before do + user.destroy! + end + + it_behaves_like 'when object does not exist' do + let(:log_payload) { { 'message' => 'Current user not found.', 'current_user_id' => user.id } } + end + end + + context 'when the merge request does not exist' do + before do + merge_request.destroy! + end + + it_behaves_like 'when object does not exist' do + let(:log_payload) { { 'message' => 'Merge request not found.', 'merge_request_id' => merge_request.id } } + end + end +end -- GitLab