Refactor dependency proxy code

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

11 files changed, 164 insertions, 74 deletions

diff --git a/ee/app/controllers/groups/dependency_proxy_for_containers_controller.rb b/ee/app/controllers/groups/dependency_proxy_for_containers_controller.rb
index 54f76eb406a..e5fb6bc795d 100644
--- a/ee/app/controllers/groups/dependency_proxy_for_containers_controller.rb
+++ b/ee/app/controllers/groups/dependency_proxy_for_containers_controller.rb
@@ -6,17 +6,27 @@ class Groups::DependencyProxyForContainersController < Groups::ApplicationContro
   before_action :ensure_feature_enabled!
   before_action :ensure_token_granted!
 
+  attr_reader :token
+
   def manifest
-    response = DependencyProxy::PullManifestService.new(image, tag, token).execute
+    result = DependencyProxy::PullManifestService.new(image, tag, token).execute
 
-    render status: response[:code], json: response[:body]
+    if result[:status] == :success
+      render json: result[:manifest]
+    else
+      render status: result[:http_status], json: result[:message]
+    end
   end
 
   def blob
-    blob = DependencyProxy::FindOrCreateBlobService
+    result = DependencyProxy::FindOrCreateBlobService
       .new(group, image, token, params[:sha]).execute
 
-    send_upload(blob.file)
+    if result[:status] == :success
+      send_upload(result[:blob].file)
+    else
+      head result[:http_status]
+    end
   end
 
   private
@@ -29,10 +39,6 @@ class Groups::DependencyProxyForContainersController < Groups::ApplicationContro
     params[:tag]
   end
 
-  def token
-    @token
-  end
-
   def ensure_feature_enabled!
     render_404 unless Gitlab.config.dependency_proxy.enabled &&
         group.feature_available?(:dependency_proxy) &&
@@ -40,12 +46,12 @@ class Groups::DependencyProxyForContainersController < Groups::ApplicationContro
   end
 
   def ensure_token_granted!
-    response = DependencyProxy::RequestTokenService.new(image).execute
+    result = DependencyProxy::RequestTokenService.new(image).execute
 
-    if response[:code] == 200 and response[:body].present?
-      @token = response[:body]
+    if result[:status] == :success
+      @token = result[:token]
     else
-      render status: response[:code], json: response[:body]
+      render status: result[:http_status], json: result[:message]
     end
   end
 end
diff --git a/ee/app/services/dependency_proxy/base_service.rb b/ee/app/services/dependency_proxy/base_service.rb
index 3a6f38d7caf..1b2d4b14a27 100644
--- a/ee/app/services/dependency_proxy/base_service.rb
+++ b/ee/app/services/dependency_proxy/base_service.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 module DependencyProxy
-  class BaseService
+  class BaseService < ::BaseService
     private
 
     def registry
@@ -13,12 +13,5 @@ module DependencyProxy
         Authorization: "Bearer #{@token}"
       }
     end
-
-    def to_response(code, body)
-      {
-        code: code,
-        body: body
-      }
-    end
   end
 end
diff --git a/ee/app/services/dependency_proxy/download_blob_service.rb b/ee/app/services/dependency_proxy/download_blob_service.rb
index f04b8fc345d..3c690683bf6 100644
--- a/ee/app/services/dependency_proxy/download_blob_service.rb
+++ b/ee/app/services/dependency_proxy/download_blob_service.rb
@@ -2,31 +2,41 @@
 
 module DependencyProxy
   class DownloadBlobService < DependencyProxy::BaseService
-    DownloadError = Class.new(StandardError)
+    class DownloadError < StandardError
+      attr_reader :http_status
 
-    def initialize(image, blob_sha, token, file_path)
+      def initialize(message, http_status)
+        @http_status = http_status
+
+        super(message)
+      end
+    end
+
+    def initialize(image, blob_sha, token)
       @image = image
       @blob_sha = blob_sha
       @token = token
-      @file_path = file_path
+      @temp_file = Tempfile.new
     end
 
     def execute
-      File.open(@file_path, "wb") do |file|
+      File.open(@temp_file.path, "wb") do |file|
         Gitlab::HTTP.get(blob_url, headers: auth_headers, stream_body: true) do |fragment|
           if [301, 302, 307].include?(fragment.code)
             # do nothing
           elsif fragment.code == 200
             file.write(fragment)
           else
-            raise DownloadError, "Non-success status code while downloading a blob. #{fragment.code}"
+            raise DownloadError.new('Non-success response code on downloading blob fragment', fragment.code)
           end
         end
       end
 
-      true
-    rescue DownloadError
-      false
+      success(file: @temp_file)
+    rescue DownloadError => exception
+      error(exception.message, exception.http_status)
+    rescue Timeout::Error => exception
+      error(exception.message, 599)
     end
 
     private
diff --git a/ee/app/services/dependency_proxy/find_or_create_blob_service.rb b/ee/app/services/dependency_proxy/find_or_create_blob_service.rb
index 10455119789..bd06f9d7628 100644
--- a/ee/app/services/dependency_proxy/find_or_create_blob_service.rb
+++ b/ee/app/services/dependency_proxy/find_or_create_blob_service.rb
@@ -14,19 +14,32 @@ module DependencyProxy
       blob = @group.dependency_proxy_blobs.find_or_build(file_name)
 
       unless blob.persisted?
-        temp_file = Tempfile.new
+        result = DependencyProxy::DownloadBlobService
+          .new(@image, @blob_sha, @token).execute
 
-        success = DependencyProxy::DownloadBlobService
-          .new(@image, @blob_sha, @token, temp_file.path).execute
+        if result[:status] == :error
+          log_failure(result)
 
-        return unless success
+          return error('Failed to download the blob', result[:http_status])
+        end
 
-        blob.file = temp_file
-        blob.size = temp_file.size
+        blob.file = result[:file]
+        blob.size = result[:file].size
         blob.save!
       end
 
-      blob
+      success(blob: blob)
+    end
+
+    private
+
+    def log_failure(result)
+      log_error(
+        "Dependency proxy: Failed to download the blob." \
+        "Blob sha: #{@blob_sha}." \
+        "Error message: #{result[:message][0, 100]}" \
+        "HTTP status: #{result[:http_status]}"
+      )
     end
   end
 end
diff --git a/ee/app/services/dependency_proxy/pull_manifest_service.rb b/ee/app/services/dependency_proxy/pull_manifest_service.rb
index 02e67ddb634..fc54ef85c96 100644
--- a/ee/app/services/dependency_proxy/pull_manifest_service.rb
+++ b/ee/app/services/dependency_proxy/pull_manifest_service.rb
@@ -11,9 +11,13 @@ module DependencyProxy
     def execute
       response = Gitlab::HTTP.get(manifest_url, headers: auth_headers)
 
-      to_response(response.code, response.body)
-    rescue Net::OpenTimeout, Net::ReadTimeout => exception
-      to_response(599, exception.message)
+      if response.success?
+        success(manifest: response.body)
+      else
+        error(response.body, response.code)
+      end
+    rescue Timeout::Error => exception
+      error(exception.message, 599)
     end
 
     private
diff --git a/ee/app/services/dependency_proxy/request_token_service.rb b/ee/app/services/dependency_proxy/request_token_service.rb
index 31ab5dfcadf..021251c798b 100644
--- a/ee/app/services/dependency_proxy/request_token_service.rb
+++ b/ee/app/services/dependency_proxy/request_token_service.rb
@@ -9,15 +9,15 @@ module DependencyProxy
     def execute
       response = Gitlab::HTTP.get(auth_url)
 
-      if response.code == 200
-        to_response(200, JSON.parse(response.body)['token'])
+      if response.success?
+        success(token: JSON.parse(response.body)['token'])
       else
-        to_response(response.code, 'Expected 200 response code for an access token')
+        error('Expected 200 response code for an access token', response.code)
       end
-    rescue Net::OpenTimeout, Net::ReadTimeout => exception
-      to_response(599, exception.message)
+    rescue Timeout::Error => exception
+      error(exception.message, 599)
     rescue JSON::ParserError
-      to_response(500, 'Failed to parse a response body for an access token')
+      error('Failed to parse a response body for an access token', 500)
     end
 
     private
diff --git a/ee/spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb b/ee/spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb
index 2114e7253ea..7711a89438c 100644
--- a/ee/spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb
+++ b/ee/spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb
@@ -4,7 +4,7 @@ require 'spec_helper'
 
 describe Groups::DependencyProxyForContainersController do
   let(:group) { create(:group) }
-  let(:token_response) { { code: 200, body: 'abcd1234' } }
+  let(:token_response) { { status: :success, token: 'abcd1234' } }
 
   before do
     allow(Gitlab.config.dependency_proxy)
@@ -16,7 +16,7 @@ describe Groups::DependencyProxyForContainersController do
 
   describe 'GET #manifest' do
     let(:manifest) { { foo: 'bar' }.to_json }
-    let(:pull_response) { { code: 200, body: manifest } }
+    let(:pull_response) { { status: :success, manifest: manifest } }
 
     before do
       allow_any_instance_of(DependencyProxy::PullManifestService)
@@ -29,7 +29,13 @@ describe Groups::DependencyProxyForContainersController do
       end
 
       context 'remote token request fails' do
-        let(:token_response) { { code: 503, body: 'Service Unavailable' } }
+        let(:token_response) do
+          {
+            status: :error,
+            http_status: 503,
+            message: 'Service Unavailable'
+          }
+        end
 
         it 'proxies status from the remote token request' do
           get_manifest
@@ -40,7 +46,13 @@ describe Groups::DependencyProxyForContainersController do
       end
 
       context 'remote manifest request fails' do
-        let(:pull_response) { { code: 400, body: '' } }
+        let(:pull_response) do
+          {
+            status: :error,
+            http_status: 400,
+            message: ''
+          }
+        end
 
         it 'proxies status from the remote manifest request' do
           get_manifest
@@ -72,10 +84,11 @@ describe Groups::DependencyProxyForContainersController do
   describe 'GET #blob' do
     let(:blob) { create(:dependency_proxy_blob) }
     let(:blob_sha) { blob.file_name.sub('.gz', '') }
+    let(:blob_response) { { status: :success, blob: blob } }
 
     before do
       allow_any_instance_of(DependencyProxy::FindOrCreateBlobService)
-        .to receive(:execute).and_return(blob)
+        .to receive(:execute).and_return(blob_response)
     end
 
     context 'feature enabled' do
@@ -83,6 +96,23 @@ describe Groups::DependencyProxyForContainersController do
         enable_dependency_proxy
       end
 
+      context 'remote blob request fails' do
+        let(:blob_response) do
+          {
+            status: :error,
+            http_status: 400,
+            message: ''
+          }
+        end
+
+        it 'proxies status from the remote blob request' do
+          get_blob
+
+          expect(response).to have_gitlab_http_status(400)
+          expect(response.body).to be_empty
+        end
+      end
+
       it 'sends a file' do
         expect(controller).to receive(:send_file).with(blob.file.path, {})
 
diff --git a/ee/spec/services/dependency_proxy/download_blob_service_spec.rb b/ee/spec/services/dependency_proxy/download_blob_service_spec.rb
index 3fa5121d627..97ed44fb90a 100644
--- a/ee/spec/services/dependency_proxy/download_blob_service_spec.rb
+++ b/ee/spec/services/dependency_proxy/download_blob_service_spec.rb
@@ -7,15 +7,38 @@ describe DependencyProxy::DownloadBlobService do
   let(:image) { 'alpine' }
   let(:token) { Digest::SHA256.hexdigest('123') }
   let(:blob_sha) { Digest::SHA256.hexdigest('ruby:2.3.9') }
-  let(:file) { Tempfile.new }
 
-  subject { described_class.new(image, blob_sha, token, file.path) }
+  subject { described_class.new(image, blob_sha, token).execute }
 
-  before do
-    stub_blob_download(image, blob_sha)
+  context 'remote request is successful' do
+    before do
+      stub_blob_download(image, blob_sha)
+    end
+
+    it { expect(subject[:status]).to eq(:success) }
+    it { expect(subject[:file]).to be_a(Tempfile) }
+    it { expect(subject[:file].size).to eq(6) }
+  end
+
+  context 'remote request is not found' do
+    before do
+      stub_blob_download(image, blob_sha, 404)
+    end
+
+    it { expect(subject[:status]).to eq(:error) }
+    it { expect(subject[:http_status]).to eq(404) }
+    it { expect(subject[:message]).to eq('Non-success response code on downloading blob fragment') }
   end
 
-  it 'downloads blob and writes it into the file' do
-    expect { subject.execute }.to change { file.size }.from(0).to(6)
+  context 'net timeout exception' do
+    before do
+      blob_url = DependencyProxy::Registry.blob_url(image, blob_sha)
+
+      stub_request(:get, blob_url).to_timeout
+    end
+
+    it { expect(subject[:status]).to eq(:error) }
+    it { expect(subject[:http_status]).to eq(599) }
+    it { expect(subject[:message]).to eq('execution expired') }
   end
 end
diff --git a/ee/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb b/ee/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb
index ada1076e7d2..b5a99d02173 100644
--- a/ee/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb
+++ b/ee/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb
@@ -23,8 +23,9 @@ describe DependencyProxy::FindOrCreateBlobService do
     end
 
     it 'downloads blob from remote registry if there is no cached one' do
-      is_expected.to be_a(DependencyProxy::Blob)
-      is_expected.to be_persisted
+      expect(subject[:status]).to eq(:success)
+      expect(subject[:blob]).to be_a(DependencyProxy::Blob)
+      expect(subject[:blob]).to be_persisted
     end
   end
 
@@ -32,8 +33,9 @@ describe DependencyProxy::FindOrCreateBlobService do
     let(:blob_sha) { blob.file_name.sub('.gz', '') }
 
     it 'uses cached blob instead of downloading one' do
-      is_expected.to be_a(DependencyProxy::Blob)
-      is_expected.to eq(blob)
+      expect(subject[:status]).to eq(:success)
+      expect(subject[:blob]).to be_a(DependencyProxy::Blob)
+      expect(subject[:blob]).to eq(blob)
     end
   end
 
@@ -42,6 +44,10 @@ describe DependencyProxy::FindOrCreateBlobService do
       stub_blob_download(image, blob_sha, 404)
     end
 
-    it { is_expected.to be nil }
+    it 'returns error message and http status' do
+      expect(subject[:status]).to eq(:error)
+      expect(subject[:message]).to eq('Failed to download the blob')
+      expect(subject[:http_status]).to eq(404)
+    end
   end
 end
diff --git a/ee/spec/services/dependency_proxy/pull_manifest_service_spec.rb b/ee/spec/services/dependency_proxy/pull_manifest_service_spec.rb
index 87d62161cca..808fdd35ccb 100644
--- a/ee/spec/services/dependency_proxy/pull_manifest_service_spec.rb
+++ b/ee/spec/services/dependency_proxy/pull_manifest_service_spec.rb
@@ -16,8 +16,8 @@ describe DependencyProxy::PullManifestService do
       stub_manifest_download(image, tag)
     end
 
-    it { expect(subject[:code]).to eq(200) }
-    it { expect(subject[:body]).to eq(manifest) }
+    it { expect(subject[:status]).to eq(:success) }
+    it { expect(subject[:manifest]).to eq(manifest) }
   end
 
   context 'remote request is not found' do
@@ -25,8 +25,9 @@ describe DependencyProxy::PullManifestService do
       stub_manifest_download(image, tag, 404, 'Not found')
     end
 
-    it { expect(subject[:code]).to eq(404) }
-    it { expect(subject[:body]).to eq('Not found') }
+    it { expect(subject[:status]).to eq(:error) }
+    it { expect(subject[:http_status]).to eq(404) }
+    it { expect(subject[:message]).to eq('Not found') }
   end
 
   context 'net timeout exception' do
@@ -36,7 +37,8 @@ describe DependencyProxy::PullManifestService do
       stub_request(:get, manifest_link).to_timeout
     end
 
-    it { expect(subject[:code]).to eq(599) }
-    it { expect(subject[:body]).to eq('execution expired') }
+    it { expect(subject[:status]).to eq(:error) }
+    it { expect(subject[:http_status]).to eq(599) }
+    it { expect(subject[:message]).to eq('execution expired') }
   end
 end
diff --git a/ee/spec/services/dependency_proxy/request_token_service_spec.rb b/ee/spec/services/dependency_proxy/request_token_service_spec.rb
index 78305f9554b..247c238f667 100644
--- a/ee/spec/services/dependency_proxy/request_token_service_spec.rb
+++ b/ee/spec/services/dependency_proxy/request_token_service_spec.rb
@@ -14,8 +14,8 @@ describe DependencyProxy::RequestTokenService do
       stub_registry_auth(image, token)
     end
 
-    it { expect(subject[:code]).to eq(200) }
-    it { expect(subject[:body]).to eq(token) }
+    it { expect(subject[:status]).to eq(:success) }
+    it { expect(subject[:token]).to eq(token) }
   end
 
   context 'remote request is not found' do
@@ -23,8 +23,9 @@ describe DependencyProxy::RequestTokenService do
       stub_registry_auth(image, token, 404)
     end
 
-    it { expect(subject[:code]).to eq(404) }
-    it { expect(subject[:body]).to eq('Expected 200 response code for an access token') }
+    it { expect(subject[:status]).to eq(:error) }
+    it { expect(subject[:http_status]).to eq(404) }
+    it { expect(subject[:message]).to eq('Expected 200 response code for an access token') }
   end
 
   context 'failed to parse response body' do
@@ -32,8 +33,9 @@ describe DependencyProxy::RequestTokenService do
       stub_registry_auth(image, token, 200, 'dasd1321: wow')
     end
 
-    it { expect(subject[:code]).to eq(500) }
-    it { expect(subject[:body]).to eq('Failed to parse a response body for an access token') }
+    it { expect(subject[:status]).to eq(:error) }
+    it { expect(subject[:http_status]).to eq(500) }
+    it { expect(subject[:message]).to eq('Failed to parse a response body for an access token') }
   end
 
   context 'net timeout exception' do
@@ -43,7 +45,8 @@ describe DependencyProxy::RequestTokenService do
       stub_request(:any, auth_link).to_timeout
     end
 
-    it { expect(subject[:code]).to eq(599) }
-    it { expect(subject[:body]).to eq('execution expired') }
+    it { expect(subject[:status]).to eq(:error) }
+    it { expect(subject[:http_status]).to eq(599) }
+    it { expect(subject[:message]).to eq('execution expired') }
   end
 end