Merge branch '280545-improve-container-registry-client' into 'master'

Improve the reliability and observability of the container registry client

See merge request gitlab-org/gitlab!50750

changelogs/unreleased/280545-improve-container-registry-client.yml

+---
+title: Improve the reliability and observability of the container registry client
+merge_request: 50750
+author:
+type: changed

lib/container_registry/client.rb

@@ -22,6 +22,23 @@ module ContainerRegistry
     # Taken from: FaradayMiddleware::FollowRedirects
     REDIRECT_CODES = Set.new [301, 302, 303, 307]
 
+    RETRY_EXCEPTIONS = [Faraday::Request::Retry::DEFAULT_EXCEPTIONS, Faraday::ConnectionFailed].flatten.freeze
+    RETRY_OPTIONS = {
+      max: 1,
+      interval: 5,
+      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?
@@ -97,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
@@ -137,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
@@ -158,6 +175,8 @@ module ContainerRegistry
 
       yield(conn) if block_given?
 
+      conn.request(:retry, RETRY_OPTIONS)
+      conn.request(:gitlab_error_callback, ERROR_CALLBACK_OPTIONS)
       conn.adapter :net_http
     end
 
@@ -188,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
@@ -205,12 +224,22 @@ module ContainerRegistry
     def faraday_redirect
       @faraday_redirect ||= faraday_base do |conn|
         conn.request :json
+
+        conn.request(:retry, RETRY_OPTIONS)
+        conn.request(:gitlab_error_callback, ERROR_CALLBACK_OPTIONS)
         conn.adapter :net_http
       end
     end
 
-    def faraday_base(&block)
-      Faraday.new(@base_uri, headers: { user_agent: "GitLab/#{Gitlab::VERSION}" }, &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: request_options,
+        &block
+      )
     end
 
     def delete_if_exists(path)

lib/gitlab/faraday.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Faraday
+    ::Faraday::Request.register_middleware(gitlab_error_callback: -> { ::Gitlab::Faraday::ErrorCallback })
+  end
+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)
+        def callback
+          self[:callback]
+        end
+      end
+    end
+  end
+end

spec/lib/container_registry/client_spec.rb

@@ -26,7 +26,54 @@ RSpec.describe ContainerRegistry::Client do
     }
   end
 
-  shared_examples '#repository_manifest' do |manifest_type|
+  let(:expected_faraday_headers) { { user_agent: "GitLab/#{Gitlab::VERSION}" } }
+  let(:expected_faraday_request_options) { Gitlab::HTTP::DEFAULT_TIMEOUT_OPTIONS }
+
+  shared_examples 'handling timeouts' do
+    let(:retry_options) do
+      ContainerRegistry::Client::RETRY_OPTIONS.merge(
+        interval: 0.1,
+        interval_randomness: 0,
+        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)
+    end
+  end
+
+  shared_examples 'handling repository manifest' do |manifest_type|
+    let(:method) { :get }
+    let(:url) {  'http://container-registry/v2/group/test/manifests/mytag' }
     let(:manifest) do
       {
         "schemaVersion" => 2,
@@ -48,7 +95,7 @@ RSpec.describe ContainerRegistry::Client do
     end
 
     it 'GET /v2/:name/manifests/mytag' do
-      stub_request(:get, "http://container-registry/v2/group/test/manifests/mytag")
+      stub_request(method, url)
         .with(headers: {
                 'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
                 'Authorization' => "bearer #{token}",
@@ -56,14 +103,24 @@ RSpec.describe ContainerRegistry::Client do
               })
         .to_return(status: 200, body: manifest.to_json, headers: { content_type: manifest_type })
 
-      expect(client.repository_manifest('group/test', 'mytag')).to eq(manifest)
+      expect_new_faraday
+
+      expect(subject).to eq(manifest)
     end
+
+    it_behaves_like 'handling timeouts'
   end
 
-  it_behaves_like '#repository_manifest', described_class::DOCKER_DISTRIBUTION_MANIFEST_V2_TYPE
-  it_behaves_like '#repository_manifest', described_class::OCI_MANIFEST_V1_TYPE
+  describe '#repository_manifest' do
+    subject { client.repository_manifest('group/test', 'mytag') }
+
+    it_behaves_like 'handling repository manifest', described_class::DOCKER_DISTRIBUTION_MANIFEST_V2_TYPE
+    it_behaves_like 'handling repository manifest', described_class::OCI_MANIFEST_V1_TYPE
+  end
 
   describe '#blob' do
+    let(:method) { :get }
+    let(:url) { 'http://container-registry/v2/group/test/blobs/sha256:0123456789012345' }
     let(:blob_headers) do
       {
           'Accept' => 'application/octet-stream',
@@ -78,16 +135,20 @@ RSpec.describe ContainerRegistry::Client do
       }
     end
 
+    subject { client.blob('group/test', 'sha256:0123456789012345') }
+
     it 'GET /v2/:name/blobs/:digest' do
-      stub_request(:get, "http://container-registry/v2/group/test/blobs/sha256:0123456789012345")
+      stub_request(method, url)
         .with(headers: blob_headers)
         .to_return(status: 200, body: "Blob")
 
-      expect(client.blob('group/test', 'sha256:0123456789012345')).to eq('Blob')
+      expect_new_faraday
+
+      expect(subject).to eq('Blob')
     end
 
     it 'follows 307 redirect for GET /v2/:name/blobs/:digest' do
-      stub_request(:get, "http://container-registry/v2/group/test/blobs/sha256:0123456789012345")
+      stub_request(method, url)
         .with(headers: blob_headers)
         .to_return(status: 307, body: '', headers: { Location: 'http://redirected' })
       # We should probably use hash_excluding here, but that requires an update to WebMock:
@@ -98,10 +159,12 @@ RSpec.describe ContainerRegistry::Client do
         end
         .to_return(status: 200, body: "Successfully redirected")
 
-      response = client.blob('group/test', 'sha256:0123456789012345')
+      expect_new_faraday(times: 2)
 
-      expect(response).to eq('Successfully redirected')
+      expect(subject).to eq('Successfully redirected')
     end
+
+    it_behaves_like 'handling timeouts'
   end
 
   describe '#upload_blob' do
@@ -111,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
@@ -173,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
@@ -375,4 +442,17 @@ RSpec.describe ContainerRegistry::Client do
         headers: { 'Allow' => 'DELETE' }
       )
   end
+
+  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: request_options
+      ).and_call_original
+      .exactly(times)
+      .times
+  end
 end

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 { 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