Merge branch...

Merge branch '7419-running-git-clone-with-a-secondary-geo-node-url-has-lfs-batch-download-redirected-to-primary' into 'master'

Resolve "Running git clone with a secondary geo node URL has LFS batch download redirected to primary"

Closes #7419

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

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

@@ -3,12 +3,29 @@ 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
-
+      # This module is responsible for determining if an incoming secondary bound
+      # HTTP request should be redirected to the primary.
+      #
+      # Why?  A secondary is not allowed to perform any write actions, so any
+      # request of this type need to be sent through to the primary.  By
+      # redirecting within code, we allow clients to git pull/push using their
+      # secondary git remote without needing an additional primary remote.
+      #
+      # Current secondary HTTP requests to redirect: -
+      #
+      # * git push
+      #   * GET   /repo.git/info/refs?service=git-receive-pack
+      #   * POST  /repo.git/git-receive-pack
+      #
+      # * git lfs push (usually happens automatically as part of a `git push`)
+      #   * POST  /repo.git/info/lfs/objects/batch (and we examine
+      #     params[:operation] to ensure we're dealing with an upload request)
+      #
+      # For more detail, see the following links:
+      #
+      # git: https://git-scm.com/book/en/v2/Git-Internals-Transfer-Protocols
+      # git-lfs: https://github.com/git-lfs/git-lfs/blob/master/docs/api
+      #
       prepended do
         before_action do
           redirect_to(primary_full_url) if redirect?
@@ -20,44 +37,69 @@ module EE
       class RouteHelper
         attr_reader :controller_name, :action_name
 
-        def initialize(controller_name, action_name, params, allowed)
+        CONTROLLER_AND_ACTIONS_TO_REDIRECT = {
+          'git_http' => %w{git_receive_pack},
+          'lfs_locks_api' => %w{create unlock verify}
+        }.freeze
+
+        def initialize(controller_name, action_name, service)
           @controller_name = controller_name
           @action_name = action_name
-          @params = params
-          @allowed = allowed
+          @service = service
         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)
+        def redirect?
+          !!CONTROLLER_AND_ACTIONS_TO_REDIRECT[controller_name]&.include?(action_name) ||
+            git_receive_pack_request?
         end
 
+        private
+
+        attr_reader :service
+
+        # Examples:
+        #
+        # /repo.git/info/refs?service=git-receive-pack returns 'git-receive-pack'
+        # /repo.git/info/refs?service=git-upload-pack returns 'git-upload-pack'
+        # /repo.git/git-receive-pack returns 'git-receive-pack'
+        # /repo.git/git-upload-pack returns 'git-upload-pack'
+        #
         def service_or_action_name
-          action_name == 'info_refs' ? params[:service] : action_name.dasherize
+          info_refs_request? ? service : action_name.dasherize
         end
 
-        private
+        # Matches:
+        #
+        # GET  /repo.git/info/refs?service=git-receive-pack
+        # POST /repo.git/git-receive-pack
+        #
+        def git_receive_pack_request?
+          service_or_action_name == 'git-receive-pack'
+        end
 
-        attr_reader :params, :allowed
+        # Matches:
+        #
+        # GET /repo.git/info/refs
+        #
+        def info_refs_request?
+          action_name == 'info_refs'
+        end
       end
 
       class GitLFSHelper
         MINIMUM_GIT_LFS_VERSION = '2.4.2'.freeze
 
-        def initialize(current_version)
+        def initialize(route_helper, operation, current_version)
+          @route_helper = route_helper
+          @operation = operation
           @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
+        def incorrect_version_response
           {
             json: { message: incorrect_version_message },
             content_type: ::LfsRequest::CONTENT_TYPE,
@@ -65,27 +107,44 @@ module EE
           }
         end
 
-        private
+        def redirect?
+          return false unless route_helper.match?('lfs_api', 'batch')
+          return true if upload?
 
-        attr_reader :current_version
+          false
+        end
 
-        def wanted_version
-          ::Gitlab::VersionInfo.parse(MINIMUM_GIT_LFS_VERSION)
+        def version_ok?
+          return false unless current_version
+
+          ::Gitlab::VersionInfo.parse(current_version) >= wanted_version
         end
 
+        private
+
+        attr_reader :route_helper, :operation, :current_version
+
         def incorrect_version_message
           translation = _("You need git-lfs version %{min_git_lfs_version} (or greater) to continue. Please visit https://git-lfs.github.com")
           translation % { min_git_lfs_version: MINIMUM_GIT_LFS_VERSION }
         end
+
+        def upload?
+          operation == 'upload'
+        end
+
+        def wanted_version
+          ::Gitlab::VersionInfo.parse(MINIMUM_GIT_LFS_VERSION)
+        end
       end
 
       def route_helper
-        @route_helper ||= RouteHelper.new(controller_name, action_name, params,
-                    ALLOWED_CONTROLLER_AND_ACTIONS)
+        @route_helper ||= RouteHelper.new(controller_name, action_name, params[:service])
       end
 
       def git_lfs_helper
