Fix health checks not working behind load balancers

The change in
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24199 caused
requests coming from a load balancer to arrive as 127.0.0.1 instead of
the actual IP.

`Rack::Request#ip` behaves slightly differently different than
`ActionDispatch::Request#remote_ip`: the former will return the first
X-Forwarded-For IP if all of the IPs are trusted proxies, while the
second one filters out all proxies and falls back to REMOTE_ADDR, which
is 127.0.0.1.

For now, we can revert back to using `Rack::Request` because these
middlewares don't manipulate parameters. The actual fix problem involves
fixing Rails: https://github.com/rails/rails/issues/28436.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/58573

5 files changed, 74 insertions, 3 deletions

diff --git a/changelogs/unreleased/sh-revert-rack-request-health-checks.yml b/changelogs/unreleased/sh-revert-rack-request-health-checks.yml
new file mode 100644
index 00000000000..5dd5e5b731c
--- /dev/null
+++ b/changelogs/unreleased/sh-revert-rack-request-health-checks.yml
@@ -0,0 +1,5 @@
+---
+title: Fix health checks not working behind load balancers
+merge_request: 26055
+author:
+type: fixed
diff --git a/lib/gitlab/middleware/basic_health_check.rb b/lib/gitlab/middleware/basic_health_check.rb
index acf8c301b8f..84e49805428 100644
--- a/lib/gitlab/middleware/basic_health_check.rb
+++ b/lib/gitlab/middleware/basic_health_check.rb
@@ -24,7 +24,13 @@ module Gitlab
       def call(env)
         return @app.call(env) unless env['PATH_INFO'] == HEALTH_PATH
 
-        request = ActionDispatch::Request.new(env)
+        # We should be using ActionDispatch::Request instead of
+        # Rack::Request to be consistent with Rails, but due to a Rails
+        # bug described in
+        # https://gitlab.com/gitlab-org/gitlab-ce/issues/58573#note_149799010
+        # hosts behind a load balancer will only see 127.0.0.1 for the
+        # load balancer's IP.
+        request = Rack::Request.new(env)
 
         return OK_RESPONSE if client_ip_whitelisted?(request)
 
diff --git a/lib/gitlab/request_context.rb b/lib/gitlab/request_context.rb
index d9811e036d3..f6d289476c5 100644
--- a/lib/gitlab/request_context.rb
+++ b/lib/gitlab/request_context.rb
@@ -13,7 +13,13 @@ module Gitlab
     end
 
     def call(env)
-      req = ActionDispatch::Request.new(env)
+      # We should be using ActionDispatch::Request instead of
+      # Rack::Request to be consistent with Rails, but due to a Rails
+      # bug described in
+      # https://gitlab.com/gitlab-org/gitlab-ce/issues/58573#note_149799010
+      # hosts behind a load balancer will only see 127.0.0.1 for the
+      # load balancer's IP.
+      req = Rack::Request.new(env)
 
       Gitlab::SafeRequestStore[:client_ip] = req.ip
 
diff --git a/spec/lib/gitlab/middleware/basic_health_check_spec.rb b/spec/lib/gitlab/middleware/basic_health_check_spec.rb
index 187d903a5e1..86bdc479b66 100644
--- a/spec/lib/gitlab/middleware/basic_health_check_spec.rb
+++ b/spec/lib/gitlab/middleware/basic_health_check_spec.rb
@@ -28,6 +28,35 @@ describe Gitlab::Middleware::BasicHealthCheck do
       end
     end
 
+    context 'with X-Forwarded-For headers' do
+      let(:load_balancer_ip) { '1.2.3.4' }
+
+      before do
+        env['HTTP_X_FORWARDED_FOR'] = "#{load_balancer_ip}, 127.0.0.1"
+        env['REMOTE_ADDR'] = '127.0.0.1'
+        env['PATH_INFO'] = described_class::HEALTH_PATH
+      end
+
+      it 'returns 200 response when endpoint is allowed' do
+        allow(Settings.monitoring).to receive(:ip_whitelist).and_return([load_balancer_ip])
+        expect(app).not_to receive(:call)
+
+        response = middleware.call(env)
+
+        expect(response[0]).to eq(200)
+        expect(response[1]).to eq({ 'Content-Type' => 'text/plain' })
+        expect(response[2]).to eq(['GitLab OK'])
+      end
+
+      it 'returns 404 when whitelist is not configured' do
+        allow(Settings.monitoring).to receive(:ip_whitelist).and_return([])
+
+        response = middleware.call(env)
+
+        expect(response[0]).to eq(404)
+      end
+    end
+
     context 'whitelisted IP' do
       before do
         env['REMOTE_ADDR'] = '127.0.0.1'
diff --git a/spec/lib/gitlab/request_context_spec.rb b/spec/lib/gitlab/request_context_spec.rb
index fd443cc1f71..3ed57c2c916 100644
--- a/spec/lib/gitlab/request_context_spec.rb
+++ b/spec/lib/gitlab/request_context_spec.rb
@@ -6,6 +6,31 @@ describe Gitlab::RequestContext do
     let(:app) { -> (env) {} }
     let(:env) { Hash.new }
 
+    context 'with X-Forwarded-For headers', :request_store do
+      let(:load_balancer_ip) { '1.2.3.4' }
+      let(:headers) do
+        {
+          'HTTP_X_FORWARDED_FOR' => "#{load_balancer_ip}, 127.0.0.1",
+          'REMOTE_ADDR' => '127.0.0.1'
+        }
+      end
+
+      let(:env) { Rack::MockRequest.env_for("/").merge(headers) }
+
+      it 'returns the load balancer IP' do
+        client_ip = nil
+
+        endpoint = proc do
+          client_ip = Gitlab::SafeRequestStore[:client_ip]
+          [200, {}, ["Hello"]]
+        end
+
+        Rails.application.middleware.build(endpoint).call(env)
+
+        expect(client_ip).to eq(load_balancer_ip)
+      end
+    end
+
     context 'when RequestStore::Middleware is used' do
       around do |example|
         RequestStore::Middleware.new(-> (env) { example.run }).call({})
@@ -15,7 +40,7 @@ describe Gitlab::RequestContext do
         let(:ip) { '192.168.1.11' }
 
         before do
-          allow_any_instance_of(ActionDispatch::Request).to receive(:ip).and_return(ip)
+          allow_any_instance_of(Rack::Request).to receive(:ip).and_return(ip)
           described_class.new(app).call(env)
         end