Merge branch '250654-get-lfs-push-mirror-working-for-github' into 'master'

Make git lfs for push mirrors work to GitHub.com

Closes #250654

See merge request gitlab-org/gitlab!43321

app/services/lfs/push_service.rb

@@ -12,7 +12,7 @@ module Lfs
 
     def execute
       lfs_objects_relation.each_batch(of: BATCH_SIZE) do |objects|
-        push_objects(objects)
+        push_objects!(objects)
       end
 
       success
@@ -30,8 +30,8 @@ module Lfs
       project.lfs_objects_for_repository_types(nil, :project)
     end
 
-    def push_objects(objects)
-      rsp = lfs_client.batch('upload', objects)
+    def push_objects!(objects)
+      rsp = lfs_client.batch!('upload', objects)
       objects = objects.index_by(&:oid)
 
       rsp.fetch('objects', []).each do |spec|
@@ -53,14 +53,14 @@ module Lfs
         return
       end
 
-      lfs_client.upload(object, upload, authenticated: authenticated)
+      lfs_client.upload!(object, upload, authenticated: authenticated)
     end
 
     def verify_object!(object, spec)
-      # TODO: the remote has requested that we make another call to verify that
-      # the object has been sent correctly.
-      # https://gitlab.com/gitlab-org/gitlab/-/issues/250654
-      log_error("LFS upload verification requested, but not supported for #{object.oid}")
+      authenticated = spec['authenticated']
+      verify = spec.dig('actions', 'verify')
+
+      lfs_client.verify!(object, verify, authenticated: authenticated)
     end
 
     def url

changelogs/unreleased/250654-get-lfs-push-mirror-working-for-github.yml

+---
+title: Make git lfs for push mirrors work to GitHub.com
+merge_request: 43321
+author:
+type: fixed

lib/gitlab/lfs/client.rb

@@ -6,6 +6,12 @@ module Gitlab
     #   * https://github.com/git-lfs/git-lfs/blob/master/docs/api/batch.md
     #   * https://github.com/git-lfs/git-lfs/blob/master/docs/api/basic-transfers.md
     class Client
+      GIT_LFS_CONTENT_TYPE = 'application/vnd.git-lfs+json'
+      DEFAULT_HEADERS = {
+        'Accept' => GIT_LFS_CONTENT_TYPE,
+        'Content-Type' => GIT_LFS_CONTENT_TYPE
+      }.freeze
+
       attr_reader :base_url
 
       def initialize(base_url, credentials:)
@@ -13,19 +19,19 @@ module Gitlab
         @credentials = credentials
       end
 
-      def batch(operation, objects)
+      def batch!(operation, objects)
         body = {
           operation: operation,
           transfers: ['basic'],
           # We don't know `ref`, so can't send it
-          objects: objects.map { |object| { oid: object.oid, size: object.size } }
+          objects: objects.as_json(only: [:oid, :size])
         }
 
         rsp = Gitlab::HTTP.post(
           batch_url,
           basic_auth: basic_auth,
           body: body.to_json,
-          headers: { 'Content-Type' => 'application/vnd.git-lfs+json' }
+          headers: build_request_headers
         )
 
         raise BatchSubmitError unless rsp.success?
@@ -40,7 +46,7 @@ module Gitlab
         body
       end
 
-      def upload(object, upload_action, authenticated:)
+      def upload!(object, upload_action, authenticated:)
         file = object.file.open
 
         params = {
@@ -60,8 +66,25 @@ module Gitlab
         file&.close
       end
 
+      def verify!(object, verify_action, authenticated:)
+        params = {
+          body: object.to_json(only: [:oid, :size]),
+          headers: build_request_headers(verify_action['header'])
+        }
+
+        params[:basic_auth] = basic_auth unless authenticated
+
+        rsp = Gitlab::HTTP.post(verify_action['href'], params)
+
+        raise ObjectVerifyError unless rsp.success?
+      end
+
       private
 
+      def build_request_headers(extra_headers = nil)
+        DEFAULT_HEADERS.merge(extra_headers || {})
+      end
+
       attr_reader :credentials
 
       def batch_url
@@ -96,6 +119,12 @@ module Gitlab
           "Failed to upload object"
         end
       end
+
+      class ObjectVerifyError < StandardError
+        def message
+          "Failed to verify object"
+        end
+      end
     end
   end
 end

spec/lib/gitlab/lfs/client_spec.rb

@@ -7,6 +7,7 @@ RSpec.describe Gitlab::Lfs::Client do
   let(:username) { 'user' }
   let(:password) { 'password' }
   let(:credentials) { { user: username, password: password, auth_method: 'password' } }
+  let(:git_lfs_content_type) { 'application/vnd.git-lfs+json' }
 
   let(:basic_auth_headers) do
     { 'Authorization' => "Basic #{Base64.strict_encode64("#{username}:#{password}")}" }
@@ -21,6 +22,15 @@ RSpec.describe Gitlab::Lfs::Client do
     }
   end
 
+  let(:verify_action) do
+    {
+      "href" => "#{base_url}/some/file/verify",
+      "header" => {
+        "Key" => "value"
+      }
+    }
+  end
+
   subject(:lfs_client) { described_class.new(base_url, credentials: credentials) }
 
   describe '#batch' do
@@ -34,10 +44,10 @@ RSpec.describe Gitlab::Lfs::Client do
         ).to_return(
           status: 200,
           body: { 'objects' => 'anything', 'transfer' => 'basic' }.to_json,
-          headers: { 'Content-Type' => 'application/vnd.git-lfs+json' }
+          headers: { 'Content-Type' => git_lfs_content_type }
         )
 
-        result = lfs_client.batch('upload', objects)
+        result = lfs_client.batch!('upload', objects)
 
         expect(stub).to have_been_requested
         expect(result).to eq('objects' => 'anything', 'transfer' => 'basic')
@@ -48,7 +58,7 @@ RSpec.describe Gitlab::Lfs::Client do
       it 'raises an error' do
         stub_batch(objects: objects, headers: basic_auth_headers).to_return(status: 400)
 
-        expect { lfs_client.batch('upload', objects) }.to raise_error(/Failed/)
+        expect { lfs_client.batch!('upload', objects) }.to raise_error(/Failed/)
       end
     end
 
@@ -56,7 +66,7 @@ RSpec.describe Gitlab::Lfs::Client do
       it 'raises an error' do
         stub_batch(objects: objects, headers: basic_auth_headers).to_return(status: 400)
 
-        expect { lfs_client.batch('upload', objects) }.to raise_error(/Failed/)
+        expect { lfs_client.batch!('upload', objects) }.to raise_error(/Failed/)
       end
     end
 
@@ -68,17 +78,22 @@ RSpec.describe Gitlab::Lfs::Client do
         ).to_return(
           status: 200,
           body: { 'transfer' => 'carrier-pigeon' }.to_json,
-          headers: { 'Content-Type' => 'application/vnd.git-lfs+json' }
+          headers: { 'Content-Type' => git_lfs_content_type }
         )
 
-        expect { lfs_client.batch('upload', objects) }.to raise_error(/Unsupported transfer/)
+        expect { lfs_client.batch!('upload', objects) }.to raise_error(/Unsupported transfer/)
       end
     end
 
     def stub_batch(objects:, headers:, operation: 'upload', transfer: 'basic')
-      objects = objects.map { |o| { oid: o.oid, size: o.size } }
+      objects = objects.as_json(only: [:oid, :size])
       body = { operation: operation, 'transfers': [transfer], objects: objects }.to_json
 
+      headers = {
+        'Accept' => git_lfs_content_type,
+        'Content-Type' => git_lfs_content_type
+      }.merge(headers)
+
       stub_request(:post, base_url + '/info/lfs/objects/batch').with(body: body, headers: headers)
     end
   end
@@ -90,7 +105,7 @@ RSpec.describe Gitlab::Lfs::Client do
       it "makes an HTTP PUT with expected parameters" do
         stub_upload(object: object, headers: upload_action['header']).to_return(status: 200)
 
-        lfs_client.upload(object, upload_action, authenticated: true)
+        lfs_client.upload!(object, upload_action, authenticated: true)
       end
     end
 
