Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Refactor Email Destination #244

Merged
merged 54 commits into from
Sep 23, 2020

Conversation

qreshi
Copy link
Contributor

@qreshi qreshi commented Aug 26, 2020

Issue #, if available: #3

Description of changes:

This PR is built upon the original one for the Email Destination feature enhancement: #102

The purpose of these changes is to refactor the Email Destination to allow for more flexible use cases and provide ease in scaling out the use of email notifications in Alerting.

Email Accounts

These changes introduce email_accounts which are stored in the opendistro-alerting-config index like other Alerting configurations. This allows for there to be multiple sender emails configured for use in Destinations. A single configured email_account can be used in multiple Destinations and is referred to by its id in the Destination configuration so that changing the email_account reflects the change in all Destinations that reference it.

# Example email account configuration
curl -X GET "localhost:9200/_opendistro/_alerting/email_accounts/YjY7mXMBx015759_IcfW"
{
  "_id" : "YjY7mXMBx015759_IcfW",
  "_seq_no" : 3,
  "_primary_term" : 2,
  "email_account" : {
    "schema_version" : 2,
    "name" : "test_account",
    "email" : "[email protected]",
    "host" : "smtp.mail.com",
    "port" : 465,
    "method" : "ssl"
  }
}

The Elasticsearch keystore will still be used for storing the username and password for an email when authentication is required. Since there can now be multiple sender emails the stored settings will be namespaced based on the name of the email_account. For example, if the email_account had the name "test_account" the settings to be added to the keystore would be as follows:

opendistro.alerting.destination.email.test_account.username
opendistro.alerting.destination.email.test_account.password

Email Groups

This PR also introduces email_groups which are similar to mailing lists. These email_groups can be used to refer to a set group of emails when creating Email Destinations. Similar to email_accounts, these email_groups can be used in multiple Destinations. The recipient list for an Email Destination can also be a combination of email_groups and single emails.

# Example email group configuration
curl -X GET "localhost:9200/_opendistro/_alerting/email_groups/YzY-mXMBx015759_dscs"
{
  "_id" : "YzY-mXMBx015759_dscs",
  "_seq_no" : 4,
  "_primary_term" : 2,
  "email_group" : {
    "schema_version" : 2,
    "name" : "test_email_group",
    "emails" : [
      {
        "email" : "[email protected]"
      },
      {
        "email" : "[email protected]"
      }
    ]
  }
}

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Aug 26, 2020

Codecov Report

Merging #244 into master will decrease coverage by 3.48%.
The diff coverage is 65.32%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #244      +/-   ##
============================================
- Coverage     78.61%   75.13%   -3.49%     
- Complexity      159      191      +32     
============================================
  Files            91      134      +43     
  Lines          3273     4403    +1130     
  Branches        464      581     +117     
============================================
+ Hits           2573     3308     +735     
- Misses          460      786     +326     
- Partials        240      309      +69     
Impacted Files Coverage Δ Complexity Δ
...search/alerting/model/destination/CustomWebhook.kt 87.32% <ø> (ø) 0.00 <0.00> (ø)
...erting/transport/TransportGetEmailAccountAction.kt 9.52% <9.52%> (ø) 0.00 <0.00> (?)
...alerting/transport/TransportGetEmailGroupAction.kt 9.52% <9.52%> (ø) 0.00 <0.00> (?)
...endistroforelasticsearch/alerting/MonitorRunner.kt 66.42% <16.00%> (-11.36%) 0.00 <0.00> (ø)
...erting/resthandler/RestSearchEmailAccountAction.kt 19.23% <19.23%> (ø) 0.00 <0.00> (?)
...alerting/resthandler/RestSearchEmailGroupAction.kt 19.23% <19.23%> (ø) 0.00 <0.00> (?)
...ing/transport/TransportDeleteEmailAccountAction.kt 22.22% <22.22%> (ø) 0.00 <0.00> (?)
...rting/transport/TransportDeleteEmailGroupAction.kt 22.22% <22.22%> (ø) 0.00 <0.00> (?)
...ing/transport/TransportSearchEmailAccountAction.kt 28.57% <28.57%> (ø) 0.00 <0.00> (?)
...rting/transport/TransportSearchEmailGroupAction.kt 28.57% <28.57%> (ø) 0.00 <0.00> (?)
... and 100 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 67b5598...1406961. Read the comment docs.

@qreshi qreshi mentioned this pull request Aug 26, 2020
@qreshi qreshi added alerting Item related to Alerting enhancement New feature or request refactor A change intended to improve the design or structure while preserving its functionality labels Aug 26, 2020
@ftianli-amzn
Copy link

We could find a better way to deal with the "patch" coverage check.

@belkirdi
Copy link

When do you think this feature will be released?
Thanks

@skkosuri-amzn
Copy link
Contributor

Looks like coverage is going down by 3.57%, that seems to more. Is it possible to increase the coverage?

lezzago
lezzago previously approved these changes Sep 23, 2020
Copy link
Contributor

@lezzago lezzago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just minor comments, everything else looks good to me

@skkosuri-amzn skkosuri-amzn self-requested a review September 23, 2020 17:05
@lezzago lezzago self-requested a review September 23, 2020 17:06
skkosuri-amzn
skkosuri-amzn previously approved these changes Sep 23, 2020
Copy link
Contributor

@skkosuri-amzn skkosuri-amzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on offline discussions we plan to address the coverage reduction in followup PR's.

@qreshi qreshi dismissed stale reviews from skkosuri-amzn and lezzago via cd528f2 September 23, 2020 19:33
@qreshi qreshi merged commit 4afcb2f into opendistro-for-elasticsearch:master Sep 23, 2020
@fbaligand
Copy link

Great!
Happy to see this PR merged!
Thanks all for your work on this!

tlfeng pushed a commit that referenced this pull request Feb 6, 2021
* Add Mail destination
  * Support SSL/TLS
  * Authentification
  * Message's body is html by dedault

* Mail destination : move settings to elasticsearch settings

* Add loading of mail settings

* Remove Dynamic setting property for mail settings. Update README.md about mail destination

* Changes from review:
- exception handling for mail server username and password
- default mail server port is 587

* Make keystore destination settings reloadable

* Updated scheduled-jobs mapping

* Removed DestinationSettings and DestinationMailSettings, added Recipient data class and updated Email data class to use new fields

* Added EmailAccount and EmailGroup data classes

* Refactored publishing of Email Destination to use EmailAccount and EmailGroup

* Refactored MailMessage and renamed to EmailMessage

* Refactored DestinationMailClient and renamed references to 'Mail' to be 'Email' in class names

* Added rest handlers for EmailAccount and EmailGroup index and delete actions

* Fix style issues

* Updated schema version for scheduled jobs index in tests

* Fixed existing email destination tests to pass with refactored changes

* Added new RestHandlers to AlertingPlugin

* Added tests for new components

* Added rest handlers for searching and getting EmailAccount and EmailGroup

* Fixed parsing error with GET API for EmailAccount and EmailGroup

* Moved Email settings from AlertingSettings to DestinationSettings and fixed issue for loading keystore settings

* Add license headers to new files

* Updated README

* Fixed incorrect call to ScheduledJob.parse in search rest handlers for EmailAccount and EmailGroup

* Change to use exists query in EmailAccount and EmailGroup search APIs

* Make Email model class streamable

* Fix tests

* Use 'use' scoped function instead of 'let' for Closeable secure settings

* Add/update tests

* Update UNCHECKED_CAST suppressions to match 'master' branch implementation

* Make Email extend Writeable

* Combine DestinationHttpResponse and DestinationEmailResponse to DestinationResponse

* Move email_accounts and email_groups APIs under 'destinations' namespace

* Make EmailAccount and EmailGroup writeable

* Add action to DELETE email_account API

* Add action to DELETE email_group API

* Add action to INDEX/UPDATE email_account API

* Add action to INDEX/UPDATE email_group API

* Add validation for name when creating/updating email_account

* Remove duplicate code from EmailAccount and EmailGroup TransportIndexAction classes

* Add action to SEARCH email_account API

* Add action to SEARCH email_group API

* Add version to EmailAccount

* Add version to EmailGroup

* Add action to GET email_account API

* Add action to GET email_group API

* Add rest tests for EmailAccount and EmailGroup

* Added validation for name and email in email model classes

* Separated calls to get info from alerting confix index in MonitorRunner to separate classes

* Fix IllegalStateException warn messages and handle null in EmailAccount/EmailGroup GetRequest

* Add JavaMail to third-party libraries list

* Fix failing test after resolving merge conflict from master

* Add missing return and remove unnecessary comments

Co-authored-by: David Chauvière <[email protected]>
Co-authored-by: David Chauviere <David Chauviere [email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
alerting Item related to Alerting enhancement New feature or request refactor A change intended to improve the design or structure while preserving its functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants