diff --git a/app/controllers/projects/alerting/notifications_controller.rb b/app/controllers/projects/alerting/notifications_controller.rb index 2241ded2db83896b77516d6f5fd9c300a4694dc8..b7ea33e47b25c4389839b63b8e98645be89a1453 100644 --- a/app/controllers/projects/alerting/notifications_controller.rb +++ b/app/controllers/projects/alerting/notifications_controller.rb @@ -14,7 +14,7 @@ class NotificationsController < Projects::ApplicationController def create token = extract_alert_manager_token(request) - result = notify_service.execute(token) + result = notify_service.execute(token, integration) head result.http_status end @@ -45,6 +45,20 @@ def notify_service_class end end + def integration + return unless Feature.enabled?(:multiple_http_integrations, project) + + AlertManagement::HttpIntegrationsFinder.new( + project, + endpoint_identifier: endpoint_identifier, + active: true + ).execute.first + end + + def endpoint_identifier + params[:endpoint_identifier] || AlertManagement::HttpIntegration::LEGACY_IDENTIFIER + end + def notification_payload @notification_payload ||= params.permit![:notification] end diff --git a/app/finders/alert_management/http_integrations_finder.rb b/app/finders/alert_management/http_integrations_finder.rb new file mode 100644 index 0000000000000000000000000000000000000000..9f511be0ace2816a3f2858bd7732c75ade8a4390 --- /dev/null +++ b/app/finders/alert_management/http_integrations_finder.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module AlertManagement + class HttpIntegrationsFinder + def initialize(project, params) + @project = project + @params = params + end + + def execute + @collection = project.alert_management_http_integrations + + filter_by_availability + filter_by_endpoint_identifier + filter_by_active + + collection + end + + private + + attr_reader :project, :params, :collection + + def filter_by_availability + return if multiple_alert_http_integrations? + + first_id = project.alert_management_http_integrations + .ordered_by_id + .select(:id) + .at_most(1) + + @collection = collection.id_in(first_id) + end + + def filter_by_endpoint_identifier + return unless params[:endpoint_identifier] + + @collection = collection.for_endpoint_identifier(params[:endpoint_identifier]) + end + + def filter_by_active + return unless params[:active] + + @collection = collection.active + end + + # Overridden in EE + def multiple_alert_http_integrations? + false + end + end +end + +::AlertManagement::HttpIntegrationsFinder.prepend_if_ee('EE::AlertManagement::HttpIntegrationsFinder') diff --git a/app/models/alert_management/http_integration.rb b/app/models/alert_management/http_integration.rb index 7f954e1d3840d565a3c23c2c20fb5aeb45c95e74..cb28e008533c11d1951102773e42cd3ddd10c97f 100644 --- a/app/models/alert_management/http_integration.rb +++ b/app/models/alert_management/http_integration.rb @@ -2,6 +2,9 @@ module AlertManagement class HttpIntegration < ApplicationRecord + LEGACY_IDENTIFIER = 'legacy' + DEFAULT_NAME_SLUG = 'http-endpoint' + belongs_to :project, inverse_of: :alert_management_http_integrations attr_encrypted :token, @@ -9,19 +12,45 @@ class HttpIntegration < ApplicationRecord key: Settings.attr_encrypted_db_key_base_truncated, algorithm: 'aes-256-gcm' + default_value_for(:endpoint_identifier, allows_nil: false) { SecureRandom.hex(8) } + default_value_for(:token) { generate_token } + validates :project, presence: true validates :active, inclusion: { in: [true, false] } - - validates :token, presence: true + validates :token, presence: true, format: { with: /\A\h{32}\z/ } validates :name, presence: true, length: { maximum: 255 } - validates :endpoint_identifier, presence: true, length: { maximum: 255 } + validates :endpoint_identifier, presence: true, length: { maximum: 255 }, format: { with: /\A[A-Za-z0-9]+\z/ } validates :endpoint_identifier, uniqueness: { scope: [:project_id, :active] }, if: :active? before_validation :prevent_token_assignment + before_validation :prevent_endpoint_identifier_assignment before_validation :ensure_token + scope :for_endpoint_identifier, -> (endpoint_identifier) { where(endpoint_identifier: endpoint_identifier) } + scope :active, -> { where(active: true) } + scope :ordered_by_id, -> { order(:id) } + + def url + return ::Gitlab::Routing.url_helpers.project_alerts_notify_url(project, format: :json) if legacy? + + ::Gitlab::Routing.url_helpers.project_alert_http_integration_url(project, name_slug, endpoint_identifier, format: :json) + end + private + def self.generate_token + SecureRandom.hex + end + + def name_slug + (name && Gitlab::Utils.slugify(name)) || DEFAULT_NAME_SLUG + end + + def legacy? + endpoint_identifier == LEGACY_IDENTIFIER + end + + # Blank token assignment triggers token reset def prevent_token_assignment if token.present? && token_changed? self.token = nil @@ -31,11 +60,13 @@ def prevent_token_assignment end def ensure_token - self.token = generate_token if token.blank? + self.token = self.class.generate_token if token.blank? end - def generate_token - SecureRandom.hex + def prevent_endpoint_identifier_assignment + if endpoint_identifier_changed? && endpoint_identifier_was.present? + self.endpoint_identifier = endpoint_identifier_was + end end end end diff --git a/app/services/projects/alerting/notify_service.rb b/app/services/projects/alerting/notify_service.rb index affac45fc3d4db8cb55b27695a52a11343d6f6a1..f5e60dc3cf01cdf8e0cdcebf26a1b9251dc4a0e9 100644 --- a/app/services/projects/alerting/notify_service.rb +++ b/app/services/projects/alerting/notify_service.rb @@ -6,9 +6,11 @@ class NotifyService < BaseService include Gitlab::Utils::StrongMemoize include ::IncidentManagement::Settings - def execute(token) + def execute(token, integration = nil) + @integration = integration + return bad_request unless valid_payload_size? - return forbidden unless alerts_service_activated? + return forbidden unless active_integration? return unauthorized unless valid_token?(token) process_alert @@ -22,6 +24,7 @@ def execute(token) private + attr_reader :integration delegate :alerts_service, :alerts_service_activated?, to: :project def process_alert @@ -66,10 +69,7 @@ def create_alert return unless alert.save alert.execute_services - SystemNoteService.create_new_alert( - alert, - alert.monitoring_tool || 'Generic Alert Endpoint' - ) + SystemNoteService.create_new_alert(alert, notification_source) end def process_incident_issues @@ -106,11 +106,27 @@ def incoming_payload end end + def notification_source + alert.monitoring_tool || integration&.name || 'Generic Alert Endpoint' + end + def valid_payload_size? Gitlab::Utils::DeepSize.new(params).valid? end + def active_integration? + if Feature.enabled?(:multiple_http_integrations, project) + return true if integration + end + + alerts_service_activated? + end + def valid_token?(token) + if Feature.enabled?(:multiple_http_integrations, project) + return token == integration.token if integration + end + token == alerts_service.token end diff --git a/app/services/projects/prometheus/alerts/notify_service.rb b/app/services/projects/prometheus/alerts/notify_service.rb index c002aca32dbea088251b82d7f2ef7cabb79f2ed3..1eb501de961c5b238355524c072fe0c12fa7c16b 100644 --- a/app/services/projects/prometheus/alerts/notify_service.rb +++ b/app/services/projects/prometheus/alerts/notify_service.rb @@ -17,7 +17,7 @@ class NotifyService < BaseService SUPPORTED_VERSION = '4' - def execute(token) + def execute(token, _integration = nil) return bad_request unless valid_payload_size? return unprocessable_entity unless self.class.processable?(params) return unauthorized unless valid_alert_manager_token?(token) diff --git a/config/feature_flags/development/multiple_http_integrations.yml b/config/feature_flags/development/multiple_http_integrations.yml new file mode 100644 index 0000000000000000000000000000000000000000..c59726c3529dfc68e58966c8e103d53b302ae4c2 --- /dev/null +++ b/config/feature_flags/development/multiple_http_integrations.yml @@ -0,0 +1,7 @@ +--- +name: multiple_http_integrations +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44485 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/255509 +type: development +group: group::health +default_enabled: false diff --git a/config/routes/project.rb b/config/routes/project.rb index eae217de1ac1eef6053328ca967619a4ed324ef7..6633acc2ba58c98dbdf9b2f5c42b82e130d1f129 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -436,6 +436,10 @@ end post 'alerts/notify', to: 'alerting/notifications#create' # rubocop:todo Cop/PutProjectRoutesUnderScope + post 'alerts/notify/:name/:endpoint_identifier', # rubocop:todo Cop/PutProjectRoutesUnderScope + to: 'alerting/notifications#create', + as: :alert_http_integration, + constraints: { endpoint_identifier: /[A-Za-z0-9]+/ } draw :legacy_builds diff --git a/ee/app/finders/ee/alert_management/http_integrations_finder.rb b/ee/app/finders/ee/alert_management/http_integrations_finder.rb new file mode 100644 index 0000000000000000000000000000000000000000..5d52d7f711940bc0833d8a955aaa5034085f4508 --- /dev/null +++ b/ee/app/finders/ee/alert_management/http_integrations_finder.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module EE + module AlertManagement + module HttpIntegrationsFinder + extend ::Gitlab::Utils::Override + + private + + override :multiple_alert_http_integrations? + def multiple_alert_http_integrations? + project.feature_available?(:multiple_alert_http_integrations) + end + end + end +end diff --git a/ee/app/models/license.rb b/ee/app/models/license.rb index e169605b2ba96b27a97a828f18c52cdf4eac63bc..ad7386b06f4ce1d63f498643462b9ffe7fcb274c 100644 --- a/ee/app/models/license.rb +++ b/ee/app/models/license.rb @@ -98,6 +98,7 @@ class License < ApplicationRecord admin_merge_request_approvers_rules merge_trains metrics_reports + multiple_alert_http_integrations multiple_approval_rules multiple_group_issue_boards object_storage diff --git a/ee/spec/finders/ee/alert_management/http_integrations_finder_spec.rb b/ee/spec/finders/ee/alert_management/http_integrations_finder_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..e6e38f52a734fa9b6cd73940344a3d08b04611c3 --- /dev/null +++ b/ee/spec/finders/ee/alert_management/http_integrations_finder_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AlertManagement::HttpIntegrationsFinder do + describe '#execute' do + let_it_be(:project) { create(:project) } + let_it_be(:active_integration) { create(:alert_management_http_integration, project: project, endpoint_identifier: 'abc123' ) } + let_it_be(:inactive_integration) { create(:alert_management_http_integration, :inactive, project: project, endpoint_identifier: 'abc123' ) } + let_it_be(:alt_identifier_integration) { create(:alert_management_http_integration, project: project) } + let_it_be(:alt_project_integration) { create(:alert_management_http_integration) } + + before do + stub_licensed_features(multiple_alert_http_integrations: true) + end + + let(:params) { {} } + + subject(:execute) { described_class.new(project, params).execute } + + context 'empty params' do + it { is_expected.to contain_exactly(active_integration, inactive_integration, alt_identifier_integration) } + end + + context 'endpoint_identifier given' do + let(:params) { { endpoint_identifier: active_integration.endpoint_identifier } } + + it { is_expected.to contain_exactly(active_integration, inactive_integration) } + + context 'but unknown' do + let(:params) { { endpoint_identifier: 'unknown' } } + + it { is_expected.to be_empty } + end + + context 'but blank' do + let(:params) { { endpoint_identifier: nil } } + + it { is_expected.to contain_exactly(active_integration, inactive_integration, alt_identifier_integration) } + end + end + + context 'active param given' do + let(:params) { { active: true } } + + it { is_expected.to contain_exactly(active_integration, alt_identifier_integration) } + + context 'but blank' do + let(:params) { { active: nil } } + + it { is_expected.to contain_exactly(active_integration, inactive_integration, alt_identifier_integration) } + end + end + end +end diff --git a/ee/spec/requests/rack_attack_spec.rb b/ee/spec/requests/rack_attack_spec.rb index 2ccff27430e4f86d113083033d911a414d725f7f..000feb022e43fea8ea8b44e17e97da8582400328 100644 --- a/ee/spec/requests/rack_attack_spec.rb +++ b/ee/spec/requests/rack_attack_spec.rb @@ -107,4 +107,16 @@ let(:path) { "/#{project.full_path}/alerts/notify" } end end + + describe 'requests to AlertManagement::HttpIntegration notify endpoint with oauth token' do + before do + allow_next_instance_of(Projects::Alerting::NotifyService) do |instance| + allow(instance).to receive(:execute).and_return(ServiceResponse.success) + end + end + + it_behaves_like 'incident management rate limiting' do + let(:path) { "/#{project.full_path}/alerts/notify/http-integration-name/eddd36969b2d3d6a" } + end + end end diff --git a/spec/controllers/projects/alerting/notifications_controller_spec.rb b/spec/controllers/projects/alerting/notifications_controller_spec.rb index 33fd73c762a70ab2d57f058fab995775aed96b5f..6acd68b419cbe7863a98a120dc8a17fc5d542c20 100644 --- a/spec/controllers/projects/alerting/notifications_controller_spec.rb +++ b/spec/controllers/projects/alerting/notifications_controller_spec.rb @@ -5,6 +5,7 @@ RSpec.describe Projects::Alerting::NotificationsController do let_it_be(:project) { create(:project) } let_it_be(:environment) { create(:environment, project: project) } + let(:params) { project_params } describe 'POST #create' do around do |example| @@ -20,7 +21,7 @@ end def make_request - post :create, params: project_params, body: payload.to_json, as: :json + post :create, params: params, body: payload.to_json, as: :json end context 'when notification service succeeds' do @@ -53,26 +54,81 @@ def make_request context 'bearer token' do context 'when set' do - it 'extracts bearer token' do - request.headers['HTTP_AUTHORIZATION'] = 'Bearer some token' + context 'when extractable' do + before do + request.headers['HTTP_AUTHORIZATION'] = 'Bearer some token' + end - expect(notify_service).to receive(:execute).with('some token') + it 'extracts bearer token' do + expect(notify_service).to receive(:execute).with('some token', nil) - make_request + make_request + end + + context 'with a corresponding integration' do + context 'with integration parameters specified' do + let_it_be_with_reload(:integration) { create(:alert_management_http_integration, project: project) } + let(:params) { project_params(endpoint_identifier: integration.endpoint_identifier, name: integration.name) } + + context 'the integration is active' do + it 'extracts and finds the integration' do + expect(notify_service).to receive(:execute).with('some token', integration) + + make_request + end + end + + context 'when the integration is inactive' do + before do + integration.update!(active: false) + end + + it 'does not find an integration' do + expect(notify_service).to receive(:execute).with('some token', nil) + + make_request + end + end + + context 'when multiple endpoints are disabled' do + before do + stub_feature_flags(multiple_http_integrations: false) + end + + it 'does not find an integration' do + expect(notify_service).to receive(:execute).with('some token', nil) + + make_request + end + end + end + + context 'without integration parameters specified' do + let_it_be(:integration) { create(:alert_management_http_integration, :legacy, project: project) } + + it 'extracts and finds the legacy integration' do + expect(notify_service).to receive(:execute).with('some token', integration) + + make_request + end + end + end end - it 'pass nil if cannot extract a non-bearer token' do - request.headers['HTTP_AUTHORIZATION'] = 'some token' + context 'when inextractable' do + it 'passes nil for a non-bearer token' do + request.headers['HTTP_AUTHORIZATION'] = 'some token' - expect(notify_service).to receive(:execute).with(nil) + expect(notify_service).to receive(:execute).with(nil, nil) - make_request + make_request + end end end context 'when missing' do it 'passes nil' do - expect(notify_service).to receive(:execute).with(nil) + expect(notify_service).to receive(:execute).with(nil, nil) make_request end diff --git a/spec/factories/alert_management/http_integrations.rb b/spec/factories/alert_management/http_integrations.rb index 9311cb3e1145dd9d174ea488fbab06502b45541d..2b5864c8587e2816851b096f7bf3b608b16f1de1 100644 --- a/spec/factories/alert_management/http_integrations.rb +++ b/spec/factories/alert_management/http_integrations.rb @@ -10,5 +10,11 @@ trait :inactive do active { false } end + + trait :legacy do + endpoint_identifier { 'legacy' } + end + + initialize_with { new(**attributes) } end end diff --git a/spec/finders/alert_management/http_integrations_finder_spec.rb b/spec/finders/alert_management/http_integrations_finder_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..d65de2cdbbd5f5a39b58cf51f328eb5bb1798e12 --- /dev/null +++ b/spec/finders/alert_management/http_integrations_finder_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AlertManagement::HttpIntegrationsFinder do + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:integration) { create(:alert_management_http_integration, project: project ) } + let_it_be(:extra_integration) { create(:alert_management_http_integration, project: project ) } + let_it_be(:alt_project_integration) { create(:alert_management_http_integration) } + + let(:params) { {} } + + describe '#execute' do + subject(:execute) { described_class.new(project, params).execute } + + context 'empty params' do + it { is_expected.to contain_exactly(integration) } + end + + context 'endpoint_identifier param given' do + let(:params) { { endpoint_identifier: integration.endpoint_identifier } } + + it { is_expected.to contain_exactly(integration) } + + context 'matches an unavailable integration' do + let(:params) { { endpoint_identifier: extra_integration.endpoint_identifier } } + + it { is_expected.to be_empty } + end + + context 'but unknown' do + let(:params) { { endpoint_identifier: 'unknown' } } + + it { is_expected.to be_empty } + end + + context 'but blank' do + let(:params) { { endpoint_identifier: nil } } + + it { is_expected.to contain_exactly(integration) } + end + end + + context 'active param given' do + let(:params) { { active: true } } + + it { is_expected.to contain_exactly(integration) } + + context 'when integration is disabled' do + before do + integration.update!(active: false) + end + + it { is_expected.to be_empty } + end + + context 'but blank' do + let(:params) { { active: nil } } + + it { is_expected.to contain_exactly(integration) } + end + end + + context 'project has no integrations' do + subject(:execute) { described_class.new(create(:project), params).execute } + + it { is_expected.to be_empty } + end + end +end diff --git a/spec/models/alert_management/http_integration_spec.rb b/spec/models/alert_management/http_integration_spec.rb index 37d67dfe09a22829f62a4e38e03910794456c8ba..a3e7b47c1168fee70a00565286d1046e3d37ec87 100644 --- a/spec/models/alert_management/http_integration_spec.rb +++ b/spec/models/alert_management/http_integration_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe AlertManagement::HttpIntegration do + include ::Gitlab::Routing.url_helpers + let_it_be(:project) { create(:project) } subject(:integration) { build(:alert_management_http_integration) } @@ -15,19 +17,17 @@ it { is_expected.to validate_presence_of(:project) } it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_length_of(:name).is_at_most(255) } - it { is_expected.to validate_presence_of(:endpoint_identifier) } - it { is_expected.to validate_length_of(:endpoint_identifier).is_at_most(255) } context 'when active' do # Using `create` instead of `build` the integration so `token` is set. # Uniqueness spec saves integration with `validate: false` otherwise. - subject { create(:alert_management_http_integration) } + subject { create(:alert_management_http_integration, :legacy) } it { is_expected.to validate_uniqueness_of(:endpoint_identifier).scoped_to(:project_id, :active) } end context 'when inactive' do - subject { create(:alert_management_http_integration, :inactive) } + subject { create(:alert_management_http_integration, :legacy, :inactive) } it { is_expected.not_to validate_uniqueness_of(:endpoint_identifier).scoped_to(:project_id, :active) } end @@ -51,10 +51,6 @@ context 'when unsaved' do context 'when unassigned' do - before do - integration.valid? - end - it_behaves_like 'valid token' end @@ -89,4 +85,75 @@ end end end + + describe '#endpoint_identifier' do + subject { integration.endpoint_identifier } + + context 'when defined on initialize' do + let(:integration) { described_class.new } + + it { is_expected.to match(/\A\h{16}\z/) } + end + + context 'when included in initialization args' do + let(:integration) { described_class.new(endpoint_identifier: 'legacy') } + + it { is_expected.to eq('legacy') } + end + + context 'when reassigning' do + let(:integration) { create(:alert_management_http_integration) } + let!(:starting_identifier) { subject } + + it 'does not allow reassignment' do + integration.endpoint_identifier = 'newValidId' + integration.save! + + expect(integration.reload.endpoint_identifier).to eq(starting_identifier) + end + end + end + + describe '#url' do + subject { integration.url } + + it do + is_expected.to eq( + project_alert_http_integration_url( + integration.project, + 'datadog', + integration.endpoint_identifier, + format: :json + ) + ) + end + + context 'when name is not defined' do + let(:integration) { described_class.new(project: project) } + + it do + is_expected.to eq( + project_alert_http_integration_url( + integration.project, + 'http-endpoint', + integration.endpoint_identifier, + format: :json + ) + ) + end + end + + context 'for a legacy integration' do + let(:integration) { build(:alert_management_http_integration, :legacy) } + + it do + is_expected.to eq( + project_alerts_notify_url( + integration.project, + format: :json + ) + ) + end + end + end end diff --git a/spec/services/projects/alerting/notify_service_spec.rb b/spec/services/projects/alerting/notify_service_spec.rb index 809b12910a178d9726ea8544314c0fed0c3317e4..95aa7cc3ea496a93d65d2e740877409e9fe94827 100644 --- a/spec/services/projects/alerting/notify_service_spec.rb +++ b/spec/services/projects/alerting/notify_service_spec.rb @@ -34,13 +34,11 @@ let(:payload) { ActionController::Parameters.new(payload_raw).permit! } - subject { service.execute(token) } - - context 'with activated Alerts Service' do - let_it_be_with_reload(:alerts_service) { create(:alerts_service, project: project) } + subject { service.execute(token, nil) } + shared_examples 'notifcations are handled correctly' do context 'with valid token' do - let(:token) { alerts_service.token } + let(:token) { integration.token } let(:incident_management_setting) { double(send_email?: email_enabled, create_issue?: issue_enabled, auto_close_incident?: auto_close_enabled) } let(:email_enabled) { false } let(:issue_enabled) { false } @@ -197,7 +195,7 @@ it 'creates a system note corresponding to alert creation' do expect { subject }.to change(Note, :count).by(1) - expect(Note.last.note).to include('Generic Alert Endpoint') + expect(Note.last.note).to include(source) end end end @@ -247,15 +245,42 @@ it_behaves_like 'does not process incident issues due to error', http_status: :unauthorized it_behaves_like 'does not an create alert management alert' end + end + + context 'with an Alerts Service' do + let_it_be_with_reload(:integration) { create(:alerts_service, project: project) } + + it_behaves_like 'notifcations are handled correctly' do + let(:source) { 'Generic Alert Endpoint' } + end context 'with deactivated Alerts Service' do before do - alerts_service.update!(active: false) + integration.update!(active: false) end it_behaves_like 'does not process incident issues due to error', http_status: :forbidden it_behaves_like 'does not an create alert management alert' end end + + context 'with an HTTP Integration' do + let_it_be_with_reload(:integration) { create(:alert_management_http_integration, project: project) } + + subject { service.execute(token, integration) } + + it_behaves_like 'notifcations are handled correctly' do + let(:source) { integration.name } + end + + context 'with deactivated HTTP Integration' do + before do + integration.update!(active: false) + end + + it_behaves_like 'does not process incident issues due to error', http_status: :unauthorized + it_behaves_like 'does not an create alert management alert' + end + end end end