@@ -101,7 +116,7 @@ RSpec.describe Gitlab::Lfs::Client do
           headers: basic_auth_headers.merge(upload_action['header'])
         ).to_return(status: 200)
 
-        lfs_client.upload(object, upload_action, authenticated: false)
+        lfs_client.upload!(object, upload_action, authenticated: false)
 
         expect(stub).to have_been_requested
       end
@@ -110,13 +125,13 @@ RSpec.describe Gitlab::Lfs::Client do
     context 'LFS object has no file' do
       let(:object) { LfsObject.new }
 
-      it 'makes an HJTT PUT with expected parameters' do
+      it 'makes an HTTP PUT with expected parameters' do
         stub = stub_upload(
           object: object,
           headers: upload_action['header']
         ).to_return(status: 200)
 
-        lfs_client.upload(object, upload_action, authenticated: true)
+        lfs_client.upload!(object, upload_action, authenticated: true)
 
         expect(stub).to have_been_requested
       end
@@ -126,7 +141,7 @@ RSpec.describe Gitlab::Lfs::Client do
       it 'raises an error' do
         stub_upload(object: object, headers: upload_action['header']).to_return(status: 400)
 
-        expect { lfs_client.upload(object, upload_action, authenticated: true) }.to raise_error(/Failed/)
+        expect { lfs_client.upload!(object, upload_action, authenticated: true) }.to raise_error(/Failed/)
       end
     end
 
@@ -134,15 +149,73 @@ RSpec.describe Gitlab::Lfs::Client do
       it 'raises an error' do
         stub_upload(object: object, headers: upload_action['header']).to_return(status: 500)
 
-        expect { lfs_client.upload(object, upload_action, authenticated: true) }.to raise_error(/Failed/)
+        expect { lfs_client.upload!(object, upload_action, authenticated: true) }.to raise_error(/Failed/)
       end
     end
 
     def stub_upload(object:, headers:)
+      headers = {
+        'Content-Type' => 'application/octet-stream',
+        'Content-Length' => object.size.to_s
+      }.merge(headers)
+
       stub_request(:put, upload_action['href']).with(
         body: object.file.read,
         headers: headers.merge('Content-Length' => object.size.to_s)
       )
     end
   end
+
+  describe "#verify" do
+    let_it_be(:object) { create(:lfs_object) }
+
+    context 'server returns 200 OK to an authenticated request' do
+      it "makes an HTTP POST with expected parameters" do
+        stub_verify(object: object, headers: verify_action['header']).to_return(status: 200)
+
+        lfs_client.verify!(object, verify_action, authenticated: true)
+      end
+    end
+
+    context 'server returns 200 OK to an unauthenticated request' do
+      it "makes an HTTP POST with expected parameters" do
+        stub = stub_verify(
+          object: object,
+          headers: basic_auth_headers.merge(upload_action['header'])
+        ).to_return(status: 200)
+
+        lfs_client.verify!(object, verify_action, authenticated: false)
+
+        expect(stub).to have_been_requested
+      end
+    end
+
+    context 'server returns 400 error' do
+      it 'raises an error' do
+        stub_verify(object: object, headers: verify_action['header']).to_return(status: 400)
+
+        expect { lfs_client.verify!(object, verify_action, authenticated: true) }.to raise_error(/Failed/)
+      end
+    end
+
+    context 'server returns 500 error' do
+      it 'raises an error' do
+        stub_verify(object: object, headers: verify_action['header']).to_return(status: 500)
+
+        expect { lfs_client.verify!(object, verify_action, authenticated: true) }.to raise_error(/Failed/)
+      end
+    end
+
+    def stub_verify(object:, headers:)
+      headers = {
+        'Accept' => git_lfs_content_type,
+        'Content-Type' => git_lfs_content_type
+      }.merge(headers)
+
+      stub_request(:post, verify_action['href']).with(
+        body: object.to_json(only: [:oid, :size]),
+        headers: headers
+      )
+    end
+  end
 end

spec/services/lfs/push_service_spec.rb

@@ -19,7 +19,7 @@ RSpec.describe Lfs::PushService do
       stub_lfs_batch(lfs_object)
 
       expect(lfs_client)
