Merge branch 'rp-use-gitlab-yaml-loader-dashboard-stages' into 'master'

Use Gitlab::Config::Loader::Yaml to load yaml in metrics dashboard code

See merge request gitlab-org/gitlab!34834

app/services/metrics/dashboard/base_service.rb

@@ -84,6 +84,17 @@ module Metrics
         params[:dashboard_path]
       end
 
+      def load_yaml(data)
+        ::Gitlab::Config::Loader::Yaml.new(data).load_raw!
+      rescue Gitlab::Config::Loader::Yaml::NotHashError
+        # Raise more informative error in app/models/performance_monitoring/prometheus_dashboard.rb.
+        {}
+      rescue Gitlab::Config::Loader::Yaml::DataTooLargeError => exception
+        raise Gitlab::Metrics::Dashboard::Errors::LayoutError, exception.message
+      rescue Gitlab::Config::Loader::FormatError
+        raise Gitlab::Metrics::Dashboard::Errors::LayoutError, _('Invalid yaml')
+      end
+
       # @return [Hash] an unmodified dashboard
       def get_raw_dashboard
         raise NotImplementedError

app/services/metrics/dashboard/custom_dashboard_service.rb

@@ -42,7 +42,7 @@ module Metrics
       def get_raw_dashboard
         yml = self.class.file_finder(project).read(dashboard_path)
 
-        YAML.safe_load(yml)
+        load_yaml(yml)
       end
 
       def cache_key

app/services/metrics/dashboard/predefined_dashboard_service.rb

@@ -40,7 +40,7 @@ module Metrics
       def get_raw_dashboard
         yml = File.read(Rails.root.join(dashboard_path))
 
-        YAML.safe_load(yml)
+        load_yaml(yml)
       end
 
       def sequence

changelogs/unreleased/rp-use-gitlab-yaml-loader-dashboard-stages.yml

+---
+title: Display error if metrics dashboard YAML is too large
+merge_request: 34834
+author:
+type: changed

lib/gitlab/config/loader/yaml.rb

@@ -5,6 +5,7 @@ module Gitlab
     module Loader
       class Yaml
         DataTooLargeError = Class.new(Loader::FormatError)
+        NotHashError = Class.new(Loader::FormatError)
 
         include Gitlab::Utils::StrongMemoize
 
@@ -23,7 +24,7 @@ module Gitlab
 
         def load_raw!
           raise DataTooLargeError, 'The parsed YAML is too big' if too_big?
-          raise Loader::FormatError, 'Invalid configuration format' unless hash?
+          raise NotHashError, 'Invalid configuration format' unless hash?
 
           @config
         end

locale/gitlab.pot

@@ -12464,6 +12464,9 @@ msgstr ""
 msgid "Invalid two-factor code."
 msgstr ""
 
+msgid "Invalid yaml"
+msgstr ""
+
 msgid "Invitation"
 msgstr ""
 

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

@@ -118,7 +118,7 @@ RSpec.describe Gitlab::Metrics::Dashboard::Finder, :use_clean_rails_memory_store
   end
 
   describe '.find_raw' do
-    let(:dashboard) { YAML.load_file(Rails.root.join('config', 'prometheus', 'common_metrics.yml')) }
+    let(:dashboard) { load_dashboard_yaml(File.read(Rails.root.join('config', 'prometheus', 'common_metrics.yml'))) }
     let(:params) { {} }
 
     subject { described_class.find_raw(project, **params) }
@@ -132,7 +132,7 @@ RSpec.describe Gitlab::Metrics::Dashboard::Finder, :use_clean_rails_memory_store
     end
 
     context 'when an existing project dashboard is specified' do
-      let(:dashboard) { YAML.safe_load(fixture_file('lib/gitlab/metrics/dashboard/sample_dashboard.yml')) }
+      let(:dashboard) { load_sample_dashboard }
       let(:params) { { dashboard_path: '.gitlab/dashboards/test.yml' } }
       let(:project) { project_with_dashboard(params[:dashboard_path]) }
 

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

@@ -3,9 +3,11 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Metrics::Dashboard::Processor do
+  include MetricsDashboardHelpers
+
   let(:project) { build(:project) }
   let(:environment) { create(:environment, project: project) }
