Store the secondary Geo node ID

At this point, we are the primary node, and the secondary has
redirected to us. We need to store the secondary Geo node ID
because Gitaly will make a request to the /internal/post_receive
API endpoint after we’ve responded OK to Workhorse.

Later, in /internal/post_receive, we will fetch the node ID and
generate a replication lag message.

config/routes/project.rb

 resources :projects, only: [:index, :new, :create]
 
+Gitlab.ee do
+  scope "/-/push_from_secondary/:geo_node_id" do
+    draw :git_http
+  end
+end
+
 draw :git_http
 
 get '/projects/:id' => 'projects#resolve'

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

@@ -155,7 +155,13 @@ module EE
       end
 
       def primary_full_url
-        ::Gitlab::Utils.append_path(::Gitlab::Geo.primary_node.internal_url, request_fullpath_for_primary)
+        path = File.join(secondary_referrer_path_prefix, request_fullpath_for_primary)
+
+        ::Gitlab::Utils.append_path(::Gitlab::Geo.primary_node.internal_url, path)
+      end
+
+      def secondary_referrer_path_prefix
+        File.join(::Gitlab::Geo::GitPushHttp::PATH_PREFIX, ::Gitlab::Geo.current_node.id.to_s)
       end
 
       def redirect?

ee/app/controllers/ee/projects/git_http_controller.rb

@@ -13,6 +13,21 @@ module EE
         render json: ::Gitlab::Workhorse.git_http_ok(repository, repo_type, user, action_name, show_all_refs: geo_request?)
       end
 
+      override :git_receive_pack
+      def git_receive_pack
+        # Authentication/authorization already happened in `before_action`s
+
+        if ::Gitlab::Geo.primary?
+          # This ID is used by the /internal/post_receive API call
+          gl_id = ::Gitlab::GlId.gl_id(user)
+          gl_repository = repo_type.identifier_for_subject(project)
+          node_id = params["geo_node_id"]
+          ::Gitlab::Geo::GitPushHttp.new(gl_id, gl_repository).cache_referrer_node(node_id)
+        end
+
+        super
+      end
+
       private
 
       def user

ee/changelogs/unreleased/mk-add-secondary-lag-message-on-http-push-geo.yml

+---
+title: Add secondary lag message on Git push over HTTP
+merge_request: 15901
+author:
+type: added

ee/lib/ee/api/internal/base.rb

@@ -14,6 +14,26 @@ module EE
             def lfs_authentication_url(project)
               project.lfs_http_url_to_repo(params[:operation])
             end
+
+            override :ee_post_receive_response_hook
+            def ee_post_receive_response_hook(response)
+              response.add_basic_message(geo_secondary_lag_message) if ::Gitlab::Geo.primary?
+            end
+
+            def geo_secondary_lag_message
+              lag = current_replication_lag
+              return if lag.to_i <= 0
+
+              "Current replication lag: #{lag} seconds"
+            end
+
+            def current_replication_lag
+              fetch_geo_node_referrer&.status&.db_replication_lag_seconds
+            end
+
+            def fetch_geo_node_referrer
+              ::Gitlab::Geo::GitPushHttp.new(params[:identifier], params[:gl_repository]).fetch_referrer_node
+            end
           end
         end
       end

