Merge branch 'sy-alert-runbook-ui-be' into 'master'

Allow saving of runbooks to PrometheusAlert

See merge request gitlab-org/gitlab!38234

app/controllers/projects/prometheus/alerts_controller.rb

@@ -39,7 +39,7 @@ module Projects
 
           render json: serialize_as_json(@alert)
         else
-          head :no_content
+          head :bad_request
         end
       end
 
@@ -49,7 +49,7 @@ module Projects
 
           render json: serialize_as_json(alert)
         else
-          head :no_content
+          head :bad_request
         end
       end
 
@@ -59,14 +59,14 @@ module Projects
 
           head :ok
         else
-          head :no_content
+          head :bad_request
         end
       end
 
       private
 
       def alerts_params
-        params.permit(:operator, :threshold, :environment_id, :prometheus_metric_id)
+        params.permit(:operator, :threshold, :environment_id, :prometheus_metric_id, :runbook_url)
       end
 
       def notify_service

app/models/prometheus_alert.rb

@@ -22,6 +22,8 @@ class PrometheusAlert < ApplicationRecord
   after_destroy :clear_prometheus_adapter_cache!
 
   validates :environment, :project, :prometheus_metric, presence: true
+  validates :runbook_url, length: { maximum: 255 }, allow_blank: true,
+            addressable_url: { enforce_sanitization: true, ascii_only: true }
   validate :require_valid_environment_project!
   validate :require_valid_metric_project!
 
@@ -59,6 +61,9 @@ class PrometheusAlert < ApplicationRecord
         "gitlab" => "hook",
         "gitlab_alert_id" => prometheus_metric_id,
         "gitlab_prometheus_alert_id" => id
+      },
+      "annotations" => {
+        "runbook" => runbook_url
       }
     }
   end

app/serializers/prometheus_alert_entity.rb

@@ -7,6 +7,7 @@ class PrometheusAlertEntity < Grape::Entity
   expose :title
   expose :query
   expose :threshold
+  expose :runbook_url
 
   expose :operator do |prometheus_alert|
     prometheus_alert.computed_operator

changelogs/unreleased/sy-alert-runbook-ui-be.yml

+---
+title: Add support for runbook url to PrometheusAlert table
+merge_request: 38234
+author:
+type: added

db/migrate/20200729191227_add_runbook_to_prometheus_alert.rb

+# frozen_string_literal: true
+
+class AddRunbookToPrometheusAlert < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    # limit is added in 20200501000002_add_text_limit_to_sprints_extended_title
+    add_column :prometheus_alerts, :runbook_url, :text # rubocop:disable Migration/AddLimitToTextColumns
+  end
+
+  def down
+    remove_column :prometheus_alerts, :runbook_url
+  end
+end

db/migrate/20200729200808_add_text_limit_to_runbook_on_prometheus_alerts.rb

+# frozen_string_literal: true
+
+class AddTextLimitToRunbookOnPrometheusAlerts < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_text_limit :prometheus_alerts, :runbook_url, 255
+  end
+
+  def down
+    remove_text_limit :prometheus_alerts, :runbook_url
+  end
+end

db/schema_migrations/20200729191227

+34d53fcb98b82f3da7eeacd7bfabfd4118b51c448418f20227f7d5b05a0077dc
\ No newline at end of file

db/schema_migrations/20200729200808

+a1a896fc3fe060b34da9cde46ac0ddb8b973d077bcae3bb36677f9d02d2a3509
\ No newline at end of file

db/structure.sql

@@ -14637,7 +14637,9 @@ CREATE TABLE public.prometheus_alerts (
     operator integer NOT NULL,
     environment_id integer NOT NULL,
     project_id integer NOT NULL,
-    prometheus_metric_id integer NOT NULL
+    prometheus_metric_id integer NOT NULL,
+    runbook_url text,
+    CONSTRAINT check_cb76d7e629 CHECK ((char_length(runbook_url) <= 255))
 );
 
 CREATE SEQUENCE public.prometheus_alerts_id_seq

spec/controllers/projects/prometheus/alerts_controller_spec.rb

@@ -111,6 +111,7 @@ RSpec.describe Projects::Prometheus::AlertsController do
   describe 'GET #show' do
     let(:alert) do
       create(:prometheus_alert,
+             :with_runbook_url,
              project: project,
              environment: environment,
              prometheus_metric: metric)
@@ -140,6 +141,7 @@ RSpec.describe Projects::Prometheus::AlertsController do
           'query' => alert.query,
           'operator' => alert.computed_operator,
           'threshold' => alert.threshold,
+          'runbook_url' => alert.runbook_url,
           'alert_path' => alert_path(alert)
         }
       end
@@ -225,7 +227,8 @@ RSpec.describe Projects::Prometheus::AlertsController do
         'title' => metric.title,
         'query' => metric.query,
         'operator' => '>',
-        'threshold' => 1.0
+        'threshold' => 1.0,
+        'runbook_url' => 'https://sample.runbook.com'
       }
     end
 
@@ -234,6 +237,7 @@ RSpec.describe Projects::Prometheus::AlertsController do
         opts,
         operator: '>',
         threshold: '1',
+        runbook_url: 'https://sample.runbook.com',
         environment_id: environment,
         prometheus_metric_id: metric
       )
@@ -250,14 +254,14 @@ RSpec.describe Projects::Prometheus::AlertsController do
       expect(json_response).to include(alert_params)
     end
 
-    it 'returns no_content for an invalid metric' do
+    it 'returns bad_request for an invalid metric' do
       make_request(prometheus_metric_id: 'invalid')
 
-      expect(response).to have_gitlab_http_status(:no_content)
+      expect(response).to have_gitlab_http_status(:bad_request)
     end
 
     it_behaves_like 'unprivileged'
-    it_behaves_like 'project non-specific environment', :no_content
+    it_behaves_like 'project non-specific environment', :bad_request
   end
 
   describe 'PUT #update' do
@@ -304,6 +308,12 @@ RSpec.describe Projects::Prometheus::AlertsController do
       expect(json_response).to include(alert_params)
     end
 
+    it 'returns bad_request for an invalid alert data' do
+      make_request(runbook_url: 'bad-url')
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+    end
+
     it_behaves_like 'unprivileged'
     it_behaves_like 'project non-specific environment', :not_found
     it_behaves_like 'project non-specific metric', :not_found

spec/factories/prometheus_alert.rb

@@ -13,5 +13,9 @@ FactoryBot.define do
     prometheus_metric do |alert|
       build(:prometheus_metric, project: alert.project)
     end
+
+    trait :with_runbook_url do
+      runbook_url { 'https://runbooks.gitlab.com/metric_gt_1'}
+    end
   end
 end

spec/models/prometheus_alert_spec.rb

@@ -74,6 +74,34 @@ RSpec.describe PrometheusAlert do
     end
   end
 
+  describe 'runbook validations' do
+    it 'disallow invalid urls' do
+      unsafe_url = %{https://replaceme.com/'><script>alert(document.cookie)</script>}
+      non_ascii_url = 'http://gitlab.com/user/project1/wiki/something€'
+      excessively_long_url = 'https://gitla' + 'b' * 1024 + '.com'
+
+      is_expected.not_to allow_values(
+        unsafe_url,
+        non_ascii_url,
+        excessively_long_url
+      ).for(:runbook_url)
+    end
+
+    it 'allow valid urls' do
+      external_url = 'http://runbook.gitlab.com/'
+      internal_url = 'http://192.168.1.1'
+      blank_url = ''
+      nil_url = nil
+
+      is_expected.to allow_value(
+        external_url,
+        internal_url,
+        blank_url,
+        nil_url
+      ).for(:runbook_url)
+    end
+  end
+
   describe '#full_query' do
     before do
       subject.operator = "gt"
@@ -91,6 +119,7 @@ RSpec.describe PrometheusAlert do
       subject.operator = "gt"
       subject.threshold = 1
       subject.prometheus_metric = metric
+      subject.runbook_url = 'runbook'
     end
 
     it 'returns the params of the prometheus alert' do
@@ -102,7 +131,11 @@ RSpec.describe PrometheusAlert do
           "gitlab" => "hook",
           "gitlab_alert_id" => metric.id,
           "gitlab_prometheus_alert_id" => subject.id
-        })
+        },
+        "annotations" => {
+          "runbook" => "runbook"
+        }
+      )
     end
   end
 end

spec/serializers/prometheus_alert_entity_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe PrometheusAlertEntity do
     end
 
     it 'exposes prometheus_alert attributes' do
-      expect(subject).to include(:id, :title, :query, :operator, :threshold)
+      expect(subject).to include(:id, :title, :query, :operator, :threshold, :runbook_url)
     end
 
     it 'exposes alert_path' do