Merge branch 'sy-truncate-alert-fields' into 'master'

Truncate AlertManagement::Alert fields which may be too long

See merge request gitlab-org/gitlab!45099

app/models/alert_management/alert.rb

@@ -38,12 +38,16 @@ module AlertManagement
 
     sha_attribute :fingerprint
 
+    TITLE_MAX_LENGTH = 200
+    DESCRIPTION_MAX_LENGTH = 1_000
+    SERVICE_MAX_LENGTH = 100
+    TOOL_MAX_LENGTH = 100
     HOSTS_MAX_LENGTH = 255
 
-    validates :title,           length: { maximum: 200 }, presence: true
-    validates :description,     length: { maximum: 1_000 }
-    validates :service,         length: { maximum: 100 }
-    validates :monitoring_tool, length: { maximum: 100 }
+    validates :title,           length: { maximum: TITLE_MAX_LENGTH }, presence: true
+    validates :description,     length: { maximum: DESCRIPTION_MAX_LENGTH }
+    validates :service,         length: { maximum: SERVICE_MAX_LENGTH }
+    validates :monitoring_tool, length: { maximum: TOOL_MAX_LENGTH }
     validates :project,         presence: true
     validates :events,          presence: true
     validates :severity,        presence: true
@@ -54,7 +58,7 @@ module AlertManagement
       conditions: -> { not_resolved },
       message: -> (object, data) { _('Cannot have multiple unresolved alerts') }
     }, unless: :resolved?
-    validate :hosts_length
+    validate :hosts_format
 
     enum severity: {
       critical: 0,
@@ -251,10 +255,11 @@ module AlertManagement
       Gitlab::DataBuilder::Alert.build(self)
     end
 
-    def hosts_length
+    def hosts_format
       return unless hosts
 
       errors.add(:hosts, "hosts array is over #{HOSTS_MAX_LENGTH} chars") if hosts.join.length > HOSTS_MAX_LENGTH
+      errors.add(:hosts, "hosts array cannot be nested") if hosts.flatten != hosts
     end
   end
 end

changelogs/unreleased/sy-truncate-alert-fields.yml

+---
+title: Truncate over-long alert fields instead of return error response
+merge_request: 45099
+author:
+type: changed

lib/gitlab/alert_management/payload/base.rb

@@ -88,19 +88,19 @@ module Gitlab
         # AlertManagement::Alert directly for read operations.
         def alert_params
           {
-            description: description,
+            description: description&.truncate(::AlertManagement::Alert::DESCRIPTION_MAX_LENGTH),
             ended_at: ends_at,
             environment: environment,
             fingerprint: gitlab_fingerprint,
-            hosts: Array(hosts),
-            monitoring_tool: monitoring_tool,
+            hosts: truncate_hosts(Array(hosts).flatten),
+            monitoring_tool: monitoring_tool&.truncate(::AlertManagement::Alert::TOOL_MAX_LENGTH),
             payload: payload,
             project_id: project.id,
             prometheus_alert: gitlab_alert,
-            service: service,
+            service: service&.truncate(::AlertManagement::Alert::SERVICE_MAX_LENGTH),
             severity: severity,
             started_at: starts_at,
-            title: title
+            title: title&.truncate(::AlertManagement::Alert::TITLE_MAX_LENGTH)
           }.transform_values(&:presence).compact
         end
 
@@ -135,6 +135,18 @@ module Gitlab
 
         def plain_gitlab_fingerprint; end
 
+        def truncate_hosts(hosts)
+          return hosts if hosts.join.length <= ::AlertManagement::Alert::HOSTS_MAX_LENGTH
+
+          hosts.inject([]) do |new_hosts, host|
+            remaining_length = ::AlertManagement::Alert::HOSTS_MAX_LENGTH - new_hosts.join.length
+
+            break new_hosts unless remaining_length > 0
+
+            new_hosts << host.to_s.truncate(remaining_length, omission: '')
+          end
+        end
+
         def value_for_paths(paths)
           target_path = paths.find { |path| payload&.dig(*path) }
 

spec/lib/gitlab/alert_management/payload/base_spec.rb

@@ -120,14 +120,107 @@ RSpec.describe Gitlab::AlertManagement::Payload::Base do
   end
 
   describe '#alert_params' do
+    subject { parsed_payload.alert_params }
+
+    context 'with every key' do
+      let_it_be(:raw_payload) { { 'key' => 'value' } }
+      let_it_be(:stubs) do
+        {
+          description: 'description',
+          ends_at: Time.current,
+          environment: create(:environment, project: project),
+          gitlab_fingerprint: 'gitlab_fingerprint',
+          hosts: 'hosts',
+          monitoring_tool: 'monitoring_tool',
+          gitlab_alert: create(:prometheus_alert, project: project),
+          service: 'service',
+          severity: 'critical',
+          starts_at: Time.current,
+          title: 'title'
+        }
+      end
+
+      let(:expected_result) do
+        {
+          description: stubs[:description],
+          ended_at: stubs[:ends_at],
+          environment: stubs[:environment],
+          fingerprint: stubs[:gitlab_fingerprint],
+          hosts: [stubs[:hosts]],
+          monitoring_tool: stubs[:monitoring_tool],
+          payload: raw_payload,
+          project_id: project.id,
+          prometheus_alert: stubs[:gitlab_alert],
+          service: stubs[:service],
+          severity: stubs[:severity],
+          started_at: stubs[:starts_at],
+          title: stubs[:title]
+        }
+      end
+
       before do
-      allow(parsed_payload).to receive(:title).and_return('title')
-      allow(parsed_payload).to receive(:description).and_return('description')
+        allow(parsed_payload).to receive_messages(stubs)
       end
 
-    subject { parsed_payload.alert_params }
+      it { is_expected.to eq(expected_result) }
+
+      it 'can generate a valid new alert' do
+        expect(::AlertManagement::Alert.new(subject.except(:ended_at))).to be_valid
+      end
+    end
+
+    context 'with too-long strings' do
+      let_it_be(:stubs) do
+        {
+          description: 'a' * (::AlertManagement::Alert::DESCRIPTION_MAX_LENGTH + 1),
+          hosts: 'b' * (::AlertManagement::Alert::HOSTS_MAX_LENGTH + 1),
+          monitoring_tool: 'c' * (::AlertManagement::Alert::TOOL_MAX_LENGTH + 1),
+          service: 'd' * (::AlertManagement::Alert::SERVICE_MAX_LENGTH + 1),
+          title: 'e' * (::AlertManagement::Alert::TITLE_MAX_LENGTH + 1)
+        }
+      end
 
-    it { is_expected.to eq({ description: 'description', project_id: project.id, title: 'title' }) }
+      before do
+        allow(parsed_payload).to receive_messages(stubs)
+      end
+
+      it do
+        is_expected.to eq({
+          description: stubs[:description].truncate(AlertManagement::Alert::DESCRIPTION_MAX_LENGTH),
+          hosts: ['b' * ::AlertManagement::Alert::HOSTS_MAX_LENGTH],
+          monitoring_tool: stubs[:monitoring_tool].truncate(::AlertManagement::Alert::TOOL_MAX_LENGTH),
+          service: stubs[:service].truncate(::AlertManagement::Alert::SERVICE_MAX_LENGTH),
+          project_id: project.id,
+          title: stubs[:title].truncate(::AlertManagement::Alert::TITLE_MAX_LENGTH)
+        })
+      end
+    end
+
+    context 'with too-long hosts array' do
+      let(:hosts) { %w(abc def ghij) }
+      let(:shortened_hosts) { %w(abc def ghi) }
+
+      before do
+        stub_const('::AlertManagement::Alert::HOSTS_MAX_LENGTH', 9)
+        allow(parsed_payload).to receive(:hosts).and_return(hosts)
+      end
+
+      it { is_expected.to eq(hosts: shortened_hosts, project_id: project.id) }
+
+      context 'with host cut off between elements' do
+        let(:hosts) { %w(abcde fghij) }
+        let(:shortened_hosts) { %w(abcde fghi) }
+
+        it { is_expected.to eq({ hosts: shortened_hosts, project_id: project.id }) }
+      end
+
+      context 'with nested hosts' do
+        let(:hosts) { ['abc', ['de', 'f'], 'g', 'hij'] } # rubocop:disable Style/WordArray
+        let(:shortened_hosts) { %w(abc de f g hi) }
+
+        it { is_expected.to eq({ hosts: shortened_hosts, project_id: project.id }) }
+      end
+    end
   end
 
   describe '#gitlab_fingerprint' do

spec/models/alert_management/alert_spec.rb

@@ -170,6 +170,12 @@ RSpec.describe AlertManagement::Alert do
 
         it { is_expected.to be_valid }
       end
+
+      context 'nested array' do
+        let(:hosts) { ['111.111.111.111', ['111.111.111.111']] }
+
+        it { is_expected.not_to be_valid }
+      end
     end
   end
 

spec/services/alert_management/process_prometheus_alert_service_spec.rb

@@ -117,15 +117,19 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
           end
 
           context 'when alert cannot be created' do
+            let(:errors) { double(messages: { hosts: ['hosts array is over 255 chars'] })}
+
             before do
-              payload['annotations']['title'] = 'description' * 50
+              allow(service).to receive(:alert).and_call_original
+              allow(service).to receive_message_chain(:alert, :save).and_return(false)
+              allow(service).to receive_message_chain(:alert, :errors).and_return(errors)
             end
 
             it 'writes a warning to the log' do
               expect(Gitlab::AppLogger).to receive(:warn).with(
                 message: 'Unable to create AlertManagement::Alert',
                 project_id: project.id,
-                alert_errors: { title: ["is too long (maximum is 200 characters)"] }
+                alert_errors: { hosts: ['hosts array is over 255 chars'] }
               )
 
               execute