Fix manifest workhorse upload

Use find or create logic when uploading a new
manifest rather than only create to prevent
collisions.

Changelog: fixed

app/controllers/groups/dependency_proxy_for_containers_controller.rb

@@ -73,13 +73,23 @@ class Groups::DependencyProxyForContainersController < ::Groups::DependencyProxy
   end
 
   def upload_manifest
-    @group.dependency_proxy_manifests.create!(
+    attrs = {
       file_name: manifest_file_name,
       content_type: request.headers[Gitlab::Workhorse::SEND_DEPENDENCY_CONTENT_TYPE_HEADER],
-      digest: request.headers['Docker-Content-Digest'],
+      digest: request.headers[DependencyProxy::Manifest::DIGEST_HEADER],
       file: params[:file],
       size: params[:file].size
-    )
+    }
+
+    manifest = @group.dependency_proxy_manifests
+                     .active
+                     .find_by_file_name(manifest_file_name)
+
+    if manifest
+      manifest.update!(attrs)
+    else
+      @group.dependency_proxy_manifests.create!(attrs)
+    end
 
     event_name = tracking_event_name(object_type: :manifest, from_cache: false)
     track_package_event(event_name, :dependency_proxy, namespace: group, user: auth_user)
@@ -105,7 +115,7 @@ class Groups::DependencyProxyForContainersController < ::Groups::DependencyProxy
   def send_manifest(manifest, from_cache:)
     # Technical debt: change to read_at https://gitlab.com/gitlab-org/gitlab/-/issues/341536
     manifest.touch
-    response.headers['Docker-Content-Digest'] = manifest.digest
+    response.headers[DependencyProxy::Manifest::DIGEST_HEADER] = manifest.digest
     response.headers['Content-Length'] = manifest.size
     response.headers['Docker-Distribution-Api-Version'] = DependencyProxy::DISTRIBUTION_API_VERSION
     response.headers['Etag'] = "\"#{manifest.digest}\""

app/models/dependency_proxy/manifest.rb

@@ -8,6 +8,7 @@ class DependencyProxy::Manifest < ApplicationRecord
   belongs_to :group
 
   MAX_FILE_SIZE = 10.megabytes.freeze
+  DIGEST_HEADER = 'Docker-Content-Digest'
 
   validates :group, presence: true
   validates :file, presence: true

app/services/dependency_proxy/head_manifest_service.rb

@@ -14,7 +14,10 @@ module DependencyProxy
       response = Gitlab::HTTP.head(manifest_url, headers: auth_headers.merge(Accept: ACCEPT_HEADERS))
 
       if response.success?
-        success(digest: response.headers['docker-content-digest'], content_type: response.headers['content-type'])
+        success(
+          digest: response.headers[DependencyProxy::Manifest::DIGEST_HEADER],
+          content_type: response.headers['content-type']
+        )
       else
         error(response.body, response.code)
       end

app/services/dependency_proxy/pull_manifest_service.rb

@@ -20,7 +20,13 @@ module DependencyProxy
           file.write(response.body)
           file.flush
 
-          yield(success(file: file, digest: response.headers['docker-content-digest'], content_type: response.headers['content-type']))
+          yield(
+            success(
+              file: file,
+              digest: response.headers[DependencyProxy::Manifest::DIGEST_HEADER],
+              content_type: response.headers['content-type']
+            )
+          )
         ensure
           file.close
           file.unlink

ee/spec/services/geo/container_repository_sync_spec.rb

@@ -60,7 +60,7 @@ RSpec.describe Geo::ContainerRepositorySync, :geo do
 
     tags.each do |tag, digest|
       stub_request(:head, "#{repository_url}/manifests/#{tag}")
-        .to_return(status: 200, body: "", headers: { 'docker-content-digest' => digest })
+        .to_return(status: 200, body: "", headers: { DependencyProxy::Manifest::DIGEST_HEADER => digest })
     end
   end
 
@@ -73,7 +73,7 @@ RSpec.describe Geo::ContainerRepositorySync, :geo do
 
     tags.each do |tag, digest|
       stub_request(:head, "#{repository_url}/manifests/#{tag}")
-        .to_return(status: 200, body: "", headers: { 'docker-content-digest' => digest })
+        .to_return(status: 200, body: "", headers: { DependencyProxy::Manifest::DIGEST_HEADER => digest })
     end
   end
 

lib/container_registry/client.rb

@@ -90,7 +90,7 @@ module ContainerRegistry
 
     def repository_tag_digest(name, reference)
       response = faraday.head("/v2/#{name}/manifests/#{reference}")
-      response.headers['docker-content-digest'] if response.success?
+      response.headers[DependencyProxy::Manifest::DIGEST_HEADER] if response.success?
     end
 
     def delete_repository_tag_by_digest(name, reference)
@@ -171,7 +171,7 @@ module ContainerRegistry
         req.body = Gitlab::Json.pretty_generate(manifest)
       end
 
