From 6ed955e19248c5332be4ade44dc56ce0f4f5fd59 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Wed, 19 May 2021 19:08:04 +0200 Subject: [PATCH] Set current project/group in WebHookWorker context Currently this isn't always reliably detected, depending on how the worker is triggered. By setting this explicitly we always see the related project and namespace in Kibana, and also the subscription plan in EE. This also sets the `related_class` field to the webhook type, which is useful for troubleshooting purposes. --- app/models/hooks/project_hook.rb | 5 +++ app/models/hooks/web_hook.rb | 5 +++ app/services/web_hook_service.rb | 4 ++- ee/app/models/hooks/group_hook.rb | 5 +++ ee/spec/models/hooks/group_hook_spec.rb | 11 +++++++ ee/spec/services/web_hook_service_spec.rb | 38 +++++++++++++++++++++++ spec/models/hooks/project_hook_spec.rb | 11 +++++++ spec/models/hooks/service_hook_spec.rb | 10 ++++++ spec/models/hooks/system_hook_spec.rb | 10 ++++++ spec/services/web_hook_service_spec.rb | 14 +++++++++ 10 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 ee/spec/services/web_hook_service_spec.rb diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb index a28b97e63e5ef1..d1584a62bfb71b 100644 --- a/app/models/hooks/project_hook.rb +++ b/app/models/hooks/project_hook.rb @@ -39,6 +39,11 @@ def web_hooks_disable_failed? def rate_limit project.actual_limits.limit_for(:web_hook_calls) end + + override :application_context + def application_context + super.merge(project: project) + end end ProjectHook.prepend_mod_with('ProjectHook') diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 02b4feb4ccc277..f0c3791079a799 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -80,6 +80,11 @@ def rate_limit nil end + # Custom attributes to be included in the worker context. + def application_context + { related_class: type } + end + private def web_hooks_disable_failed? diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 654d9356739783..43eda7481080a2 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -94,7 +94,9 @@ def async_execute if rate_limited?(hook) log_rate_limit(hook) else - WebHookWorker.perform_async(hook.id, data, hook_name) + Gitlab::ApplicationContext.with_context(hook.application_context) do + WebHookWorker.perform_async(hook.id, data, hook_name) + end end end diff --git a/ee/app/models/hooks/group_hook.rb b/ee/app/models/hooks/group_hook.rb index 5f0d4b0d050800..c3ce2629709a8e 100644 --- a/ee/app/models/hooks/group_hook.rb +++ b/ee/app/models/hooks/group_hook.rb @@ -42,4 +42,9 @@ def web_hooks_disable_failed? def rate_limit group.actual_limits.limit_for(:web_hook_calls) end + + override :application_context + def application_context + super.merge(namespace: group) + end end diff --git a/ee/spec/models/hooks/group_hook_spec.rb b/ee/spec/models/hooks/group_hook_spec.rb index 374b526407a43a..9e5f994d66f648 100644 --- a/ee/spec/models/hooks/group_hook_spec.rb +++ b/ee/spec/models/hooks/group_hook_spec.rb @@ -29,4 +29,15 @@ expect(hook_ultimate.rate_limit).to be(500) end end + + describe '#application_context' do + let_it_be(:hook) { build(:group_hook) } + + it 'includes the type and group' do + expect(hook.application_context).to eq( + related_class: 'GroupHook', + namespace: hook.group + ) + end + end end diff --git a/ee/spec/services/web_hook_service_spec.rb b/ee/spec/services/web_hook_service_spec.rb new file mode 100644 index 00000000000000..51a3d71b7c3afe --- /dev/null +++ b/ee/spec/services/web_hook_service_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WebHookService do + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:project_hook) { create(:project_hook, project: project) } + + let(:service_instance) { described_class.new(project_hook, {}, :push_hooks) } + + describe '#async_execute' do + context 'when hook has custom context attributes' do + it 'includes the subscription plan in the worker context' do + expect(WebHookWorker).to receive(:perform_async) do + expect(Gitlab::ApplicationContext.current).to include( + 'meta.subscription_plan' => 'default' + ) + end + + service_instance.async_execute + end + + context 'when the rate-limiting feature flag is disabled' do + before do + stub_feature_flags(web_hooks_rate_limit: false) + end + + it 'does not include the subscription plan in the worker context' do + expect(WebHookWorker).to receive(:perform_async) do + expect(Gitlab::ApplicationContext.current).not_to include('meta.subscription_plan') + end + + service_instance.async_execute + end + end + end + end +end diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb index 88149465232139..d811f67d16b4f1 100644 --- a/spec/models/hooks/project_hook_spec.rb +++ b/spec/models/hooks/project_hook_spec.rb @@ -39,4 +39,15 @@ expect(hook.rate_limit).to be(100) end end + + describe '#application_context' do + let_it_be(:hook) { build(:project_hook) } + + it 'includes the type and project' do + expect(hook.application_context).to include( + related_class: 'ProjectHook', + project: hook.project + ) + end + end end diff --git a/spec/models/hooks/service_hook_spec.rb b/spec/models/hooks/service_hook_spec.rb index 651716c3280972..4ce2e729d892b2 100644 --- a/spec/models/hooks/service_hook_spec.rb +++ b/spec/models/hooks/service_hook_spec.rb @@ -30,4 +30,14 @@ expect(hook.rate_limit).to be_nil end end + + describe '#application_context' do + let(:hook) { build(:service_hook) } + + it 'includes the type' do + expect(hook.application_context).to eq( + related_class: 'ServiceHook' + ) + end + end end diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb index a72034f1ac5005..a99263078b39b3 100644 --- a/spec/models/hooks/system_hook_spec.rb +++ b/spec/models/hooks/system_hook_spec.rb @@ -177,4 +177,14 @@ expect(hook.rate_limit).to be_nil end end + + describe '#application_context' do + let(:hook) { build(:system_hook) } + + it 'includes the type' do + expect(hook.application_context).to eq( + related_class: 'SystemHook' + ) + end + end end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index b3fd4e336405bc..5468d05aada8f2 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -430,5 +430,19 @@ def expect_to_rate_limit(hook, threshold:, throttled: false) end end end + + context 'when hook has custom context attributes' do + it 'includes the attributes in the worker context' do + expect(WebHookWorker).to receive(:perform_async) do + expect(Gitlab::ApplicationContext.current).to include( + 'meta.project' => project_hook.project.full_path, + 'meta.root_namespace' => project.root_ancestor.path, + 'meta.related_class' => 'ProjectHook' + ) + end + + service_instance.async_execute + end + end end end -- GitLab