Merge branch '345165-add-disabled-predicate-methods-to-WebHook' into 'master'

Add predicates to WebHook for disabled states

See merge request gitlab-org/gitlab!75175

app/models/hooks/project_hook.rb

@@ -31,10 +31,6 @@ class ProjectHook < WebHook
     _('Webhooks')
   end
 
-  def web_hooks_disable_failed?
-    Feature.enabled?(:web_hooks_disable_failed, project)
-  end
-
   override :rate_limit
   def rate_limit
     project.actual_limits.limit_for(:web_hook_calls)
@@ -44,6 +40,13 @@ class ProjectHook < WebHook
   def application_context
     super.merge(project: project)
   end
+
+  private
+
+  override :web_hooks_disable_failed?
+  def web_hooks_disable_failed?
+    Feature.enabled?(:web_hooks_disable_failed, project)
+  end
 end
 
 ProjectHook.prepend_mod_with('ProjectHook')

app/models/hooks/web_hook.rb

@@ -34,9 +34,19 @@ class WebHook < ApplicationRecord
   end
 
   def executable?
-    return true unless web_hooks_disable_failed?
+    !temporarily_disabled? && !permanently_disabled?
+  end
+
+  def temporarily_disabled?
+    return false unless web_hooks_disable_failed?
+
+    disabled_until.present? && disabled_until >= Time.current
+  end
+
+  def permanently_disabled?
+    return false unless web_hooks_disable_failed?
 
-    recent_failures <= FAILURE_THRESHOLD && (disabled_until.nil? || disabled_until < Time.current)
+    recent_failures > FAILURE_THRESHOLD
   end
 
   # rubocop: disable CodeReuse/ServiceClass
@@ -69,6 +79,8 @@ class WebHook < ApplicationRecord
   end
 
   def disable!
+    return if permanently_disabled?
+
     update_attribute(:recent_failures, FAILURE_THRESHOLD + 1)
   end
 
@@ -80,7 +92,7 @@ class WebHook < ApplicationRecord
   end
 
   def backoff!
-    return if backoff_count >= MAX_FAILURES && disabled_until && disabled_until > Time.current
+    return if permanently_disabled? || (backoff_count >= MAX_FAILURES && temporarily_disabled?)
 
     assign_attributes(disabled_until: next_backoff.from_now, backoff_count: backoff_count.succ.clamp(0, MAX_FAILURES))
     save(validate: false)
@@ -93,7 +105,19 @@ class WebHook < ApplicationRecord
     save(validate: false)
   end
 
-  # Overridden in ProjectHook and GroupHook, other webhooks are not rate-limited.
+  # @return [Boolean] Whether or not the WebHook is currently throttled.
+  def rate_limited?
+    return false unless rate_limit
+
+    Gitlab::ApplicationRateLimiter.peek(
+      :web_hook_calls,
+      scope: [self],
+      threshold: rate_limit
+    )
+  end
+
+  # Threshold for the rate-limit.
+  # Overridden in ProjectHook and GroupHook, other WebHooks are not rate-limited.
   def rate_limit
     nil
   end

ee/app/models/hooks/group_hook.rb

@@ -34,8 +34,9 @@ class GroupHook < WebHook
     _('Group Hooks')
   end
 
-  def web_hooks_disable_failed?
-    Feature.enabled?(:web_hooks_disable_failed, group)
+  override :application_context
+  def application_context
+    super.merge(namespace: group)
   end
 
   override :rate_limit
@@ -43,8 +44,10 @@ class GroupHook < WebHook
     group.actual_limits.limit_for(:web_hook_calls)
   end
 
-  override :application_context
-  def application_context
-    super.merge(namespace: group)
+  private
+
+  override :web_hooks_disable_failed?
+  def web_hooks_disable_failed?
+    Feature.enabled?(:web_hooks_disable_failed, group)
   end
 end

spec/models/hooks/web_hook_spec.rb

@@ -330,6 +330,20 @@ RSpec.describe WebHook do
       expect { hook.backoff! }.to change(hook, :backoff_count).by(1)
     end
 
+    context 'when the hook is permanently disabled' do
+      before do
+        allow(hook).to receive(:permanently_disabled?).and_return(true)
+      end
+
+      it 'does not set disabled_until' do
+        expect { hook.backoff! }.not_to change(hook, :disabled_until)
+      end
+
+      it 'does not increment the backoff count' do
+        expect { hook.backoff! }.not_to change(hook, :backoff_count)
+      end
+    end
+
     context 'when we have backed off MAX_FAILURES times' do
       before do
         stub_const("#{described_class}::MAX_FAILURES", 5)
@@ -392,4 +406,77 @@ RSpec.describe WebHook do
       end
     end
   end
+
+  describe '#temporarily_disabled?' do
+    it 'is false when not temporarily disabled' do
+      expect(hook).not_to be_temporarily_disabled
+    end
+
+    context 'when hook has been told to back off' do
+      before do
+        hook.backoff!
+      end
+
+      it 'is true' do
+        expect(hook).to be_temporarily_disabled
+      end
+
+      it 'is false when `web_hooks_disable_failed` flag is disabled' do
+        stub_feature_flags(web_hooks_disable_failed: false)
+
+        expect(hook).not_to be_temporarily_disabled
+      end
+    end
+  end
+
+  describe '#permanently_disabled?' do
+    it 'is false when not disabled' do
+      expect(hook).not_to be_permanently_disabled
+    end
+
+    context 'when hook has been disabled' do
+      before do
+        hook.disable!
+      end
+
+      it 'is true' do
+        expect(hook).to be_permanently_disabled
+      end
+
+      it 'is false when `web_hooks_disable_failed` flag is disabled' do
+        stub_feature_flags(web_hooks_disable_failed: false)
+
+        expect(hook).not_to be_permanently_disabled
+      end
+    end
+  end
+
+  describe '#rate_limited?' do
+    context 'when there are rate limits' do
+      before do
+        allow(hook).to receive(:rate_limit).and_return(3)
+      end
+
+      it 'is false when hook has not been rate limited' do
+        expect(Gitlab::ApplicationRateLimiter).to receive(:peek).and_return(false)
+        expect(hook).not_to be_rate_limited
+      end
+
+      it 'is true when hook has been rate limited' do
+        expect(Gitlab::ApplicationRateLimiter).to receive(:peek).and_return(true)
+        expect(hook).to be_rate_limited
+      end
+    end
+
+    context 'when there are no rate limits' do
+      before do
+        allow(hook).to receive(:rate_limit).and_return(nil)
+      end
+
+      it 'does not call Gitlab::ApplicationRateLimiter, and is false' do
+        expect(Gitlab::ApplicationRateLimiter).not_to receive(:peek)
+        expect(hook).not_to be_rate_limited
+      end
+    end
+  end
 end