ee/lib/gitlab/geo/git_push_http.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Geo
+    class GitPushHttp
+      PATH_PREFIX = '/-/push_from_secondary'
+      CACHE_KEY_PREFIX = 'git_receive_pack:geo_node_id'
+      EXPIRES_IN = 5.minutes
+
+      def initialize(gl_id, gl_repository)
+        @gl_id = gl_id
+        @gl_repository = gl_repository
+      end
+
+      def cache_referrer_node(geo_node_id)
+        geo_node_id = geo_node_id.to_i
+        return unless geo_node_id > 0
+
+        Rails.cache.write(cache_key, geo_node_id, expires_in: EXPIRES_IN)
+      end
+
+      def fetch_referrer_node
+        id = Rails.cache.read(cache_key)
+
+        if id
+          # There is a race condition but since this is only used to display a
+          # notice, it's ok. If we didn't delete it, then a subsequent push
+          # directly to the primary would inappropriately show the secondary lag
+          # notice again.
+          Rails.cache.delete(cache_key)
+
+          GeoNode.find_by_id(id)
+        end
+      end
+
+      private
+
+      def cache_key
+        [
+          CACHE_KEY_PREFIX,
+          @gl_id,
+          @gl_repository
+        ].join(':')
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/geo/git_push_http_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Geo::GitPushHttp, :geo, :use_clean_rails_memory_store_caching do
+  include EE::GeoHelpers
+
+  let(:gl_id) { 'user-1234' }
+  let(:gl_repository) { 'project-77777' }
+  let(:cache_key) { "#{described_class::CACHE_KEY_PREFIX}:#{gl_id}:#{gl_repository}" }
+  set(:secondary) { create(:geo_node) }
+  subject { described_class.new(gl_id, gl_repository) }
+
+  describe '#cache_referrer_node' do
+    context 'when geo_node_id is present' do
+      context 'when geo_node_id is an integer' do
+        it 'stores the ID in cache' do
+          subject.cache_referrer_node(secondary.id)
+
+          value = Rails.cache.read(cache_key)
+          expect(value).to eq(secondary.id)
+        end
+
+        it 'stores the ID with an expiration' do
+          Timecop.freeze do
+            subject.cache_referrer_node(secondary.id)
+
+            Timecop.travel(described_class::EXPIRES_IN + 20.seconds) do
+              value = Rails.cache.read(cache_key)
+              expect(value).to be_nil
+            end
+          end
+        end
+      end
+
+      context 'when geo_node_id is not an integer' do
+        it 'does not cache anything' do
+          subject.cache_referrer_node('bad input')
+
+          value = Rails.cache.read(cache_key)
+          expect(value).to be_nil
+        end
+      end
+    end
+
+    context 'when geo_node_id is blank' do
+      it 'does not cache anything' do
+        subject.cache_referrer_node(' ')
+
+        value = Rails.cache.read(cache_key)
+        expect(value).to be_nil
+      end
+    end
+  end
+
+  describe '#fetch_referrer_node' do
+    context 'when there is a cached ID' do
+      it 'deletes the key' do
+        Rails.cache.write(cache_key, secondary.id, expires_in: described_class::EXPIRES_IN)
+
+        subject.fetch_referrer_node
+
+        expect(subject.fetch_referrer_node).to be_nil
+      end
+
+      context 'when the GeoNode exists' do
+        it 'returns the GeoNode with the cached ID' do
+          Rails.cache.write(cache_key, secondary.id, expires_in: described_class::EXPIRES_IN)
+
+          expect(subject.fetch_referrer_node).to eq(secondary)
+        end
+      end
+
+      context 'when the GeoNode does not exist' do
+        it 'returns nil' do
+          Rails.cache.write(cache_key, 9999998, expires_in: described_class::EXPIRES_IN)
+
+          expect(subject.fetch_referrer_node).to be_nil
+        end
+      end
+    end
+
+    context 'when there is no cached ID' do
+      it 'returns nil' do
+        expect(subject.fetch_referrer_node).to be_nil
+      end
+    end
+  end
+end

ee/spec/requests/api/internal/base_spec.rb

@@ -4,6 +4,113 @@ require 'spec_helper'
 describe API::Internal::Base do
   include EE::GeoHelpers
 