-        @git_lfs_helper ||= GitLFSHelper.new(current_git_lfs_version)
+        # params[:operation] explained: https://github.com/git-lfs/git-lfs/blob/master/docs/api/batch.md#requests
+        @git_lfs_helper ||= GitLFSHelper.new(route_helper, params[:operation], request.headers['User-Agent'])
       end
 
       def request_fullpath_for_primary
@@ -97,25 +156,28 @@ module EE
         File.join(::Gitlab::Geo.primary_node.url, request_fullpath_for_primary)
       end
 
-      def current_git_lfs_version
-        request.headers['User-Agent']
-      end
-
       def redirect?
-        ::Gitlab::Geo.secondary_with_primary? && match? && !filtered?
-      end
+        # Don't redirect if we're not a secondary with a primary
+        return false unless ::Gitlab::Geo.secondary_with_primary?
 
-      def match?
-        route_helper.service_or_action_name == 'git-receive-pack' ||
-          route_helper.allowed_match?
-      end
+        # Redirect as the request matches RouteHelper::CONTROLLER_AND_ACTIONS_TO_REDIRECT
+        return true if route_helper.redirect?
+
+        # Look to redirect, as we're an LFS batch upload request
+        if git_lfs_helper.redirect?
+          # Redirect as git-lfs version is at least 2.4.2
+          return true if git_lfs_helper.version_ok?
+
+          # git-lfs 2.4.2 is really only required for requests that involve
+          # redirection, so we only render if it's an LFS upload operation
+          #
+          render(git_lfs_helper.incorrect_version_response)
 
-      def filtered?
-        if route_helper.match?('lfs_api', 'batch') && !git_lfs_helper.version_ok?
-          render(git_lfs_helper.incorrect_version_opts)
-          return true
+          # Don't redirect
+          return false
         end
 
+        # Don't redirect
         false
       end
     end

ee/changelogs/unreleased/7419-running-git-clone-with-a-secondary-geo-node-url-has-lfs-batch-download-redirected-to-primary.yml

+---
+title: 'Geo: LFS batch downloads are OK to be handled by secondary'
+merge_request: 7209
+author:
+type: fixed

ee/spec/requests/git_http_geo_spec.rb

@@ -121,11 +121,10 @@ describe "Git HTTP requests (Geo)" do
     context 'API' do
       describe 'POST batch' do
         def make_request
-          post url, {}, env.merge(extra_env)
+          post url, args, env
         end
 
-        let(:extra_env) { {} }
-        let(:incorrect_version_regex) { /You need git-lfs version 2.4.2/ }
+        let(:args) { {} }
         let(:url) { "/#{project.full_path}.git/info/lfs/objects/batch" }
 
         subject do
@@ -133,28 +132,82 @@ describe "Git HTTP requests (Geo)" do
           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)' } }
+        before do
+          allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+          project.update_attribute(:lfs_enabled, true)
+          env['Content-Type'] = LfsRequest::CONTENT_TYPE
+        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)
+        context 'operation upload' do
+          let(:args) { { 'operation' => 'upload' }.to_json }
+
+          context 'with the correct git-lfs version' do
+            before do
+              env['User-Agent'] = 'git-lfs/2.4.2 (GitHub; darwin amd64; go 1.10.2)'
+            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
 
-        where(:description, :version) do
-          'outdated' | 'git-lfs/2.4.1'
-          'unknown'  | 'git-lfs'
+          context 'with an incorrect git-lfs version' do
+            where(:description, :version) do
+              'outdated' | 'git-lfs/2.4.1'
+              'unknown'  | 'git-lfs'
+            end
+
+            with_them do
+              context "that is #{description}" do
+                before do
+                  env['User-Agent'] = "#{version} (GitHub; darwin amd64; go 1.10.2)"
+                end
+
+                it 'is forbidden' do
+                  is_expected.to have_gitlab_http_status(:forbidden)
+                  expect(json_response['message']).to match(/You need git-lfs version 2.4.2/)
+                end
+              end
+            end
+          end
         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)" } }
+        context 'operation download' do
+          let(:user) { create(:user) }
+          let(:authorization) { ActionController::HttpAuthentication::Basic.encode_credentials(user.username, user.password) }
+          let(:lfs_object) { create(:lfs_object, :with_file) }
+          let(:args) do
+            {
+              'operation' => 'download',
+              'objects' => [{ 'oid' => lfs_object.oid, 'size' => lfs_object.size }]
+            }.to_json
+          end
+
+          before do
+            project.add_maintainer(user)
+            env['Authorization'] = authorization
+          end
+
+          it 'is handled by the secondary' do
+            is_expected.to have_gitlab_http_status(:ok)
+          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
+              before do
+                env['User-Agent'] = "#{version} (GitHub; darwin amd64; go 1.10.2)"
+              end
 
-            it 'errors out' do
-              is_expected.to have_gitlab_http_status(:forbidden)
-              expect(json_response['message']).to match(incorrect_version_regex)
+              it 'is handled by the secondary' do
+                is_expected.to have_gitlab_http_status(:ok)
+              end
             end
           end
         end