-      response.headers['docker-content-digest'] if response.success?
+      response.headers[DependencyProxy::Manifest::DIGEST_HEADER] if response.success?
     end
 
     private

spec/controllers/groups/dependency_proxy_for_containers_controller_spec.rb

@@ -425,28 +425,28 @@ RSpec.describe Groups::DependencyProxyForContainersController do
     end
   end
 
-  describe 'GET #authorize_upload_blob' do
+  describe 'POST #authorize_upload_blob' do
     let(:blob_sha) { 'a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4' }
     let(:maximum_size) { DependencyProxy::Blob::MAX_FILE_SIZE }
 
     subject do
       request.headers.merge!(workhorse_internal_api_request_header)
 
-      get :authorize_upload_blob, params: { group_id: group.to_param, image: 'alpine', sha: blob_sha }
+      post :authorize_upload_blob, params: { group_id: group.to_param, image: 'alpine', sha: blob_sha }
     end
 
     it_behaves_like 'without permission'
     it_behaves_like 'authorize action with permission'
   end
 
-  describe 'GET #upload_blob' do
+  describe 'POST #upload_blob' do
     let(:blob_sha) { 'a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4' }
     let(:file) { fixture_file_upload("spec/fixtures/dependency_proxy/#{blob_sha}.gz", 'application/gzip') }
 
     subject do
       request.headers.merge!(workhorse_internal_api_request_header)
 
-      get :upload_blob, params: {
+      post :upload_blob, params: {
         group_id: group.to_param,
         image: 'alpine',
         sha: blob_sha,
@@ -469,31 +469,45 @@ RSpec.describe Groups::DependencyProxyForContainersController do
     end
   end
 
-  describe 'GET #authorize_upload_manifest' do
+  describe 'POST #authorize_upload_manifest' do
     let(:maximum_size) { DependencyProxy::Manifest::MAX_FILE_SIZE }
 
     subject do
       request.headers.merge!(workhorse_internal_api_request_header)
 
-      get :authorize_upload_manifest, params: { group_id: group.to_param, image: 'alpine', tag: 'latest' }
+      post :authorize_upload_manifest, params: { group_id: group.to_param, image: 'alpine', tag: 'latest' }
     end
 
     it_behaves_like 'without permission'
     it_behaves_like 'authorize action with permission'
   end
 
-  describe 'GET #upload_manifest' do
-    let(:file) { fixture_file_upload("spec/fixtures/dependency_proxy/manifest", 'application/json') }
+  describe 'POST #upload_manifest' do
+    let_it_be(:file) { fixture_file_upload("spec/fixtures/dependency_proxy/manifest", 'application/json') }
+    let_it_be(:image) { 'alpine' }
+    let_it_be(:tag) { 'latest' }
+    let_it_be(:content_type) { 'v2/manifest' }
+    let_it_be(:digest) { 'foo' }
+    let_it_be(:file_name) { "#{image}:#{tag}.json" }
 
     subject do
-      request.headers.merge!(workhorse_internal_api_request_header)
-
-      get :upload_manifest, params: {
+      request.headers.merge!(
+        workhorse_internal_api_request_header.merge!(
+          {
+            Gitlab::Workhorse::SEND_DEPENDENCY_CONTENT_TYPE_HEADER => content_type,
+            DependencyProxy::Manifest::DIGEST_HEADER => digest
+          }
+        )
+      )
+      params = {
         group_id: group.to_param,
-        image: 'alpine',
-        tag: 'latest',
-        file: file
+        image: image,
+        tag: tag,
+        file: file,
+        file_name: file_name
       }
+
+      post :upload_manifest, params: params
     end
 
     it_behaves_like 'without permission'
@@ -501,13 +515,30 @@ RSpec.describe Groups::DependencyProxyForContainersController do
     context 'with a valid user' do
       before do
         group.add_guest(user)
+      end
 
-        expect_next_found_instance_of(Group) do |instance|
-          expect(instance).to receive_message_chain(:dependency_proxy_manifests, :create!)
+      it_behaves_like 'a package tracking event', described_class.name, 'pull_manifest'
+
+      context 'with no existing manifest' do
+        it 'creates a manifest' do
+          expect { subject }.to change { group.dependency_proxy_manifests.count }.by(1)
+
+          manifest = group.dependency_proxy_manifests.first.reload
+          expect(manifest.content_type).to eq(content_type)
+          expect(manifest.digest).to eq(digest)
+          expect(manifest.file_name).to eq(file_name)
         end
       end
 
-      it_behaves_like 'a package tracking event', described_class.name, 'pull_manifest'
+      context 'with existing stale manifest' do
+        let_it_be(:old_digest) { 'asdf' }
+        let_it_be_with_reload(:manifest) { create(:dependency_proxy_manifest, file_name: file_name, digest: old_digest, group: group) }
+
+        it 'updates the existing manifest' do
+          expect { subject }.to change { group.dependency_proxy_manifests.count }.by(0)
+            .and change { manifest.reload.digest }.from(old_digest).to(digest)
+        end
+      end
     end
   end
 

spec/lib/container_registry/client_spec.rb

@@ -279,7 +279,7 @@ RSpec.describe ContainerRegistry::Client do
     it 'uploads the manifest and returns the digest' do
       stub_request(:put, "http://container-registry/v2/path/manifests/tagA")
         .with(body: "{\n  \"foo\": \"bar\"\n}", headers: manifest_headers)
-        .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:123' })
+        .to_return(status: 200, body: "", headers: { DependencyProxy::Manifest::DIGEST_HEADER => 'sha256:123' })
 
       expect_new_faraday(timeout: false)
 

spec/lib/container_registry/tag_spec.rb

@@ -213,7 +213,7 @@ RSpec.describe ContainerRegistry::Tag do
     before do
       stub_request(:head, 'http://registry.gitlab/v2/group/test/manifests/tag')
         .with(headers: headers)
-        .to_return(status: 200, headers: { 'Docker-Content-Digest' => 'sha256:digest' })
+        .to_return(status: 200, headers: { DependencyProxy::Manifest::DIGEST_HEADER => 'sha256:digest' })
     end
 
     describe '#digest' do

spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe DependencyProxy::FindOrCreateManifestService do
   let(:token) { Digest::SHA256.hexdigest('123') }
   let(:headers) do
     {
-      'docker-content-digest' => dependency_proxy_manifest.digest,
+      DependencyProxy::Manifest::DIGEST_HEADER => dependency_proxy_manifest.digest,
       'content-type' => dependency_proxy_manifest.content_type
     }
   end
@@ -100,8 +100,8 @@ RSpec.describe DependencyProxy::FindOrCreateManifestService do
         let(:content_type) { 'new-content-type' }
 
         before do
-          stub_manifest_head(image, tag, headers: { 'docker-content-digest' => digest, 'content-type' => content_type })
-          stub_manifest_download(image, tag, headers: { 'docker-content-digest' => digest, 'content-type' => content_type })
+          stub_manifest_head(image, tag, headers: { DependencyProxy::Manifest::DIGEST_HEADER => digest, 'content-type' => content_type })
+          stub_manifest_download(image, tag, headers: { DependencyProxy::Manifest::DIGEST_HEADER => digest, 'content-type' => content_type })
         end
 
         it_behaves_like 'returning no manifest'

spec/services/dependency_proxy/head_manifest_service_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe DependencyProxy::HeadManifestService do
   let(:content_type) { 'foo' }
   let(:headers) do
     {
-      'docker-content-digest' => digest,
+      DependencyProxy::Manifest::DIGEST_HEADER => digest,
       'content-type' => content_type
     }
   end

spec/services/dependency_proxy/pull_manifest_service_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe DependencyProxy::PullManifestService do
   let(:digest) { '12345' }
   let(:content_type) { 'foo' }
   let(:headers) do
-    { 'docker-content-digest' => digest, 'content-type' => content_type }
+    { DependencyProxy::Manifest::DIGEST_HEADER => digest, 'content-type' => content_type }
   end
 
   subject { described_class.new(image, tag, token).execute_with_manifest(&method(:check_response)) }

spec/support/shared_contexts/services/projects/container_repository/delete_tags_service_shared_context.rb

@@ -31,14 +31,14 @@ RSpec.shared_context 'container repository delete tags service shared context' d
     end
   end
 
-  def stub_put_manifest_request(tag, status = 200, headers = { 'docker-content-digest' => 'sha256:dummy' })
+  def stub_put_manifest_request(tag, status = 200, headers = { DependencyProxy::Manifest::DIGEST_HEADER => 'sha256:dummy' })
     stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/#{tag}")
       .to_return(status: status, body: '', headers: headers)
   end
 
   def stub_tag_digest(tag, digest)
     stub_request(:head, "http://registry.gitlab/v2/#{repository.path}/manifests/#{tag}")
-      .to_return(status: 200, body: '', headers: { 'docker-content-digest' => digest })
+      .to_return(status: 200, body: '', headers: { DependencyProxy::Manifest::DIGEST_HEADER => digest })
   end
 
   def stub_digest_config(digest, created_at)

spec/support/shared_examples/features/dependency_proxy_shared_examples.rb

@@ -26,7 +26,7 @@ RSpec.shared_examples 'a successful manifest pull' do
     subject
 
     expect(response).to have_gitlab_http_status(:ok)
-    expect(response.headers['Docker-Content-Digest']).to eq(manifest.digest)
+    expect(response.headers[DependencyProxy::Manifest::DIGEST_HEADER]).to eq(manifest.digest)
     expect(response.headers['Content-Length']).to eq(manifest.size)
     expect(response.headers['Docker-Distribution-Api-Version']).to eq(DependencyProxy::DISTRIBUTION_API_VERSION)
     expect(response.headers['Etag']).to eq("\"#{manifest.digest}\"")