Make metrics dashboard embeds work with new and old URLs

Modify the regex for detecting metrics dashboard URLs in markdown
so that it detects both the old and the new URLs.

changelogs/unreleased/227929-metrics-dashboard-embeds-with-new-urls.yml

+---
+title: Fix Metrics dashboard embeds when using new URLs
+merge_request: 39876
+author:
+type: fixed

lib/banzai/filter/inline_metrics_filter.rb

@@ -10,7 +10,6 @@ module Banzai
       # the cost of doing a full regex match.
       def xpath_search
         "descendant-or-self::a[contains(@href,'metrics') and \
-          contains(@href,'environments') and \
           starts-with(@href, '#{gitlab_domain}')]"
       end
 
@@ -29,7 +28,7 @@ module Banzai
           params['project'],
           params['environment'],
           embedded: true,
-          **query_params(params['url'])
+          **query_params(params['url']).except(:environment)
         )
       end
     end

lib/gitlab/metrics/dashboard/url.rb

@@ -10,20 +10,23 @@ module Gitlab
 
           QUERY_PATTERN = '(?<query>\?[a-zA-Z0-9%.()+_=-]+(&[a-zA-Z0-9%.()+_=-]+)*)?'
           ANCHOR_PATTERN = '(?<anchor>\#[a-z0-9_-]+)?'
-          OPTIONAL_DASH_PATTERN = '(?:/-)?'
+          DASH_PATTERN = '(?:/-)'
 
-          # Matches urls for a metrics dashboard. This could be
-          # either the /metrics endpoint or the /metrics_dashboard
-          # endpoint.
+          # Matches urls for a metrics dashboard.
+          # This regex needs to match the old metrics URL, the new metrics URL,
+          # and the dashboard URL (inline_metrics_redactor_filter.rb
+          # uses this regex to match against the dashboard URL.)
           #
