diff options
author | Douglas Barbosa Alexandre <dbalexandre@gmail.com> | 2019-03-12 20:59:17 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-04-23 10:10:45 +0000 |
commit | eccfd3ea350a6e4cc0e8ddba3211735e2ae159ed (patch) | |
tree | 2dfd7c2baa4ca8445dee363fb7eb3caa09b5eaa7 | |
parent | be180a44801d8e6755fa07d259b8900228f5c0c1 (diff) | |
download | gitlab-ce-eccfd3ea350a6e4cc0e8ddba3211735e2ae159ed.tar.gz |
Merge branch 'sh-revert-rack-request-health-checks' into 'master'
Fix health checks not working behind load balancers
Closes #58573
See merge request gitlab-org/gitlab-ce!26055
(cherry picked from commit ef19ded4b0b5cc3aabb50b3432c8711f23a2742b)
01203e71 Fix health checks not working behind load balancers
-rw-r--r-- | changelogs/unreleased/sh-revert-rack-request-health-checks.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/middleware/basic_health_check.rb | 8 | ||||
-rw-r--r-- | lib/gitlab/request_context.rb | 8 | ||||
-rw-r--r-- | spec/lib/gitlab/middleware/basic_health_check_spec.rb | 29 | ||||
-rw-r--r-- | spec/lib/gitlab/request_context_spec.rb | 27 |
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 |