Merge branch '6195-http-git-push-to-secondary-for-git-lfs-currently-does-not-work' into 'master'

Resolve "Git LFS: Support for HTTP git push to secondary"

Closes #6195

See merge request gitlab-org/gitlab-ee!6109

app/controllers/projects/lfs_api_controller.rb

@@ -98,7 +98,7 @@ class Projects::LfsApiController < Projects::GitHttpClientController
   end
 
   def lfs_check_batch_operation!
-    if upload_request? && Gitlab::Database.read_only?
+    if batch_operation_disallowed?
       render(
         json: {
           message: lfs_read_only_message
@@ -109,6 +109,11 @@ class Projects::LfsApiController < Projects::GitHttpClientController
     end
   end
 
+  # Overridden in EE
+  def batch_operation_disallowed?
+    upload_request? && Gitlab::Database.read_only?
+  end
+
   # Overridden in EE
   def lfs_read_only_message
     _('You cannot write to this read-only GitLab instance.')

doc/administration/geo/replication/index.md

@@ -203,8 +203,8 @@ extra limitations may be in place.
 
 - Pushing code to a secondary redirects the request to the primary instead of handling it directly [gitlab-ee#1381](https://gitlab.com/gitlab-org/gitlab-ee/issues/1381):
     * Only push via HTTP is currently supported
+    * Git LFS is supported
     * Pushing via SSH is currently not supported: [gitlab-ee#5387](https://gitlab.com/gitlab-org/gitlab-ee/issues/5387)
-    * Git LFS is currently not supported: [gitlab-ee#6195](https://gitlab.com/gitlab-org/gitlab-ee/issues/6195)
 - The primary node has to be online for OAuth login to happen (existing sessions and Git are not affected)
 - The installation takes multiple manual steps that together can take about an hour depending on circumstances; we are
   working on improving this experience, see [gitlab-org/omnibus-gitlab#2978] for details.

ee/app/controllers/ee/projects/git_http_client_controller.rb

@@ -3,18 +3,90 @@ module EE
     module GitHttpClientController
       extend ActiveSupport::Concern
 
+      ALLOWED_CONTROLLER_AND_ACTIONS = {
+        'git_http' => %w{git_receive_pack},
+        'lfs_api' => %w{batch},
+        'lfs_locks_api' => %w{create unlock verify}
+      }.freeze
+
       prepended do
-        before_action :redirect_push_to_primary, only: [:info_refs]
+        before_action do
+          redirect_to(primary_full_url) if redirect?
+        end
       end
 
       private
 
-      def redirect_push_to_primary
-        redirect_to(primary_full_url) if redirect_push_to_primary?
+      class RouteHelper
+        attr_reader :controller_name, :action_name
+
+        def initialize(controller_name, action_name, params, allowed)
+          @controller_name = controller_name
+          @action_name = action_name
+          @params = params
+          @allowed = allowed
+        end
+
+        def match?(c_name, a_name)
+          controller_name == c_name && action_name == a_name
+        end
+
+        def allowed_match?
+          !!allowed[controller_name]&.include?(action_name)
+        end
+
+        def service_or_action_name
+          action_name == 'info_refs' ? params[:service] : action_name.dasherize
+        end
+
+        private
+
+        attr_reader :params, :allowed
       end
 
-      def primary_full_url
-        File.join(::Gitlab::Geo.primary_node.url, request_fullpath_for_primary)
+      class GitLFSHelper
+        MINIMUM_GIT_LFS_VERSION = '2.4.2'.freeze
+
+        def initialize(current_version)
+          @current_version = current_version
+        end
+
+        def version_ok?
+          return false unless current_version
+
+          ::Gitlab::VersionInfo.parse(current_version) >= wanted_version
+        end
+
+        def incorrect_version_opts
+          {
+            json: { message: incorrect_version_message },
+            content_type: ::LfsRequest::CONTENT_TYPE,
+            status: 403
+          }
+        end
+
+        private
+
+        attr_reader :current_version
+
+        def wanted_version
+          ::Gitlab::VersionInfo.parse(MINIMUM_GIT_LFS_VERSION)
+        end
+
+        def incorrect_version_message
+          _("You need git-lfs version %{min_git_lfs_version} (or greater) to
+            continue. Please visit https://git-lfs.github.com") %
+            { min_git_lfs_version: MINIMUM_GIT_LFS_VERSION }
+        end
+      end
+
+      def route_helper
+        @route_helper ||= RouteHelper.new(controller_name, action_name, params,
+                    ALLOWED_CONTROLLER_AND_ACTIONS)
+      end
+
+      def git_lfs_helper
+        @git_lfs_helper ||= GitLFSHelper.new(current_git_lfs_version)
       end
 
       def request_fullpath_for_primary
@@ -22,12 +94,30 @@ module EE
         request.fullpath.sub(relative_url_root, '')
       end
 
-      def redirect_push_to_primary?
-        git_push_request? && ::Gitlab::Geo.secondary_with_primary?
+      def primary_full_url
+        File.join(::Gitlab::Geo.primary_node.url, request_fullpath_for_primary)
       end
 
-      def git_push_request?
-        git_command == 'git-receive-pack'
+      def current_git_lfs_version
+        request.headers['User-Agent']
+      end
+
+      def redirect?
+        ::Gitlab::Geo.secondary_with_primary? && match? && !filtered?
+      end
+
+      def match?
+        route_helper.service_or_action_name == 'git-receive-pack' ||
+          route_helper.allowed_match?
+      end
+
+      def filtered?
+        if route_helper.match?('lfs_api', 'batch') && !git_lfs_helper.version_ok?
+          render(git_lfs_helper.incorrect_version_opts)
+          return true
+        end
+
+        false
       end
     end
   end

ee/app/controllers/ee/projects/lfs_api_controller.rb

@@ -3,11 +3,27 @@ module EE
     module LfsApiController
       extend ::Gitlab::Utils::Override
 
+      override :batch_operation_disallowed?
+      def batch_operation_disallowed?
+        super_result = super
+        return true if super_result && !::Gitlab::Geo.enabled?
+
+        if super_result && ::Gitlab::Geo.enabled?
+          return true if !::Gitlab::Geo.primary? && !::Gitlab::Geo.secondary?
+          return true if ::Gitlab::Geo.secondary? && !::Gitlab::Geo.primary_node_configured?
+        end
+
+        false
+      end
+
       override :lfs_read_only_message
       def lfs_read_only_message
         return super unless ::Gitlab::Geo.secondary_with_primary?
 
-        (_('You cannot write to a read-only secondary GitLab Geo instance. Please use %{link_to_primary_node} instead.') % { link_to_primary_node: geo_primary_default_url_to_repo(project) }).html_safe
+        (_('You cannot write to a read-only secondary GitLab Geo instance.
+          Please use %{link_to_primary_node} instead.') %
+          { link_to_primary_node: geo_primary_default_url_to_repo(project) }
+        ).html_safe
       end
     end
   end

ee/changelogs/unreleased/6195-http-git-push-to-secondary-for-git-lfs-currently-does-not-work.yml

+---
+title: 'Geo: HTTP git-lfs push (upload) and locks (verify, lock and unlock) to secondary now redirects to the primary'
+merge_request: 6109
+author:
+type: added

ee/spec/requests/git_http_geo_spec.rb

@@ -5,6 +5,7 @@ describe "Git HTTP requests (Geo)" do
   include ::EE::GeoHelpers
   include GitHttpHelpers
   include WorkhorseHelpers
+  using RSpec::Parameterized::TableSyntax
 
   set(:project) { create(:project, :repository, :private) }
   set(:primary) { create(:geo_node, :primary) }
@@ -116,6 +117,80 @@ describe "Git HTTP requests (Geo)" do
     end
   end
 
+  context 'git-lfs' do
+    context 'API' do
+      describe 'POST batch' do
+        def make_request
+          post url, {}, env.merge(extra_env)
+        end
+
+        let(:extra_env) { {} }
+        let(:incorrect_version_regex) { /You need git-lfs version 2.4.2/ }
+        let(:url) { "/#{project.full_path}.git/info/lfs/objects/batch" }
+
+        subject do
+          make_request
+          response
+        end
+
+        context 'with the correct git-lfs version' do
+          let(:extra_env) { { 'User-Agent' => 'git-lfs/2.4.2 (GitHub; darwin amd64; go 1.10.2)' } }
+
+          it 'redirects to the primary' do
+            is_expected.to have_gitlab_http_status(:redirect)
+            redirect_location = "#{primary.url.chomp('/')}#{url}"
+            expect(subject.header['Location']).to eq(redirect_location)
+          end
+        end
+
+        where(:description, :version) do
+          'outdated' | 'git-lfs/2.4.1'
+          'unknown'  | 'git-lfs'
+        end
+
+        with_them do
+          context "with an #{description} git-lfs version" do
+            let(:extra_env) { { 'User-Agent' => "#{version} (GitHub; darwin amd64; go 1.10.2)" } }
+
+            it 'errors out' do
+              is_expected.to have_gitlab_http_status(:forbidden)
+              expect(json_response['message']).to match(incorrect_version_regex)
+            end
+          end
+        end
+      end
+    end
+
+    context 'Locks API' do
+      where(:description, :path, :args) do
+        'create' | 'info/lfs/locks'          | {}
+        'verify' | 'info/lfs/locks/verify'   | {}
+        'unlock' | 'info/lfs/locks/1/unlock' | { id: 1 }
+      end
+
+      with_them do
+        describe "POST #{description}" do
+          def make_request
+            post url, args, env
+          end
+
+          let(:url) { "/#{project.full_path}.git/#{path}" }
+
+          subject do
+            make_request
+            response
+          end
+
+          it 'redirects to the primary' do
+            is_expected.to have_gitlab_http_status(:redirect)
+            redirect_location = "#{primary.url.chomp('/')}#{url}"
+            expect(subject.header['Location']).to eq(redirect_location)
+          end
+        end
+      end
+    end
+  end
+
   def valid_geo_env
     geo_env(auth_token)
   end

lib/gitlab/middleware/read_only/controller.rb

@@ -4,8 +4,18 @@ module Gitlab
       class Controller
         DISALLOWED_METHODS = %w(POST PATCH PUT DELETE).freeze
         APPLICATION_JSON = 'application/json'.freeze
+        APPLICATION_JSON_TYPES = %W{#{APPLICATION_JSON} application/vnd.git-lfs+json}.freeze
         ERROR_MESSAGE = 'You cannot perform write operations on a read-only instance'.freeze
 
+        WHITELISTED_GIT_ROUTES = {
+          'projects/git_http' => %w{git_upload_pack git_receive_pack}
+        }.freeze
+
+        WHITELISTED_GIT_LFS_ROUTES = {
+          'projects/lfs_api' => %w{batch},
+          'projects/lfs_locks_api' => %w{verify create unlock}
+        }.freeze
+
         def initialize(app, env)
           @app = app
           @env = env
@@ -36,7 +46,7 @@ module Gitlab
         end
 
         def json_request?
-          request.media_type == APPLICATION_JSON
+          APPLICATION_JSON_TYPES.include?(request.media_type)
         end
 
         def rack_flash
@@ -63,22 +73,27 @@ module Gitlab
           grack_route || ReadOnly.internal_routes.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route
         end
 
-        def sidekiq_route
-          request.path.start_with?('/admin/sidekiq')
-        end
-
         def grack_route
           # Calling route_hash may be expensive. Only do it if we think there's a possible match
-          return false unless request.path.end_with?('.git/git-upload-pack')
+          return false unless
+            request.path.end_with?('.git/git-upload-pack', '.git/git-receive-pack')
 
-          route_hash[:controller] == 'projects/git_http' && route_hash[:action] == 'git_upload_pack'
+          WHITELISTED_GIT_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
         end
 
         def lfs_route
           # Calling route_hash may be expensive. Only do it if we think there's a possible match
-          return false unless request.path.end_with?('/info/lfs/objects/batch')
+          unless request.path.end_with?('/info/lfs/objects/batch',
+            '/info/lfs/locks', '/info/lfs/locks/verify') ||
+              %r{/info/lfs/locks/\d+/unlock\z}.match?(request.path)
+            return false
+          end
+
+          WHITELISTED_GIT_LFS_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
+        end
 
-          route_hash[:controller] == 'projects/lfs_api' && route_hash[:action] == 'batch'
+        def sidekiq_route
+          request.path.start_with?('/admin/sidekiq')
         end
       end
     end

spec/lib/gitlab/middleware/read_only_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe Gitlab::Middleware::ReadOnly do
   include Rack::Test::Methods
+  using RSpec::Parameterized::TableSyntax
 
   RSpec::Matchers.define :be_a_redirect do
     match do |response|
@@ -117,39 +118,41 @@ describe Gitlab::Middleware::ReadOnly do
     context 'whitelisted requests' do
       it 'expects a POST internal request to be allowed' do
         expect(Rails.application.routes).not_to receive(:recognize_path)
-
         response = request.post("/api/#{API::API.version}/internal")
 
         expect(response).not_to be_a_redirect
         expect(subject).not_to disallow_request
       end
 
-      it 'expects a POST LFS request to batch URL to be allowed' do
-        expect(Rails.application.routes).to receive(:recognize_path).and_call_original
-        response = request.post('/root/rouge.git/info/lfs/objects/batch')
+      it 'expects requests to sidekiq admin to be allowed' do
+        response = request.post('/admin/sidekiq')
 
         expect(response).not_to be_a_redirect
         expect(subject).not_to disallow_request
-      end
 
-      it 'expects a POST request to git-upload-pack URL to be allowed' do
-        expect(Rails.application.routes).to receive(:recognize_path).and_call_original
-        response = request.post('/root/rouge.git/git-upload-pack')
+        response = request.get('/admin/sidekiq')
 
         expect(response).not_to be_a_redirect
         expect(subject).not_to disallow_request
       end
 
-      it 'expects requests to sidekiq admin to be allowed' do
-        response = request.post('/admin/sidekiq')
-
-        expect(response).not_to be_a_redirect
-        expect(subject).not_to disallow_request
+      where(:description, :path) do
+        'LFS request to batch'        | '/root/rouge.git/info/lfs/objects/batch'
+        'LFS request to locks verify' | '/root/rouge.git/info/lfs/locks/verify'
+        'LFS request to locks create' | '/root/rouge.git/info/lfs/locks'
+        'LFS request to locks unlock' | '/root/rouge.git/info/lfs/locks/1/unlock'
+        'request to git-upload-pack'  | '/root/rouge.git/git-upload-pack'
+        'request to git-receive-pack' | '/root/rouge.git/git-receive-pack'
+      end
 
-        response = request.get('/admin/sidekiq')
+      with_them do
+        it "expects a POST #{description} URL to be allowed" do
+          expect(Rails.application.routes).to receive(:recognize_path).and_call_original
+          response = request.post(path)
 
-        expect(response).not_to be_a_redirect
-        expect(subject).not_to disallow_request
+          expect(response).not_to be_a_redirect
+          expect(subject).not_to disallow_request
+        end
       end
     end
   end