+  set(:primary_node) { create(:geo_node, :primary) }
+  set(:secondary_node) { create(:geo_node) }
+
+  describe 'POST /internal/post_receive', :geo do
+    set(:user) { create(:user) }
+    let(:key) { create(:key, user: user) }
+    set(:project) { create(:project, :repository, :wiki_repo) }
+    let(:secret_token) { Gitlab::Shell.secret_token }
+    let(:gl_repository) { "project-#{project.id}" }
+    let(:reference_counter) { double('ReferenceCounter') }
+
+    let(:identifier) { 'key-123' }
+
+    let(:valid_params) do
+      {
+        gl_repository: gl_repository,
+        secret_token: secret_token,
+        identifier: identifier,
+        changes: changes,
+        push_options: {}
+      }
+    end
+
+    let(:branch_name) { 'feature' }
+
+    let(:changes) do
+      "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{branch_name}"
+    end
+
+    let(:git_push_http) { double('GitPushHttp') }
+
+    before do
+      project.add_developer(user)
+      allow(described_class).to receive(:identify).and_return(user)
+      allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(user)
+      stub_current_geo_node(primary_node)
+    end
+
+    context 'when the push was redirected from a Geo secondary to the primary' do
+      before do
+        expect(Gitlab::Geo::GitPushHttp).to receive(:new).with(identifier, gl_repository).and_return(git_push_http)
+        expect(git_push_http).to receive(:fetch_referrer_node).and_return(secondary_node)
+      end
+
+      context 'when the secondary has a GeoNodeStatus' do
+        context 'when the GeoNodeStatus db_replication_lag_seconds is greater than 0' do
+          let!(:status) { create(:geo_node_status, geo_node: secondary_node, db_replication_lag_seconds: 17) }
+
+          it 'includes current Geo secondary lag in the output' do
+            post api('/internal/post_receive'), params: valid_params
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(json_response['messages']).to include({
+              'type' => 'basic',
+              'message' => "Current replication lag: 17 seconds"
+            })
+          end
+        end
+
+        context 'when the GeoNodeStatus db_replication_lag_seconds is 0' do
+          let!(:status) { create(:geo_node_status, geo_node: secondary_node, db_replication_lag_seconds: 0) }
+
+          it 'does not include current Geo secondary lag in the output' do
+            post api('/internal/post_receive'), params: valid_params
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(json_response['messages']).not_to include({ 'message' => a_string_matching('replication lag'), 'type' => anything })
+          end
+        end
+
+        context 'when the GeoNodeStatus db_replication_lag_seconds is nil' do
+          let!(:status) { create(:geo_node_status, geo_node: secondary_node, db_replication_lag_seconds: nil) }
+
+          it 'does not include current Geo secondary lag in the output' do
+            post api('/internal/post_receive'), params: valid_params
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(json_response['messages']).not_to include({ 'message' => a_string_matching('replication lag'), 'type' => anything })
+          end
+        end
+      end
+
+      context 'when the secondary does not have a GeoNodeStatus' do
+        it 'does not include current Geo secondary lag in the output' do
+          post api('/internal/post_receive'), params: valid_params
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['messages']).not_to include({ 'message' => a_string_matching('replication lag'), 'type' => anything })
+        end
+      end
+    end
+
+    context 'when the push was not redirected from a Geo secondary to the primary' do
+      before do
+        expect(Gitlab::Geo::GitPushHttp).to receive(:new).with(identifier, gl_repository).and_return(git_push_http)
+        expect(git_push_http).to receive(:fetch_referrer_node).and_return(nil)
+      end
+
+      it 'does not include current Geo secondary lag in the output' do
+        post api('/internal/post_receive'), params: valid_params
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['messages']).not_to include({ 'message' => a_string_matching('replication lag'), 'type' => anything })
+      end
+    end
+  end
+
   describe "POST /internal/allowed" do
     set(:user) { create(:user) }
     set(:key) { create(:key, user: user) }
@@ -147,17 +254,14 @@ describe API::Internal::Base do
     end
   end
 
-  describe "POST /internal/lfs_authenticate" do
+  describe "POST /internal/lfs_authenticate", :geo do
     let(:user) { create(:user) }
     let(:project) { create(:project, :repository) }
     let(:secret_token) { Gitlab::Shell.secret_token }
 
     context 'for a secondary node' do
-      let!(:primary) { create(:geo_node, :primary) }
-      let!(:secondary) { create(:geo_node) }
-
       before do
-        stub_current_geo_node(secondary)
+        stub_current_geo_node(secondary_node)
         project.add_developer(user)
       end
 

ee/spec/requests/git_http_geo_spec.rb

