Add a faraday error callback middleware

Use it in `ContainerRegistry::Client` to log the error class with
the target url

lib/container_registry/client.rb

@@ -24,13 +24,21 @@ module ContainerRegistry
 
     RETRY_EXCEPTIONS = [Faraday::Request::Retry::DEFAULT_EXCEPTIONS, Faraday::ConnectionFailed].flatten.freeze
     RETRY_OPTIONS = {
-      max: 3,
+      max: 1,
       interval: 5,
-      interval_randomness: 0.5,
-      backoff_factor: 2,
       exceptions: RETRY_EXCEPTIONS
     }.freeze
 
+    ERROR_CALLBACK_OPTIONS = {
+      callback: -> (env, exception) do
+        Gitlab::ErrorTracking.log_exception(
+          exception,
+          class: name,
+          url: env[:url]
+        )
+      end
+    }.freeze
+
     def self.supports_tag_delete?
       registry_config = Gitlab.config.registry
       return false unless registry_config.enabled && registry_config.api_url.present?
@@ -106,12 +114,12 @@ module ContainerRegistry
     end
 
     def upload_blob(name, content, digest)
-      upload = faraday.post("/v2/#{name}/blobs/uploads/")
+      upload = faraday(timeout_enabled: false).post("/v2/#{name}/blobs/uploads/")
       return upload unless upload.success?
 
       location = URI(upload.headers['location'])
 
-      faraday.put("#{location.path}?#{location.query}") do |req|
+      faraday(timeout_enabled: false).put("#{location.path}?#{location.query}") do |req|
         req.params['digest'] = digest
         req.headers['Content-Type'] = 'application/octet-stream'
         req.body = content
@@ -146,7 +154,7 @@ module ContainerRegistry
     end
 
     def put_tag(name, reference, manifest)
-      response = faraday.put("/v2/#{name}/manifests/#{reference}") do |req|
+      response = faraday(timeout_enabled: false).put("/v2/#{name}/manifests/#{reference}") do |req|
         req.headers['Content-Type'] = DOCKER_DISTRIBUTION_MANIFEST_V2_TYPE
         req.body = Gitlab::Json.pretty_generate(manifest)
       end
@@ -168,6 +176,7 @@ module ContainerRegistry
       yield(conn) if block_given?
 
       conn.request(:retry, RETRY_OPTIONS)
+      conn.request(:error_callback, ERROR_CALLBACK_OPTIONS)
       conn.adapter :net_http
     end
 
@@ -198,8 +207,8 @@ module ContainerRegistry
       faraday_redirect.get(uri)
     end
 
-    def faraday
-      @faraday ||= faraday_base do |conn|
+    def faraday(timeout_enabled: true)
+      @faraday ||= faraday_base(timeout_enabled: timeout_enabled) do |conn|
         initialize_connection(conn, @options, &method(:accept_manifest))
       end
     end
@@ -217,15 +226,18 @@ module ContainerRegistry
         conn.request :json
 
         conn.request(:retry, RETRY_OPTIONS)
+        conn.request(:error_callback, ERROR_CALLBACK_OPTIONS)
         conn.adapter :net_http
       end
     end
 
-    def faraday_base(&block)
+    def faraday_base(timeout_enabled: true, &block)
+      request_options = timeout_enabled ? Gitlab::HTTP::DEFAULT_TIMEOUT_OPTIONS : nil
+
       Faraday.new(
         @base_uri,
         headers: { user_agent: "GitLab/#{Gitlab::VERSION}" },
-        request: Gitlab::HTTP::DEFAULT_TIMEOUT_OPTIONS,
+        request: request_options,
         &block
       )
     end

lib/gitlab/faraday/error_callback.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Faraday
+    # Simple Faraday Middleware that catches any error risen during the request and run the configured callback.
+    # (https://lostisland.github.io/faraday/middleware/)
+    #
+    # By default, a no op callback is setup.
+    #
+    # Note that the error is not swallowed: it will be rerisen again. In that regard, this callback acts more
+    # like an error spy than anything else.
+    #
+    # The callback has access to the request `env` and the exception instance. For more details, see
+    # https://lostisland.github.io/faraday/middleware/custom
+    #
+    # Faraday.new do |conn|
+    #   conn.request(
+    #     :error_callback,
+    #     callback: -> (env, exception) { Rails.logger.debug("Error #{exception.class.name} when trying to contact #{env[:url]}" ) }
+    #   )
+    #   conn.adapter(:net_http)
+    # end
+    class ErrorCallback < ::Faraday::Middleware
+      def initialize(app, options = nil)
+        super(app)
+        @options = ::Gitlab::Faraday::ErrorCallback::Options.from(options) # rubocop: disable CodeReuse/ActiveRecord
+      end
+
+      def call(env)
+        @app.call(env)
+      rescue => e
+        @options.callback.call(env, e)
+        raise
+      end
+
+      class Options < ::Faraday::Options.new(:callback)
+        DEFAULT_CALLBACK = -> (_env, _exception) { }
+
+        def callback
+          self[:callback] || DEFAULT_CALLBACK
+        end
+      end
+    end
+  end
+end
+
+::Faraday::Request.register_middleware(error_callback: -> { ::Gitlab::Faraday::ErrorCallback })

