Merge branch '214557-gitlab-alert-fingerprinting' into 'master'

Add fingerprinting for entire payloads

See merge request gitlab-org/gitlab!34820

app/services/projects/alerting/notify_service.rb

@@ -76,7 +76,7 @@ module Projects
       end
 
       def parsed_payload
-        Gitlab::Alerting::NotificationPayloadParser.call(params.to_h)
+        Gitlab::Alerting::NotificationPayloadParser.call(params.to_h, project)
       end
 
       def valid_token?(token)

ee/app/models/license.rb

@@ -74,6 +74,7 @@ class License < ApplicationRecord
     file_locks
     geo
     github_project_service_integration
+    generic_alert_fingerprinting
     group_allowed_email_domains
     group_ip_restriction
     group_project_templates

ee/lib/ee/gitlab/alerting/notification_payload_parser.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Alerting
+      module NotificationPayloadParser
+        extend ::Gitlab::Utils::Override
+
+        EXCLUDED_PAYLOAD_FINGERPRINT_PARAMS = %w(start_time hosts).freeze
+
+        # Currently we use full payloads, when generating a fingerprint.
+        # This results in a quite strict fingerprint.
+        # Over time we can relax these rules.
+        # See https://gitlab.com/gitlab-org/gitlab/-/issues/214557#note_362795447
+        override :fingerprint
+        def fingerprint
+          return super if payload[:fingerprint].present? || !generic_alert_fingerprinting_enabled?
+
+          payload_excluding_params = payload.excluding(EXCLUDED_PAYLOAD_FINGERPRINT_PARAMS)
+
+          return if payload_excluding_params.none? { |_, v| v.present? }
+
+          ::Gitlab::AlertManagement::Fingerprint.generate(payload_excluding_params)
+        end
+
+        private
+
+        def generic_alert_fingerprinting_enabled?
+          project.feature_available?(:generic_alert_fingerprinting) &&
+            project.beta_feature_available?(:generic_alert_fingerprinting)
+        end
+      end
+    end
+  end
+end

ee/spec/lib/ee/gitlab/alerting/notification_payload_parser_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Alerting::NotificationPayloadParser do
+  let(:project) { build_stubbed(:project) }
+
+  describe '.call' do
+    subject(:parsed) { described_class.call(payload, project) }
+
+    let(:payload) do
+      {
+        'title' => 'alert title',
+        'start_time' => Time.current,
+        'description' => 'Description',
+        'monitoring_tool' => 'Monitoring tool name',
+        'service' => 'Service',
+        'hosts' => ['gitlab.com'],
+        'severity' => 'low'
+      }
+    end
+
+    describe 'fingerprint' do
+      subject(:fingerprint) { parsed.dig('annotations', 'fingerprint') }
+
+      context 'license feature enabled' do
+        before do
+          stub_licensed_features(generic_alert_fingerprinting: true)
+        end
+
+        it 'generates the fingerprint from the payload' do
+          fingerprint_payload = payload.excluding('start_time', 'hosts')
+          expected_fingerprint = Gitlab::AlertManagement::Fingerprint.generate(fingerprint_payload)
+
+          expect(fingerprint).to eq(expected_fingerprint)
+        end
+
+        context 'feature not enabled' do
+          before do
+            stub_feature_flags(generic_alert_fingerprinting: false)
+          end
+
+          it { is_expected.to eq(nil) }
+        end
+
+        context 'payload has no values' do
+          let(:payload) do
+            {
+              'start_time' => Time.current,
+              'hosts' => ['gitlab.com'],
+              'title' => ' '
+            }
+          end
+
+          it { is_expected.to eq(nil) }
+        end
+      end
+
+      context 'license feature not enabled' do
+        it { is_expected.to eq(nil) }
+      end
+    end
+  end
+end

ee/spec/services/projects/alerting/notify_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::Alerting::NotifyService do
+  let_it_be(:project, refind: true) { create(:project) }
+
+  describe '#execute' do
+    let(:service) { described_class.new(project, nil, payload) }
+    let(:token) { alerts_service.token }
+    let(:payload) do
+      {
+        title: 'Test alert title'
+      }
+    end
+
+    let(:alerts_service) { create(:alerts_service, project: project) }
+
+    subject { service.execute(token) }
+
+    context 'existing alert with same payload fingerprint' do
+      let(:existing_alert) do
+        service.execute(token)
+        AlertManagement::Alert.last!
+      end
+
+      before do
+        stub_licensed_features(generic_alert_fingerprinting: fingerprinting_enabled)
+        existing_alert # create existing alert
+      end
+
+      context 'generic fingerprinting feature not enabled' do
+        let(:fingerprinting_enabled) { false }
+
+        it 'creates AlertManagement::Alert' do
+          expect { subject }.to change(AlertManagement::Alert, :count)
+        end
+
+        it 'does not increment the existing alert count' do
+          expect { subject }.not_to change { existing_alert.reload.events }
+        end
+      end
+
+      context 'generic fingerprinting feature enabled' do
+        let(:fingerprinting_enabled) { true }
+
+        it 'does not create AlertManagement::Alert' do
+          expect { subject }.not_to change(AlertManagement::Alert, :count)
+        end
+
+        it 'increments the existing alert count' do
+          expect { subject }.to change { existing_alert.reload.events }.from(1).to(2)
+        end
+      end
+    end
+  end
+end

lib/gitlab/alert_management/alert_params.rb

@@ -8,7 +8,7 @@ module Gitlab
       }.freeze
 
       def self.from_generic_alert(project:, payload:)
-        parsed_payload = Gitlab::Alerting::NotificationPayloadParser.call(payload).with_indifferent_access
+        parsed_payload = Gitlab::Alerting::NotificationPayloadParser.call(payload, project).with_indifferent_access
         annotations = parsed_payload[:annotations]
 
         {

lib/gitlab/alert_management/fingerprint.rb

@@ -10,11 +10,14 @@ module Gitlab
       def generate(data)
         return unless data.present?
 
-        if data.is_a?(Array)
-          data = flatten_array(data)
-        end
+        string = case data
+                 when Array then flatten_array(data)
+                 when Hash then flatten_hash(data)
+                 else
+                   data.to_s
+                 end
 
-        Digest::SHA1.hexdigest(data.to_s)
+        Digest::SHA1.hexdigest(string)
       end
 
       private
@@ -22,6 +25,11 @@ module Gitlab
       def flatten_array(array)
         array.flatten.map!(&:to_s).join
       end
+
+      def flatten_hash(hash)
+        # Sort hash so SHA generated is the same
+        Gitlab::Utils::SafeInlineHash.merge_keys!(hash).sort.to_s
+      end
     end
   end
 end

lib/gitlab/alerting/notification_payload_parser.rb

@@ -8,12 +8,13 @@ module Gitlab
       DEFAULT_TITLE = 'New: Incident'
       DEFAULT_SEVERITY = 'critical'
 
-      def initialize(payload)
+      def initialize(payload, project)
         @payload = payload.to_h.with_indifferent_access
+        @project = project
       end
 
-      def self.call(payload)
-        new(payload).call
+      def self.call(payload, project)
+        new(payload, project).call
       end
 
       def call
@@ -25,7 +26,7 @@ module Gitlab
 
       private
 
-      attr_reader :payload
+      attr_reader :payload, :project
 
       def title
         payload[:title].presence || DEFAULT_TITLE
@@ -84,3 +85,5 @@ module Gitlab
     end
   end
 end
+
+Gitlab::Alerting::NotificationPayloadParser.prepend_if_ee('EE::Gitlab::Alerting::NotificationPayloadParser')

spec/lib/gitlab/alert_management/fingerprint_spec.rb

@@ -13,34 +13,62 @@ RSpec.describe Gitlab::AlertManagement::Fingerprint do
     context 'when data is an array' do
       let(:data) { [1, 'fingerprint', 'given'] }
 
-      it 'flattens the array' do
-        expect_next_instance_of(described_class) do |obj|
-          expect(obj).to receive(:flatten_array)
-        end
-
-        subject
-      end
-
       it 'returns the hashed fingerprint' do
         expected_fingerprint = Digest::SHA1.hexdigest(data.flatten.map!(&:to_s).join)
         expect(subject).to eq(expected_fingerprint)
       end
-    end
 
-    context 'when data is a non-array type' do
-      where(:data) do
-        [
-          111,
-          'fingerprint',
-          :fingerprint,
-          true,
-          { test: true }
-        ]
+      context 'with a variety of data' do
+        where(:data) do
+          [
+            111,
+            'fingerprint',
+            :fingerprint,
+            true
+          ]
+        end
+
+        with_them do
+          it 'performs like a hashed fingerprint' do
+            expect(subject).to eq(Digest::SHA1.hexdigest(data.to_s))
+          end
+        end
       end
+    end
 
-      with_them do
+    context 'when data is a hash' do
+      let(:data) { { test: true } }
+
+      shared_examples 'fingerprinted Hash' do
         it 'performs like a hashed fingerprint' do
-          expect(subject).to eq(Digest::SHA1.hexdigest(data.to_s))
+          flattened_hash = Gitlab::Utils::SafeInlineHash.merge_keys!(data).sort.to_s
+          expect(subject).to eq(Digest::SHA1.hexdigest(flattened_hash))
+        end
+      end
+
+      it_behaves_like 'fingerprinted Hash'
+
+      context 'hashes with different order' do
+        it 'calculates the same result' do
+          data = { test: true, another_test: 1 }
+          data_hash = described_class.generate(data)
+
+          reverse_data = { another_test: 1, test: true }
+          reverse_data_hash = described_class.generate(reverse_data)
+
+          expect(data_hash).to eq(reverse_data_hash)
+        end
+      end
+
+      context 'hash is too large' do
+        before do
+          expect_next_instance_of(Gitlab::Utils::SafeInlineHash) do |obj|
+            expect(obj).to receive(:valid?).and_return(false)
+          end
+        end
+
+        it 'raises an error' do
+          expect { subject }.to raise_error(ArgumentError)
         end
       end
     end

spec/lib/gitlab/alerting/notification_payload_parser_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Alerting::NotificationPayloadParser do
+  let_it_be(:project) { build(:project) }
+
   describe '.call' do
     let(:starts_at) { Time.current.change(usec: 0) }
     let(:payload) do
@@ -17,7 +19,7 @@ RSpec.describe Gitlab::Alerting::NotificationPayloadParser do
       }
     end
 
-    subject { described_class.call(payload) }
+    subject { described_class.call(payload, project) }
 
     it 'returns Prometheus-like payload' do
       is_expected.to eq(