Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AWS SES -- email notification #550

Merged
merged 6 commits into from
Jun 23, 2024

Conversation

vnandwana
Copy link
Contributor

Updated email notification plugin, adding info about using AWS SES.

Copy link
Member

@sbrossie sbrossie left a comment

Choose a reason for hiding this comment

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

Is there anyway we can make it easier for the users - not have to install packages, ... ?

userguide/tutorials/metrics-cloudwatch.adoc Outdated Show resolved Hide resolved

The tenant/account configuration can be done by executing the required API endpoints via *cURL* as explained below:

[[tenant-config]]
. *Configure the tenant* using the following cURL command (Replace `<events>` with a comma-separated list of the events for which the emails need to be sent. For example, to configure the tenant to send emails for *INVOICE_CREATION* and *INVOICE_PAYMENT_SUCCESS*, specify `org.killbill.billing.plugin.email-notifications.defaultEvents=INVOICE_CREATION,INVOICE_PAYMENT_SUCCESS`):
*Configure the tenant* using the following cURL command (Replace `<events>` with a comma-separated list of the events for which the emails need to be sent. For example, to configure the tenant to send emails for *INVOICE_CREATION* and *INVOICE_PAYMENT_SUCCESS*, specify `org.killbill.billing.plugin.email-notifications.defaultEvents=INVOICE_CREATION,INVOICE_PAYMENT_SUCCESS`):
Copy link
Member

Choose a reason for hiding this comment

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

Is this on purpose (no more space) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it was earlier. I just removed the dot (.) from the beginning of the line so that it doesn't add a number section.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think tenant/account configuration (Steps 1 and 2 in the plugin configuration section) would be required when AWS SES is used as well.

If you agree, we can maybe add a section below SMTP Server Notes called AWS SES Specific Configuration that only includes the additional configuration for AWS SES and make it clear that this is only the additional configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tenant configuration mentioned in the SMTP section is not required for AWS SES. I have made some changes to the doc, hopefully, these comments are addressed. Kindly check when you get a chance. Thank you.



=== Configure Events
*Configure the account* using the following `cURL` command (Replace `{accountId}` with the id of the account for which emails need to be sent and `<events>` with a comma-separated list of the events for which the emails need to be sent. For example, to configure the account to send emails for *INVOICE_CREATION* and *INVOICE_PAYMENT_SUCCESS*, specify `"INVOICE_CREATION", "INVOICE_PAYMENT_SUCCESS"`):
Copy link
Member

Choose a reason for hiding this comment

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

Same here: Is this on purpose (no more space) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here as well, I just removed the dot (.) from the beginning of the line so that it doesn't add a number section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason to get rid of the numbering?

Copy link
Member

@sbrossie sbrossie left a comment

Choose a reason for hiding this comment

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

Left a few comments to address.

@vnandwana
Copy link
Contributor Author

Is there anyway we can make it easier for the users - not have to install packages, ... ?

Could you please elaborate on your thoughts? A user needs to install this plugin (just like any other plugins), are you referring to simplifying the AWS SES configuration process -- https://github.com/killbill/killbill-docs/pull/550/files#diff-371363a9cb5621593e52291217cd15e1d82ba9479fb62d0bfcc3e4ce6072fb78R69

@vnandwana
Copy link
Contributor Author

Is there anyway we can make it easier for the users - not have to install packages, ... ?

Could you please elaborate on your thoughts? A user needs to install this plugin (just like any other plugins), are you referring to simplifying the AWS SES configuration process -- https://github.com/killbill/killbill-docs/pull/550/files#diff-371363a9cb5621593e52291217cd15e1d82ba9479fb62d0bfcc3e4ce6072fb78R69

This PR had X-Ray daemon installation as well, if you're referring to simplifying this, its already been done.

@vnandwana
Copy link
Contributor Author

@reshmabidikar @sbrossie I have addressed the review comments, kindly check at your convenience. I have verified these changes in preview mode using the AsciiDoc plugin for IntelliJ IDEA.

Please let me know if any further changes are required. Thank you.

@sbrossie
Copy link
Member

@vnandwana I fixed the merge conflict. @reshmabidikar Can you double check your comments have been addressed - and if so, move ahead with merging this PR?

Copy link
Contributor

@reshmabidikar reshmabidikar left a comment

Choose a reason for hiding this comment

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

