Merge branch '219046-fix-readiness-probe-500-on-db-down' into 'master'

Improve healtchecks 500 response when DB/Redis are not available

See merge request gitlab-org/gitlab!34844

changelogs/unreleased/219046-fix-readiness-probe-500-on-db-down.yml

+---
+title: Expand healtchecks `500`s when DB is not available
+merge_request: 34844
+author:
+type: fixed

doc/user/admin_area/monitoring/health_check.md

@@ -137,8 +137,8 @@ This check is being exempt from Rack Attack.
 
 ## Access token (Deprecated)
 
-> NOTE: **Note:**
-> Access token has been deprecated in GitLab 9.4 in favor of [IP whitelist](#ip-whitelist).
+NOTE: **Note:**
+Access token has been deprecated in GitLab 9.4 in favor of [IP whitelist](#ip-whitelist).
 
 An access token needs to be provided while accessing the probe endpoints. The current
 accepted token can be found under the **Admin Area > Monitoring > Health check**
@@ -152,6 +152,10 @@ The access token can be passed as a URL parameter:
 https://gitlab.example.com/-/readiness?token=ACCESS_TOKEN
 ```
 
+NOTE: **Note:**
+In case the database or Redis service are unaccessible, the probe endpoints response is not guaranteed to be correct.
+You should switch to [IP whitelist](#ip-whitelist) from deprecated access token to avoid it.
+
 <!-- ## Troubleshooting
 
 Include any troubleshooting steps that you can foresee. If you know beforehand what issues

lib/gitlab/health_checks/probes/collection.rb

@@ -20,6 +20,12 @@ module Gitlab
             success ? 200 : 503,
             status(success).merge(payload(readiness))
           )
+        rescue => e
+          exception_payload = { message: "#{e.class} : #{e.message}" }
+
+          Probes::Status.new(
+            500,
+            status(false).merge(exception_payload))
         end
 
         private

spec/lib/gitlab/health_checks/probes/collection_spec.rb

@@ -47,6 +47,20 @@ RSpec.describe Gitlab::HealthChecks::Probes::Collection do
             status: 'failed', message: 'check error')
         end
       end
+
+      context 'when check raises exception not handled inside the check' do
+        before do
+          expect(Gitlab::HealthChecks::Redis::RedisCheck).to receive(:readiness).and_raise(
+            ::Redis::CannotConnectError, 'Redis down')
+        end
+
+        it 'responds with failure including the exception info' do
+          expect(subject.http_status).to eq(500)
+
+          expect(subject.json[:status]).to eq('failed')
+          expect(subject.json[:message]).to eq('Redis::CannotConnectError : Redis down')
+        end
+      end
     end
 
     context 'without checks' do

spec/requests/health_controller_spec.rb

@@ -129,6 +129,40 @@ RSpec.describe HealthController do
           expect(response).to have_gitlab_http_status(:service_unavailable)
           expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
         end
+
+        context 'when DB is not accessible and connection raises an exception' do
+          before do
+            expect(Gitlab::HealthChecks::DbCheck)
+              .to receive(:readiness)
+              .and_raise(PG::ConnectionBad, 'could not connect to server')
+          end
+
+          it 'responds with 500 including the exception info' do
+            subject
+
+            expect(response).to have_gitlab_http_status(:internal_server_error)
+            expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
+            expect(json_response).to eq(
+              { 'status' => 'failed', 'message' => 'PG::ConnectionBad : could not connect to server' })
+          end
+        end
+
+        context 'when any exception happens during the probing' do
+          before do
+            expect(Gitlab::HealthChecks::Redis::RedisCheck)
+              .to receive(:readiness)
+              .and_raise(::Redis::CannotConnectError, 'Redis down')
+          end
+
+          it 'responds with 500 including the exception info' do
+            subject
+
+            expect(response).to have_gitlab_http_status(:internal_server_error)
+            expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
+            expect(json_response).to eq(
+              { 'status' => 'failed', 'message' => 'Redis::CannotConnectError : Redis down' })
+          end
+        end
       end
     end