From 3dc7750091d1665663b205a34475e09b4007c883 Mon Sep 17 00:00:00 2001
From: Mikolaj Wawrzyniak <mwawrzyniak@gitlab.com>
Date: Tue, 30 Jun 2020 13:27:11 +0200
Subject: [PATCH] Extract metrics prometheus api proxy into module
To avoid duplicated code when implementing prometheus api proxy
behavior we need to extrac common logic into dedicated module
---
.../metrics/dashboard/prometheus_api_proxy.rb | 53 +++++
.../environments/prometheus_api_controller.rb | 46 +---
config/routes/project.rb | 2 +-
.../prometheus_api_controller_spec.rb | 206 +++---------------
.../prometheus_api_proxy_shared_examples.rb | 147 +++++++++++++
5 files changed, 239 insertions(+), 215 deletions(-)
create mode 100644 app/controllers/concerns/metrics/dashboard/prometheus_api_proxy.rb
create mode 100644 spec/support/shared_examples/controllers/metrics/dashboard/prometheus_api_proxy_shared_examples.rb
diff --git a/app/controllers/concerns/metrics/dashboard/prometheus_api_proxy.rb b/app/controllers/concerns/metrics/dashboard/prometheus_api_proxy.rb
new file mode 100644
index 0000000000000..e0e3f628cc580
--- /dev/null
+++ b/app/controllers/concerns/metrics/dashboard/prometheus_api_proxy.rb
@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+module Metrics::Dashboard::PrometheusApiProxy
+ extend ActiveSupport::Concern
+ include RenderServiceResults
+
+ included do
+ before_action :authorize_read_prometheus!, only: [:prometheus_proxy]
+ end
+
+ def prometheus_proxy
+ variable_substitution_result =
+ proxy_variable_substitution_service.new(proxyable, permit_params).execute
+
+ if variable_substitution_result[:status] == :error
+ return error_response(variable_substitution_result)
+ end
+
+ prometheus_result = Prometheus::ProxyService.new(
+ proxyable,
+ proxy_method,
+ proxy_path,
+ variable_substitution_result[:params]
+ ).execute
+
+ return continue_polling_response if prometheus_result.nil?
+ return error_response(prometheus_result) if prometheus_result[:status] == :error
+
+ success_response(prometheus_result)
+ end
+
+ private
+
+ def proxyable
+ raise NotImplementedError, "#{self.class} must implement method: #{__callee__}"
+ end
+
+ def proxy_variable_substitution_service
+ raise NotImplementedError, "#{self.class} must implement method: #{__callee__}"
+ end
+
+ def permit_params
+ params.permit!
+ end
+
+ def proxy_method
+ request.method
+ end
+
+ def proxy_path
+ params[:proxy_path]
+ end
+end
diff --git a/app/controllers/projects/environments/prometheus_api_controller.rb b/app/controllers/projects/environments/prometheus_api_controller.rb
index 98fcc594d6e3c..f0bb5360f8430 100644
--- a/app/controllers/projects/environments/prometheus_api_controller.rb
+++ b/app/controllers/projects/environments/prometheus_api_controller.rb
@@ -1,51 +1,17 @@
# frozen_string_literal: true
class Projects::Environments::PrometheusApiController < Projects::ApplicationController
- include RenderServiceResults
+ include Metrics::Dashboard::PrometheusApiProxy
- before_action :authorize_read_prometheus!
- before_action :environment
-
- def proxy
- variable_substitution_result =
- variable_substitution_service.new(environment, permit_params).execute
-
- if variable_substitution_result[:status] == :error
- return error_response(variable_substitution_result)
- end
-
- prometheus_result = Prometheus::ProxyService.new(
- environment,
- proxy_method,
- proxy_path,
- variable_substitution_result[:params]
- ).execute
-
- return continue_polling_response if prometheus_result.nil?
- return error_response(prometheus_result) if prometheus_result[:status] == :error
-
- success_response(prometheus_result)
- end
+ before_action :proxyable
private
- def variable_substitution_service
- Prometheus::ProxyVariableSubstitutionService
- end
-
- def permit_params
- params.permit!
- end
-
- def environment
- @environment ||= project.environments.find(params[:id])
+ def proxyable
+ @proxyable ||= project.environments.find(params[:id])
end
- def proxy_method
- request.method
- end
-
- def proxy_path
- params[:proxy_path]
+ def proxy_variable_substitution_service
+ Prometheus::ProxyVariableSubstitutionService
end
end
diff --git a/config/routes/project.rb b/config/routes/project.rb
index 0ad4d7e0ea097..d2fe2be48e559 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -257,7 +257,7 @@
# This route is also defined in gitlab-workhorse. Make sure to update accordingly.
get '/terminal.ws/authorize', to: 'environments#terminal_websocket_authorize', format: false
- get '/prometheus/api/v1/*proxy_path', to: 'environments/prometheus_api#proxy', as: :prometheus_api
+ get '/prometheus/api/v1/*proxy_path', to: 'environments/prometheus_api#prometheus_proxy', as: :prometheus_api
get '/sample_metrics', to: 'environments/sample_metrics#query'
end
diff --git a/spec/controllers/projects/environments/prometheus_api_controller_spec.rb b/spec/controllers/projects/environments/prometheus_api_controller_spec.rb
index 17952aa06832f..68d50cf19f0dc 100644
--- a/spec/controllers/projects/environments/prometheus_api_controller_spec.rb
+++ b/spec/controllers/projects/environments/prometheus_api_controller_spec.rb
@@ -3,215 +3,73 @@
require 'spec_helper'
RSpec.describe Projects::Environments::PrometheusApiController do
- let_it_be(:project) { create(:project) }
- let_it_be(:environment) { create(:environment, project: project) }
let_it_be(:user) { create(:user) }
+ let_it_be(:project) { create(:project) }
+ let_it_be(:proxyable) { create(:environment, project: project) }
before do
project.add_reporter(user)
sign_in(user)
end
- describe 'GET #proxy' do
- let(:prometheus_proxy_service) { instance_double(Prometheus::ProxyService) }
-
- let(:expected_params) do
- ActionController::Parameters.new(
- environment_params(
- proxy_path: 'query',
- controller: 'projects/environments/prometheus_api',
- action: 'proxy'
- )
- ).permit!
- end
-
- context 'with valid requests' do
- before do
- allow(Prometheus::ProxyService).to receive(:new)
- .with(environment, 'GET', 'query', expected_params)
- .and_return(prometheus_proxy_service)
-
- allow(prometheus_proxy_service).to receive(:execute)
- .and_return(service_result)
+ describe 'GET #prometheus_proxy' do
+ it_behaves_like 'metrics dashboard prometheus api proxy' do
+ let(:proxyable_params) do
+ {
+ id: proxyable.id.to_s,
+ namespace_id: project.namespace.full_path,
+ project_id: project.name
+ }
end
- context 'with success result' do
- let(:service_result) { { status: :success, body: prometheus_body } }
+ context 'with variables' do
let(:prometheus_body) { '{"status":"success"}' }
- let(:prometheus_json_body) { Gitlab::Json.parse(prometheus_body) }
+ let(:pod_name) { "pod1" }
- it 'returns prometheus response' do
- get :proxy, params: environment_params
-
- expect(Prometheus::ProxyService).to have_received(:new)
- .with(environment, 'GET', 'query', expected_params)
- expect(response).to have_gitlab_http_status(:ok)
- expect(json_response).to eq(prometheus_json_body)
+ before do
+ expected_params[:query] = %{up{pod_name="#{pod_name}"}}
+ expected_params[:variables] = { 'pod_name' => pod_name }
end
- context 'with format string' do
- before do
- expected_params[:query] = %{up{environment="#{environment.slug}"}}
- end
-
- it 'replaces variables with values' do
- get :proxy, params: environment_params.merge(query: 'up{environment="{{ci_environment_slug}}"}')
-
- expect(Prometheus::ProxyService).to have_received(:new)
- .with(environment, 'GET', 'query', expected_params)
- end
-
- context 'with nil query' do
- let(:params_without_query) do
- environment_params.except(:query)
- end
-
- before do
- expected_params.delete(:query)
- end
-
- it 'does not raise error' do
- get :proxy, params: params_without_query
+ it 'replaces variables with values' do
+ get :prometheus_proxy, params: prometheus_proxy_params.merge(
+ query: 'up{pod_name="{{pod_name}}"}', variables: { 'pod_name' => pod_name }
+ )
- expect(Prometheus::ProxyService).to have_received(:new)
- .with(environment, 'GET', 'query', expected_params)
- end
- end
+ expect(response).to have_gitlab_http_status(:success)
+ expect(Prometheus::ProxyService).to have_received(:new)
+ .with(proxyable, 'GET', 'query', expected_params)
end
- context 'with variables' do
- let(:pod_name) { "pod1" }
-
- before do
- expected_params[:query] = %{up{pod_name="#{pod_name}"}}
- expected_params[:variables] = { 'pod_name' => pod_name }
- end
-
- it 'replaces variables with values' do
- get :proxy, params: environment_params.merge(
- query: 'up{pod_name="{{pod_name}}"}', variables: { 'pod_name' => pod_name }
+ context 'with invalid variables' do
+ let(:params_with_invalid_variables) do
+ prometheus_proxy_params.merge(
+ query: 'up{pod_name="{{pod_name}}"}', variables: ['a']
)
-
- expect(response).to have_gitlab_http_status(:success)
- expect(Prometheus::ProxyService).to have_received(:new)
- .with(environment, 'GET', 'query', expected_params)
- end
-
- context 'with invalid variables' do
- let(:params_with_invalid_variables) do
- environment_params.merge(
- query: 'up{pod_name="{{pod_name}}"}', variables: ['a']
- )
- end
-
- it 'returns 400' do
- get :proxy, params: params_with_invalid_variables
-
- expect(response).to have_gitlab_http_status(:bad_request)
- expect(Prometheus::ProxyService).not_to receive(:new)
- end
- end
- end
- end
-
- context 'with nil result' do
- let(:service_result) { nil }
-
- it 'returns 204 no_content' do
- get :proxy, params: environment_params
-
- expect(json_response['status']).to eq(_('processing'))
- expect(json_response['message']).to eq(_('Not ready yet. Try again later.'))
- expect(response).to have_gitlab_http_status(:no_content)
- end
- end
-
- context 'with 404 result' do
- let(:service_result) { { http_status: 404, status: :success, body: '{"body": "value"}' } }
-
- it 'returns body' do
- get :proxy, params: environment_params
-
- expect(response).to have_gitlab_http_status(:not_found)
- expect(json_response['body']).to eq('value')
- end
- end
-
- context 'with error result' do
- context 'with http_status' do
- let(:service_result) do
- { http_status: :service_unavailable, status: :error, message: 'error message' }
- end
-
- it 'sets the http response status code' do
- get :proxy, params: environment_params
-
- expect(response).to have_gitlab_http_status(:service_unavailable)
- expect(json_response['status']).to eq('error')
- expect(json_response['message']).to eq('error message')
end
- end
-
- context 'without http_status' do
- let(:service_result) { { status: :error, message: 'error message' } }
- it 'returns bad_request' do
- get :proxy, params: environment_params
+ it 'returns 400' do
+ get :prometheus_proxy, params: params_with_invalid_variables
expect(response).to have_gitlab_http_status(:bad_request)
- expect(json_response['status']).to eq('error')
- expect(json_response['message']).to eq('error message')
+ expect(Prometheus::ProxyService).not_to receive(:new)
end
end
end
- end
- context 'with inappropriate requests' do
context 'with anonymous user' do
+ let(:prometheus_body) { nil }
+
before do
sign_out(user)
end
it 'redirects to signin page' do
- get :proxy, params: environment_params
+ get :prometheus_proxy, params: prometheus_proxy_params
expect(response).to redirect_to(new_user_session_path)
end
end
-
- context 'without correct permissions' do
- before do
- project.team.truncate
- end
-
- it 'returns 404' do
- get :proxy, params: environment_params
-
- expect(response).to have_gitlab_http_status(:not_found)
- end
- end
end
-
- context 'with invalid environment id' do
- let(:other_environment) { create(:environment) }
-
- it 'returns 404' do
- get :proxy, params: environment_params(id: other_environment.id)
-
- expect(response).to have_gitlab_http_status(:not_found)
- end
- end
- end
-
- private
-
- def environment_params(params = {})
- {
- id: environment.id.to_s,
- namespace_id: project.namespace.full_path,
- project_id: project.name,
- proxy_path: 'query',
- query: '1'
- }.merge(params)
end
end
diff --git a/spec/support/shared_examples/controllers/metrics/dashboard/prometheus_api_proxy_shared_examples.rb b/spec/support/shared_examples/controllers/metrics/dashboard/prometheus_api_proxy_shared_examples.rb
new file mode 100644
index 0000000000000..94cd6971f7c92
--- /dev/null
+++ b/spec/support/shared_examples/controllers/metrics/dashboard/prometheus_api_proxy_shared_examples.rb
@@ -0,0 +1,147 @@
+# frozen_string_literal: true
+
+RSpec.shared_examples_for 'metrics dashboard prometheus api proxy' do
+ let(:service_params) { [proxyable, 'GET', 'query', expected_params] }
+ let(:service_result) { { status: :success, body: prometheus_body } }
+ let(:prometheus_proxy_service) { instance_double(Prometheus::ProxyService) }
+ let(:proxyable_params) do
+ {
+ id: proxyable.id.to_s
+ }
+ end
+ let(:expected_params) do
+ ActionController::Parameters.new(
+ prometheus_proxy_params(
+ proxy_path: 'query',
+ controller: described_class.controller_path,
+ action: 'prometheus_proxy'
+ )
+ ).permit!
+ end
+
+ before do
+ allow_next_instance_of(Prometheus::ProxyService, *service_params) do |proxy_service|
+ allow(proxy_service).to receive(:execute).and_return(service_result)
+ end
+ end
+
+ context 'with valid requests' do
+ context 'with success result' do
+ let(:prometheus_body) { '{"status":"success"}' }
+ let(:prometheus_json_body) { Gitlab::Json.parse(prometheus_body) }
+
+ it 'returns prometheus response' do
+ get :prometheus_proxy, params: prometheus_proxy_params
+
+ expect(Prometheus::ProxyService).to have_received(:new).with(*service_params)
+ expect(response).to have_gitlab_http_status(:ok)
+ expect(json_response).to eq(prometheus_json_body)
+ end
+
+ context 'with nil query' do
+ let(:params_without_query) do
+ prometheus_proxy_params.except(:query)
+ end
+
+ before do
+ expected_params.delete(:query)
+ end
+
+ it 'does not raise error' do
+ get :prometheus_proxy, params: params_without_query
+
+ expect(Prometheus::ProxyService).to have_received(:new).with(*service_params)
+ end
+ end
+ end
+
+ context 'with nil result' do
+ let(:service_result) { nil }
+
+ it 'returns 204 no_content' do
+ get :prometheus_proxy, params: prometheus_proxy_params
+
+ expect(json_response['status']).to eq(_('processing'))
+ expect(json_response['message']).to eq(_('Not ready yet. Try again later.'))
+ expect(response).to have_gitlab_http_status(:no_content)
+ end
+ end
+
+ context 'with 404 result' do
+ let(:service_result) { { http_status: 404, status: :success, body: '{"body": "value"}' } }
+
+ it 'returns body' do
+ get :prometheus_proxy, params: prometheus_proxy_params
+
+ expect(response).to have_gitlab_http_status(:not_found)
+ expect(json_response['body']).to eq('value')
+ end
+ end
+
+ context 'with error result' do
+ context 'with http_status' do
+ let(:service_result) do
+ { http_status: :service_unavailable, status: :error, message: 'error message' }
+ end
+
+ it 'sets the http response status code' do
+ get :prometheus_proxy, params: prometheus_proxy_params
+
+ expect(response).to have_gitlab_http_status(:service_unavailable)
+ expect(json_response['status']).to eq('error')
+ expect(json_response['message']).to eq('error message')
+ end
+ end
+
+ context 'without http_status' do
+ let(:service_result) { { status: :error, message: 'error message' } }
+
+ it 'returns bad_request' do
+ get :prometheus_proxy, params: prometheus_proxy_params
+
+ expect(response).to have_gitlab_http_status(:bad_request)
+ expect(json_response['status']).to eq('error')
+ expect(json_response['message']).to eq('error message')
+ end
+ end
+ end
+ end
+
+ context 'with inappropriate requests' do
+ let(:prometheus_body) { nil }
+
+ context 'without correct permissions' do
+ let(:user2) { create(:user) }
+
+ before do
+ sign_out(user)
+ sign_in(user2)
+ end
+
+ it 'returns 404' do
+ get :prometheus_proxy, params: prometheus_proxy_params
+
+ expect(response).to have_gitlab_http_status(:not_found)
+ end
+ end
+ end
+
+ context 'with invalid proxyable id' do
+ let(:prometheus_body) { nil }
+
+ it 'returns 404' do
+ get :prometheus_proxy, params: prometheus_proxy_params(id: proxyable.id + 1)
+
+ expect(response).to have_gitlab_http_status(:not_found)
+ end
+ end
+
+ private
+
+ def prometheus_proxy_params(params = {})
+ {
+ proxy_path: 'query',
+ query: '1'
+ }.merge(proxyable_params).merge(params)
+ end
+end
--
GitLab