@@ -113,7 +113,7 @@ describe "Git HTTP requests (Geo)", :geo do
 
         it 'redirects to the primary' do
           is_expected.to have_gitlab_http_status(:redirect)
-          redirect_location = "#{primary.url.chomp('/')}#{url}?service=git-receive-pack"
+          redirect_location = "#{redirected_primary_url}?service=git-receive-pack"
           expect(subject.header['Location']).to eq(redirect_location)
         end
       end
@@ -166,7 +166,7 @@ describe "Git HTTP requests (Geo)", :geo do
 
               it 'redirects to the primary' do
                 is_expected.to have_gitlab_http_status(:redirect)
-                redirect_location = "#{primary.url.chomp('/')}#{url}"
+                redirect_location = "#{redirected_primary_url}"
                 expect(subject.header['Location']).to eq(redirect_location)
               end
             end
@@ -254,97 +254,136 @@ describe "Git HTTP requests (Geo)", :geo do
 
             it 'redirects to the primary' do
               is_expected.to have_gitlab_http_status(:redirect)
-              redirect_location = "#{primary.url.chomp('/')}#{url}"
+              redirect_location = "#{redirected_primary_url}"
               expect(subject.header['Location']).to eq(redirect_location)
             end
           end
         end
       end
     end
+
+    def redirected_primary_url
+      "#{primary.url.chomp('/')}#{::Gitlab::Geo::GitPushHttp::PATH_PREFIX}/#{secondary.id}#{url}"
+    end
   end
 
-  context 'when current node is the primary' do
+  context 'when current node is the primary', :use_clean_rails_memory_store_caching do
     let(:current_node) { primary }
 
     describe 'POST git_receive_pack' do
-      def make_request
-        post url, params: {}, headers: env
-      end
-
-      let(:url) { "/#{project.full_path}.git/git-receive-pack" }
-
-      before do
-        env['Geo-GL-Id'] = geo_gl_id
-      end
-
       subject do
         make_request
         response
       end
 
-      context 'when gl_id is incorrectly provided via HTTP headers' do
-        where(:geo_gl_id) do
-          [
-            nil,
-            ''
-          ]
+      context 'when HTTP redirected from a secondary node' do
+        def make_request
+          post url, headers: auth_env(user.username, user.password, nil)
         end
 
-        with_them do
-          it 'returns a 403' do
-            is_expected.to have_gitlab_http_status(:forbidden)
-            expect(response.body).to eql('You are not allowed to upload code for this project.')
-          end
+        let(:identifier) { "user-#{user.id}" }
+        let(:gl_repository) { "project-#{project.id}" }
+        let(:url) { "#{::Gitlab::Geo::GitPushHttp::PATH_PREFIX}/#{secondary.id}/#{project.full_path}.git/git-receive-pack" }
+
+        # The bigger picture request flow relevant to this feature is:
+        #
+        #   * The HTTP request hits NGINX
+        #   * Then Workhorse
+        #   * Then Rails (the scope of request tests is limited to this line item)
+        #   * Rails responds OK to Workhorse
+        #   * Workhorse connects to Gitaly: SmartHTTP Service, ReceivePack RPC
+        #   * In a pre-receive hook, Gitaly makes a request to Rails' POST /api/v4/internal/allowed
+        #   * Rails says OK
+        #   * In a post-receive hook, Gitaly makes a request to Rails' POST /api/v4/internal/post_receive
+        #   * Rails responds to Gitaly, including a collection of messages, which includes the replication lag message
+        #   * Gitaly outputs the messages in the stream of Proto messages
+        #   * Pipe the output through Workhorse and NGINX
+        #
+        # See https://gitlab.com/gitlab-org/gitlab-ee/issues/9195
+        #
+        it 'stores the secondary node ID so the internal API post_receive request can generate the replication lag message' do
+          is_expected.to have_gitlab_http_status(:ok)
+
+          stored_node = ::Gitlab::Geo::GitPushHttp.new(identifier, gl_repository).fetch_referrer_node
+          expect(stored_node).to eq(secondary)
         end
       end
 
