Commit bbb17ea1 authored by Peter Leitzen's avatar Peter Leitzen 👶 Committed by Michael Kozono

Handle possible HTTP exception for Sentry client

Prior this commit exceptions raised during a HTTP request
weren't caught by the Sentry client and were passed to the user.

In addition the Sentry client tried to catch a non-existent error
`Sentry::Client::SentryError`.

Now, the Sentry client catches all possible errors coming from
a HTTP request.
parent ae91b321
......@@ -15,8 +15,8 @@ module ErrorTracking
result = setting.list_sentry_projects
rescue Sentry::Client::Error => e
return error(e.message, :bad_request)
rescue Sentry::Client::SentryError => e
return error(e.message, :unprocessable_entity)
rescue Sentry::Client::MissingKeysError => e
return error(e.message, :internal_server_error)
end
success(projects: result[:projects])
......
---
title: Handle possible HTTP exception for Sentry client
merge_request: 27080
author:
type: fixed
......@@ -47,9 +47,11 @@ module Sentry
end
def http_get(url, params = {})
resp = Gitlab::HTTP.get(url, **request_params.merge(params))
response = handle_request_exceptions do
Gitlab::HTTP.get(url, **request_params.merge(params))
end
handle_response(resp)
handle_response(response)
end
def get_issues(issue_status:, limit:)
......@@ -63,14 +65,36 @@ module Sentry
http_get(projects_api_url)
end
def handle_request_exceptions
yield
rescue HTTParty::Error => e
Gitlab::Sentry.track_acceptable_exception(e)
raise_error 'Error when connecting to Sentry'
rescue Net::OpenTimeout
raise_error 'Connection to Sentry timed out'
rescue SocketError
raise_error 'Received SocketError when trying to connect to Sentry'
rescue OpenSSL::SSL::SSLError
raise_error 'Sentry returned invalid SSL data'
rescue Errno::ECONNREFUSED
raise_error 'Connection refused'
rescue => e
Gitlab::Sentry.track_acceptable_exception(e)
raise_error "Sentry request failed due to #{e.class}"
end
def handle_response(response)
unless response.code == 200
raise Client::Error, "Sentry response status code: #{response.code}"
raise_error "Sentry response status code: #{response.code}"
end
response
end
def raise_error(message)
raise Client::Error, message
end
def projects_api_url
projects_url = URI(@url)
projects_url.path = '/api/0/projects/'
......
......@@ -61,13 +61,37 @@ describe Sentry::Client do
end
end
shared_examples 'maps exceptions' do
exceptions = {
HTTParty::Error => 'Error when connecting to Sentry',
Net::OpenTimeout => 'Connection to Sentry timed out',
SocketError => 'Received SocketError when trying to connect to Sentry',
OpenSSL::SSL::SSLError => 'Sentry returned invalid SSL data',
Errno::ECONNREFUSED => 'Connection refused',
StandardError => 'Sentry request failed due to StandardError'
}
exceptions.each do |exception, message|
context "#{exception}" do
before do
stub_request(:get, sentry_request_url).to_raise(exception)
end
it do
expect { subject }
.to raise_exception(Sentry::Client::Error, message)
end
end
end
end
describe '#list_issues' do
let(:issue_status) { 'unresolved' }
let(:limit) { 20 }
let(:sentry_api_response) { issues_sample_response }
let(:sentry_request_url) { sentry_url + '/issues/?limit=20&query=is:unresolved' }
let!(:sentry_api_request) { stub_sentry_request(sentry_url + '/issues/?limit=20&query=is:unresolved', body: sentry_api_response) }
let!(:sentry_api_request) { stub_sentry_request(sentry_request_url, body: sentry_api_response) }
subject { client.list_issues(issue_status: issue_status, limit: limit) }
......@@ -121,16 +145,14 @@ describe Sentry::Client do
# Sentry API returns 404 if there are extra slashes in the URL!
context 'extra slashes in URL' do
let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects//sentry-org/sentry-project/' }
let(:client) { described_class.new(sentry_url, token) }
let!(:valid_req_stub) do
stub_sentry_request(
'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project/' \
let(:sentry_request_url) do
'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project/' \
'issues/?limit=20&query=is:unresolved'
)
end
it 'removes extra slashes in api url' do
expect(client.url).to eq(sentry_url)
expect(Gitlab::HTTP).to receive(:get).with(
URI('https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project/issues/'),
anything
......@@ -138,7 +160,7 @@ describe Sentry::Client do
subject
expect(valid_req_stub).to have_been_requested
expect(sentry_api_request).to have_been_requested
end
end
......@@ -169,6 +191,8 @@ describe Sentry::Client do
expect { subject }.to raise_error(Sentry::Client::MissingKeysError, 'Sentry API response is missing keys. key not found: "id"')
end
end
it_behaves_like 'maps exceptions'
end
describe '#list_projects' do
......@@ -260,12 +284,18 @@ describe Sentry::Client do
expect(valid_req_stub).to have_been_requested
end
end
context 'when exception is raised' do
let(:sentry_request_url) { sentry_list_projects_url }
it_behaves_like 'maps exceptions'
end
end
private
def stub_sentry_request(url, body: {}, status: 200, headers: {})
WebMock.stub_request(:get, url)
stub_request(:get, url)
.to_return(
status: status,
headers: { 'Content-Type' => 'application/json' }.merge(headers),
......
......@@ -51,14 +51,28 @@ describe ErrorTracking::ListProjectsService do
end
context 'sentry client raises exception' do
before do
expect(error_tracking_setting).to receive(:list_sentry_projects)
.and_raise(Sentry::Client::Error, 'Sentry response status code: 500')
context 'Sentry::Client::Error' do
before do
expect(error_tracking_setting).to receive(:list_sentry_projects)
.and_raise(Sentry::Client::Error, 'Sentry response status code: 500')
end
it 'returns error response' do
expect(result[:message]).to eq('Sentry response status code: 500')
expect(result[:http_status]).to eq(:bad_request)
end
end
it 'returns error response' do
expect(result[:message]).to eq('Sentry response status code: 500')
expect(result[:http_status]).to eq(:bad_request)
context 'Sentry::Client::MissingKeysError' do
before do
expect(error_tracking_setting).to receive(:list_sentry_projects)
.and_raise(Sentry::Client::MissingKeysError, 'Sentry API response is missing keys. key not found: "id"')
end
it 'returns error response' do
expect(result[:message]).to eq('Sentry API response is missing keys. key not found: "id"')
expect(result[:http_status]).to eq(:internal_server_error)
end
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