Commit 19db65f7 authored by Alex Kalderimis's avatar Alex Kalderimis

Call executable scope in hooks_for

Also: clamp the next backoff value to `(10.minutes, 1.day)`.

This requires 8 doublings to reach the maximum cooldown.
parent fb89cccf
...@@ -29,7 +29,7 @@ module TriggerableHooks ...@@ -29,7 +29,7 @@ module TriggerableHooks
callable_scopes = triggers.keys + [:all] callable_scopes = triggers.keys + [:all]
return none unless callable_scopes.include?(trigger) return none unless callable_scopes.include?(trigger)
public_send(trigger) # rubocop:disable GitlabSecurity/PublicSend executable.public_send(trigger) # rubocop:disable GitlabSecurity/PublicSend
end end
def select_active(hooks_scope, data) def select_active(hooks_scope, data)
......
...@@ -5,6 +5,7 @@ class WebHook < ApplicationRecord ...@@ -5,6 +5,7 @@ class WebHook < ApplicationRecord
FAILURE_THRESHOLD = 3 # three strikes FAILURE_THRESHOLD = 3 # three strikes
INITIAL_BACKOFF = 10.minutes INITIAL_BACKOFF = 10.minutes
MAX_BACKOFF = 1.day
BACKOFF_GROWTH_FACTOR = 2.0 BACKOFF_GROWTH_FACTOR = 2.0
attr_encrypted :token, attr_encrypted :token,
...@@ -59,7 +60,11 @@ class WebHook < ApplicationRecord ...@@ -59,7 +60,11 @@ class WebHook < ApplicationRecord
end end
def next_backoff def next_backoff
INITIAL_BACKOFF * (BACKOFF_GROWTH_FACTOR**backoff_count) return MAX_BACKOFF if backoff_count >= 8 # optimization to prevent expensive exponentiation and possible overflows
(INITIAL_BACKOFF * (BACKOFF_GROWTH_FACTOR**backoff_count))
.clamp(INITIAL_BACKOFF, MAX_BACKOFF)
.seconds
end end
def disable! def disable!
......
...@@ -461,7 +461,7 @@ module EE ...@@ -461,7 +461,7 @@ module EE
return unless feature_available?(:group_webhooks) return unless feature_available?(:group_webhooks)
self_and_ancestor_hooks = GroupHook.where(group_id: self_and_ancestors) self_and_ancestor_hooks = GroupHook.where(group_id: self_and_ancestors)
self_and_ancestor_hooks.executable.hooks_for(hooks_scope).each do |hook| self_and_ancestor_hooks.hooks_for(hooks_scope).each do |hook|
hook.async_execute(data, hooks_scope.to_s) hook.async_execute(data, hooks_scope.to_s)
end end
end end
......
...@@ -255,6 +255,16 @@ RSpec.describe WebHook do ...@@ -255,6 +255,16 @@ RSpec.describe WebHook do
expect(hook.next_backoff).to eq(80.minutes) expect(hook.next_backoff).to eq(80.minutes)
end end
end end
context 'when the previous backoff was large' do
before do
hook.backoff_count = 8 # last value before MAX_BACKOFF
end
it 'does not exceed the max backoff value' do
expect(hook.next_backoff).to eq(described_class::MAX_BACKOFF)
end
end
end end
describe '#enable!' do describe '#enable!' do
......
...@@ -202,6 +202,16 @@ RSpec.describe WebHookService do ...@@ -202,6 +202,16 @@ RSpec.describe WebHookService do
it 'does not change the disabled_until attribute' do it 'does not change the disabled_until attribute' do
expect { service_instance.execute }.not_to change(project_hook, :disabled_until) expect { service_instance.execute }.not_to change(project_hook, :disabled_until)
end end
context 'when the hook had previously failed' do
before do
project_hook.update!(recent_failures: 2)
end
it 'resets the failure count' do
expect { service_instance.execute }.to change(project_hook, :recent_failures).to(0)
end
end
end end
context 'with bad request' do context 'with bad request' do
...@@ -261,6 +271,34 @@ RSpec.describe WebHookService do ...@@ -261,6 +271,34 @@ RSpec.describe WebHookService do
it 'increases the backoff count' do it 'increases the backoff count' do
expect { service_instance.execute }.to change(project_hook, :backoff_count).by(1) expect { service_instance.execute }.to change(project_hook, :backoff_count).by(1)
end end
context 'when the previous cool-off was near the maximum' do
before do
project_hook.update!(disabled_until: 5.minutes.ago, backoff_count: 8)
end
it 'sets the disabled_until attribute' do
expect { service_instance.execute }.to change(project_hook, :disabled_until).to(1.day.from_now)
end
it 'sets the last_backoff attribute' do
expect { service_instance.execute }.to change(project_hook, :backoff_count).by(1)
end
end
context 'when we have backed-off many many times' do
before do
project_hook.update!(disabled_until: 5.minutes.ago, backoff_count: 365)
end
it 'sets the disabled_until attribute' do
expect { service_instance.execute }.to change(project_hook, :disabled_until).to(1.day.from_now)
end
it 'sets the last_backoff attribute' do
expect { service_instance.execute }.to change(project_hook, :backoff_count).by(1)
end
end
end end
context 'with unsafe response body' do context 'with unsafe response body' 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