Looks good Vijay. I reviewed the doc from your fork to see how it reads and have left a few minor comments to address.


== Prerequisites

* Ensure that you have Kill Bill, Kaui, and the database set up as explained in the https://docs.killbill.io/latest/getting_started.html[__Getting Started Guide__].

* Ensure that you have https://curl.haxx.se/[_cURL_] installed. If you are on Windows, we recommend that you use https://git-scm.com/download/win[_Git Bash_] to run the `cURL` commands.

* If you plan to use AWS SES, obtain your AWS SES credentials (access key and secret key) and ensure that the IAM user has the ses:SendEmail permission.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If you plan to use AWS SES, obtain your AWS SES credentials (access key and secret key) and ensure that the IAM user has the ses:SendEmail permission.
* If you plan to use AWS SES, obtain your AWS SES credentials (access key and secret key) and ensure that the IAM user has the `ses:SendEmail permission`.


.. If a tenant is configured with some events, but the account is not configured, then emails will be sent based on what is configured at the tenant level.
[[smtp_server_notes]]
== SMTP Server Notes
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be H3 so that the doc reads better:

Suggested change
== SMTP Server Notes
=== SMTP Server Notes

*Note:* Ensure that the credentials used have the necessary permissions, specifically `ses:SendEmail`, on the resource arn:aws:ses:<region>:<account>:identity/<sender's email>.

=== SMTP Configuration
A tenant needs to be configured with the SMTP properties required for sending emails. Additionally, the tenant can also be configured with the events for which emails should be sent. In addition to the per-tenant configuration, the plugin also allows a more granular account-level configuration for the set of emails to be sent for the particular account. Thus, either the tenant or the account needs to be configured with the events for which the email needs to be sent.

The tenant/account configuration can be done by executing the required API endpoints via *cURL* as explained below:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The tenant/account configuration can be done by executing the required API endpoints via *cURL* as explained below:
The tenant configuration can be done by executing the required API endpoints via *cURL* as explained below:

*Note:* Ensure that the credentials used have the necessary permissions, specifically `ses:SendEmail`, on the resource arn:aws:ses:<region>:<account>:identity/<sender's email>.

=== SMTP Configuration
A tenant needs to be configured with the SMTP properties required for sending emails. Additionally, the tenant can also be configured with the events for which emails should be sent. In addition to the per-tenant configuration, the plugin also allows a more granular account-level configuration for the set of emails to be sent for the particular account. Thus, either the tenant or the account needs to be configured with the events for which the email needs to be sent.
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the text In addition to the per-tenant configuration, the plugin also allows a more granular account-level configuration for the set of emails to be sent for the particular account. Thus, either the tenant or the account needs to be configured with the events for which the email needs to be sent. to the Configure Events section below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't move this as the statement is good for SMTP configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, but from what I read, the account configuration is now moved to the Configure Events section, hence I suggested moving the above text to the Configure Events. If you disagree, feel free to leave it as it is and merge the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets keep this in SMTP as the following is not applicable for SES:
"per-tenant configuration" or "either the tenant or the account needs to be configured".

I'm not authorized to merge code to any of the public repositories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reshmabidikar If no further changes are required, kindly merge this branch. I don't have permission to merge code to this branch. Thank you.

org.killbill.mail.from=<Default sender's email>
org.killbill.aws.region=<AWS region, default region is us-east-1>

AWS SES requires authentication for sending emails. System-wide environment variables can be configured for all users by appending them to the /etc/environment file. Open the `/etc/environment` file in a text editor with root privileges, set the following variables:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
AWS SES requires authentication for sending emails. System-wide environment variables can be configured for all users by appending them to the /etc/environment file. Open the `/etc/environment` file in a text editor with root privileges, set the following variables:
AWS SES requires authentication for sending emails. System-wide environment variables can be configured for all users by appending them to the `/etc/environment` file. Open the `/etc/environment` file in a text editor with root privileges, set the following variables:

@vnandwana
Copy link
Contributor Author

@reshmabidikar @sbrossie Thank you for your review comments. I have addressed all these comments. Could you take a one final look?

Copy link
Contributor

@reshmabidikar reshmabidikar left a comment

Choose a reason for hiding this comment

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

Apart from #550 (comment), everything looks good. If you think the change in my comment is not required, feel free to merge and we can revisit once PR is merged.

@reshmabidikar reshmabidikar merged commit ad8a290 into killbill:v3 Jun 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants