Use hook relay for web_hooks/fire action (#5011)

app/avo/resources/web_hook_resource.rb

@@ -19,7 +19,7 @@ class GlobalFilter < ScopeBooleanFilter; end
 
   field :hook_relay_stream, as: :text do
     stream_name = "webhook_id-#{model.id}"
-    link_to stream_name, "https://app.hookrelay.dev/hooks/#{ENV['HOOK_RELAY_STREAM_ID']}?started_at=P1W&stream=#{stream_name}"
+    link_to stream_name, "https://app.hookrelay.dev/hooks/#{ENV['HOOK_RELAY_HOOK_ID']}?started_at=P1W&stream=#{stream_name}"
   end
 
   field :disabled_reason, as: :text

app/controllers/api/v1/web_hooks_controller.rb

@@ -35,11 +35,13 @@ def fire
 
     authorize webhook
 
-    if webhook.fire(request.protocol.delete("://"), request.host_with_port,
-                    @rubygem.most_recent_version, delayed: false)
-      render plain: webhook.deployed_message(@rubygem)
+    response = webhook.fire(request.protocol.delete("://"), request.host_with_port,
+                            @rubygem.most_recent_version, delayed: false)
+
+    if response.fetch("status") == "success"
+      render plain: webhook.deployed_message(@rubygem) + hook_relay_message(response)
     else
-      render_bad_request webhook.failed_message(@rubygem)
+      render_bad_request webhook.failed_message(@rubygem) + hook_relay_message(response)
     end
   end
 
@@ -55,4 +57,21 @@ def set_url
     render_bad_request "URL was not provided" unless params[:url]
     @url = params[:url]
   end
+
+  def hook_relay_message(response)
+    status = response.fetch("status")
+    msg = +""
+    msg << "\nFailed with status #{status.inspect}: #{response['failure_reason']}" if status != "success"
+    if response.key?("responses") && response["responses"].any?
+      r = response.dig("responses", -1)
+      msg << "\nError: #{r['error']}" if r["error"]
+      msg << "\n\nResponse: #{r['code']}"
+      r.fetch("headers", []).each do |k, v|
+        msg << "\n#{k}: #{v}"
+      end
+      msg << "\n\n#{r['body']}"
+    end
+
+    msg
+  end
 end

app/jobs/notify_web_hook_job.rb

@@ -13,6 +13,15 @@ class NotifyWebHookJob < ApplicationJob
   before_perform { @host_with_port = @kwargs.fetch(:host_with_port) }
   before_perform { @version = @kwargs.fetch(:version) }
   before_perform { @rubygem = @version.rubygem }
+  before_perform { @poll_delivery = @kwargs.fetch(:poll_delivery, false) }
+  before_perform do
+    @http = Faraday.new("https://api.hookrelay.dev", request: { timeout: TIMEOUT_SEC }) do |f|
+      f.request :json
+      f.response :logger, logger, headers: false, errors: true
+      f.response :json
+      f.response :raise_error
+    end
+  end
 
   attr_reader :webhook, :protocol, :host_with_port, :version, :rubygem
 
@@ -21,7 +30,6 @@ class NotifyWebHookJob < ApplicationJob
 
   # has to come after the retry on
   discard_on(Faraday::UnprocessableEntityError) do |j, e|
-    raise unless j.use_hook_relay?
     logger.info({ webhook_id: j.webhook.id, url: j.webhook.url, response: JSON.parse(e.response_body) })
     j.webhook.increment! :failure_count
   end
@@ -31,11 +39,7 @@ def perform(*)
     set_tag "gemcutter.notifier.url", url
     set_tag "gemcutter.notifier.webhook_id", webhook.id
 
-    if use_hook_relay?
-      post_hook_relay
-    else
-      post_directly
-    end
+    post_hook_relay
   end
   statsd_count_success :perform, "Webhook.perform"
 
@@ -48,44 +52,53 @@ def authorization
   end
 
   def hook_relay_url
-    "https://api.hookrelay.dev/hooks/#{ENV['HOOK_RELAY_ACCOUNT_ID']}/#{ENV['HOOK_RELAY_HOOK_ID']}/webhook_id-#{webhook.id}"
-  end
-
-  def use_hook_relay?
-    # can't use hook relay for `perform_now` (aka an unenqueued job)
-    # because then we won't actually hit the webhook URL synchronously
-    enqueued_at.present? && ENV["HOOK_RELAY_ACCOUNT_ID"].present? && ENV["HOOK_RELAY_HOOK_ID"].present?
+    "https://api.hookrelay.dev/hooks/#{account_id}/#{hook_id}/webhook_id-#{webhook.id || 'fire'}"
   end
 
   def post_hook_relay
     response = post(hook_relay_url)
-    delivery_id = JSON.parse(response.body).fetch("id")
+    delivery_id = response.body.fetch("id")
     Rails.logger.info do
       { webhook_id: webhook.id, url: webhook.url, delivery_id:, full_name: version.full_name, message: "Sent webhook to HookRelay" }
     end
+    return poll_delivery(delivery_id) if @poll_delivery
     true
   end
 
-  def post_directly
-    post(webhook.url)
-    true
-  rescue *ERRORS
-    webhook.increment! :failure_count
-    false
-  end
-
   def post(url)
-    Faraday.new(nil, request: { timeout: TIMEOUT_SEC }) do |f|
-      f.request :json
-      f.response :logger, logger, headers: false, errors: true
-      f.response :raise_error
-    end.post(
+    @http.post(
       url, payload,
       {
         "Authorization"   => authorization,
         "HR_TARGET_URL"   => webhook.url,
-        "HR_MAX_ATTEMPTS" => "3"
+        "HR_MAX_ATTEMPTS" => @poll_delivery ? "1" : "3"
       }
     )
   end
+
+  def poll_delivery(delivery_id)
+    deadline = (Rails.env.test? ? 0.01 : 10).seconds.from_now
+    response = nil
+    until Time.zone.now > deadline
+      sleep 0.5
+      response = @http.get("https://app.hookrelay.dev/api/v1/accounts/#{account_id}/hooks/#{hook_id}/deliveries/#{delivery_id}", nil, {
+                             "Authorization" => "Bearer #{ENV['HOOK_RELAY_API_KEY']}"
+                           })
+      status = response.body.fetch("status")
+
+      break if status == "success"
+    end
+
+    response.body || raise("Failed to get delivery status after 10 seconds")
+  end
+
+  private
+
+  def account_id
+    ENV["HOOK_RELAY_ACCOUNT_ID"]
+  end
+
+  def hook_id
+    ENV["HOOK_RELAY_HOOK_ID"]
+  end
 end

app/models/web_hook.rb

@@ -21,12 +21,12 @@ class WebHook < ApplicationRecord
   scope :disabled, -> { where.not(disabled_at: nil) }
 
   def fire(protocol, host_with_port, version, delayed: true)
-    job = NotifyWebHookJob.new(webhook: self, protocol:, host_with_port:, version:)
+    job = NotifyWebHookJob.new(webhook: self, protocol:, host_with_port:, version:, poll_delivery: !delayed)
 
     if delayed
       job.enqueue
     else
-      job.perform_now
+      job.send(:_perform_job)
     end
   end
 

script/dev

@@ -6,6 +6,7 @@ export AVO_LICENSE_KEY="op://bq25xfqpafelzdxwqu3mdq6rza/netp2leghd7zqdxlkp5asy67
 export DATADOG_CSP_API_KEY="op://bq25xfqpafelzdxwqu3mdq6rza/eisktguwm3pcfkwm7avqamdnxq/credential"
 export HOOK_RELAY_ACCOUNT_ID="op://bq25xfqpafelzdxwqu3mdq6rza/s3fs2zznjssijxouugddmwnuma/username"
 export HOOK_RELAY_HOOK_ID="op://bq25xfqpafelzdxwqu3mdq6rza/s3fs2zznjssijxouugddmwnuma/credential"
+export HOOK_RELAY_API_KEY="op://bq25xfqpafelzdxwqu3mdq6rza/s3fs2zznjssijxouugddmwnuma/API Key"
 export LAUNCH_DARKLY_SDK_KEY="op://bq25xfqpafelzdxwqu3mdq6rza/3o73k6fcenzrbspwcm4g4j5z2a/credential"
 
-exec op run --account 5MS5DHTO5NH7BAHTSIGT5NOAIE -- "${@}"
+exec op run --no-masking --account 5MS5DHTO5NH7BAHTSIGT5NOAIE -- "${@}"

test/functional/api/v1/web_hooks_controller_test.rb

@@ -9,6 +9,10 @@ def self.should_not_find_it
     end
   end
 
+  setup do
+    NotifyWebHookJob.any_instance.stubs(:sleep)
+  end
+
   context "with incorrect api key" do
     context "no api key" do
       should "forbid access when creating a web hook" do
@@ -83,7 +87,18 @@ def self.should_respond_to(format)
 
       context "On POST to fire for all gems" do
         setup do
-          stub_request(:post, @url)
+          stub_request(:post, "https://api.hookrelay.dev/hooks///webhook_id-fire")
+            .with(headers: {
+                    "Content-Type" => "application/json",
+                "HR_TARGET_URL" => @url,
+                "HR_MAX_ATTEMPTS" => "1"
+                  }).to_return(status: 200, body: '{"id":"delivery-id"}', headers: { "Content-Type" => "application/json" })
+
+          stub_request(:get, "https://app.hookrelay.dev/api/v1/accounts//hooks//deliveries/delivery-id")
+            .to_return(status: 200, body: { "status" => "success", "responses" => [
+              { "code" => 200, "body" => "OK", "headers" => { "Content-Type" => "text/plain" } }
+            ] }.to_json, headers: { "Content-Type" => "application/json" })
+
           post :fire, params: { gem_name: WebHook::GLOBAL_PATTERN,
                                 url: @url }
         end
@@ -98,7 +113,17 @@ def self.should_respond_to(format)
 
       context "On POST to fire for all gems that fails" do
         setup do
-          stub_request(:post, @url).to_timeout
+          stub_request(:post, "https://api.hookrelay.dev/hooks///webhook_id-fire")
+            .with(headers: {
+                    "Content-Type" => "application/json",
+                "HR_TARGET_URL" => @url,
+                "HR_MAX_ATTEMPTS" => "1"
+                  }).to_return(status: 200, body: '{"id":"delivery-id"}', headers: { "Content-Type" => "application/json" })
+
+          stub_request(:get, "https://app.hookrelay.dev/api/v1/accounts//hooks//deliveries/delivery-id")
+            .to_return(status: 200, body: { "status" => "failure",
+"failure_reason" => "timed out" }.to_json, headers: { "Content-Type" => "application/json" })
+
           post :fire, params: { gem_name: WebHook::GLOBAL_PATTERN,
                                 url: @url }
         end
@@ -132,7 +157,18 @@ def self.should_respond_to(format)
 
       context "On POST to fire for a specific gem" do
         setup do
-          stub_request(:post, @url)
+          stub_request(:post, "https://api.hookrelay.dev/hooks///webhook_id-fire")
+            .with(headers: {
+                    "Content-Type" => "application/json",
+                "HR_TARGET_URL" => @url,
+                "HR_MAX_ATTEMPTS" => "1"
+                  }).to_return(status: 200, body: '{"id":"delivery-id"}', headers: { "Content-Type" => "application/json" })
+
+          stub_request(:get, "https://app.hookrelay.dev/api/v1/accounts//hooks//deliveries/delivery-id")
+            .to_return(status: 200, body: { "status" => "success", "responses" => [
+              { "code" => 200, "body" => "OK", "headers" => { "Content-Type" => "text/plain" } }
+            ] }.to_json, headers: { "Content-Type" => "application/json" })
+
           post :fire, params: { gem_name: @rubygem.name,
                                 url: @url }
         end
@@ -145,7 +181,17 @@ def self.should_respond_to(format)
 
       context "On POST to fire for a specific gem that fails" do
         setup do
-          stub_request(:post, @url).to_return(status: 404)
+          stub_request(:post, "https://api.hookrelay.dev/hooks///webhook_id-fire")
+            .with(headers: {
+                    "Content-Type" => "application/json",
+                "HR_TARGET_URL" => @url,
+                "HR_MAX_ATTEMPTS" => "1"
+                  }).to_return(status: 200, body: '{"id":"delivery-id"}', headers: { "Content-Type" => "application/json" })
+
+          stub_request(:get, "https://app.hookrelay.dev/api/v1/accounts//hooks//deliveries/delivery-id")
+            .to_return(status: 200, body: { "status" => "failure",
+"failure_reason" => "exceeded", "responses" => [{ "code" => 404 }] }.to_json, headers: { "Content-Type" => "application/json" })
+
           post :fire, params: { gem_name: @rubygem.name,
                                 url: @url }
         end

test/jobs/notify_web_hook_job_test.rb

@@ -46,30 +46,12 @@ class NotifyWebHookJobTest < ActiveJob::TestCase
     end
 
     should "succeed with hook relay" do
-      NotifyWebHookJob.any_instance.expects(:use_hook_relay?).once.returns(true)
       stub_request(:post, "https://api.hookrelay.dev/hooks///webhook_id-#{@hook.id}")
         .with(headers: {
                 "Content-Type" => "application/json",
                 "HR_TARGET_URL" => @hook.url,
                 "HR_MAX_ATTEMPTS" => "3"
-              }).to_return(status: 200, body: { id: 12_345 }.to_json)
-
-      perform_enqueued_jobs do
-        @job.enqueue
-      end
-
-      assert_performed_jobs 1, only: NotifyWebHookJob
-      assert_enqueued_jobs 0, only: NotifyWebHookJob
-    end
-
-    should "succeed without hook relay" do
-      NotifyWebHookJob.any_instance.expects(:use_hook_relay?).once.returns(false)
-      stub_request(:post, @hook.url)
-        .with(headers: {
-                "Content-Type" => "application/json",
-                "HR_TARGET_URL" => @hook.url,
-                "HR_MAX_ATTEMPTS" => "3"
-              }).to_return(status: 200, body: "")
+              }).to_return_json(status: 200, body: { id: 12_345 })
 
       perform_enqueued_jobs do
         @job.enqueue
