From 467a411e8892ecd6a3be7cd2f6772665f2c63651 Mon Sep 17 00:00:00 2001
From: David Wilkins <dwilkins@gitlab.com>
Date: Wed, 7 Aug 2019 02:42:20 +0000
Subject: Convert RestClient to Gitlab::HTTP for Prometheus Monitor

- Closes #60024

- Change PrometheusClient.new to accept a base url instead of an
  already created RestClient

- Use Gitlab::HTTP in PrometheusClient instead of creating RestClient
  in PrometheusService

- Move http_options from PrometheusService to
  PrometheusClient (follow_redirects: false)

- ensure that base urls don't have the trailing slash

- Created a `PrometheusClient#url` method that might not be strictly
  required

- Change rescued exceptions from RestClient::* to
  HTTParty::ResponseError where possible and StandardError for the
  rest
---
 app/models/clusters/applications/prometheus.rb     |  2 +-
 app/models/concerns/prometheus_adapter.rb          |  6 +-
 app/models/project_services/prometheus_service.rb  |  9 +--
 app/services/prometheus/proxy_service.rb           |  2 +-
 lib/gitlab/prometheus_client.rb                    | 82 +++++++++++++---------
 lib/sentry/client.rb                               |  2 +-
 .../services/user_activates_issue_tracker_spec.rb  |  2 +-
 .../services/user_activates_youtrack_spec.rb       |  2 +-
 spec/lib/gitlab/bitbucket_import/importer_spec.rb  |  2 +-
 spec/lib/gitlab/prometheus_client_spec.rb          | 47 ++++++++++---
 spec/lib/sentry/client_spec.rb                     |  2 +-
 .../clusters/applications/prometheus_spec.rb       | 15 ++--
 spec/models/concerns/prometheus_adapter_spec.rb    |  4 +-
 .../project_services/prometheus_service_spec.rb    |  4 --
 .../lfs_download_link_list_service_spec.rb         |  4 +-
 spec/services/prometheus/proxy_service_spec.rb     |  2 +-
 spec/support/matchers/be_url.rb                    |  4 ++
 17 files changed, 112 insertions(+), 79 deletions(-)

diff --git a/app/models/clusters/applications/prometheus.rb b/app/models/clusters/applications/prometheus.rb
index 5eb535cab58..08e52f32bb3 100644
--- a/app/models/clusters/applications/prometheus.rb
+++ b/app/models/clusters/applications/prometheus.rb
@@ -83,7 +83,7 @@ module Clusters
 
         # ensures headers containing auth data are appended to original k8s client options
         options = kube_client.rest_client.options.merge(headers: kube_client.headers)
-        RestClient::Resource.new(proxy_url, options)
+        Gitlab::PrometheusClient.new(proxy_url, options)
       rescue Kubeclient::HttpError
         # If users have mistakenly set parameters or removed the depended clusters,
         # `proxy_url` could raise an exception because gitlab can not communicate with the cluster.
diff --git a/app/models/concerns/prometheus_adapter.rb b/app/models/concerns/prometheus_adapter.rb
index c2542dbe743..9ac4722c6b1 100644
--- a/app/models/concerns/prometheus_adapter.rb
+++ b/app/models/concerns/prometheus_adapter.rb
@@ -14,10 +14,6 @@ module PrometheusAdapter
       raise NotImplementedError
     end
 
-    def prometheus_client_wrapper
-      Gitlab::PrometheusClient.new(prometheus_client)
-    end
-
     def can_query?
       prometheus_client.present?
     end
@@ -35,7 +31,7 @@ module PrometheusAdapter
     def calculate_reactive_cache(query_class_name, *args)
       return unless prometheus_client
 
-      data = Object.const_get(query_class_name, false).new(prometheus_client_wrapper).query(*args)
+      data = Object.const_get(query_class_name, false).new(prometheus_client).query(*args)
       {
         success: true,
         data: data,
diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb
index c68a9d923c8..6eff2ea2e3a 100644
--- a/app/models/project_services/prometheus_service.rb
+++ b/app/models/project_services/prometheus_service.rb
@@ -63,15 +63,16 @@ class PrometheusService < MonitoringService
 
   # Check we can connect to the Prometheus API
   def test(*args)
-    Gitlab::PrometheusClient.new(prometheus_client).ping
-
+    prometheus_client.ping
     { success: true, result: 'Checked API endpoint' }
   rescue Gitlab::PrometheusClient::Error => err
     { success: false, result: err }
   end
 
   def prometheus_client
-    RestClient::Resource.new(api_url, max_redirects: 0) if should_return_client?
+    return unless should_return_client?
+
+    Gitlab::PrometheusClient.new(api_url)
   end
 
   def prometheus_available?
@@ -84,7 +85,7 @@ class PrometheusService < MonitoringService
   private
 
   def should_return_client?
-    api_url && manual_configuration? && active? && valid?
+    api_url.present? && manual_configuration? && active? && valid?
   end
 
   def synchronize_service_state
diff --git a/app/services/prometheus/proxy_service.rb b/app/services/prometheus/proxy_service.rb
index c5d2b84878b..a62eb76b8ce 100644
--- a/app/services/prometheus/proxy_service.rb
+++ b/app/services/prometheus/proxy_service.rb
@@ -98,7 +98,7 @@ module Prometheus
     end
 
     def prometheus_client_wrapper
-      prometheus_adapter&.prometheus_client_wrapper
+      prometheus_adapter&.prometheus_client
     end
 
     def can_query?
diff --git a/lib/gitlab/prometheus_client.rb b/lib/gitlab/prometheus_client.rb
index f13156f898e..9fefffefcde 100644
--- a/lib/gitlab/prometheus_client.rb
+++ b/lib/gitlab/prometheus_client.rb
@@ -3,6 +3,7 @@
 module Gitlab
   # Helper methods to interact with Prometheus network services & resources
   class PrometheusClient
+    include Gitlab::Utils::StrongMemoize
     Error = Class.new(StandardError)
     QueryError = Class.new(Gitlab::PrometheusClient::Error)
 
@@ -14,10 +15,17 @@ module Gitlab
     # Minimal value of the `step` parameter for `query_range` in seconds.
     QUERY_RANGE_MIN_STEP = 60
 
-    attr_reader :rest_client, :headers
+    # Key translation between RestClient and Gitlab::HTTP (HTTParty)
+    RESTCLIENT_GITLAB_HTTP_KEYMAP = {
+      ssl_cert_store: :cert_store
+    }.freeze
 
-    def initialize(rest_client)
-      @rest_client = rest_client
+    attr_reader :api_url, :options
+    private :api_url, :options
+
+    def initialize(api_url, options = {})
+      @api_url = api_url.chomp('/')
+      @options = options
     end
 
     def ping
@@ -27,14 +35,10 @@ module Gitlab
     def proxy(type, args)
       path = api_path(type)
       get(path, args)
-    rescue RestClient::ExceptionWithResponse => ex
-      if ex.response
-        ex.response
-      else
-        raise PrometheusClient::Error, "Network connection error"
-      end
-    rescue RestClient::Exception
-      raise PrometheusClient::Error, "Network connection error"
+    rescue Gitlab::HTTP::ResponseError => ex
+      raise PrometheusClient::Error, "Network connection error" unless ex.response && ex.response.try(:code)
+
+      handle_response(ex.response)
     end
 
     def query(query, time: Time.now)
@@ -78,50 +82,58 @@ module Gitlab
     private
 
     def api_path(type)
-      ['api', 'v1', type].join('/')
+      [api_url, 'api', 'v1', type].join('/')
     end
 
     def json_api_get(type, args = {})
       path = api_path(type)
       response = get(path, args)
       handle_response(response)
-    rescue RestClient::ExceptionWithResponse => ex
-      if ex.response
-        handle_exception_response(ex.response)
-      else
-        raise PrometheusClient::Error, "Network connection error"
+    rescue Gitlab::HTTP::ResponseError => ex
+      raise PrometheusClient::Error, "Network connection error" unless ex.response && ex.response.try(:code)
+
+      handle_response(ex.response)
+    end
+
+    def gitlab_http_key(key)
+      RESTCLIENT_GITLAB_HTTP_KEYMAP[key] || key
+    end
+
+    def mapped_options
+      options.keys.map { |k| [gitlab_http_key(k), options[k]] }.to_h
+    end
+
+    def http_options
+      strong_memoize(:http_options) do
+        { follow_redirects: false }.merge(mapped_options)
       end
-    rescue RestClient::Exception
-      raise PrometheusClient::Error, "Network connection error"
     end
 
     def get(path, args)
-      rest_client[path].get(params: args)
+      Gitlab::HTTP.get(path, { query: args }.merge(http_options) )
     rescue SocketError
-      raise PrometheusClient::Error, "Can't connect to #{rest_client.url}"
+      raise PrometheusClient::Error, "Can't connect to #{api_url}"
     rescue OpenSSL::SSL::SSLError
-      raise PrometheusClient::Error, "#{rest_client.url} contains invalid SSL data"
+      raise PrometheusClient::Error, "#{api_url} contains invalid SSL data"
     rescue Errno::ECONNREFUSED
       raise PrometheusClient::Error, 'Connection refused'
     end
 
     def handle_response(response)
-      json_data = parse_json(response.body)
-      if response.code == 200 && json_data['status'] == 'success'
-        json_data['data'] || {}
-      else
-        raise PrometheusClient::Error, "#{response.code} - #{response.body}"
-      end
-    end
+      response_code = response.try(:code)
+      response_body = response.try(:body)
+
+      raise PrometheusClient::Error, "#{response_code} - #{response_body}" unless response_code
+
+      json_data = parse_json(response_body) if [200, 400].include?(response_code)
 
-    def handle_exception_response(response)
-      if response.code == 200 && response['status'] == 'success'
-        response['data'] || {}
-      elsif response.code == 400
-        json_data = parse_json(response.body)
+      case response_code
+      when 200
+        json_data['data'] if response['status'] == 'success'
+      when 400
         raise PrometheusClient::QueryError, json_data['error'] || 'Bad data received'
       else
-        raise PrometheusClient::Error, "#{response.code} - #{response.body}"
+        raise PrometheusClient::Error, "#{response_code} - #{response_body}"
       end
     end
 
diff --git a/lib/sentry/client.rb b/lib/sentry/client.rb
index 4022e8ff946..07cca1c8d1e 100644
--- a/lib/sentry/client.rb
+++ b/lib/sentry/client.rb
@@ -67,7 +67,7 @@ module Sentry
 
     def handle_request_exceptions
       yield
-    rescue HTTParty::Error => e
+    rescue Gitlab::HTTP::Error => e
       Gitlab::Sentry.track_acceptable_exception(e)
       raise_error 'Error when connecting to Sentry'
     rescue Net::OpenTimeout
diff --git a/spec/features/projects/services/user_activates_issue_tracker_spec.rb b/spec/features/projects/services/user_activates_issue_tracker_spec.rb
index 5803500a4d2..5f3bb794b48 100644
--- a/spec/features/projects/services/user_activates_issue_tracker_spec.rb
+++ b/spec/features/projects/services/user_activates_issue_tracker_spec.rb
@@ -61,7 +61,7 @@ describe 'User activates issue tracker', :js do
 
       context 'when the connection test fails' do
         it 'activates the service' do
-          stub_request(:head, url).to_raise(HTTParty::Error)
+          stub_request(:head, url).to_raise(Gitlab::HTTP::Error)
 
           click_link(tracker)
 
diff --git a/spec/features/projects/services/user_activates_youtrack_spec.rb b/spec/features/projects/services/user_activates_youtrack_spec.rb
index 67f0aa214e2..8fdeddfdfb4 100644
--- a/spec/features/projects/services/user_activates_youtrack_spec.rb
+++ b/spec/features/projects/services/user_activates_youtrack_spec.rb
@@ -48,7 +48,7 @@ describe 'User activates issue tracker', :js do
 
       context 'when the connection test fails' do
         it 'activates the service' do
-          stub_request(:head, url).to_raise(HTTParty::Error)
+          stub_request(:head, url).to_raise(Gitlab::HTTP::Error)
 
           click_link(tracker)
           fill_form
diff --git a/spec/lib/gitlab/bitbucket_import/importer_spec.rb b/spec/lib/gitlab/bitbucket_import/importer_spec.rb
index 62b688d4d3e..280941ff601 100644
--- a/spec/lib/gitlab/bitbucket_import/importer_spec.rb
+++ b/spec/lib/gitlab/bitbucket_import/importer_spec.rb
@@ -173,7 +173,7 @@ describe Gitlab::BitbucketImport::Importer do
     context 'when importing a pull request throws an exception' do
       before do
         allow(pull_request).to receive(:raw).and_return('hello world')
-        allow(subject.client).to receive(:pull_request_comments).and_raise(HTTParty::Error)
+        allow(subject.client).to receive(:pull_request_comments).and_raise(Gitlab::HTTP::Error)
       end
 
       it 'logs an error without the backtrace' do
diff --git a/spec/lib/gitlab/prometheus_client_spec.rb b/spec/lib/gitlab/prometheus_client_spec.rb
index f15ae83a02c..0a4e8dbced5 100644
--- a/spec/lib/gitlab/prometheus_client_spec.rb
+++ b/spec/lib/gitlab/prometheus_client_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
 describe Gitlab::PrometheusClient do
   include PrometheusHelpers
 
-  subject { described_class.new(RestClient::Resource.new('https://prometheus.example.com')) }
+  subject { described_class.new('https://prometheus.example.com') }
 
   describe '#ping' do
     it 'issues a "query" request to the API endpoint' do
@@ -79,8 +79,16 @@ describe Gitlab::PrometheusClient do
         expect(req_stub).to have_been_requested
       end
 
-      it 'raises a Gitlab::PrometheusClient::Error error when a RestClient::Exception is rescued' do
-        req_stub = stub_prometheus_request_with_exception(prometheus_url, RestClient::Exception)
+      it 'raises a Gitlab::PrometheusClient::Error error when a Gitlab::HTTP::ResponseError is rescued' do
+        req_stub = stub_prometheus_request_with_exception(prometheus_url, Gitlab::HTTP::ResponseError)
+
+        expect { subject }
+          .to raise_error(Gitlab::PrometheusClient::Error, "Network connection error")
+        expect(req_stub).to have_been_requested
+      end
+
+      it 'raises a Gitlab::PrometheusClient::Error error when a Gitlab::HTTP::ResponseError with a code is rescued' do
+        req_stub = stub_prometheus_request_with_exception(prometheus_url, Gitlab::HTTP::ResponseError.new(code: 400))
 
         expect { subject }
           .to raise_error(Gitlab::PrometheusClient::Error, "Network connection error")
@@ -89,13 +97,13 @@ describe Gitlab::PrometheusClient do
     end
 
     context 'ping' do
-      subject { described_class.new(RestClient::Resource.new(prometheus_url)).ping }
+      subject { described_class.new(prometheus_url).ping }
 
       it_behaves_like 'exceptions are raised'
     end
 
     context 'proxy' do
-      subject { described_class.new(RestClient::Resource.new(prometheus_url)).proxy('query', { query: '1' }) }
+      subject { described_class.new(prometheus_url).proxy('query', { query: '1' }) }
 
       it_behaves_like 'exceptions are raised'
     end
@@ -310,15 +318,32 @@ describe Gitlab::PrometheusClient do
         end
       end
 
-      context 'when RestClient::Exception is raised' do
+      context 'when Gitlab::HTTP::ResponseError is raised' do
         before do
-          stub_prometheus_request_with_exception(query_url, RestClient::Exception)
+          stub_prometheus_request_with_exception(query_url, response_error)
+        end
+
+        context "without response code" do
+          let(:response_error) { Gitlab::HTTP::ResponseError }
+          it 'raises PrometheusClient::Error' do
+            expect { subject.proxy('query', { query: prometheus_query }) }.to(
+              raise_error(Gitlab::PrometheusClient::Error, 'Network connection error')
+            )
+          end
         end
 
-        it 'raises PrometheusClient::Error' do
-          expect { subject.proxy('query', { query: prometheus_query }) }.to(
-            raise_error(Gitlab::PrometheusClient::Error, 'Network connection error')
-          )
+        context "with response code" do
+          let(:response_error) do
+            response = Net::HTTPResponse.new(1.1, 400, '{}sumpthin')
+            allow(response).to receive(:body) { '{}' }
+            Gitlab::HTTP::ResponseError.new(response)
+          end
+
+          it 'raises Gitlab::PrometheusClient::QueryError' do
+            expect { subject.proxy('query', { query: prometheus_query }) }.to(
+              raise_error(Gitlab::PrometheusClient::QueryError, 'Bad data received')
+            )
+          end
         end
       end
     end
diff --git a/spec/lib/sentry/client_spec.rb b/spec/lib/sentry/client_spec.rb
index cb14204b99a..ca2b17b44e0 100644
--- a/spec/lib/sentry/client_spec.rb
+++ b/spec/lib/sentry/client_spec.rb
@@ -63,7 +63,7 @@ describe Sentry::Client do
 
   shared_examples 'maps exceptions' do
     exceptions = {
-      HTTParty::Error => 'Error when connecting to Sentry',
+      Gitlab::HTTP::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',
diff --git a/spec/models/clusters/applications/prometheus_spec.rb b/spec/models/clusters/applications/prometheus_spec.rb
index d9f31c46f59..eb6ccba5584 100644
--- a/spec/models/clusters/applications/prometheus_spec.rb
+++ b/spec/models/clusters/applications/prometheus_spec.rb
@@ -86,16 +86,15 @@ describe Clusters::Applications::Prometheus do
                project: cluster.cluster_project.project)
       end
 
-      it 'creates proxy prometheus rest client' do
-        expect(subject.prometheus_client).to be_instance_of(RestClient::Resource)
+      it 'creates proxy prometheus_client' do
+        expect(subject.prometheus_client).to be_instance_of(Gitlab::PrometheusClient)
       end
 
-      it 'creates proper url' do
-        expect(subject.prometheus_client.url).to eq("#{kubernetes_url}/api/v1/namespaces/gitlab-managed-apps/services/prometheus-prometheus-server:80/proxy")
-      end
-
-      it 'copies options and headers from kube client to proxy client' do
-        expect(subject.prometheus_client.options).to eq(kube_client.rest_client.options.merge(headers: kube_client.headers))
+      it 'copies proxy_url, options and headers from kube client to prometheus_client' do
+        expect(Gitlab::PrometheusClient)
+          .to(receive(:new))
+          .with(a_valid_url, kube_client.rest_client.options.merge(headers: kube_client.headers))
+        subject.prometheus_client
       end
 
       context 'when cluster is not reachable' do
diff --git a/spec/models/concerns/prometheus_adapter_spec.rb b/spec/models/concerns/prometheus_adapter_spec.rb
index 25a2d290f76..3d26ba95192 100644
--- a/spec/models/concerns/prometheus_adapter_spec.rb
+++ b/spec/models/concerns/prometheus_adapter_spec.rb
@@ -40,13 +40,13 @@ describe PrometheusAdapter, :use_clean_rails_memory_store_caching do
 
     describe 'matched_metrics' do
       let(:matched_metrics_query) { Gitlab::Prometheus::Queries::MatchedMetricQuery }
-      let(:prometheus_client_wrapper) { double(:prometheus_client_wrapper, label_values: nil) }
+      let(:prometheus_client) { double(:prometheus_client, label_values: nil) }
 
       context 'with valid data' do
         subject { service.query(:matched_metrics) }
 
         before do
-          allow(service).to receive(:prometheus_client_wrapper).and_return(prometheus_client_wrapper)
+          allow(service).to receive(:prometheus_client).and_return(prometheus_client)
           synchronous_reactive_cache(service)
         end
 
diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb
index e9c7c94ad70..e5ac6ca65d6 100644
--- a/spec/models/project_services/prometheus_service_spec.rb
+++ b/spec/models/project_services/prometheus_service_spec.rb
@@ -105,10 +105,6 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do
     context 'manual configuration is enabled' do
       let(:manual_configuration) { true }
 
-      it 'returns rest client from api_url' do
-        expect(service.prometheus_client.url).to eq(api_url)
-      end
-
       it 'calls valid?' do
         allow(service).to receive(:valid?).and_call_original
 
diff --git a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb
index 80debcd3a7a..dabfd61d3f5 100644
--- a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb
+++ b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb
@@ -33,7 +33,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
 
   before do
     allow(project).to receive(:lfs_enabled?).and_return(true)
-    response = instance_double(HTTParty::Response)
+    response = instance_double(Gitlab::HTTP::Response)
     allow(response).to receive(:body).and_return(objects_response.to_json)
     allow(response).to receive(:success?).and_return(true)
     allow(Gitlab::HTTP).to receive(:post).and_return(response)
@@ -95,7 +95,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
 
     shared_examples 'JSON parse errors' do |body|
       it 'raises error' do
-        response = instance_double(HTTParty::Response)
+        response = instance_double(Gitlab::HTTP::Response)
         allow(response).to receive(:body).and_return(body)
         allow(response).to receive(:success?).and_return(true)
         allow(Gitlab::HTTP).to receive(:post).and_return(response)
diff --git a/spec/services/prometheus/proxy_service_spec.rb b/spec/services/prometheus/proxy_service_spec.rb
index 4bdb20de4c9..03bda94e9c6 100644
--- a/spec/services/prometheus/proxy_service_spec.rb
+++ b/spec/services/prometheus/proxy_service_spec.rb
@@ -131,7 +131,7 @@ describe Prometheus::ProxyService do
         allow(environment).to receive(:prometheus_adapter)
           .and_return(prometheus_adapter)
         allow(prometheus_adapter).to receive(:can_query?).and_return(true)
-        allow(prometheus_adapter).to receive(:prometheus_client_wrapper)
+        allow(prometheus_adapter).to receive(:prometheus_client)
           .and_return(prometheus_client)
       end
 
diff --git a/spec/support/matchers/be_url.rb b/spec/support/matchers/be_url.rb
index 7bd0e7fada4..69171f53891 100644
--- a/spec/support/matchers/be_url.rb
+++ b/spec/support/matchers/be_url.rb
@@ -5,3 +5,7 @@ RSpec::Matchers.define :be_url do |_|
     URI.parse(actual) rescue false
   end
 end
+
+# looks better when used like:
+#   expect(thing).to receive(:method).with(a_valid_url)
+RSpec::Matchers.alias_matcher :a_valid_url, :be_url
-- 
cgit v1.2.1