fix delete method of AWS::SNS::TopicPolicy CFn resource by thrau · Pull Request #12831 · localstack/localstack · GitHub | Latest TMZ Celebrity News & Gossip | Watch TMZ Live
Skip to content

fix delete method of AWS::SNS::TopicPolicy CFn resource #12831

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 1 commit into from
Jul 4, 2025
Merged

fix delete method of AWS::SNS::TopicPolicy CFn resource #12831

merged 1 commit into from
Jul 4, 2025
+193 −3

Conversation

thrau
Copy link
Member

@thrau thrau commented Jul 4, 2025 •
edited
Loading

Motivation

This was a short detour on my main quest to get a CloudTrail scenario test working. The test uses a CDK construct to configure CloudTrail to send delivery events to SNS, which also creates a topic policy for SNS. I noticed the stacks weren’t deleting properly because the TopicPolicy resource was stuck in a retry loop.

I also raised getmoto/moto#9041 to get rid of the SNS:Receive action in the default policy that moto creates incorrectly.

Changes

  • Deleting a topic policy via cloudformation now correctly resets the topic's policy to the default policy

@thrau thrau added the semver: patch Non-breaking changes which can be included in patch releases label Jul 4, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 4, 2025

Test Results - Preflight, Unit

21 795 tests  ±0   20 138 ✅ ±0   6m 14s ⏱️ -14s
     1 suites ±0    1 657 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 34ce328. ± Comparison against base commit e558996.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 4, 2025

Test Results (amd64) - Acceptance

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

Results for commit 34ce328. ± Comparison against base commit e558996.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 4, 2025

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   1h 51m 6s ⏱️
3 677 tests 3 009 ✅ 668 💤 0 ❌
3 683 runs  3 009 ✅ 674 💤 0 ❌

Results for commit 34ce328.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 4, 2025

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 21m 17s ⏱️ - 21m 29s
3 653 tests  - 1 260  2 982 ✅  - 1 155  671 💤  - 105  0 ❌ ±0 
3 655 runs   - 1 260  2 982 ✅  - 1 155  673 💤  - 105  0 ❌ ±0 

Results for commit 34ce328. ± Comparison against base commit e558996.

This pull request removes 1261 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.cloudformation.resources.test_sns ‑ test_sns_topic_policy_resets_to_default

bentsku
bentsku approved these changes Jul 4, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot for addressing this, and also fix the default Action in Moto, this is great!

tests/aws/services/cloudformation/resources/test_sns.py
@markers.aws.validated
@markers.snapshot.skip_snapshot_verify(
paths=[
"$..Statement..Action", # TODO: see https://github.com/getmoto/moto/pull/9041
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks a lot for fixing this 😄 this is gonna remove a few snapshot skips!

localstack-core/localstack/services/sns/resource_providers/aws_sns_topicpolicy.py
except ClientError as err:
if "NotFound" not in err.response["Error"]["Code"]:
raise

return ProgressEvent(
status=OperationStatus.IN_PROGRESS,
status=OperationStatus.SUCCESS,
Copy link
Contributor

Choose a reason for hiding this comment

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

question: I suppose this is the main fix, right? the Delete operation would never finish as it never returned SUCCESS?

Copy link
Member Author

Choose a reason for hiding this comment

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

this + the default policy, because sending an empty attribute value to moto raises and error here: https://github.com/getmoto/moto/blob/338cb256a4ab938f24b5b29ec079ed26a9d0c093/moto/sns/models.py#L137-L139

localstack-core/localstack/services/sns/models.py
@@ -25,6 +26,38 @@
SnsMessageProtocols = Literal[SnsProtocols, SnsApplicationPlatforms]


def create_default_sns_topic_policy(topic_arn: str) -> dict:
Copy link
Contributor

Choose a reason for hiding this comment

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

nice addition! I'm always torn with this kind of utils in models.py, but it makes sense. It's going to nicely be used when we start creating topics in LocalStack directly 👀

Copy link
Member Author

@thrau thrau Jul 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

happy to put it elsewhere. what would be a good place? should we create a utils module, or should it just go into the provider? i could also move it to the resource provider directly

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 it is good for now! If we internalize, we’ll create a util folder and move things around! Sorry for the delay 😄

@bentsku
Copy link
Contributor

bentsku commented Jul 4, 2025 •
edited
Loading

As a side note, it will hopefully be possible in the future to skip list item like the troublesome SNS:Receive with this PR: localstack/localstack-snapshot#16

Right now, it's not possible to skip individual list item

@bentsku bentsku mentioned this pull request Jul 4, 2025
Open
@thrau thrau merged commit 1fcf3e9 into master Jul 4, 2025
45 of 46 checks passed
@thrau thrau deleted the cfn/sns-topic-policy branch July 4, 2025 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

@baermat baermat Awaiting requested review from baermat baermat is a code owner

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

@pinzon pinzon Awaiting requested review from pinzon pinzon is a code owner

@simonrw simonrw Awaiting requested review from simonrw simonrw is a code owner

Assignees
No one assigned
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

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

TMZ Celebrity News – Breaking Stories, Videos & Gossip

Looking for the latest TMZ celebrity news? You've come to the right place. From shocking Hollywood scandals to exclusive videos, TMZ delivers it all in real time.

Whether it’s a red carpet slip-up, a viral paparazzi moment, or a legal drama involving your favorite stars, TMZ news is always first to break the story. Stay in the loop with daily updates, insider tips, and jaw-dropping photos.

🎥 Watch TMZ Live

TMZ Live brings you daily celebrity news and interviews straight from the TMZ newsroom. Don’t miss a beat—watch now and see what’s trending in Hollywood.