-      context 'when gl_id is provided via HTTP headers' do
-        context 'but is invalid' do
+      context 'when proxying an SSH request from a secondary node' do
+        def make_request
+          post url, params: {}, headers: env
+        end
+
+        let(:url) { "/#{project.full_path}.git/git-receive-pack" }
+
+        before do
+          env['Geo-GL-Id'] = geo_gl_id
+        end
+
+        context 'when gl_id is incorrectly provided via HTTP headers' do
           where(:geo_gl_id) do
             [
-              'key-999',
-              'key-1',
-              'key-999',
-              'junk',
-              'junk-1',
-              'kkey-1'
+              nil,
+              ''
             ]
           end
 
           with_them do
             it 'returns a 403' do
               is_expected.to have_gitlab_http_status(:forbidden)
-              expect(response.body).to eql('Geo push user is invalid.')
+              expect(response.body).to eql('You are not allowed to upload code for this project.')
             end
           end
         end
 
-        context 'and is valid' do
-          context 'but the user has no access' do
-            let(:geo_gl_id) { "key-#{key_for_user_without_any_access.id}" }
+        context 'when gl_id is provided via HTTP headers' do
+          context 'but is invalid' do
+            where(:geo_gl_id) do
+              [
+                'key-999',
+                'key-1',
+                'key-999',
+                'junk',
+                'junk-1',
+                'kkey-1'
+              ]
+            end
 
-            it 'returns a 404' do
-              is_expected.to have_gitlab_http_status(:not_found)
-              expect(response.body).to eql('The project you were looking for could not be found.')
+            with_them do
+              it 'returns a 403' do
+                is_expected.to have_gitlab_http_status(:forbidden)
+                expect(response.body).to eql('Geo push user is invalid.')
+              end
             end
           end
 
-          context 'but the user does not have push access' do
-            let(:geo_gl_id) { "key-#{key_for_user_without_push_access.id}" }
+          context 'and is valid' do
+            context 'but the user has no access' do
+              let(:geo_gl_id) { "key-#{key_for_user_without_any_access.id}" }
 
-            it 'returns a 403' do
-              is_expected.to have_gitlab_http_status(:forbidden)
-              expect(response.body).to eql('You are not allowed to push code to this project.')
+              it 'returns a 404' do
+                is_expected.to have_gitlab_http_status(:not_found)
+                expect(response.body).to eql('The project you were looking for could not be found.')
+              end
             end
-          end
 
-          context 'and the user has push access' do
-            let(:geo_gl_id) { "key-#{key.id}" }
+            context 'but the user does not have push access' do
+              let(:geo_gl_id) { "key-#{key_for_user_without_push_access.id}" }
 
-            it 'returns a 200' do
-              is_expected.to have_gitlab_http_status(:ok)
-              expect(json_response['GL_ID']).to match("user-#{user.id}")
-              expect(json_response['GL_REPOSITORY']).to match(Gitlab::GlRepository::PROJECT.identifier_for_subject(project))
+              it 'returns a 403' do
+                is_expected.to have_gitlab_http_status(:forbidden)
+                expect(response.body).to eql('You are not allowed to push code to this project.')
+              end
+            end
+
+            context 'and the user has push access' do
+              let(:geo_gl_id) { "key-#{key.id}" }
+
+              it 'returns a 200' do
+                is_expected.to have_gitlab_http_status(:ok)
+                expect(json_response['GL_ID']).to match("user-#{user.id}")
+                expect(json_response['GL_REPOSITORY']).to match(Gitlab::GlRepository::PROJECT.identifier_for_subject(project))
+              end
             end
           end
         end

lib/api/internal/base.rb

@@ -22,6 +22,10 @@ module API
           # easily.
           project.http_url_to_repo
         end
+
+        def ee_post_receive_response_hook(response)
+          # Hook for EE to add messages
+        end
       end
 
       namespace 'internal' do
@@ -265,6 +269,8 @@ module API
             response.add_basic_message(project_created_message)
           end
 
+          ee_post_receive_response_hook(response)
+
           present response, with: Entities::InternalPostReceive::Response
         end
       end