@@ -87,30 +69,12 @@ class NotifyWebHookJobTest < ActiveJob::TestCase
     end
 
     should "discard the job on a 422 with hook relay" do
-      NotifyWebHookJob.any_instance.expects(:use_hook_relay?).twice.returns(true)
       stub_request(:post, "https://api.hookrelay.dev/hooks///webhook_id-#{@hook.id}")
         .with(headers: {
                 "Content-Type" => "application/json",
                 "HR_TARGET_URL" => @hook.url,
                 "HR_MAX_ATTEMPTS" => "3"
-              }).to_return(status: 422, body: { error: "Invalid url" }.to_json)
-
-      perform_enqueued_jobs do
-        @job.enqueue
-      end
-
-      assert_performed_jobs 1, only: NotifyWebHookJob
-      assert_enqueued_jobs 0, only: NotifyWebHookJob
-    end
-
-    should "finish the job on a 422 without hook relay" do
-      NotifyWebHookJob.any_instance.expects(:use_hook_relay?).once.returns(false)
-      stub_request(:post, @hook.url)
-        .with(headers: {
-                "Content-Type" => "application/json",
-                "HR_TARGET_URL" => @hook.url,
-                "HR_MAX_ATTEMPTS" => "3"
-              }).to_return(status: 422, body: "")
+              }).to_return_json(status: 422, body: { error: "Invalid url" })
 
       perform_enqueued_jobs do
         @job.enqueue