Integrate with Notifications plugin for Alerting backend by qreshi · Pull Request #401 · opensearch-project/alerting

qreshi · 2022-04-13T16:35:00Z

Issue #, if available: #105

Description of changes:
This PR depends on opensearch-project/common-utils#158 and opensearch-project/notifications#406 so the GitHub Actions are expected to fail without at least common-utils being merged in

The main changes in this PR are:

Integrating with the Notifications plugin, supporting execution of the notification actions before/after migration of the Destination configs to Notifications
- In the case of the Destination failing migration, it will be sent through the passthrough API (this avoids hard failures on a write block but still requires the Notification plugin to be installed since the passthrough API does not work without it)
Fixes some bugs
- For the Destination migration, email_group configs were being checked with a string match which could also appear in the Email Destination (this would cause it to use the wrong parser so the fetching of the different config types has been split up)
- The Alerting email keystore settings were not correctly declaring the fallback setting to the legacy opendistro*. variants this would cause runtime errors when it tried to load the keystore settings and failed on falling back to the old setting
  - This has been fixed (although it should not be used anymore since the Notification plugin has its own namespace for these settings that will take precedence, Alerting's will be used as a fallback for that)

CheckList:
[x] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

rishabhmaurya · 2022-04-15T22:57:32Z

-                logger.info("Migrated ${migratedDestinations.size} destinations")
-                val failedDeletedDestinations = deleteOldDestinations(client, migratedDestinations)
-                logger.info("Failed to delete ${failedDeletedDestinations.size} destinations from migration process cleanup")
+                val migratedConfigs = createNotificationChannelIfNotExists(client, configsToMigrate)


This might be scope of this PR, but what if the user who created the notification config, is deleted? Will it still migrate it? Or will it fail and keep retrying forever?

You mean the user we're trying to inject in the thread context for the object is deleted?

Hmm, Security might handle that and treat it as no permissions? We'll want to test that when we test Alerting/Notif security integration. That's out of scope for this PR for now but a good callout.

rishabhmaurya · 2022-04-15T23:02:13Z

+        // Adding a check on TEST_ACTION Destination type here to avoid supporting it as a LegacyBaseMessage type
+        // just for Alerting integration tests
+        if (config.destination?.isTestAction() == true) {
+            return "test action"


can we define this constant somewhere else and use it both at the time assert and here.

rishabhmaurya · 2022-04-15T23:04:14Z

+            return "test action"
+        }
+
+        if (config.destination?.isAllowed(allowList) == false) {


can we rename allowList to destinationsAllowList for better readability.

getsaurabh02 · 2022-04-16T23:22:02Z

+        // Adding a check on TEST_ACTION Destination type here to avoid supporting it as a LegacyBaseMessage type
+        // just for Alerting integration tests
+        if (config.destination?.isTestAction() == true) {
+            return "test action"


getsaurabh02 · 2022-04-16T23:22:55Z

        }

+        private fun <T> fallback(key: String, affixSetting: AffixSetting<T>, regex: String, replacement: String): Setting<T>? {
+            return if ("_na_" == key) {


same as above

…d Notification Channels Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

…to either Notification Channels or Destination Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

…TEST_ACTION for tests Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

…vice Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

codecov-commenter · 2022-04-17T16:06:25Z

Codecov Report

Merging #401 (a997d40) into main (396fc6d) will increase coverage by 0.19%.
The diff coverage is 48.09%.

@@             Coverage Diff              @@
##               main     #401      +/-   ##
============================================
+ Coverage     78.31%   78.50%   +0.19%     
+ Complexity      268      263       -5     
============================================
  Files           180      180              
  Lines          7631     7625       -6     
  Branches       1065     1069       +4     
============================================
+ Hits           5976     5986      +10     
+ Misses         1106     1083      -23     
- Partials        549      556       +7

Impacted Files	Coverage Δ
...org/opensearch/alerting/model/destination/Chime.kt	`80.00% <0.00%> (+16.00%)`	⬆️
...org/opensearch/alerting/model/destination/Slack.kt	`80.00% <0.00%> (+16.00%)`	⬆️
...destinationmigration/DestinationConversionUtils.kt	`71.11% <ø> (+2.22%)`	⬆️
...ing/destination/client/DestinationEmailClient.java	`72.50% <ø> (-5.00%)`	⬇️
...ensearch/alerting/model/destination/Destination.kt	`75.60% <23.52%> (+0.46%)`	⬆️
...in/kotlin/org/opensearch/alerting/MonitorRunner.kt	`53.51% <37.50%> (-3.06%)`	⬇️
.../util/destinationmigration/NotificationApiUtils.kt	`57.53% <44.00%> (+37.53%)`	⬆️
...pensearch/alerting/settings/DestinationSettings.kt	`70.73% <77.77%> (+0.14%)`	⬆️
...nationmigration/DestinationMigrationUtilService.kt	`81.45% <85.00%> (-1.75%)`	⬇️
...tlin/org/opensearch/alerting/util/AlertingUtils.kt	`82.60% <100.00%> (+0.79%)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 396fc6d...a997d40. Read the comment docs.

qreshi · 2022-04-17T17:29:15Z

As discussed, the non-blocking comments will be addressed in a follow-up.

Also, the only failing GitHub Actions now that the dependent PRs are merged in is the security workflow which is being fixed in #407, merging in for now.

…project#401) * Add utils for retrieving notifications to both legacy Destinations and Notification Channels Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Refactor runAction in MonitorRunner to be able to send notifications to either Notification Channels or Destination Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Fix error handling when Notification config is not found and support TEST_ACTION for tests Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Fix issue with fallback setting for Destination email keystore settings Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Add publishing email Destinations via Notifications passthrough API Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Remove unused code in NotificationApiUtils Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Use subject as the title for Email Channels Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Combine runAction() methods Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Pass accountName to LegacyEmailMessage Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Split retrieval of Destination configs in DestinationMigrationUtilService Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> Signed-off-by: Angie Zhang <langelzh@amazon.com>

qreshi requested a review from a team April 13, 2022 16:35

rishabhmaurya reviewed Apr 15, 2022

View reviewed changes

Comment thread .../kotlin/org/opensearch/alerting/util/destinationmigration/DestinationMigrationUtilService.kt

rishabhmaurya reviewed Apr 15, 2022

View reviewed changes

rishabhmaurya approved these changes Apr 16, 2022

View reviewed changes

getsaurabh02 approved these changes Apr 16, 2022

View reviewed changes

qreshi added 10 commits April 17, 2022 08:55

Add utils for retrieving notifications to both legacy Destinations an…

6d8ef81

…d Notification Channels Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

Refactor runAction in MonitorRunner to be able to send notifications …

e2fe7a6

…to either Notification Channels or Destination Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

Fix error handling when Notification config is not found and support …

75674c2

…TEST_ACTION for tests Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

Fix issue with fallback setting for Destination email keystore settings

2adaebe

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

Add publishing email Destinations via Notifications passthrough API

0ccd42d

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

Remove unused code in NotificationApiUtils

6064a4c

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

Use subject as the title for Email Channels

6803523

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

Combine runAction() methods

d830185

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

Pass accountName to LegacyEmailMessage

e76d4fd

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

Split retrieval of Destination configs in DestinationMigrationUtilSer…

a997d40

…vice Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

qreshi force-pushed the monitor-exec-notif-integration branch from 76ebd85 to a997d40 Compare April 17, 2022 15:55

qreshi merged commit 4d8643c into opensearch-project:main Apr 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate with Notifications plugin for Alerting backend#401

Integrate with Notifications plugin for Alerting backend#401
qreshi merged 10 commits intoopensearch-project:mainfrom
qreshi:monitor-exec-notif-integration

qreshi commented Apr 13, 2022 •

edited

Loading

Uh oh!

Uh oh!

rishabhmaurya Apr 15, 2022

Uh oh!

qreshi Apr 15, 2022

Uh oh!

Uh oh!

rishabhmaurya Apr 15, 2022

Uh oh!

getsaurabh02 Apr 16, 2022

Uh oh!

rishabhmaurya Apr 15, 2022

Uh oh!

getsaurabh02 Apr 16, 2022

Uh oh!

getsaurabh02 Apr 16, 2022

Uh oh!

codecov-commenter commented Apr 17, 2022 •

edited

Loading

Uh oh!

qreshi commented Apr 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

qreshi commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rishabhmaurya Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

qreshi Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rishabhmaurya Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

getsaurabh02 Apr 16, 2022

Choose a reason for hiding this comment

Uh oh!

rishabhmaurya Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

getsaurabh02 Apr 16, 2022

Choose a reason for hiding this comment

Uh oh!

getsaurabh02 Apr 16, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

qreshi commented Apr 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

qreshi commented Apr 13, 2022 •

edited

Loading

codecov-commenter commented Apr 17, 2022 •

edited

Loading