-  let(:dashboard_yml) { YAML.load_file('spec/fixtures/lib/gitlab/metrics/dashboard/sample_dashboard.yml') }
+  let(:dashboard_yml) { load_sample_dashboard }
 
   describe 'process' do
     let(:sequence) do

spec/lib/gitlab/metrics/dashboard/stages/panel_ids_inserter_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Metrics::Dashboard::Stages::PanelIdsInserter do
+  include MetricsDashboardHelpers
+
   let(:project) { build_stubbed(:project) }
 
   def fetch_panel_ids(dashboard_hash)
@@ -12,7 +14,7 @@ RSpec.describe Gitlab::Metrics::Dashboard::Stages::PanelIdsInserter do
   describe '#transform!' do
     subject(:transform!) { described_class.new(project, dashboard, nil).transform! }
 
-    let(:dashboard) { YAML.safe_load(fixture_file('lib/gitlab/metrics/dashboard/sample_dashboard.yml')).deep_symbolize_keys }
+    let(:dashboard) { load_sample_dashboard.deep_symbolize_keys }
 
     context 'when dashboard panels are present' do
       it 'assigns unique ids to each panel using PerformanceMonitoring::PrometheusPanel', :aggregate_failures do

spec/services/metrics/dashboard/custom_dashboard_service_spec.rb

@@ -9,17 +9,24 @@ RSpec.describe Metrics::Dashboard::CustomDashboardService, :use_clean_rails_memo
   let_it_be(:project) { create(:project) }
   let_it_be(:environment) { create(:environment, project: project) }
 
+  let(:dashboard_path) { '.gitlab/dashboards/test.yml' }
+  let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
+
+  subject { described_class.new(*service_params) }
+
   before do
     project.add_maintainer(user)
   end
 
+  describe '#raw_dashboard' do
+    let(:project) { project_with_dashboard(dashboard_path) }
+
+    it_behaves_like '#raw_dashboard raises error if dashboard loading fails'
+  end
+
   describe '#get_dashboard' do
-    let(:dashboard_path) { '.gitlab/dashboards/test.yml' }
-    let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
     let(:service_call) { subject.get_dashboard }
 
-    subject { described_class.new(*service_params) }
-
     context 'when the dashboard does not exist' do
       it_behaves_like 'misconfigured dashboard service response', :not_found
 

spec/services/metrics/dashboard/pod_dashboard_service_spec.rb

@@ -9,10 +9,19 @@ RSpec.describe Metrics::Dashboard::PodDashboardService, :use_clean_rails_memory_
   let_it_be(:project) { create(:project) }
   let_it_be(:environment) { create(:environment, project: project) }
 
+  let(:dashboard_path) { described_class::DASHBOARD_PATH }
+  let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
+
   before do
     project.add_maintainer(user)
   end
 
+  subject { described_class.new(*service_params) }
+
+  describe '#raw_dashboard' do
+    it_behaves_like '#raw_dashboard raises error if dashboard loading fails'
+  end
+
   describe '.valid_params?' do
     let(:params) { { dashboard_path: described_class::DASHBOARD_PATH } }
 
@@ -34,12 +43,8 @@ RSpec.describe Metrics::Dashboard::PodDashboardService, :use_clean_rails_memory_
   end
 
   describe '#get_dashboard' do
-    let(:dashboard_path) { described_class::DASHBOARD_PATH }
-    let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
     let(:service_call) { subject.get_dashboard }
 
-    subject { described_class.new(*service_params) }
-
     it_behaves_like 'valid dashboard service response'
     it_behaves_like 'caches the unprocessed dashboard for subsequent calls'
     it_behaves_like 'updates gitlab_metrics_dashboard_processing_time_ms metric'

spec/services/metrics/dashboard/self_monitoring_dashboard_service_spec.rb

@@ -9,13 +9,22 @@ RSpec.describe Metrics::Dashboard::SelfMonitoringDashboardService, :use_clean_ra
   let_it_be(:project) { create(:project) }
   let_it_be(:environment) { create(:environment, project: project) }
 
+  let(:service_params) { [project, user, { environment: environment }] }
+
   before do
     project.add_maintainer(user)
     stub_application_setting(self_monitoring_project_id: project.id)
   end
 
+  subject do
+    described_class.new(service_params)
+  end
+
+  describe '#raw_dashboard' do
+    it_behaves_like '#raw_dashboard raises error if dashboard loading fails'
+  end
+
   describe '#get_dashboard' do
-    let(:service_params) { [project, user, { environment: environment }] }
     let(:service_call) { subject.get_dashboard }
 
     subject { described_class.new(*service_params) }

spec/services/metrics/dashboard/system_dashboard_service_spec.rb

@@ -9,17 +9,22 @@ RSpec.describe Metrics::Dashboard::SystemDashboardService, :use_clean_rails_memo
   let_it_be(:project) { create(:project) }
   let_it_be(:environment) { create(:environment, project: project) }
 
+  let(:dashboard_path) { described_class::DASHBOARD_PATH }
+  let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
+
+  subject { described_class.new(*service_params) }
+
   before do
     project.add_maintainer(user)
   end
 
+  describe '#raw_dashboard' do
+    it_behaves_like '#raw_dashboard raises error if dashboard loading fails'
+  end
+
   describe '#get_dashboard' do
-    let(:dashboard_path) { described_class::DASHBOARD_PATH }
-    let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
     let(:service_call) { subject.get_dashboard }
 
-    subject { described_class.new(*service_params) }
-
     it_behaves_like 'valid dashboard service response'
     it_behaves_like 'raises error for users with insufficient permissions'
     it_behaves_like 'caches the unprocessed dashboard for subsequent calls'

spec/support/helpers/metrics_dashboard_helpers.rb

@@ -19,7 +19,11 @@ module MetricsDashboardHelpers
   end
 
   def load_sample_dashboard
-    YAML.safe_load(fixture_file('lib/gitlab/metrics/dashboard/sample_dashboard.yml'))
+    load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/sample_dashboard.yml'))
+  end
+
+  def load_dashboard_yaml(data)
+    ::Gitlab::Config::Loader::Yaml.new(data).load_raw!
   end
 
   def system_dashboard_path

spec/support/shared_examples/services/metrics/dashboard_shared_examples.rb

@@ -29,8 +29,10 @@ RSpec.shared_examples 'valid dashboard service response' do
 end
 
 RSpec.shared_examples 'caches the unprocessed dashboard for subsequent calls' do
-  it do
-    expect(YAML).to receive(:safe_load).once.and_call_original
+  specify do
+    expect_next_instance_of(::Gitlab::Config::Loader::Yaml) do |loader|
+      expect(loader).to receive(:load_raw!).once.and_call_original
+    end
 
     described_class.new(*service_params).get_dashboard
     described_class.new(*service_params).get_dashboard
@@ -128,3 +130,50 @@ RSpec.shared_examples 'updates gitlab_metrics_dashboard_processing_time_ms metri
     expect(metric.get(labels)).to be > 0
   end
 end
+
+RSpec.shared_examples '#raw_dashboard raises error if dashboard loading fails' do
+  context 'when yaml is too large' do
+    before do
+      allow_next_instance_of(::Gitlab::Config::Loader::Yaml) do |loader|
+        allow(loader).to receive(:load_raw!)
+          .and_raise(Gitlab::Config::Loader::Yaml::DataTooLargeError, 'The parsed YAML is too big')
+      end
+    end
+
+    it 'raises error' do
+      expect { subject.raw_dashboard }.to raise_error(
+        Gitlab::Metrics::Dashboard::Errors::LayoutError,
+        'The parsed YAML is too big'
+      )
+    end
+  end
+
+  context 'when yaml loader returns error' do
+    before do
+      allow_next_instance_of(::Gitlab::Config::Loader::Yaml) do |loader|
+        allow(loader).to receive(:load_raw!)
+          .and_raise(Gitlab::Config::Loader::FormatError, 'Invalid configuration format')
+      end
+    end
+
+    it 'raises error' do
+      expect { subject.raw_dashboard }.to raise_error(
+        Gitlab::Metrics::Dashboard::Errors::LayoutError,
+        'Invalid yaml'
+      )
+    end
+  end
+
+  context 'when yaml is not a hash' do
+    before do
+      allow_next_instance_of(::Gitlab::Config::Loader::Yaml) do |loader|
+        allow(loader).to receive(:load_raw!)
+          .and_raise(Gitlab::Config::Loader::Yaml::NotHashError, 'Invalid configuration format')
+      end
+    end
+
+    it 'returns nil' do
+      expect(subject.raw_dashboard).to eq({})
+    end
+  end
+end