Merge branch '241639-dependency-proxy-manifests' into 'master'

Dependency proxy caches the manifest

See merge request gitlab-org/gitlab!48845

app/controllers/groups/dependency_proxy_for_containers_controller.rb

@@ -13,10 +13,10 @@ class Groups::DependencyProxyForContainersController < Groups::ApplicationContro
   feature_category :dependency_proxy
 
   def manifest
-    result = DependencyProxy::PullManifestService.new(image, tag, token).execute
+    result = DependencyProxy::FindOrCreateManifestService.new(group, image, tag, token).execute
 
     if result[:status] == :success
-      render json: result[:manifest]
+      send_upload(result[:manifest].file)
     else
       render status: result[:http_status], json: result[:message]
     end

app/models/dependency_proxy/manifest.rb

@@ -11,4 +11,6 @@ class DependencyProxy::Manifest < ApplicationRecord
   validates :digest, presence: true
 
   mount_file_store_uploader DependencyProxy::FileUploader
+
+  scope :find_or_initialize_by_file_name, ->(file_name) { find_or_initialize_by(file_name: file_name) }
 end

app/models/group.rb

@@ -73,6 +73,7 @@ class Group < Namespace
 
   has_one :dependency_proxy_setting, class_name: 'DependencyProxy::GroupSetting'
   has_many :dependency_proxy_blobs, class_name: 'DependencyProxy::Blob'
+  has_many :dependency_proxy_manifests, class_name: 'DependencyProxy::Manifest'
 
   accepts_nested_attributes_for :variables, allow_destroy: true
 

app/services/dependency_proxy/base_service.rb

@@ -2,6 +2,16 @@
 
 module DependencyProxy
   class BaseService < ::BaseService
+    class DownloadError < StandardError
+      attr_reader :http_status
+
+      def initialize(message, http_status)
+        @http_status = http_status
+
+        super(message)
+      end
+    end
+
     private
 
     def registry

app/services/dependency_proxy/download_blob_service.rb

@@ -2,16 +2,6 @@
 
 module DependencyProxy
   class DownloadBlobService < DependencyProxy::BaseService
-    class DownloadError < StandardError
-      attr_reader :http_status
-
-      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

app/services/dependency_proxy/find_or_create_manifest_service.rb

+# frozen_string_literal: true
+
+module DependencyProxy
+  class FindOrCreateManifestService < DependencyProxy::BaseService
+    def initialize(group, image, tag, token)
+      @group = group
+      @image = image
+      @tag = tag
+      @token = token
+      @file_name = "#{@image}:#{@tag}.json"
+      @manifest = nil
+    end
+
+    def execute
+      @manifest = @group.dependency_proxy_manifests
+                        .find_or_initialize_by_file_name(@file_name)
+
+      head_result = DependencyProxy::HeadManifestService.new(@image, @tag, @token).execute
+
+      return success(manifest: @manifest) if cached_manifest_matches?(head_result)
+
+      pull_new_manifest
+      respond
+    rescue Timeout::Error, *Gitlab::HTTP::HTTP_ERRORS
+      respond
+    end
+
+    private
+
+    def pull_new_manifest
+      DependencyProxy::PullManifestService.new(@image, @tag, @token).execute_with_manifest do |new_manifest|
+        @manifest.update!(
+          digest: new_manifest[:digest],
+          file: new_manifest[:file],
+          size: new_manifest[:file].size
+        )
+      end
+    end
+
+    def cached_manifest_matches?(head_result)
+      @manifest && @manifest.digest == head_result[:digest]
+    end
+
+    def respond
+      if @manifest.persisted?
+        success(manifest: @manifest)
+      else
+        error('Failed to download the manifest from the external registry', 503)
+      end
+    end
+  end
+end

app/services/dependency_proxy/head_manifest_service.rb

+# frozen_string_literal: true
+
+module DependencyProxy
+  class HeadManifestService < DependencyProxy::BaseService
+    def initialize(image, tag, token)
+      @image = image
+      @tag = tag
+      @token = token
+    end
+
+    def execute
+      response = Gitlab::HTTP.head(manifest_url, headers: auth_headers)
+
+      if response.success?
+        success(digest: response.headers['docker-content-digest'])
+      else
+        error(response.body, response.code)
+      end
+    rescue Timeout::Error => exception
+      error(exception.message, 599)
+    end
+
+    private
+
+    def manifest_url
+      registry.manifest_url(@image, @tag)
+    end
+  end
+end

app/services/dependency_proxy/pull_manifest_service.rb

@@ -8,13 +8,25 @@ module DependencyProxy
       @token = token
     end
 
-    def execute
+    def execute_with_manifest
+      raise ArgumentError, 'Block must be provided' unless block_given?
+
       response = Gitlab::HTTP.get(manifest_url, headers: auth_headers)
 
       if response.success?
-        success(manifest: response.body)
+        file = Tempfile.new
+
+        begin
+          file.write(response)
+          file.flush
+
+          yield(success(file: file, digest: response.headers['docker-content-digest']))
+        ensure
+          file.close
+          file.unlink
+        end
       else
-        error(response.body, response.code)
+        yield(error(response.body, response.code))
       end
     rescue Timeout::Error => exception
       error(exception.message, 599)

changelogs/unreleased/241639-dependency-proxy-manifests.yml

+---
+title: The dependency proxy caches manifests and makes HEAD requests to help with
+  rate limiting
+merge_request: 48845
+author:
+type: changed

db/migrate/20201202160105_add_group_file_name_index_to_dependency_proxy_manifests.rb

+# frozen_string_literal: true
+
+class AddGroupFileNameIndexToDependencyProxyManifests < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  NEW_INDEX = 'index_dependency_proxy_manifests_on_group_id_and_file_name'
+  OLD_INDEX = 'index_dependency_proxy_manifests_on_group_id_and_digest'
+
+  def up
+    add_concurrent_index :dependency_proxy_manifests, [:group_id, :file_name], name: NEW_INDEX, unique: true
+    remove_concurrent_index_by_name :dependency_proxy_manifests, OLD_INDEX
+  end
+
+  def down
+    add_concurrent_index :dependency_proxy_manifests, [:group_id, :digest], name: OLD_INDEX
+    remove_concurrent_index_by_name :dependency_proxy_manifests, NEW_INDEX
+  end
+end

db/schema_migrations/20201202160105

+a011331d225cef852d2402add6fb2b77e7325b87d58343a9367e0dd31a32ed7f
\ No newline at end of file

db/structure.sql

@@ -21197,7 +21197,7 @@ CREATE INDEX index_dependency_proxy_blobs_on_group_id_and_file_name ON dependenc
 
 CREATE INDEX index_dependency_proxy_group_settings_on_group_id ON dependency_proxy_group_settings USING btree (group_id);
 
-CREATE INDEX index_dependency_proxy_manifests_on_group_id_and_digest ON dependency_proxy_manifests USING btree (group_id, digest);
+CREATE UNIQUE INDEX index_dependency_proxy_manifests_on_group_id_and_file_name ON dependency_proxy_manifests USING btree (group_id, file_name);
 
 CREATE INDEX index_deploy_key_id_on_protected_branch_push_access_levels ON protected_branch_push_access_levels USING btree (deploy_key_id);
 

spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb

@@ -101,11 +101,11 @@ RSpec.describe Groups::DependencyProxyForContainersController do
   end
 
   describe 'GET #manifest' do
-    let(:manifest) { { foo: 'bar' }.to_json }
+    let_it_be(:manifest) { create(:dependency_proxy_manifest) }
     let(:pull_response) { { status: :success, manifest: manifest } }
 
     before do
-      allow_next_instance_of(DependencyProxy::PullManifestService) do |instance|
+      allow_next_instance_of(DependencyProxy::FindOrCreateManifestService) do |instance|
         allow(instance).to receive(:execute).and_return(pull_response)
       end
     end
@@ -155,11 +155,17 @@ RSpec.describe Groups::DependencyProxyForContainersController do
         end
       end
 
-      it 'returns 200 with manifest file' do
+      it 'sends a file' do
+        expect(controller).to receive(:send_file).with(manifest.file.path, {})
+
+        subject
+      end
+
+      it 'returns Content-Disposition: attachment' do
         subject
 
         expect(response).to have_gitlab_http_status(:ok)
-        expect(response.body).to eq(manifest)
+        expect(response.headers['Content-Disposition']).to match(/^attachment/)
       end
     end
 
@@ -171,7 +177,7 @@ RSpec.describe Groups::DependencyProxyForContainersController do
   end
 
   describe 'GET #blob' do
-    let(:blob) { create(:dependency_proxy_blob) }
+    let_it_be(:blob) { create(:dependency_proxy_blob) }
     let(:blob_sha) { blob.file_name.sub('.gz', '') }
     let(:blob_response) { { status: :success, blob: blob } }
 

spec/factories/dependency_proxy.rb

@@ -11,6 +11,6 @@ FactoryBot.define do
     group
     file { fixture_file_upload('spec/fixtures/dependency_proxy/manifest') }
     digest { 'sha256:5ab5a6872b264fe4fd35d63991b9b7d8425f4bc79e7cf4d563c10956581170c9' }
-    file_name { 'manifest' }
+    file_name { 'alpine:latest.json' }
   end
 end

spec/models/dependency_proxy/manifest_spec.rb

@@ -28,4 +28,25 @@ RSpec.describe DependencyProxy::Manifest, type: :model do
       it_behaves_like 'mounted file in object store'
     end
   end
+
+  describe '.find_or_initialize_by_file_name' do
+    subject { DependencyProxy::Manifest.find_or_initialize_by_file_name(file_name) }
+
+    context 'no manifest exists' do
+      let_it_be(:file_name) { 'foo' }
+
+      it 'initializes a manifest' do
+        expect(DependencyProxy::Manifest).to receive(:new).with(file_name: file_name)
+
+        subject
+      end
+    end
+
+    context 'manifest exists' do
+      let_it_be(:dependency_proxy_manifest) { create(:dependency_proxy_manifest) }
+      let_it_be(:file_name) { dependency_proxy_manifest.file_name }
+
+      it { is_expected.to eq(dependency_proxy_manifest) }
+    end
+  end
 end

spec/models/group_spec.rb

@@ -30,6 +30,7 @@ RSpec.describe Group do
     it { is_expected.to have_many(:services) }
     it { is_expected.to have_one(:dependency_proxy_setting) }
     it { is_expected.to have_many(:dependency_proxy_blobs) }
+    it { is_expected.to have_many(:dependency_proxy_manifests) }
 
     describe '#members & #requesters' do
       let(:requester) { create(:user) }

spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe DependencyProxy::FindOrCreateManifestService do
+  include DependencyProxyHelpers
+
+  let_it_be(:image) { 'alpine' }
+  let_it_be(:tag) { 'latest' }
+  let_it_be(:dependency_proxy_manifest) { create(:dependency_proxy_manifest, file_name: "#{image}:#{tag}.json") }
+  let(:manifest) { dependency_proxy_manifest.file.read }
+  let(:group) { dependency_proxy_manifest.group }
+  let(:token) { Digest::SHA256.hexdigest('123') }
+  let(:headers) { { 'docker-content-digest' => dependency_proxy_manifest.digest } }
+
+  describe '#execute' do
+    subject { described_class.new(group, image, tag, token).execute }
+
+    context 'when no manifest exists' do
+      let_it_be(:image) { 'new-image' }
+
+      before do
+        stub_manifest_head(image, tag, digest: dependency_proxy_manifest.digest)
+        stub_manifest_download(image, tag, headers: headers)
+      end
+
+      it 'downloads manifest from remote registry if there is no cached one', :aggregate_failures do
+        expect { subject }.to change { group.dependency_proxy_manifests.count }.by(1)
+        expect(subject[:status]).to eq(:success)
+        expect(subject[:manifest]).to be_a(DependencyProxy::Manifest)
+        expect(subject[:manifest]).to be_persisted
+      end
+    end
+
+    context 'when manifest exists' do
+      before do
+        stub_manifest_head(image, tag, digest: dependency_proxy_manifest.digest)
+      end
+
+      shared_examples 'using the cached manifest' do
+        it 'uses cached manifest instead of downloading one', :aggregate_failures do
+          expect(subject[:status]).to eq(:success)
+          expect(subject[:manifest]).to be_a(DependencyProxy::Manifest)
+          expect(subject[:manifest]).to eq(dependency_proxy_manifest)
+        end
+      end
+
+      it_behaves_like 'using the cached manifest'
+
+      context 'when digest is stale' do
+        let(:digest) { 'new-digest' }
+
+        before do
+          stub_manifest_head(image, tag, digest: digest)
+          stub_manifest_download(image, tag, headers: { 'docker-content-digest' => digest })
+        end
+
+        it 'downloads the new manifest and updates the existing record', :aggregate_failures do
+          expect(subject[:status]).to eq(:success)
+          expect(subject[:manifest]).to eq(dependency_proxy_manifest)
+          expect(subject[:manifest].digest).to eq(digest)
+        end
+      end
+
+      context 'failed connection' do
+        before do
+          expect(DependencyProxy::HeadManifestService).to receive(:new).and_raise(Net::OpenTimeout)
+        end
+
+        it_behaves_like 'using the cached manifest'
+
+        context 'and no manifest is cached' do
+          let_it_be(:image) { 'new-image' }
+
+          it 'returns an error', :aggregate_failures do
+            expect(subject[:status]).to eq(:error)
+            expect(subject[:http_status]).to eq(503)
+            expect(subject[:message]).to eq('Failed to download the manifest from the external registry')
+          end
+        end
+      end
+    end
+  end
+end

spec/services/dependency_proxy/head_manifest_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe DependencyProxy::HeadManifestService do
+  include DependencyProxyHelpers
+
+  let(:image) { 'alpine' }
+  let(:tag) { 'latest' }
+  let(:token) { Digest::SHA256.hexdigest('123') }
+  let(:digest) { '12345' }
+
+  subject { described_class.new(image, tag, token).execute }
+
+  context 'remote request is successful' do
+    before do
+      stub_manifest_head(image, tag, digest: digest)
+    end
+
+    it { expect(subject[:status]).to eq(:success) }
+    it { expect(subject[:digest]).to eq(digest) }
+  end
+
+  context 'remote request is not found' do
+    before do
+      stub_manifest_head(image, tag, status: 404, body: 'Not found')
+    end
+
+    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
+    before do
+      manifest_link = DependencyProxy::Registry.manifest_url(image, tag)
+
+      stub_full_request(manifest_link, method: :head).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

spec/services/dependency_proxy/pull_manifest_service_spec.rb

@@ -8,26 +8,43 @@ RSpec.describe DependencyProxy::PullManifestService do
   let(:tag) { '3.9' }
   let(:token) { Digest::SHA256.hexdigest('123') }
   let(:manifest) { { foo: 'bar' }.to_json }
+  let(:digest) { '12345' }
+  let(:headers) { { 'docker-content-digest' => digest } }
 
-  subject { described_class.new(image, tag, token).execute }
+  subject { described_class.new(image, tag, token).execute_with_manifest(&method(:check_response)) }
 
   context 'remote request is successful' do
     before do
-      stub_manifest_download(image, tag)
+      stub_manifest_download(image, tag, headers: headers)
     end
 
-    it { expect(subject[:status]).to eq(:success) }
-    it { expect(subject[:manifest]).to eq(manifest) }
+    it 'successfully returns the manifest' do
+      def check_response(response)
+        response[:file].rewind
+
+        expect(response[:status]).to eq(:success)
+        expect(response[:file].read).to eq(manifest)
+        expect(response[:digest]).to eq(digest)
+      end
+
+      subject
+    end
   end
 
   context 'remote request is not found' do
     before do
-      stub_manifest_download(image, tag, 404, 'Not found')
+      stub_manifest_download(image, tag, status: 404, body: 'Not found')
     end
 
-    it { expect(subject[:status]).to eq(:error) }
-    it { expect(subject[:http_status]).to eq(404) }
-    it { expect(subject[:message]).to eq('Not found') }
+    it 'returns a 404 not found error' do
+      def check_response(response)
+        expect(response[:status]).to eq(:error)
+        expect(response[:http_status]).to eq(404)
+        expect(response[:message]).to eq('Not found')
+      end
+
+      subject
+    end
   end
 
   context 'net timeout exception' do
@@ -37,8 +54,20 @@ RSpec.describe DependencyProxy::PullManifestService do
       stub_full_request(manifest_link).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') }
+    it 'returns a 599 error' do
+      def check_response(response)
+        expect(response[:status]).to eq(:error)
+        expect(response[:http_status]).to eq(599)
+        expect(response[:message]).to eq('execution expired')
+      end
+
+      subject
+    end
+  end
+
+  context 'no block is given' do
+    subject { described_class.new(image, tag, token).execute_with_manifest }
+
+    it { expect { subject }.to raise_error(ArgumentError, 'Block must be provided') }
   end
 end

spec/support/helpers/dependency_proxy_helpers.rb

@@ -11,11 +11,18 @@ module DependencyProxyHelpers
       .to_return(status: status, body: body || auth_body)
   end
 
-  def stub_manifest_download(image, tag, status = 200, body = nil)
+  def stub_manifest_download(image, tag, status: 200, body: nil, headers: {})
     manifest_url = registry.manifest_url(image, tag)
 
     stub_full_request(manifest_url)
-      .to_return(status: status, body: body || manifest)
+      .to_return(status: status, body: body || manifest, headers: headers)
+  end
+
+  def stub_manifest_head(image, tag, status: 200, body: nil, digest: '123456')
+    manifest_url = registry.manifest_url(image, tag)
+
+    stub_full_request(manifest_url, method: :head)
+      .to_return(status: status, body: body, headers: { 'docker-content-digest' => digest } )
   end
 
   def stub_blob_download(image, blob_sha, status = 200, body = '123456')