Skip to content

SES: fix infinite loop with SNS event destination #12972

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 11, 2025
Merged

SES: fix infinite loop with SNS event destination #12972

merged 3 commits into from
Aug 11, 2025

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Aug 7, 2025

Motivation

While testing an SES/SNS sample, we encountered an issue where sending an email with SES with an SNS event destination configured that would have another SES subscriber, it would trigger an infinite loop.

There were 2 problems:

The following steps required for setting up event publishing are covered in the topics below:

  • You must create a configuration set using the Amazon SES console or API.
  • Add one or more event destinations (CloudWatch, Firehose, Pinpoint, or SNS) to the configuration set, and configure parameters unique to the event destination.
  • When you send an email, you specify which configuration set to use that contains your event destination.

There is still one somewhat big issue because Moto only accepts one EventDestination per configuration set, even if AWS accepts multiple, Moto will overwrite it every time you call CreateConfigurationSetEventDestination, but this is out of scope.

Changes

  • refactor a bit the SNS sending to centralize in one point and not have to modify the code 3 times, and for it to be easy to extend. We will probably need to refactor it again if we want to extend the SNSEmitter and add new kind of destinations
  • add a new LocalStack only test because we cannot really look into received emails when validating against AWS
  • write some TODOs about behavior problem of Event Destinations

@bentsku bentsku added this to the 4.8 milestone Aug 7, 2025
@bentsku bentsku self-assigned this Aug 7, 2025
@bentsku bentsku added aws:ses Amazon Simple Email Service semver: patch Non-breaking changes which can be included in patch releases labels Aug 7, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 7, 2025 •
edited
Loading

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   2m 44s ⏱️ - 1h 39m 57s
166 tests  - 4 459  161 ✅  - 4 025  5 💤  - 434  0 ❌ ±0 
168 runs   - 4 459  161 ✅  - 4 025  7 💤  - 434  0 ❌ ±0 

Results for commit dbdc8b6. ± Comparison against base commit cd7b481.

This pull request removes 4460 and adds 1 tests. Note that renamed tests count towards both. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.ses.test_ses.TestSES ‑ test_ses_sns_topic_integration_send_email_ses_destination

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 7, 2025 •
edited
Loading

Test Results - Preflight, Unit

22 063 tests  ±0   20 329 ✅ ±0   6m 22s ⏱️ -1s
     1 suites ±0    1 734 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit dbdc8b6. ± Comparison against base commit cd7b481.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 7, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 6s ⏱️ -1s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit dbdc8b6. ± Comparison against base commit cd7b481.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 7, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

  5 files    5 suites   11m 35s ⏱️
190 tests 185 ✅  5 💤 0 ❌
196 runs  185 ✅ 11 💤 0 ❌

Results for commit dbdc8b6.

♻️ This comment has been updated with latest results.

@bentsku bentsku marked this pull request as ready for review August 7, 2025 22:21
@bentsku bentsku requested a review from viren-nadkarni as a code owner August 7, 2025 22:21
viren-nadkarni
viren-nadkarni requested changes Aug 11, 2025
View reviewed changes
Copy link
Member

@viren-nadkarni viren-nadkarni left a comment

Choose a reason for hiding this comment

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

Just one request. Other affected tests also need updating but it's out of scope.

tests/aws/services/ses/test_ses.py Outdated
sender_email_address, recipient_email_address = setup_email_addresses()
config_set_name = f"config-set-{short_uid()}"

EMAILS.clear()
Copy link
Member

Choose a reason for hiding this comment

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

I would ask to use the REST endpoint for SES email introspection because tests are not guaranteed to run in the same process as LocalStack.

  • GET /_aws/ses
  • DELETE /_aws/ses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, makes sense 👍 will update

Copy link
Member

@viren-nadkarni viren-nadkarni Aug 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Pinging @giograno: Are there test fixtures for the LocalStack SDK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that actually would be super great to use 👍 I think we can do a second pass to update all usage of EMAILS and use the SDK instead 👀

Copy link
Member

Choose a reason for hiding this comment

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

@viren-nadkarni, we don't have it atm, but we should totally do it. Happy to take this :D

@bentsku bentsku force-pushed the fix-ses-sns-infinite-loop branch from 145adce to dbdc8b6 Compare August 11, 2025 12:26
@bentsku bentsku requested a review from viren-nadkarni August 11, 2025 12:27
viren-nadkarni
viren-nadkarni approved these changes Aug 11, 2025
View reviewed changes
Copy link
Member

@viren-nadkarni viren-nadkarni left a comment

Choose a reason for hiding this comment

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

Thanks, looks good 💯

@bentsku bentsku merged commit e82a823 into main Aug 11, 2025
40 checks passed
@bentsku bentsku deleted the fix-ses-sns-infinite-loop branch August 11, 2025 13:07
@giograno giograno mentioned this pull request Aug 11, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giograno giograno giograno left review comments

@viren-nadkarni viren-nadkarni viren-nadkarni approved these changes

Assignees

@bentsku bentsku

Labels
aws:ses Amazon Simple Email Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.8
Development

Successfully merging this pull request may close these issues.
3 participants
@bentsku @viren-nadkarni @giograno