Filter params based on the proxy_path

- Permit params in ProxyService class to avoid having to make changes
to both ProxyService and to PrometheusApiController when adding support
for more prometheus apis.
- Also refactor the cache specs.

2 files changed, 59 insertions, 32 deletions

diff --git a/app/services/prometheus/proxy_service.rb b/app/services/prometheus/proxy_service.rb
index 4a0d1dcd83f..c08adfda851 100644
--- a/app/services/prometheus/proxy_service.rb
+++ b/app/services/prometheus/proxy_service.rb
@@ -13,8 +13,14 @@ module Prometheus
     attr_accessor :proxyable, :method, :path, :params
 
     PROXY_SUPPORT = {
-      'query' => 'GET',
-      'query_range' => 'GET'
+      'query' => {
+        method: ['GET'],
+        params: [:query, :time, :timeout]
+      },
+      'query_range' => {
+        method: ['GET'],
+        params: [:query, :start, :end, :step, :timeout]
+      }
     }.freeze
 
     def self.from_cache(proxyable_class_name, proxyable_id, method, path, params)
@@ -35,9 +41,11 @@ module Prometheus
     def initialize(proxyable, method, path, params)
       @proxyable = proxyable
       @path = path
+
       # Convert ActionController::Parameters to hash because reactive_cache_worker
       # does not play nice with ActionController::Parameters.
-      @params = params.to_hash
+      @params = permit_params(params, path).to_hash
+
       @method = method
     end
 
@@ -94,8 +102,16 @@ module Prometheus
       prometheus_adapter&.can_query?
     end
 
+    def permit_params(params, path)
+      if params.is_a?(ActionController::Parameters)
+        params.permit(PROXY_SUPPORT.dig(path, :params))
+      else
+        params
+      end
+    end
+
     def can_proxy?
-      PROXY_SUPPORT[@path] == @method
+      PROXY_SUPPORT.dig(@path, :method)&.include?(@method)
     end
   end
 end
diff --git a/spec/services/prometheus/proxy_service_spec.rb b/spec/services/prometheus/proxy_service_spec.rb
index c271fc586c1..8de5f491427 100644
--- a/spec/services/prometheus/proxy_service_spec.rb
+++ b/spec/services/prometheus/proxy_service_spec.rb
@@ -9,7 +9,7 @@ describe Prometheus::ProxyService do
   set(:environment) { create(:environment, project: project) }
 
   describe '#initialize' do
-    let(:params) { ActionController::Parameters.new(query: '1').permit! }
+    let(:params) { ActionController::Parameters.new(query: '1') }
 
     it 'initializes attributes' do
       result = described_class.new(environment, 'GET', 'query', { query: '1' })
@@ -25,12 +25,23 @@ describe Prometheus::ProxyService do
 
       expect(result.params).to be_an_instance_of(Hash)
     end
+
+    context 'with unknown params' do
+      let(:params) { ActionController::Parameters.new(query: '1', other_param: 'val') }
+
+      it 'filters unknown params' do
+        result = described_class.new(environment, 'GET', 'query', params)
+
+        expect(result.params).to eq('query' => '1')
+      end
+    end
   end
 
   describe '#execute' do
     let(:prometheus_adapter) { instance_double(PrometheusService) }
+    let(:params) { ActionController::Parameters.new(query: '1') }
 
-    subject { described_class.new(environment, 'GET', 'query', { query: '1' }) }
+    subject { described_class.new(environment, 'GET', 'query', params) }
 
     context 'when prometheus_adapter is nil' do
       before do
@@ -62,7 +73,7 @@ describe Prometheus::ProxyService do
     end
 
     context 'cannot proxy' do
-      subject { described_class.new(environment, 'POST', 'query', { query: '1' }) }
+      subject { described_class.new(environment, 'POST', 'garbage', params) }
 
       it 'returns error' do
         expect(subject.execute).to eq(
@@ -72,44 +83,42 @@ describe Prometheus::ProxyService do
       end
     end
 
-    context 'when cached', :use_clean_rails_memory_store_caching do
+    context 'with caching', :use_clean_rails_memory_store_caching do
       let(:return_value) { { 'http_status' => 200, 'body' => 'body' } }
-      let(:opts) { [environment.class.name, environment.id, 'GET', 'query', { query: '1' }] }
 
-      before do
-        stub_reactive_cache(subject, return_value, opts)
+      let(:opts) do
+        [environment.class.name, environment.id, 'GET', 'query', { 'query' => '1' }]
+      end
 
+      before do
         allow(environment).to receive(:prometheus_adapter)
           .and_return(prometheus_adapter)
         allow(prometheus_adapter).to receive(:can_query?).and_return(true)
       end
 
-      it 'returns cached value' do
-        result = subject.execute
-
-        expect(result[:http_status]).to eq(return_value[:http_status])
-        expect(result[:body]).to eq(return_value[:body])
-      end
-    end
+      context 'when value present in cache' do
+        before do
+          stub_reactive_cache(subject, return_value, opts)
+        end
 
-    context 'when not cached' do
-      let(:return_value) { { 'http_status' => 200, 'body' => 'body' } }
-      let(:opts) { [environment.class.name, environment.id, 'GET', 'query', { query: '1' }] }
+        it 'returns cached value' do
+          result = subject.execute
 
-      before do
-        allow(environment).to receive(:prometheus_adapter)
-          .and_return(prometheus_adapter)
-        allow(prometheus_adapter).to receive(:can_query?).and_return(true)
+          expect(result[:http_status]).to eq(return_value[:http_status])
+          expect(result[:body]).to eq(return_value[:body])
+        end
       end
 
-      it 'returns nil' do
-        expect(ReactiveCachingWorker)
-          .to receive(:perform_async)
-          .with(subject.class, subject.id, *opts)
+      context 'when value not present in cache' do
+        it 'returns nil' do
+          expect(ReactiveCachingWorker)
+            .to receive(:perform_async)
+            .with(subject.class, subject.id, *opts)
 
-        result = subject.execute
+          result = subject.execute
 
-        expect(result).to eq(nil)
+          expect(result).to eq(nil)
+        end
       end
     end
 
@@ -172,7 +181,9 @@ describe Prometheus::ProxyService do
 
   describe '.from_cache' do
     it 'initializes an instance of ProxyService class' do
-      result = described_class.from_cache(environment.class.name, environment.id, 'GET', 'query', { query: '1' })
+      result = described_class.from_cache(
+        environment.class.name, environment.id, 'GET', 'query', { query: '1' }
+      )
 
       expect(result).to be_an_instance_of(described_class)
       expect(result.proxyable).to eq(environment)