spec/lib/container_registry/client_spec.rb

@@ -30,20 +30,44 @@ RSpec.describe ContainerRegistry::Client do
   let(:expected_faraday_request_options) { Gitlab::HTTP::DEFAULT_TIMEOUT_OPTIONS }
 
   shared_examples 'handling timeouts' do
-    it 'handles network timeouts' do
-      actual_retries = 0
-      retry_options = ContainerRegistry::Client::RETRY_OPTIONS.merge(
+    let(:retry_options) do
+      ContainerRegistry::Client::RETRY_OPTIONS.merge(
         interval: 0.1,
         interval_randomness: 0,
-        backoff_factor: 0,
-        retry_block: -> (_, _, _, _) { actual_retries += 1 }
+        backoff_factor: 0
       )
+    end
 
+    before do
       stub_request(method, url).to_timeout
+    end
+
+    it 'handles network timeouts' do
+      actual_retries = 0
+      retry_options_with_block = retry_options.merge(
+        retry_block: -> (_, _, _, _) { actual_retries += 1 }
+      )
+
+      stub_const('ContainerRegistry::Client::RETRY_OPTIONS', retry_options_with_block)
+
+      expect { subject }.to raise_error(Faraday::ConnectionFailed)
+      expect(actual_retries).to eq(retry_options_with_block[:max])
+    end
+
+    it 'logs the error' do
       stub_const('ContainerRegistry::Client::RETRY_OPTIONS', retry_options)
 
+      expect(Gitlab::ErrorTracking)
+        .to receive(:log_exception)
+        .exactly(retry_options[:max] + 1)
+        .times
+        .with(
+          an_instance_of(Faraday::ConnectionFailed),
+          class: described_class.name,
+          url: URI(url)
+        )
+
       expect { subject }.to raise_error(Faraday::ConnectionFailed)
-      expect(actual_retries).to eq(3)
     end
   end
 
@@ -150,6 +174,8 @@ RSpec.describe ContainerRegistry::Client do
       it 'starts the upload and posts the blob' do
         stub_upload('path', 'content', 'sha256:123')
 
+        expect_new_faraday(timeout: false)
+
         expect(subject).to be_success
       end
     end
@@ -212,6 +238,8 @@ RSpec.describe ContainerRegistry::Client do
         .with(body: "{\n  \"foo\": \"bar\"\n}", headers: manifest_headers)
         .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:123' })
 
+      expect_new_faraday(timeout: false)
+
       expect(subject).to eq 'sha256:123'
     end
   end
@@ -415,13 +443,14 @@ RSpec.describe ContainerRegistry::Client do
       )
   end
 
-  def expect_new_faraday(times: 1)
+  def expect_new_faraday(times: 1, timeout: true)
+    request_options = timeout ? expected_faraday_request_options : nil
     expect(Faraday)
       .to receive(:new)
       .with(
         'http://container-registry',
         headers: expected_faraday_headers,
-        request: expected_faraday_request_options
+        request: request_options
       ).and_call_original
       .exactly(times)
       .times

spec/lib/gitlab/faraday/error_callback_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Faraday::ErrorCallback do
+  let(:app) { double(:app) }
+  let(:middleware) { described_class.new(app, {}) }
+
+  describe '#call' do
+    let(:env) { { url: 'http://target.url' } }
+
+    subject { middleware.call(env) }
+
+    context 'with no errors' do
+      before do
+        expect(app).to receive(:call).with(env).and_return('success')
+      end
+
+      it { is_expected.to eq('success') }
+    end
+
+    context 'with errors' do
+      before do
+        expect(app).to receive(:call).and_raise(ArgumentError, 'Kaboom!')
+      end
+
+      context 'with no callback' do
+        it 'uses the default callback' do
+          expect(Gitlab::Faraday::ErrorCallback::Options::DEFAULT_CALLBACK).to receive(:call).and_call_original
+
+          expect { subject }.to raise_error(ArgumentError, 'Kaboom!')
+        end
+      end
+
+      context 'with a custom callback' do
+        let(:options) { { callback: callback } }
+
+        it 'uses the custom callback' do
+          count = 0
+          target_url = nil
+          exception_class = nil
+
+          callback = proc do |env, exception|
+            count += 1
+            target_url = env[:url].to_s
+            exception_class = exception.class.name
+          end
+
+          options = { callback: callback }
+          middleware = described_class.new(app, options)
+
+          expect(callback).to receive(:call).and_call_original
+          expect { middleware.call(env) }.to raise_error(ArgumentError, 'Kaboom!')
+          expect(count).to eq(1)
+          expect(target_url).to eq('http://target.url')
+          expect(exception_class).to eq(ArgumentError.name)
+        end
+      end
+    end
+  end
+end