Merge branch '35242-allow-template-variables' into 'master'

Refactor code to substitute variables in Prometheus queries

See merge request gitlab-org/gitlab!19994

app/controllers/projects/environments/prometheus_api_controller.rb

@@ -7,23 +7,34 @@ class Projects::Environments::PrometheusApiController < Projects::ApplicationCon
   before_action :environment
 
   def proxy
-    result = Prometheus::ProxyService.new(
+    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,
-      proxy_params
+      variable_substitution_result[:params]
     ).execute
 
-    return continue_polling_response if result.nil?
-    return error_response(result) if result[:status] == :error
+    return continue_polling_response if prometheus_result.nil?
+    return error_response(prometheus_result) if prometheus_result[:status] == :error
 
-    success_response(result)
+    success_response(prometheus_result)
   end
 
   private
 
-  def query_context
-    Gitlab::Prometheus::QueryVariables.call(environment)
+  def variable_substitution_service
+    Prometheus::ProxyVariableSubstitutionService
+  end
+
+  def permit_params
+    params.permit!
   end
 
   def environment
@@ -37,15 +48,4 @@ class Projects::Environments::PrometheusApiController < Projects::ApplicationCon
   def proxy_path
     params[:proxy_path]
   end
-
-  def proxy_params
-    substitute_query_variables(params).permit!
-  end
-
-  def substitute_query_variables(params)
-    query = params[:query]
-    return params unless query
-
-    params.merge(query: query % query_context)
-  end
 end

app/services/prometheus/proxy_variable_substitution_service.rb

+# frozen_string_literal: true
+
+module Prometheus
+  class ProxyVariableSubstitutionService < BaseService
+    include Stepable
+
+    steps :add_params_to_result, :substitute_ruby_variables
+
+    def initialize(environment, params = {})
+      @environment, @params = environment, params.deep_dup
+    end
+
+    def execute
+      execute_steps
+    end
+
+    private
+
+    def add_params_to_result(result)
+      result[:params] = params
+
+      success(result)
+    end
+
+    def substitute_ruby_variables(result)
+      return success(result) unless query
+
+      # The % operator doesn't replace variables if the hash contains string
+      # keys.
+      result[:params][:query] = query % predefined_context.symbolize_keys
+
+      success(result)
+    rescue TypeError, ArgumentError => exception
+      log_error(exception.message)
+      Gitlab::Sentry.track_acceptable_exception(exception, extra: {
+        template_string: query,
+        variables: predefined_context
+      })
+
+      error(_('Malformed string'))
+    end
+
+    def predefined_context
+      @predefined_context ||= Gitlab::Prometheus::QueryVariables.call(@environment)
+    end
+
+    def query
+      params[:query]
+    end
+  end
+end

locale/gitlab.pot

@@ -10614,6 +10614,9 @@ msgstr ""
 msgid "Makes this issue confidential."
 msgstr ""
 
+msgid "Malformed string"
+msgstr ""
+
 msgid "Manage"
 msgstr ""
 

spec/controllers/projects/environments/prometheus_api_controller_spec.rb

@@ -63,9 +63,7 @@ describe Projects::Environments::PrometheusApiController do
 
           context 'with nil query' do
             let(:params_without_query) do
-              params = environment_params
-              params.delete(:query)
-              params
+              environment_params.except(:query)
             end
 
             before do

spec/lib/gitlab/prometheus/query_variables_spec.rb

@@ -14,7 +14,7 @@ describe Gitlab::Prometheus::QueryVariables do
 
     it do
       is_expected.to include(environment_filter:
-                             %{container_name!="POD",environment="#{slug}"})
+                             %Q[container_name!="POD",environment="#{slug}"])
     end
 
     context 'without deployment platform' do

spec/services/prometheus/proxy_variable_substitution_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Prometheus::ProxyVariableSubstitutionService do
+  describe '#execute' do
+    let_it_be(:environment) { create(:environment) }
+
+    let(:params_keys) { { query: 'up{environment="%{ci_environment_slug}"}' } }
+    let(:params) { ActionController::Parameters.new(params_keys).permit! }
+    let(:result) { subject.execute }
+
+    subject { described_class.new(environment, params) }
+
+    shared_examples 'success' do
+      it 'replaces variables with values' do
+        expect(result[:status]).to eq(:success)
+        expect(result[:params][:query]).to eq(expected_query)
+      end
+    end
+
+    shared_examples 'error' do |message|
+      it 'returns error' do
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq(message)
+      end
+    end
+
+    context 'does not alter params passed to the service' do
+      it do
+        subject.execute
+
+        expect(params).to eq(
+          ActionController::Parameters.new(
+            query: 'up{environment="%{ci_environment_slug}"}'
+          ).permit!
+        )
+      end
+    end
+
+    context 'with predefined variables' do
+      it_behaves_like 'success' do
+        let(:expected_query) { %Q[up{environment="#{environment.slug}"}] }
+      end
+
+      context 'with nil query' do
+        let(:params_keys) { {} }
+
+        it_behaves_like 'success' do
+          let(:expected_query) { nil }
+        end
+      end
+    end
+
+    context 'ruby template rendering' do
+      let(:params_keys) do
+        { query: 'up{env=%{ci_environment_slug},%{environment_filter}}' }
+      end
+
+      it_behaves_like 'success' do
+        let(:expected_query) do
+          "up{env=#{environment.slug},container_name!=\"POD\"," \
+          "environment=\"#{environment.slug}\"}"
+        end
+      end
+
+      context 'with multiple occurrences of variable in string' do
+        let(:params_keys) do
+          { query: 'up{env1=%{ci_environment_slug},env2=%{ci_environment_slug}}' }
+        end
+
+        it_behaves_like 'success' do
+          let(:expected_query) { "up{env1=#{environment.slug},env2=#{environment.slug}}" }
+        end
+      end
+
+      context 'with multiple variables in string' do
+        let(:params_keys) do
+          { query: 'up{env=%{ci_environment_slug},%{environment_filter}}' }
+        end
+
+        it_behaves_like 'success' do
+          let(:expected_query) do
+            "up{env=#{environment.slug}," \
+            "container_name!=\"POD\",environment=\"#{environment.slug}\"}"
+          end
+        end
+      end
+
+      context 'with unknown variables in string' do
+        let(:params_keys) { { query: 'up{env=%{env_slug}}' } }
+
+        it_behaves_like 'success' do
+          let(:expected_query) { 'up{env=%{env_slug}}' }
+        end
+      end
+
+      # This spec is needed if there are multiple keys in the context provided
+      # by `Gitlab::Prometheus::QueryVariables.call(environment)` which is
+      # passed to the Ruby `%` operator.
+      # If the number of keys in the context is one, there is no need for
+      # this spec.
+      context 'with extra variables in context' do
+        let(:params_keys) { { query: 'up{env=%{ci_environment_slug}}' } }
+
+        it_behaves_like 'success' do
+          let(:expected_query) { "up{env=#{environment.slug}}" }
+        end
+
+        it 'has more than one variable in context' do
+          expect(Gitlab::Prometheus::QueryVariables.call(environment).size).to be > 1
+        end
+      end
+
+      # The ruby % operator will not replace known variables if there are unknown
+      # variables also in the string. It doesn't raise an error
+      # (though the `sprintf` and `format` methods do).
+      context 'with unknown and known variables in string' do
+        let(:params_keys) do
+          { query: 'up{env=%{ci_environment_slug},other_env=%{env_slug}}' }
+        end
+
+        it_behaves_like 'success' do
+          let(:expected_query) { 'up{env=%{ci_environment_slug},other_env=%{env_slug}}' }
+        end
+      end
+
+      context 'when rendering raises error' do
+        context 'when TypeError is raised' do
+          let(:params_keys) { { query: '{% a %}' } }
+
+          it_behaves_like 'error', 'Malformed string'
+        end
+
+        context 'when ArgumentError is raised' do
+          let(:params_keys) { { query: '%<' } }
+
+          it_behaves_like 'error', 'Malformed string'
+        end
+      end
+    end
+  end
+end