Merge branch '215563-remove-ruby-syntax' into 'master'

Remove support for Ruby variable interpolation in custom dashboards

See merge request gitlab-org/gitlab!31581

app/services/prometheus/proxy_variable_substitution_service.rb

@@ -17,8 +17,7 @@ module Prometheus
     steps :validate_variables,
       :add_params_to_result,
       :substitute_params,
-      :substitute_ruby_variables,
-      :substitute_liquid_variables
+      :substitute_variables
 
     def initialize(environment, params = {})
       @environment, @params = environment, params.deep_dup
@@ -56,7 +55,7 @@ module Prometheus
       success(result)
     end
 
-    def substitute_liquid_variables(result)
+    def substitute_variables(result)
       return success(result) unless query(result)
 
       result[:params][:query] = gsub(query(result), full_context)
@@ -64,24 +63,6 @@ module Prometheus
       success(result)
     end
 
-    def substitute_ruby_variables(result)
-      return success(result) unless query(result)
-
-      # The % operator doesn't replace variables if the hash contains string
-      # keys.
-      result[:params][:query] = query(result) % predefined_context.symbolize_keys
-
-      success(result)
-    rescue TypeError, ArgumentError => exception
-      log_error(exception.message)
-      Gitlab::ErrorTracking.track_exception(exception, {
-        template_string: query(result),
-        variables: predefined_context
-      })
-
-      error(_('Malformed string'))
-    end
-
     def gsub(string, context)
       # Search for variables of the form `{{variable}}` in the string and replace
       # them with their value.
@@ -95,11 +76,11 @@ module Prometheus
     end
 
     def predefined_context
-      @predefined_context ||= Gitlab::Prometheus::QueryVariables.call(@environment)
+      Gitlab::Prometheus::QueryVariables.call(@environment).stringify_keys
     end
 
     def full_context
-      @full_context ||= predefined_context.stringify_keys.reverse_merge(variables_hash)
+      @full_context ||= predefined_context.reverse_merge(variables_hash)
     end
 
     def variables

changelogs/unreleased/215563-remove-ruby-syntax.yml

+---
+title: Remove support for Ruby format variable interpolation (`%{variable}`) in custom
+  dashboards
+merge_request: 31581
+author:
+type: removed

doc/user/project/integrations/prometheus.md

@@ -192,10 +192,11 @@ GitLab supports a limited set of [CI variables](../../../ci/variables/README.md)
 NOTE: **Note:**
 Variables for Prometheus queries must be lowercase.
 
-There are 2 methods to specify a variable in a query or dashboard:
+Variables can be specified using double curly braces, such as `"{{ci_environment_slug}}"` ([added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/20793) in GitLab 12.7).
 
-1. Variables can be specified using double curly braces, such as `{{ci_environment_slug}}` ([added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/20793) in GitLab 12.7).
-1. You can also enclose it in quotation marks with curly braces with a leading percent, for example `"%{ci_environment_slug}"`. This method is deprecated though and support will be [removed in the next major release](https://gitlab.com/gitlab-org/gitlab/issues/37990).
+Support for the `"%{ci_environment_slug}"` format was
+[removed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/31581) in GitLab 13.0.
+Queries that continue to use the old format will show no data.
 
 #### Query Variables from URL
 

locale/gitlab.pot

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

spec/controllers/projects/environments/prometheus_api_controller_spec.rb

@@ -55,7 +55,7 @@ describe Projects::Environments::PrometheusApiController do
           end
 
           it 'replaces variables with values' do
-            get :proxy, params: environment_params.merge(query: 'up{environment="%{ci_environment_slug}"}')
+            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)

spec/services/prometheus/proxy_variable_substitution_service_spec.rb

@@ -6,7 +6,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
   describe '#execute' do
     let_it_be(:environment) { create(:environment) }
 
-    let(:params_keys) { { query: 'up{environment="%{ci_environment_slug}"}' } }
+    let(:params_keys) { { query: 'up{environment="{{ci_environment_slug}}"}' } }
     let(:params) { ActionController::Parameters.new(params_keys).permit! }
     let(:result) { subject.execute }
 
@@ -32,21 +32,13 @@ describe Prometheus::ProxyVariableSubstitutionService do
 
         expect(params).to eq(
           ActionController::Parameters.new(
-            query: 'up{environment="%{ci_environment_slug}"}'
+            query: 'up{environment="{{ci_environment_slug}}"}'
           ).permit!
         )
       end
     end
 
     context 'with predefined variables' do
-      let(:params_keys) { { query: 'up{%{environment_filter}}' } }
-
-      it_behaves_like 'success' do
-        let(:expected_query) do
-          %Q[up{container_name!="POD",environment="#{environment.slug}"}]
-        end
-      end
-
       context 'with nil query' do
         let(:params_keys) { {} }
 
@@ -64,18 +56,6 @@ describe Prometheus::ProxyVariableSubstitutionService do
           let(:expected_query) { %Q[up{environment="#{environment.slug}"}] }
         end
       end
-
-      context 'with ruby and liquid formats' do
-        let(:params_keys) do
-          { query: 'up{%{environment_filter},env2="{{ci_environment_slug}}"}' }
-        end
-
-        it_behaves_like 'success' do
-          let(:expected_query) do
-            %Q[up{container_name!="POD",environment="#{environment.slug}",env2="#{environment.slug}"}]
-          end
-        end
-      end
     end
 
     context 'with custom variables' do
@@ -92,20 +72,6 @@ describe Prometheus::ProxyVariableSubstitutionService do
         let(:expected_query) { %q[up{pod_name="pod1"}] }
       end
 
-      context 'with ruby variable interpolation format' do
-        let(:params_keys) do
-          {
-            query: 'up{pod_name="%{pod_name}"}',
-            variables: ['pod_name', pod_name]
-          }
-        end
-
-        it_behaves_like 'success' do
-          # Custom variables cannot be used with the Ruby interpolation format.
-          let(:expected_query) { "up{pod_name=\"%{pod_name}\"}" }
-        end
-      end
-
       context 'with predefined variables in variables parameter' do
         let(:params_keys) do
           {
@@ -145,108 +111,6 @@ describe Prometheus::ProxyVariableSubstitutionService do
           let(:expected_query) { 'up{pod_name="{{pod_name}}"}' }
         end
       end
-
-      context 'with ruby and liquid variables' do
-        let(:params_keys) do
-          {
-            query: 'up{env1="%{ruby_variable}",env2="{{ liquid_variable }}"}',
-            variables: %w(ruby_variable value liquid_variable env_slug)
-          }
-        end
-
-        it_behaves_like 'success' do
-          # It should replace only liquid variables with their values
-          let(:expected_query) { %q[up{env1="%{ruby_variable}",env2="env_slug"}] }
-        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
 
     context 'gsub variable substitution tolerance for weirdness' do