Skip to content

SNS: validate cross-region behavior #12673

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
Jun 2, 2025
Merged

SNS: validate cross-region behavior #12673

merged 3 commits into from
Jun 2, 2025

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented May 28, 2025

Motivation

We've got a report with #12670 that we didn't support cross-region subscribe calls. It seems it is not allowed by AWS, but we didn't have test coverage over it.

This PR adds more AWS validated tests testing cross-region behavior for operations that would allow cross-account access, as well as a cross-region delivery scenario.

Changes

  • add tests around cross-region access in SNS
  • add test for DeleteTopic idempotency
  • fix behavior in provider, to raise proper error message in case of cross-region access

@bentsku bentsku added this to the 4.5 milestone May 28, 2025
@bentsku bentsku self-assigned this May 28, 2025
@bentsku bentsku added aws:sns Amazon Simple Notification Service semver: patch Non-breaking changes which can be included in patch releases labels May 28, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented May 28, 2025 •
edited
Loading

Test Results - Preflight, Unit

21 579 tests  ±0   19 927 ✅ ±0   6m 12s ⏱️ -4s
     1 suites ±0    1 652 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 5db98a5. ± Comparison against base commit a298730b.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 28, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

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

Results for commit 5db98a5. ± Comparison against base commit a298730b.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 28, 2025 •
edited
Loading

Test Results - Alternative Providers

597 tests   420 ✅  14m 58s ⏱️
  4 suites  177 💤
  4 files      0 ❌

Results for commit 5db98a5.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 28, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 0m 17s ⏱️ - 44m 36s
2 724 tests  - 1 744  2 619 ✅  - 1 461  105 💤  - 283  0 ❌ ±0 
2 726 runs   - 1 744  2 619 ✅  - 1 461  107 💤  - 283  0 ❌ ±0 

Results for commit 5db98a5. ± Comparison against base commit a298730b.

This pull request removes 1747 and adds 3 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.sns.test_sns.TestSNSMultiRegions ‑ test_cross_region_access
tests.aws.services.sns.test_sns.TestSNSMultiRegions ‑ test_cross_region_delivery_sqs
tests.aws.services.sns.test_sns.TestSNSTopicCrud ‑ test_delete_topic_idempotency

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 28, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   2h 23m 29s ⏱️
4 826 tests 4 285 ✅ 541 💤 0 ❌
4 832 runs  4 285 ✅ 547 💤 0 ❌

Results for commit 5db98a5.

♻️ This comment has been updated with latest results.

@bentsku bentsku marked this pull request as ready for review May 28, 2025 13:50
@bentsku bentsku requested a review from baermat as a code owner May 28, 2025 13:50
baermat
baermat approved these changes Jun 2, 2025
View reviewed changes
Copy link
Member

@baermat baermat left a comment

Choose a reason for hiding this comment

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

LGTM 👍

tests/aws/services/sns/test_sns.py
Comment on lines +4486 to +4488
sns_region2_client.subscribe(
TopicArn=topic_arn, Protocol="email", Endpoint="devil@hell.com"
)
Copy link
Member

Choose a reason for hiding this comment

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

nice email 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copy pasted it from another test, I hadn't even realized 😂

@bentsku bentsku merged commit df48f25 into master Jun 2, 2025
57 checks passed
@bentsku bentsku deleted the validate-sns-cross-account branch June 2, 2025 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baermat baermat baermat approved these changes

Assignees

@bentsku bentsku

Labels
aws:sns Amazon Simple Notification Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.5
Development

Successfully merging this pull request may close these issues.
2 participants
@bentsku @baermat