Commit a08d4cad authored by syasonik's avatar syasonik

Defend against dashboard errors, rework sequence

parent 25f95771
...@@ -158,7 +158,7 @@ class Projects::EnvironmentsController < Projects::ApplicationController ...@@ -158,7 +158,7 @@ class Projects::EnvironmentsController < Projects::ApplicationController
end end
def metrics_dashboard def metrics_dashboard
render_403 && return unless Feature.enabled?(:environment_metrics_use_prometheus_endpoint, @project) return render_403 unless Feature.enabled?(:environment_metrics_use_prometheus_endpoint, @project)
result = Gitlab::MetricsDashboard::Service.new(@project, @current_user, environment: environment).get_dashboard result = Gitlab::MetricsDashboard::Service.new(@project, @current_user, environment: environment).get_dashboard
respond_to do |format| respond_to do |format|
......
...@@ -3,21 +3,19 @@ ...@@ -3,21 +3,19 @@
module Gitlab module Gitlab
module MetricsDashboard module MetricsDashboard
# Responsible for processesing a dashboard hash, inserting # Responsible for processesing a dashboard hash, inserting
# relevantDB records & sorting for proper rendering in # relevant DB records & sorting for proper rendering in
# the UI. These includes shared metric info, custom metrics # the UI. These includes shared metric info, custom metrics
# info, and alerts (only in EE). # info, and alerts (only in EE).
class Processor class Processor
def initialize(project, environment) SEQUENCE = [
@project = project
@environment = environment
end
def sequence
[
Stages::CommonMetricsInserter, Stages::CommonMetricsInserter,
Stages::ProjectMetricsInserter, Stages::ProjectMetricsInserter,
Stages::Sorter Stages::Sorter
] ].freeze
def initialize(project, environment)
@project = project
@environment = environment
end end
# Returns a new dashboard hash with the results of # Returns a new dashboard hash with the results of
...@@ -30,6 +28,12 @@ module Gitlab ...@@ -30,6 +28,12 @@ module Gitlab
dashboard dashboard
end end
private
def sequence
SEQUENCE
end
end end
end end
end end
...@@ -14,6 +14,8 @@ module Gitlab ...@@ -14,6 +14,8 @@ module Gitlab
dashboard = process_dashboard(dashboard_string) dashboard = process_dashboard(dashboard_string)
success(dashboard: dashboard) success(dashboard: dashboard)
rescue Gitlab::MetricsDashboard::Stages::BaseStage::DashboardLayoutError => e
error(e.message, :unprocessable_entity)
end end
private private
...@@ -27,6 +29,7 @@ module Gitlab ...@@ -27,6 +29,7 @@ module Gitlab
"metrics_dashboard_#{SYSTEM_DASHBOARD_NAME}" "metrics_dashboard_#{SYSTEM_DASHBOARD_NAME}"
end end
# Returns a new dashboard Hash, supplemented with DB info
def process_dashboard(dashboard) def process_dashboard(dashboard)
Processor.new(project, params[:environment]).process(dashboard) Processor.new(project, params[:environment]).process(dashboard)
end end
......
...@@ -4,6 +4,8 @@ module Gitlab ...@@ -4,6 +4,8 @@ module Gitlab
module MetricsDashboard module MetricsDashboard
module Stages module Stages
class BaseStage class BaseStage
DashboardLayoutError = Class.new(StandardError)
DEFAULT_PANEL_TYPE = 'area-chart' DEFAULT_PANEL_TYPE = 'area-chart'
attr_reader :project, :environment attr_reader :project, :environment
...@@ -23,9 +25,27 @@ module Gitlab ...@@ -23,9 +25,27 @@ module Gitlab
protected protected
def missing_panel_groups!
raise DashboardLayoutError.new('Top-level key :panel_groups must be an array')
end
def missing_panels!
raise DashboardLayoutError.new('Each "panel_group" must define an array :panels')
end
def missing_metrics!
raise DashboardLayoutError.new('Each "panel" must define an array :metrics')
end
def for_metrics(dashboard) def for_metrics(dashboard)
missing_panel_groups! unless dashboard[:panel_groups].is_a?(Array)
dashboard[:panel_groups].each do |panel_group| dashboard[:panel_groups].each do |panel_group|
missing_panels! unless panel_group[:panels].is_a?(Array)
panel_group[:panels].each do |panel| panel_group[:panels].each do |panel|
missing_metrics! unless panel[:metrics].is_a?(Array)
panel[:metrics].each do |metric| panel[:metrics].each do |metric|
yield metric yield metric
end end
......
...@@ -60,15 +60,21 @@ module Gitlab ...@@ -60,15 +60,21 @@ module Gitlab
end end
def find_panel_group(panel_groups, metric) def find_panel_group(panel_groups, metric)
return unless panel_groups
panel_groups.find { |group| group[:group] == metric.group_title } panel_groups.find { |group| group[:group] == metric.group_title }
end end
def find_panel(panels, metric) def find_panel(panels, metric)
return unless panels
panel_identifiers = [DEFAULT_PANEL_TYPE, metric.title, metric.y_label] panel_identifiers = [DEFAULT_PANEL_TYPE, metric.title, metric.y_label]
panels.find { |panel| panel.values_at(:type, :title, :y_label) == panel_identifiers } panels.find { |panel| panel.values_at(:type, :title, :y_label) == panel_identifiers }
end end
def find_metric(metrics, metric) def find_metric(metrics, metric)
return unless metrics
metrics.find { |m| m[:id] == metric.identifier } metrics.find { |m| m[:id] == metric.identifier }
end end
......
...@@ -5,6 +5,8 @@ module Gitlab ...@@ -5,6 +5,8 @@ module Gitlab
module Stages module Stages
class Sorter < BaseStage class Sorter < BaseStage
def transform!(dashboard) def transform!(dashboard)
missing_panel_groups! unless dashboard[:panel_groups].is_a? Array
sort_groups!(dashboard) sort_groups!(dashboard)
sort_panels!(dashboard) sort_panels!(dashboard)
end end
...@@ -19,6 +21,8 @@ module Gitlab ...@@ -19,6 +21,8 @@ module Gitlab
# Sorts the panels in the dashboard by the :weight key # Sorts the panels in the dashboard by the :weight key
def sort_panels!(dashboard) def sort_panels!(dashboard)
dashboard[:panel_groups].each do |group| dashboard[:panel_groups].each do |group|
missing_panels! unless group[:panels].is_a? Array
group[:panels] = group[:panels].sort_by { |panel| -panel[:weight].to_i } group[:panels] = group[:panels].sort_by { |panel| -panel[:weight].to_i }
end end
end end
......
...@@ -482,6 +482,19 @@ describe Projects::EnvironmentsController do ...@@ -482,6 +482,19 @@ describe Projects::EnvironmentsController do
expect(json_response.keys).to contain_exactly('dashboard', 'status') expect(json_response.keys).to contain_exactly('dashboard', 'status')
expect(json_response['dashboard']).to be_an_instance_of(Hash) expect(json_response['dashboard']).to be_an_instance_of(Hash)
end end
context 'when the dashboard could not be provided' do
before do
allow(YAML).to receive(:load_file).and_return({})
end
it 'returns an error response' do
get :metrics_dashboard, params: environment_params(format: :json)
expect(response).to have_gitlab_http_status(:unprocessable_entity)
expect(json_response.keys).to contain_exactly('message', 'status', 'http_status')
end
end
end end
end end
......
...@@ -47,6 +47,32 @@ describe Gitlab::MetricsDashboard::Processor do ...@@ -47,6 +47,32 @@ describe Gitlab::MetricsDashboard::Processor do
expect(actual_metrics_order).to eq expected_metrics_order expect(actual_metrics_order).to eq expected_metrics_order
end end
end end
shared_examples_for 'errors with message' do |expected_message|
it 'raises a DashboardLayoutError' do
error_class = Gitlab::MetricsDashboard::Stages::BaseStage::DashboardLayoutError
expect { dashboard }.to raise_error(error_class, expected_message)
end
end
context 'when the dashboard is missing panel_groups' do
let(:dashboard_yml) { {} }
it_behaves_like 'errors with message', 'Top-level key :panel_groups must be an array'
end
context 'when the dashboard contains a panel_group which is missing panels' do
let(:dashboard_yml) { { panel_groups: [{}] } }
it_behaves_like 'errors with message', 'Each "panel_group" must define an array :panels'
end
context 'when the dashboard contains a panel which is missing metrics' do
let(:dashboard_yml) { { panel_groups: [{ panels: [{}] }] } }
it_behaves_like 'errors with message', 'Each "panel" must define an array :metrics'
end
end end
private private
......
...@@ -24,5 +24,21 @@ describe Gitlab::MetricsDashboard::Service, :use_clean_rails_memory_store_cachin ...@@ -24,5 +24,21 @@ describe Gitlab::MetricsDashboard::Service, :use_clean_rails_memory_store_cachin
described_class.new(project, environment).get_dashboard described_class.new(project, environment).get_dashboard
described_class.new(project, environment).get_dashboard described_class.new(project, environment).get_dashboard
end end
context 'when the dashboard is configured incorrectly' do
let(:bad_dashboard) { {} }
before do
allow(described_class).to receive(:system_dashboard).and_return(bad_dashboard)
end
it 'returns an appropriate message and status code' do
result = described_class.new(project, environment).get_dashboard
expect(result.keys).to contain_exactly(:message, :http_status, :status)
expect(result[:status]).to eq(:error)
expect(result[:status]).to eq(:unprocessable_entity)
end
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment