From 6b0c32d540e851b93885d687cafd7d2b552c6ab8 Mon Sep 17 00:00:00 2001 From: Samuel Giddins Date: Wed, 25 Sep 2024 13:09:22 -0700 Subject: [PATCH] Use hook relay for web_hooks/fire action (#5011) --- app/avo/resources/web_hook_resource.rb | 2 +- .../api/v1/web_hooks_controller.rb | 27 ++++++-- app/jobs/notify_web_hook_job.rb | 69 +++++++++++-------- app/models/web_hook.rb | 4 +- script/dev | 3 +- .../api/v1/web_hooks_controller_test.rb | 54 +++++++++++++-- test/jobs/notify_web_hook_job_test.rb | 40 +---------- test/models/web_hook_test.rb | 54 +++++---------- 8 files changed, 139 insertions(+), 114 deletions(-) diff --git a/app/avo/resources/web_hook_resource.rb b/app/avo/resources/web_hook_resource.rb index ee2c22d59f9..93c9e703a45 100644 --- a/app/avo/resources/web_hook_resource.rb +++ b/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 diff --git a/app/controllers/api/v1/web_hooks_controller.rb b/app/controllers/api/v1/web_hooks_controller.rb index 16dd6040d60..77a66eb1fd6 100644 --- a/app/controllers/api/v1/web_hooks_controller.rb +++ b/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 diff --git a/app/jobs/notify_web_hook_job.rb b/app/jobs/notify_web_hook_job.rb index 30543a30147..1f1de9abac0 100644 --- a/app/jobs/notify_web_hook_job.rb +++ b/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 diff --git a/app/models/web_hook.rb b/app/models/web_hook.rb index 21723d0d17d..4dca75e5368 100644 --- a/app/models/web_hook.rb +++ b/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 diff --git a/script/dev b/script/dev index a58371a0a8f..73576e5911e 100755 --- a/script/dev +++ b/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 -- "${@}" diff --git a/test/functional/api/v1/web_hooks_controller_test.rb b/test/functional/api/v1/web_hooks_controller_test.rb index 583c16a3b08..306aed704b3 100644 --- a/test/functional/api/v1/web_hooks_controller_test.rb +++ b/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 diff --git a/test/jobs/notify_web_hook_job_test.rb b/test/jobs/notify_web_hook_job_test.rb index 7ab3aaa48c6..88425ca8012 100644 --- a/test/jobs/notify_web_hook_job_test.rb +++ b/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 diff --git a/test/models/web_hook_test.rb b/test/models/web_hook_test.rb index 87468c90ddf..048eced3c6b 100644 --- a/test/models/web_hook_test.rb +++ b/test/models/web_hook_test.rb @@ -6,6 +6,19 @@ class WebHookTest < ActiveSupport::TestCase should belong_to :user should belong_to(:rubygem).optional(true) + def stub_hook_relay_request(url, webhook_id, authorization, max_attempts = 3) + stub_request(:post, "https://api.hookrelay.dev/hooks///webhook_id-#{webhook_id}") + .with( + headers: { + "Accept" => "*/*", + "Authorization" => authorization, + "Content-Type" => "application/json", + "Hr-Max-Attempts" => max_attempts, + "Hr-Target-Url" => url + }.compact + ) + end + should "be valid for normal hook" do hook = create(:web_hook) @@ -162,7 +175,7 @@ class WebHookTest < ActiveSupport::TestCase should "include an Authorization header" do authorization = Digest::SHA2.hexdigest(@rubygem.name + @version.number + @hook.user.api_key) - stub_request(:post, @url).with(headers: { "Authorization" => authorization }) + stub_hook_relay_request(@url, @hook.id, authorization).to_return_json(status: 200, body: { id: 1 }) perform_enqueued_jobs only: NotifyWebHookJob do @hook.fire("https", "rubygems.org", @version) @@ -172,7 +185,7 @@ class WebHookTest < ActiveSupport::TestCase should "include an Authorization header for a user with no API key" do @hook.user.update(api_key: nil) authorization = Digest::SHA2.hexdigest(@rubygem.name + @version.number) - stub_request(:post, @url).with(headers: { "Authorization" => authorization }) + stub_hook_relay_request(@url, @hook.id, authorization).to_return_json(status: 200, body: { id: 1 }) perform_enqueued_jobs only: NotifyWebHookJob do @hook.fire("https", "rubygems.org", @version) @@ -183,7 +196,8 @@ class WebHookTest < ActiveSupport::TestCase @hook.user.update(api_key: nil) create(:api_key, owner: @hook.user) authorization = Digest::SHA2.hexdigest(@rubygem.name + @version.number + @hook.user.api_keys.first.hashed_key) - stub_request(:post, @url).with(headers: { "Authorization" => authorization }) + stub_hook_relay_request(@url, @hook.id, authorization).to_return(status: 200, body: { id: 1 }.to_json, + headers: { "Content-Type" => "application/json" }) perform_enqueued_jobs only: NotifyWebHookJob do @hook.fire("https", "rubygems.org", @version) @@ -191,7 +205,7 @@ class WebHookTest < ActiveSupport::TestCase end should "not increment failure count for hook" do - stub_request(:post, @url).to_return(status: 200, body: "", headers: {}) + stub_hook_relay_request(@url, @hook.id, nil).to_return_json(status: 200, body: { id: 1 }) perform_enqueued_jobs only: NotifyWebHookJob do @hook.fire("https", "rubygems.org", @version) @@ -201,38 +215,6 @@ class WebHookTest < ActiveSupport::TestCase end end - context "with invalid URL" do - setup do - @url = "http://someinvaliddomain.com" - @user = create(:user) - @rubygem = create(:rubygem) - @version = create(:version, rubygem: @rubygem) - @hook = create(:global_web_hook, url: @url, user: @user) - end - - should "increment failure count for hook on errors" do - [ - SocketError, - Timeout::Error, - Errno::EINVAL, - Errno::ECONNRESET, - EOFError, - Net::HTTPBadResponse, - Net::HTTPHeaderSyntaxError, - Net::ProtocolError - ].each_with_index do |exception, index| - stub_request(:post, @url).to_raise(exception) - - perform_enqueued_jobs only: NotifyWebHookJob do - @hook.fire("https", "rubygems.org", @version) - end - - assert_equal index + 1, @hook.reload.failure_count - assert_predicate @hook, :global? - end - end - end - context "yaml" do setup do @webhook = create(:web_hook)