Merge branch '60149-nameerror-uninitialized-constant-sentry-client-sentryerror' into 'master'

Handle possible HTTP exception for Sentry client

Closes #60149

See merge request gitlab-org/gitlab-ce!27080

app/services/error_tracking/list_projects_service.rb

@@ -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])

changelogs/unreleased/60149-nameerror-uninitialized-constant-sentry-client-sentryerror.yml

+---
+title: Handle possible HTTP exception for Sentry client
+merge_request: 27080
+author:
+type: fixed

lib/sentry/client.rb

@@ -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/'

spec/lib/sentry/client_spec.rb

@@ -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),

spec/services/error_tracking/list_projects_service_spec.rb

@@ -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