-          # EX - https://<host>/<namespace>/<project>/environments/<env_id>/metrics
+          # EX - Old URL: https://<host>/<namespace>/<project>/environments/<env_id>/metrics
+          # OR
+          # New URL: https://<host>/<namespace>/<project>/-/metrics?environment=<env_id>
+          # OR
+          # dashboard URL: https://<host>/<namespace>/<project>/environments/<env_id>/metrics_dashboard
           def metrics_regex
             strong_memoize(:metrics_regex) do
               regex_for_project_metrics(
                 %r{
-                    /environments
-                    /(?<environment>\d+)
-                    /(metrics_dashboard|metrics)
+                    ( #{environment_metrics_regex} ) | ( #{non_environment_metrics_regex} )
                 }x
               )
             end
@@ -36,6 +39,7 @@ module Gitlab
             strong_memoize(:grafana_regex) do
               regex_for_project_metrics(
                 %r{
+                  #{DASH_PATTERN}?
                   /grafana
                   /metrics_dashboard
                 }x
@@ -44,16 +48,22 @@ module Gitlab
           end
 
           # Matches dashboard urls for a metric chart embed
-          # for cluster metrics
+          # for cluster metrics.
+          # This regex needs to match the dashboard URL as well, not just the trigger URL.
+          # The inline_metrics_redactor_filter.rb uses this regex to match against
+          # the dashboard URL.
           #
           # EX - https://<host>/<namespace>/<project>/-/clusters/<cluster_id>/?group=Cluster%20Health&title=Memory%20Usage&y_label=Memory%20(GiB)
+          # dashboard URL - https://<host>/<namespace>/<project>/-/clusters/<cluster_id>/metrics_dashboard?group=Cluster%20Health&title=Memory%20Usage&y_label=Memory%20(GiB)
           def clusters_regex
             strong_memoize(:clusters_regex) do
               regex_for_project_metrics(
                 %r{
+                  #{DASH_PATTERN}?
                   /clusters
                   /(?<cluster_id>\d+)
                   /?
+                  ( (/metrics) | ( /metrics_dashboard\.json ) )?
                 }x
               )
             end
@@ -67,10 +77,11 @@ module Gitlab
             strong_memoize(:alert_regex) do
               regex_for_project_metrics(
                 %r{
+                  #{DASH_PATTERN}?
                   /prometheus
                   /alerts
                   /(?<alert>\d+)
-                  /metrics_dashboard
+                  /metrics_dashboard(\.json)?
                 }x
               )
             end
@@ -95,16 +106,37 @@ module Gitlab
 
           private
 
+          def environment_metrics_regex
+            %r{
+              #{DASH_PATTERN}?
+              /environments
+              /(?<environment>\d+)
+              /(metrics_dashboard|metrics)
+            }x
+          end
+
+          def non_environment_metrics_regex
+            %r{
+              #{DASH_PATTERN}
+              /metrics
+              (?=                             # Lookahead to ensure there is an environment query param
+                \?
+                .*
+                environment=(?<environment>\d+)
+                .*
+              )
+            }x
+          end
+
           def regex_for_project_metrics(path_suffix_pattern)
             %r{
-              (?<url>
+              ^(?<url>
                 #{gitlab_host_pattern}
                 #{project_path_pattern}
-                #{OPTIONAL_DASH_PATTERN}
                 #{path_suffix_pattern}
                 #{QUERY_PATTERN}
                 #{ANCHOR_PATTERN}
-              )
+              )$
             }x
           end
 

spec/lib/banzai/filter/inline_metrics_filter_spec.rb

@@ -5,25 +5,68 @@ require 'spec_helper'
 RSpec.describe Banzai::Filter::InlineMetricsFilter do
   include FilterSpecHelper
 
-  let(:params) { ['foo', 'bar', 12] }
-  let(:query_params) { {} }
-
-  let(:trigger_url) { urls.metrics_namespace_project_environment_url(*params, query_params) }
+  let(:environment_id) { 12 }
   let(:dashboard_url) { urls.metrics_dashboard_namespace_project_environment_url(*params, **query_params, embedded: true) }
 
-  it_behaves_like 'a metrics embed filter'
+  let(:query_params) do
+    {
+      dashboard: 'config/prometheus/common_metrics.yml',
+      group: 'System metrics (Kubernetes)',
+      title: 'Core Usage (Pod Average)',
+      y_label: 'Cores per Pod'
+    }
+  end
+
+  context 'with /-/environments/:environment_id/metrics URL' do
+    let(:params) { ['group', 'project', environment_id] }
+    let(:trigger_url) { urls.metrics_namespace_project_environment_url(*params, **query_params) }
+
+    context 'with no query params' do
+      let(:query_params) { {} }
+
+      it_behaves_like 'a metrics embed filter'
+    end
+
+    context 'with query params' do
+      it_behaves_like 'a metrics embed filter'
+    end
+  end
 
-  context 'with query params specified' do
-    let(:query_params) do
-      {
-        dashboard: 'config/prometheus/common_metrics.yml',
-        group: 'System metrics (Kubernetes)',
-        title: 'Core Usage (Pod Average)',
-        y_label: 'Cores per Pod'
-      }
+  context 'with /-/metrics?environment=:environment_id URL' do
+    let(:params) { %w(group project) }
+    let(:trigger_url) { urls.namespace_project_metrics_dashboard_url(*params, **query_params) }
+    let(:dashboard_url) do
+      urls.metrics_dashboard_namespace_project_environment_url(
+        *params.append(environment_id),
+        **query_params.except(:environment),
+        embedded: true
+      )
     end
 
-    it_behaves_like 'a metrics embed filter'
+    context 'with query params' do
+      it_behaves_like 'a metrics embed filter' do
+        before do
+          query_params.merge!(environment: environment_id)
+        end
+      end
+    end
+
+    context 'with only environment in query params' do
+      let(:query_params) { { environment: environment_id } }
+
+      it_behaves_like 'a metrics embed filter'
+    end
+
+    context 'with no query params' do
+      let(:query_params) { {} }
+
+      it 'ignores metrics URL without environment parameter' do
+        input = %(<a href="#{trigger_url}">example</a>)
+        filtered_input = filter(input).to_s
+
+        expect(CGI.unescape_html(filtered_input)).to eq(input)
+      end
+    end
   end
 
   it 'leaves links to other dashboards unchanged' do

spec/lib/banzai/filter/inline_metrics_redactor_filter_spec.rb

@@ -22,6 +22,13 @@ RSpec.describe Banzai::Filter::InlineMetricsRedactorFilter do
     it_behaves_like 'redacts the embed placeholder'
     it_behaves_like 'retains the embed placeholder when applicable'
 
+    context 'with /-/metrics?environment=:environment_id URL' do
+      let(:url) { urls.project_metrics_dashboard_url(project, embedded: true, environment: 1) }
+
+      it_behaves_like 'redacts the embed placeholder'
+      it_behaves_like 'retains the embed placeholder when applicable'
+    end
+
     context 'for a grafana dashboard' do
       let(:url) { urls.project_grafana_api_metrics_dashboard_url(project, embedded: true) }
 
@@ -33,7 +40,7 @@ RSpec.describe Banzai::Filter::InlineMetricsRedactorFilter do
       let_it_be(:cluster) { create(:cluster, :provided_by_gcp, :project, projects: [project]) }
       let(:params) { [project.namespace.path, project.path, cluster.id] }
       let(:query_params) { { group: 'Cluster Health', title: 'CPU Usage', y_label: 'CPU (cores)' } }
-      let(:url) { urls.metrics_dashboard_namespace_project_cluster_url(*params, **query_params) }
+      let(:url) { urls.metrics_dashboard_namespace_project_cluster_url(*params, **query_params, format: :json) }
 
       context 'with user who can read cluster' do
         it_behaves_like 'redacts the embed placeholder'

spec/lib/gitlab/metrics/dashboard/url_spec.rb

@@ -6,11 +6,12 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
   include Gitlab::Routing.url_helpers
 
   describe '#metrics_regex' do
+    let(:environment_id) { 1 }
     let(:url_params) do
       [
         'foo',
         'bar',
-        1,
+        environment_id,
         {
           start: '2019-08-02T05:43:09.000Z',
           dashboard: 'config/prometheus/common_metrics.yml',
@@ -33,12 +34,42 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
 
     subject { described_class.metrics_regex }
 
-    context 'for metrics route' do
+    context 'for /-/environments/:environment_id/metrics route' do
       let(:url) { metrics_namespace_project_environment_url(*url_params) }
 
       it_behaves_like 'regex which matches url when expected'
     end
 
+    context 'for /-/metrics?environment=:environment_id route' do
+      let(:url) { namespace_project_metrics_dashboard_url(*url_params) }
+      let(:url_params) do
+        [
+          'namespace1',
+          'project1',
+          {
+            environment: environment_id,
+            start: '2019-08-02T05:43:09.000Z',
+            dashboard: 'config/prometheus/common_metrics.yml',
+            group: 'awesome group',
+            anchor: 'title'
+          }
+        ]
+      end
+
+      let(:expected_params) do
+        {
+          'url' => url,
+          'namespace' => 'namespace1',
+          'project' => 'project1',
+          'environment' => "#{environment_id}",
+          'query' => "?dashboard=config%2Fprometheus%2Fcommon_metrics.yml&environment=#{environment_id}&group=awesome+group&start=2019-08-02T05%3A43%3A09.000Z",
+          'anchor' => '#title'
+        }
+      end
+
+      it_behaves_like 'regex which matches url when expected'
+    end
+
     context 'for metrics_dashboard route' do
       let(:url) { metrics_dashboard_namespace_project_environment_url(*url_params) }
 
@@ -47,16 +78,19 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
   end
 
   describe '#clusters_regex' do
-    let(:url) do
-      Gitlab::Routing.url_helpers.namespace_project_cluster_url(
+    let(:url) { Gitlab::Routing.url_helpers.namespace_project_cluster_url(*url_params) }
+    let(:url_params) do
+      [
         'foo',
         'bar',
         '1',
-        group: 'Cluster Health',
-        title: 'Memory Usage',
-        y_label: 'Memory 20(GiB)',
-        anchor: 'title'
-      )
+        {
+          group: 'Cluster Health',
+          title: 'Memory Usage',
+          y_label: 'Memory 20(GiB)',
+          anchor: 'title'
+        }
+      ]
     end
 
     let(:expected_params) do
@@ -73,6 +107,27 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
     subject { described_class.clusters_regex }
 
     it_behaves_like 'regex which matches url when expected'
+
+    context 'for metrics_dashboard route' do
+      let(:url) do
+        metrics_dashboard_namespace_project_cluster_url(
+          *url_params, cluster_type: :project, embedded: true, format: :json
+        )
+      end
+
+      let(:expected_params) do
+        {
+          'url' => url,
+          'namespace' => 'foo',
+          'project' => 'bar',
+          'cluster_id' => '1',
+          'query' => '?cluster_type=project&embedded=true',
+          'anchor' => nil
+        }
+      end
+
+      it_behaves_like 'regex which matches url when expected'
+    end
   end
 
   describe '#grafana_regex' do
@@ -103,15 +158,18 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
   end
 
   describe '#alert_regex' do
-    let(:url) do
-      Gitlab::Routing.url_helpers.metrics_dashboard_namespace_project_prometheus_alert_url(
+    let(:url) { Gitlab::Routing.url_helpers.metrics_dashboard_namespace_project_prometheus_alert_url(*url_params) }
+    let(:url_params) do
+      [
         'foo',
         'bar',
         '1',
-        start: '2020-02-10T12:59:49.938Z',
-        end: '2020-02-10T20:59:49.938Z',
-        anchor: "anchor"
-      )
+        {
+          start: '2020-02-10T12:59:49.938Z',
+          end: '2020-02-10T20:59:49.938Z',
+          anchor: "anchor"
+        }
+      ]
     end
 
     let(:expected_params) do
@@ -128,6 +186,21 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
     subject { described_class.alert_regex }
 
     it_behaves_like 'regex which matches url when expected'
+
+    it_behaves_like 'regex which matches url when expected' do
+      let(:url) { Gitlab::Routing.url_helpers.metrics_dashboard_namespace_project_prometheus_alert_url(*url_params, format: :json) }
+
+      let(:expected_params) do
+        {
+          'url' => url,
+          'namespace' => 'foo',
+          'project' => 'bar',
+          'alert' => '1',
+          'query' => nil,
+          'anchor' => nil
+        }
+      end
+    end
   end
 
   describe '#build_dashboard_url' do