From 1f2e7ec1df09c4891352edf2d28ed3b8bfef8c16 Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Mon, 4 Mar 2019 20:53:51 +0530 Subject: [PATCH 01/10] Check setting before sending alert email Check the incident_management_settings before sending alert emails to developers. --- ee/app/models/license.rb | 1 + .../prometheus/alerts/notify_service.rb | 18 ++++- .../prometheus/alerts/notify_service_spec.rb | 68 ++++++++++++++++++- 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/ee/app/models/license.rb b/ee/app/models/license.rb index 0104ea9becd661..c072c6144ea19f 100644 --- a/ee/app/models/license.rb +++ b/ee/app/models/license.rb @@ -95,6 +95,7 @@ class License < ActiveRecord::Base tracing insights web_ide_terminal + incident_management ] EEU_FEATURES.freeze diff --git a/ee/app/services/projects/prometheus/alerts/notify_service.rb b/ee/app/services/projects/prometheus/alerts/notify_service.rb index bb2ba6cdbbdd8f..fafeb0db26822b 100644 --- a/ee/app/services/projects/prometheus/alerts/notify_service.rb +++ b/ee/app/services/projects/prometheus/alerts/notify_service.rb @@ -8,7 +8,7 @@ def execute(token) return false unless valid_version? return false unless valid_alert_manager_token?(token) - send_alert_email(project, firings) if firings.any? + send_alert_email if send_email? && firings.any? persist_events(project, params) true @@ -16,6 +16,20 @@ def execute(token) private + def has_incident_management_license? + project.feature_available?(:incident_management) + end + + def send_email? + return true unless has_incident_management_license? + + setting = project.incident_management_setting + + return true if setting.nil? + + setting.send_email + end + def firings @firings ||= alerts_by_status('firing') end @@ -89,7 +103,7 @@ def compare_token(expected, actual) ActiveSupport::SecurityUtils.variable_size_secure_compare(expected, actual) end - def send_alert_email(projects, firing_alerts) + def send_alert_email notification_service .async .prometheus_alerts_fired(project, firings) diff --git a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 82d08308938f72..48a5f33e269bcb 100644 --- a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -9,9 +9,8 @@ let(:token_input) { 'token' } let(:subject) { service.execute(token_input) } - shared_examples 'notifies alerts' do + shared_examples 'sends notification email' do let(:notification_service) { spy } - let(:create_events_service) { spy } it 'sends a notification for firing alerts only' do expect(NotificationService) @@ -23,6 +22,10 @@ expect(subject).to eq(true) end + end + + shared_examples 'persists events' do + let(:create_events_service) { spy } it 'persists events' do expect(Projects::Prometheus::Alerts::CreateEventsService) @@ -36,6 +39,11 @@ end end + shared_examples 'notifies alerts' do + it_behaves_like 'sends notification email' + it_behaves_like 'persists events' + end + shared_examples 'no notifications' do let(:notification_service) { spy } let(:create_events_service) { spy } @@ -171,6 +179,62 @@ end end end + + context 'no incident_management license' do + before do + create(:prometheus_service, project: project) + + create(:project_alerting_setting, + project: project, + token: token) + + create(:project_incident_management_setting, send_email: false, project: project) + + stub_licensed_features(incident_management: false) + end + + include_examples 'notifies alerts' + end + + context 'with incident_management license' do + before do + create(:prometheus_service, project: project) + + create(:project_alerting_setting, + project: project, + token: token) + + stub_licensed_features(incident_management: true) + end + + context 'when incident_management_setting does not exist' do + include_examples 'notifies alerts' + end + + context 'when incident_management_setting.send_email is true' do + before do + create(:project_incident_management_setting, send_email: true, project: project) + end + + include_examples 'notifies alerts' + end + + context 'incident_management_setting.send_email is false' do + before do + create(:project_incident_management_setting, send_email: false, project: project) + end + + it_behaves_like 'persists events' + + it 'does not send notification' do + expect(project.feature_available?(:incident_management)).to eq(true) + expect(project).to receive(:incident_management_setting).and_call_original + expect(NotificationService).not_to receive(:new) + + expect(subject).to eq(true) + end + end + end end context 'with invalid payload' do -- GitLab From bbad45058d829404c4cad751b5ce6edb2189f49c Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Mon, 4 Mar 2019 22:08:49 +0530 Subject: [PATCH 02/10] Stub license manually since helper is not helping --- .../projects/prometheus/alerts/notify_service_spec.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 48a5f33e269bcb..470e218177cea1 100644 --- a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -190,7 +190,9 @@ create(:project_incident_management_setting, send_email: false, project: project) - stub_licensed_features(incident_management: false) + allow(project).to receive(:feature_available?).and_call_original + allow(project).to receive(:feature_available?) + .with(:incident_management).and_return(false) end include_examples 'notifies alerts' @@ -204,7 +206,9 @@ project: project, token: token) - stub_licensed_features(incident_management: true) + allow(project).to receive(:feature_available?).and_call_original + allow(project).to receive(:feature_available?) + .with(:incident_management).and_return(true) end context 'when incident_management_setting does not exist' do -- GitLab From 3e2875c39cf6bf562273998e865bdc84fcc3a342 Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Tue, 5 Mar 2019 17:28:24 +0530 Subject: [PATCH 03/10] Refactor notify_service specs - Use it_behaves_like instead of include_examples. - Remove some extra whitespace. --- .../prometheus/alerts/notify_service_spec.rb | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 470e218177cea1..5369e980140601 100644 --- a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -183,11 +183,7 @@ context 'no incident_management license' do before do create(:prometheus_service, project: project) - - create(:project_alerting_setting, - project: project, - token: token) - + create(:project_alerting_setting, project: project, token: token) create(:project_incident_management_setting, send_email: false, project: project) allow(project).to receive(:feature_available?).and_call_original @@ -195,16 +191,13 @@ .with(:incident_management).and_return(false) end - include_examples 'notifies alerts' + it_behaves_like 'notifies alerts' end context 'with incident_management license' do before do create(:prometheus_service, project: project) - - create(:project_alerting_setting, - project: project, - token: token) + create(:project_alerting_setting, project: project, token: token) allow(project).to receive(:feature_available?).and_call_original allow(project).to receive(:feature_available?) @@ -212,7 +205,7 @@ end context 'when incident_management_setting does not exist' do - include_examples 'notifies alerts' + it_behaves_like 'notifies alerts' end context 'when incident_management_setting.send_email is true' do @@ -220,11 +213,11 @@ create(:project_incident_management_setting, send_email: true, project: project) end - include_examples 'notifies alerts' + it_behaves_like 'notifies alerts' end context 'incident_management_setting.send_email is false' do - before do + let(:incident_mgmt_settings) do create(:project_incident_management_setting, send_email: false, project: project) end @@ -232,7 +225,8 @@ it 'does not send notification' do expect(project.feature_available?(:incident_management)).to eq(true) - expect(project).to receive(:incident_management_setting).and_call_original + expect(project).to receive(:incident_management_setting).and_return(incident_mgmt_settings) + expect(incident_mgmt_settings).to receive(:send_email) expect(NotificationService).not_to receive(:new) expect(subject).to eq(true) -- GitLab From 93fd186cd5b63484597463ff90e31d10b2b44ae0 Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Tue, 5 Mar 2019 17:31:18 +0530 Subject: [PATCH 04/10] Add after_initialize method to set defaults - Required if we want build_project_incident_management_settings to have values already set. --- .../project_incident_management_setting.rb | 7 +++++++ .../services/projects/prometheus/alerts/notify_service.rb | 4 +--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/ee/app/models/incident_management/project_incident_management_setting.rb b/ee/app/models/incident_management/project_incident_management_setting.rb index 0aa75ecf4f7e91..730be1abe976e7 100644 --- a/ee/app/models/incident_management/project_incident_management_setting.rb +++ b/ee/app/models/incident_management/project_incident_management_setting.rb @@ -6,8 +6,15 @@ class ProjectIncidentManagementSetting < ApplicationRecord validate :issue_template_exists, if: :create_issue? + after_initialize :set_defaults + private + def set_defaults + self.send_email = true + self.create_issue = false + end + def issue_template_exists return unless issue_template_key.present? diff --git a/ee/app/services/projects/prometheus/alerts/notify_service.rb b/ee/app/services/projects/prometheus/alerts/notify_service.rb index fafeb0db26822b..69c5cbc6c0760c 100644 --- a/ee/app/services/projects/prometheus/alerts/notify_service.rb +++ b/ee/app/services/projects/prometheus/alerts/notify_service.rb @@ -23,9 +23,7 @@ def has_incident_management_license? def send_email? return true unless has_incident_management_license? - setting = project.incident_management_setting - - return true if setting.nil? + setting = project.incident_management_setting || project.build_incident_management_setting setting.send_email end -- GitLab From caccda85f2bf354b89b4770e4a4d1b94a4be6d52 Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Wed, 6 Mar 2019 11:13:01 +0530 Subject: [PATCH 05/10] Remove an assert --- .../projects/prometheus/alerts/notify_service_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 5369e980140601..a0e23878b6d09d 100644 --- a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -217,7 +217,7 @@ end context 'incident_management_setting.send_email is false' do - let(:incident_mgmt_settings) do + before do create(:project_incident_management_setting, send_email: false, project: project) end @@ -225,8 +225,6 @@ it 'does not send notification' do expect(project.feature_available?(:incident_management)).to eq(true) - expect(project).to receive(:incident_management_setting).and_return(incident_mgmt_settings) - expect(incident_mgmt_settings).to receive(:send_email) expect(NotificationService).not_to receive(:new) expect(subject).to eq(true) -- GitLab From e0f6c33934a3701c2cb32df81d42f3d0ab904852 Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Wed, 6 Mar 2019 11:15:45 +0530 Subject: [PATCH 06/10] Remove after_initialize callback --- .../project_incident_management_setting.rb | 7 ------- 1 file changed, 7 deletions(-) diff --git a/ee/app/models/incident_management/project_incident_management_setting.rb b/ee/app/models/incident_management/project_incident_management_setting.rb index 730be1abe976e7..0aa75ecf4f7e91 100644 --- a/ee/app/models/incident_management/project_incident_management_setting.rb +++ b/ee/app/models/incident_management/project_incident_management_setting.rb @@ -6,15 +6,8 @@ class ProjectIncidentManagementSetting < ApplicationRecord validate :issue_template_exists, if: :create_issue? - after_initialize :set_defaults - private - def set_defaults - self.send_email = true - self.create_issue = false - end - def issue_template_exists return unless issue_template_key.present? -- GitLab From 3a3ce4233620f96546373488b287dd45cb3c8b42 Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Wed, 6 Mar 2019 19:14:33 +0530 Subject: [PATCH 07/10] Change behavior when feature flag is disabled If incident_management feature flag is disabled, send the alert email. But if incident_management license is not present , do not send the alert email. --- .../prometheus/alerts/notify_service.rb | 8 +++-- .../prometheus/alerts/notify_service_spec.rb | 29 +++++++++++++++++-- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/ee/app/services/projects/prometheus/alerts/notify_service.rb b/ee/app/services/projects/prometheus/alerts/notify_service.rb index 69c5cbc6c0760c..363bf020cd0f7f 100644 --- a/ee/app/services/projects/prometheus/alerts/notify_service.rb +++ b/ee/app/services/projects/prometheus/alerts/notify_service.rb @@ -20,11 +20,15 @@ def has_incident_management_license? project.feature_available?(:incident_management) end + def incident_management_feature_enabled? + Feature.enabled?(:incident_management) + end + def send_email? - return true unless has_incident_management_license? + return true unless incident_management_feature_enabled? + return false unless has_incident_management_license? setting = project.incident_management_setting || project.build_incident_management_setting - setting.send_email end diff --git a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb index a0e23878b6d09d..7c2c55e144060e 100644 --- a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -81,6 +81,8 @@ with_them do before do + stub_feature_flags(incident_management: false) + cluster = create(:cluster, :provided_by_user, projects: [project], enabled: cluster_enabled) @@ -120,6 +122,8 @@ end before do + stub_feature_flags(incident_management: false) + stub_licensed_features(multiple_clusters: true) create(:clusters_applications_prometheus, :installed, @@ -160,6 +164,8 @@ let(:alert_manager_token) { token_input } before do + stub_feature_flags(incident_management: false) + create(:prometheus_service, project: project) if alerting_setting @@ -180,18 +186,37 @@ end end + context 'incident_management feature flag disabled' do + before do + create(:prometheus_service, project: project) + create(:project_alerting_setting, project: project, token: token) + create(:project_incident_management_setting, send_email: true, project: project) + + stub_feature_flags(incident_management: false) + end + + it_behaves_like 'notifies alerts' + end + context 'no incident_management license' do before do create(:prometheus_service, project: project) create(:project_alerting_setting, project: project, token: token) - create(:project_incident_management_setting, send_email: false, project: project) + create(:project_incident_management_setting, send_email: true, project: project) allow(project).to receive(:feature_available?).and_call_original allow(project).to receive(:feature_available?) .with(:incident_management).and_return(false) end - it_behaves_like 'notifies alerts' + it_behaves_like 'persists events' + + it 'does not send notification' do + expect(project.feature_available?(:incident_management)).to eq(false) + expect(NotificationService).not_to receive(:new) + + expect(subject).to eq(true) + end end context 'with incident_management license' do -- GitLab From 3c256256316cbf2f8eb1989d8bc56f8f67546e49 Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Thu, 7 Mar 2019 20:42:45 +0530 Subject: [PATCH 08/10] Move firings.any? into the send_email? function --- .../services/projects/prometheus/alerts/notify_service.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ee/app/services/projects/prometheus/alerts/notify_service.rb b/ee/app/services/projects/prometheus/alerts/notify_service.rb index 363bf020cd0f7f..3ca355115cf71a 100644 --- a/ee/app/services/projects/prometheus/alerts/notify_service.rb +++ b/ee/app/services/projects/prometheus/alerts/notify_service.rb @@ -8,7 +8,7 @@ def execute(token) return false unless valid_version? return false unless valid_alert_manager_token?(token) - send_alert_email if send_email? && firings.any? + send_alert_email if send_email? persist_events(project, params) true @@ -25,11 +25,12 @@ def incident_management_feature_enabled? end def send_email? - return true unless incident_management_feature_enabled? + return firings.any? unless incident_management_feature_enabled? return false unless has_incident_management_license? setting = project.incident_management_setting || project.build_incident_management_setting - setting.send_email + + setting.send_email && firings.any? end def firings -- GitLab From 6998c0b4cb2643fd242b8bbc38f8307141b922ce Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Thu, 7 Mar 2019 21:07:07 +0530 Subject: [PATCH 09/10] Clear memoization of licensed_feature_available --- .../prometheus/alerts/notify_service_spec.rb | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 7c2c55e144060e..923b83723bb638 100644 --- a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -9,6 +9,11 @@ let(:token_input) { 'token' } let(:subject) { service.execute(token_input) } + before do + # We use `set(:project)` so we make sure to clear caches + project.clear_memoization(:licensed_feature_available) + end + shared_examples 'sends notification email' do let(:notification_service) { spy } @@ -204,9 +209,7 @@ create(:project_alerting_setting, project: project, token: token) create(:project_incident_management_setting, send_email: true, project: project) - allow(project).to receive(:feature_available?).and_call_original - allow(project).to receive(:feature_available?) - .with(:incident_management).and_return(false) + stub_licensed_features(incident_management: false) end it_behaves_like 'persists events' @@ -224,9 +227,7 @@ create(:prometheus_service, project: project) create(:project_alerting_setting, project: project, token: token) - allow(project).to receive(:feature_available?).and_call_original - allow(project).to receive(:feature_available?) - .with(:incident_management).and_return(true) + stub_licensed_features(incident_management: true) end context 'when incident_management_setting does not exist' do -- GitLab From 9d048ea77ad2dc6345c57ddd2d157d55107c66c6 Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Mon, 11 Mar 2019 10:45:21 +0530 Subject: [PATCH 10/10] Send email if feature flag or license is disabled - Send email if incident_management feature flag or license is disabled. - A new issue will be created for checking the prometheus_alerts license in the execute method of the NotifyService. --- .../projects/prometheus/alerts/notify_service.rb | 4 ++-- .../projects/prometheus/alerts/notify_service_spec.rb | 9 +-------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/ee/app/services/projects/prometheus/alerts/notify_service.rb b/ee/app/services/projects/prometheus/alerts/notify_service.rb index 3ca355115cf71a..81b979e6ef2da2 100644 --- a/ee/app/services/projects/prometheus/alerts/notify_service.rb +++ b/ee/app/services/projects/prometheus/alerts/notify_service.rb @@ -25,8 +25,8 @@ def incident_management_feature_enabled? end def send_email? - return firings.any? unless incident_management_feature_enabled? - return false unless has_incident_management_license? + return firings.any? unless incident_management_feature_enabled? && + has_incident_management_license? setting = project.incident_management_setting || project.build_incident_management_setting diff --git a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 923b83723bb638..29b48135c57f10 100644 --- a/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -212,14 +212,7 @@ stub_licensed_features(incident_management: false) end - it_behaves_like 'persists events' - - it 'does not send notification' do - expect(project.feature_available?(:incident_management)).to eq(false) - expect(NotificationService).not_to receive(:new) - - expect(subject).to eq(true) - end + it_behaves_like 'notifies alerts' end context 'with incident_management license' do -- GitLab