-        .to receive(:upload)
+        .to receive(:upload!)
         .with(lfs_object, upload_action_spec(lfs_object), authenticated: true)
 
       expect(service.execute).to eq(status: :success)
@@ -28,7 +28,7 @@ RSpec.describe Lfs::PushService do
     it 'does nothing if there are no LFS objects' do
       lfs_object.destroy!
 
-      expect(lfs_client).not_to receive(:upload)
+      expect(lfs_client).not_to receive(:upload!)
 
       expect(service.execute).to eq(status: :success)
     end
@@ -36,20 +36,39 @@ RSpec.describe Lfs::PushService do
     it 'does not upload the object when upload is not requested' do
       stub_lfs_batch(lfs_object, upload: false)
 
-      expect(lfs_client).not_to receive(:upload)
+      expect(lfs_client).not_to receive(:upload!)
 
       expect(service.execute).to eq(status: :success)
     end
 
+    it 'verifies the upload if requested' do
+      stub_lfs_batch(lfs_object, verify: true)
+
+      expect(lfs_client).to receive(:upload!)
+      expect(lfs_client)
+        .to receive(:verify!)
+        .with(lfs_object, verify_action_spec(lfs_object), authenticated: true)
+
+      expect(service.execute).to eq(status: :success)
+    end
+
+    it 'skips verification if requested but upload fails' do
+      stub_lfs_batch(lfs_object, verify: true)
+
+      expect(lfs_client).to receive(:upload!) { raise 'failed' }
+      expect(lfs_client).not_to receive(:verify!)
+      expect(service.execute).to eq(status: :error, message: 'failed')
+    end
+
     it 'returns a failure when submitting a batch fails' do
-      expect(lfs_client).to receive(:batch) { raise 'failed' }
+      expect(lfs_client).to receive(:batch!) { raise 'failed' }
 
       expect(service.execute).to eq(status: :error, message: 'failed')
     end
 
     it 'returns a failure when submitting an upload fails' do
       stub_lfs_batch(lfs_object)
-      expect(lfs_client).to receive(:upload) { raise 'failed' }
+      expect(lfs_client).to receive(:upload!) { raise 'failed' }
 
       expect(service.execute).to eq(status: :error, message: 'failed')
     end
@@ -71,23 +90,28 @@ RSpec.describe Lfs::PushService do
     create(:lfs_objects_project, project: project, repository_type: type).lfs_object
   end
 
-  def stub_lfs_batch(*objects, upload: true)
+  def stub_lfs_batch(*objects, upload: true, verify: false)
     expect(lfs_client)
-      .to receive(:batch).with('upload', containing_exactly(*objects))
-      .and_return('transfer' => 'basic', 'objects' => objects.map { |o| object_spec(o, upload: upload) })
+      .to receive(:batch!).with('upload', containing_exactly(*objects))
+      .and_return('transfer' => 'basic', 'objects' => objects.map { |o| object_spec(o, upload: upload, verify: verify) })
   end
 
-  def batch_spec(*objects, upload: true)
+  def batch_spec(*objects, upload: true, verify: false)
     { 'transfer' => 'basic', 'objects' => objects.map {|o| object_spec(o, upload: upload) } }
   end
 
-  def object_spec(object, upload: true)
-    { 'oid' => object.oid, 'size' => object.size, 'authenticated' => true }.tap do |spec|
-      spec['actions'] = { 'upload' => upload_action_spec(object) } if upload
+  def object_spec(object, upload: true, verify: false)
+    { 'oid' => object.oid, 'size' => object.size, 'authenticated' => true, 'actions' => {} }.tap do |spec|
+      spec['actions']['upload'] = upload_action_spec(object) if upload
+      spec['actions']['verify'] = verify_action_spec(object) if verify
     end
   end
 
   def upload_action_spec(object)
     { 'href' => "https://example.com/#{object.oid}/#{object.size}", 'header' => { 'Key' => 'value' } }
   end
+
+  def verify_action_spec(object)
+    { 'href' => "https://example.com/#{object.oid}/#{object.size}/verify", 'header' => { 'Key' => 'value' } }
+  end
 end