Commit 01203e71 authored by Stan Hu's avatar Stan Hu

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
parent 30e52b23
---
title: Fix health checks not working behind load balancers
merge_request: 26055
author:
type: fixed
...@@ -24,7 +24,13 @@ module Gitlab ...@@ -24,7 +24,13 @@ module Gitlab
def call(env) def call(env)
return @app.call(env) unless env['PATH_INFO'] == HEALTH_PATH 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) return OK_RESPONSE if client_ip_whitelisted?(request)
......
...@@ -13,7 +13,13 @@ module Gitlab ...@@ -13,7 +13,13 @@ module Gitlab
end end
def call(env) 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 Gitlab::SafeRequestStore[:client_ip] = req.ip
......
...@@ -28,6 +28,35 @@ describe Gitlab::Middleware::BasicHealthCheck do ...@@ -28,6 +28,35 @@ describe Gitlab::Middleware::BasicHealthCheck do
end end
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 context 'whitelisted IP' do
before do before do
env['REMOTE_ADDR'] = '127.0.0.1' env['REMOTE_ADDR'] = '127.0.0.1'
......
...@@ -6,6 +6,31 @@ describe Gitlab::RequestContext do ...@@ -6,6 +6,31 @@ describe Gitlab::RequestContext do
let(:app) { -> (env) {} } let(:app) { -> (env) {} }
let(:env) { Hash.new } 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 context 'when RequestStore::Middleware is used' do
around do |example| around do |example|
RequestStore::Middleware.new(-> (env) { example.run }).call({}) RequestStore::Middleware.new(-> (env) { example.run }).call({})
...@@ -15,7 +40,7 @@ describe Gitlab::RequestContext do ...@@ -15,7 +40,7 @@ describe Gitlab::RequestContext do
let(:ip) { '192.168.1.11' } let(:ip) { '192.168.1.11' }
before do 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) described_class.new(app).call(env)
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment