Sns/v2 topic migration #13205

baermat · 2025-09-29T12:03:22Z

Motivation

First step of internalization of the SNS provider by migrating topics. This PR addresses both topics and topic attributes since they are intertwined

Changes

add create_topic, list_topics, delete_topics, get_topic_attributes, set_topic_attributes
activates all applicable tests in TestSnsTopicCrud
adds new class TestSnsTopicCrudV2 for tests that were covered in moto
- The tests were only used as reference to "what is tested", the tests themselves were written from scratch

closes PNX-83, closes PNX-79

github-actions · 2025-09-29T12:34:09Z

Test Results - Preflight, Unit

22 292 tests +11 20 549 ✅ +11 15m 18s ⏱️ -34s
1 suites ± 0 1 743 💤 ± 0
1 files ± 0 0 ❌ ± 0

Results for commit f713a3c. ± Comparison against base commit e60ff8e.

♻️ This comment has been updated with latest results.

github-actions · 2025-09-29T12:41:12Z

Test Results - Alternative Providers

132 tests 9 ✅ 23s ⏱️
1 suites 123 💤
1 files 0 ❌

Results for commit f713a3c.

♻️ This comment has been updated with latest results.

github-actions · 2025-09-29T12:43:13Z

Test Results (amd64) - Acceptance

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

Results for commit f713a3c. ± Comparison against base commit e60ff8e.

♻️ This comment has been updated with latest results.

github-actions · 2025-09-29T12:55:25Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 59m 50s ⏱️ - 59m 22s
2 824 tests - 1 968 2 711 ✅ - 1 745 113 💤 - 223 0 ❌ ±0
2 826 runs - 1 968 2 711 ✅ - 1 745 115 💤 - 223 0 ❌ ±0

Results for commit f713a3c. ± Comparison against base commit e60ff8e.

This pull request removes 1975 and adds 7 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.apigateway.test_apigateway_api.TestApigatewayTestInvoke ‑ test_failed_invoke_test_method
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_create_topic_in_multiple_regions
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_create_topic_name_constraints
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_create_topic_should_be_idempotent
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_delete_non_existent_topic
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_list_topic_paging
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_topic_get_attributes_with_fifo_false

This pull request removes 225 skipped tests and adds 2 skipped tests. Note that renamed tests count towards both.

tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input4-FAILED]
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_deployed_infra_state
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_populate_data
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_user_clicks_are_stored
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_api_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_invalid_desiredstate
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_double_create_with_client_token
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_lifecycle
…

tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_create_topic_name_constraints
tests.aws.services.sns.test_sns.TestSNSTopicCrudV2 ‑ test_topic_get_attributes_with_fifo_false

♻️ This comment has been updated with latest results.

github-actions · 2025-09-29T13:01:57Z

Test Results (amd64) - Integration, Bootstrap

5 files 5 suites 1h 28m 28s ⏱️
2 848 tests 2 738 ✅ 110 💤 0 ❌
2 854 runs 2 738 ✅ 116 💤 0 ❌

Results for commit f713a3c.

♻️ This comment has been updated with latest results.

bentsku

Nice work, this is looking great! I believe this to be a sound base to continue building on 🚀

The only issue is using plain Python class for the store attributes, once this is resolved, I believe we're good to go.
The rest is minor nits and simplification of the code.

Nice new coverage with new snapshot tests, this is really great to see! 👌

localstack-core/localstack/services/sns/v2/models.py

bentsku · 2025-09-29T17:06:16Z

localstack-core/localstack/services/sns/v2/provider.py

+                    # TODO:
+                    raise InvalidParameterException("Fix this Exception message and type")


note for later: this is about idempotency when creating, right? This was pretty tricky for subscriptions, might be worth looking into subscribe to see if we can take some things from there 👍

localstack-core/localstack/services/sns/v2/provider.py

bentsku · 2025-09-29T18:44:55Z

localstack-core/localstack/services/sns/v2/provider.py

+        topics = {"Topics": [{"TopicArn": t.arn} for t in store.topics.values()]}
+        topics = PaginatedList(topics["Topics"])
+        page, nxt = topics.get_page(
+            lambda topic: topic["TopicArn"], next_token=next_token, page_size=100


nit: using the topic ARN as the pagination token might be a bit of an issue (some client might not handle the arn format for that), but this can left as is for now, just a thought

tests/aws/services/sns/test_sns.py

localstack-bot · 2025-10-06T09:37:51Z

Currently, only patch changes are allowed on main. Your PR labels (semver: minor, docs: skip) indicate that it cannot be merged into the main at this time.

bentsku

Nice, this is great!

Thanks for addressing the comments, I believe there are only a few left and then we're good to merge 👍

It would be good to move the CreateTopic logic in the operation so that it's clearer where it is applied, and remove some redundant attributes of the topic.

The rest are questions, nits and thoughts about cross-region topic access that we will need to consider.

Once the CreateTopic logic has been cleaned up, we're good to merge 👍

localstack-core/localstack/services/sns/v2/models.py

tests/aws/services/sns/test_sns.py

localstack-core/localstack/services/sns/v2/provider.py

bentsku · 2025-10-06T14:22:46Z

localstack-core/localstack/services/sns/v2/provider.py

+        """
+        :param arn: the Topic ARN
+        :param context: the RequestContext of the request
+        :return: the Moto model Topic


nit: comment: not the Moto model anymore

2nd comment: we will want to add the multi region flag sooner than later I believe, because right now we changed the behavior to raise an exception if the region of the request is different from the ARN, but this not should not be the default behavior for many operations. Just need to be kept in mind for the follow-up PRs

localstack-core/localstack/services/sns/v2/models.py

bentsku

LGTM! thanks for addressing the comments! this is good to go, my comments are mostly about keeping tracks of some things to fix in follow-ups 👍 this looks great, thanks a lot 🙏

bentsku · 2025-10-07T13:22:05Z

localstack-core/localstack/services/sns/v2/provider.py

+            {
+                "ContentBasedDeduplication": "false",
+                "FifoTopic": "false",
+                "SignatureVersion": "2",


nit: I think this is an oversight from the test, looking at all our tests, they all set the SignatureVersion to 2 when creating a fifo topic.

But looking at AWS, it doesn't seem like it sets this attribute by default:

$ aws us-east-1 sns create-topic --name test-topic.fifo --attributes FifoTopic=true { "TopicArn": "arn:aws:sns:us-east-1:<account-id>:test-topic.fifo" } $ aws sns get-topic-attributes --topic-arn arn:aws:sns:us-east-1:<account-id>:test-topic.fifo { "Attributes": { "Policy": "{\"Version\":\"2008-10-17\",\"Id\":\"__default_policy_ID\",\"Statement\":[{\"Sid\":\"__default_statement_ID\",\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"*\"},\"Action\":[\"SNS:GetTopicAttributes\",\"SNS:SetTopicAttributes\",\"SNS:AddPermission\",\"SNS:RemovePermission\",\"SNS:DeleteTopic\",\"SNS:Subscribe\",\"SNS:ListSubscriptionsByTopic\",\"SNS:Publish\"],\"Resource\":\"arn:aws:sns:us-east-1:<account-id>:test-topic.fifo\",\"Condition\":{\"StringEquals\":{\"AWS:SourceOwner\":\"<account-id>\"}}}]}", "Owner": "<account-id>", "SubscriptionsPending": "0", "TopicArn": "arn:aws:sns:us-east-1:<account-id>:test-topic.fifo", "EffectiveDeliveryPolicy": "{\"http\":{\"defaultHealthyRetryPolicy\":{\"minDelayTarget\":20,\"maxDelayTarget\":20,\"numRetries\":3,\"numMaxDelayRetries\":0,\"numNoDelayRetries\":0,\"numMinDelayRetries\":0,\"backoffFunction\":\"linear\"},\"disableSubscriptionOverrides\":false,\"defaultRequestPolicy\":{\"headerContentType\":\"text/plain; charset=UTF-8\"}}}", "SubscriptionsConfirmed": "0", "FifoTopic": "true", "DisplayName": "", "ContentBasedDeduplication": "false", "SubscriptionsDeleted": "0" } }

But this is a nit, not blocking, something to consider to remove in a follow-up PR 👍 let's get this one merged! Sorry I missed it earlier

bentsku · 2025-10-07T13:23:54Z

tests/aws/services/sns/test_sns.py

+        # Invalid chars
+        for c in [":", ";", "!", "@", "|", "^", "%", " "]:
+            with pytest.raises(ClientError) as e:
+                sns_create_topic(Name=f"bad{c}name")
+            snapshot.match(f"name-invalid-char-{c}", e.value.response)


question: it looks like the snapshot issue we discussed this morning is not part of the PR, will it be part of a follow-up? Not worries, not blocking either, I was wondering 😄

works against v2 🤷

bentsku · 2025-10-07T13:26:11Z

tests/aws/services/sns/test_sns.py

+    @markers.snapshot.skip_snapshot_verify(
+        paths=[
+            "$..Attributes.EffectiveDeliveryPolicy",
+        ]
+    )


something I missed earlier too: it would be great to implement this default EffectiveDeliveryPolicy in a follow-up PR if we could keep track of it, as it is used in Publish to determine which content type to use for HTTP subscriptions. Not to be implemented in this PR either, this is just to keep track of it and remove in a follow-up for v2 👍

baermat added 5 commits September 29, 2025 11:30

working test

2947736

working test

63eab28

working test

78c2f42

add new test class based on moto, add tests to be verified

f282a46

add moto tests

e364681

add todo

fe036cb

baermat marked this pull request as ready for review September 29, 2025 12:14

baermat requested a review from bentsku as a code owner September 29, 2025 12:14

bentsku reviewed Sep 29, 2025

View reviewed changes

alexrashed modified the milestones: Playground, 4.10 Sep 30, 2025

work in review suggestions

8c5cdf7

baermat requested a review from bentsku October 6, 2025 09:37

baermat mentioned this pull request Oct 6, 2025

Sns/v2 apply subscribe changes #13226

Merged

1 task

baermat added the notes: skip Pull request does not have to be mentioned in the release notes label Oct 6, 2025

bentsku reviewed Oct 6, 2025

View reviewed changes

work in review suggestions

f713a3c

baermat requested a review from bentsku October 7, 2025 08:02

bentsku approved these changes Oct 7, 2025

View reviewed changes

baermat merged commit f239baf into main Oct 7, 2025
40 checks passed

baermat deleted the sns/v2-topic-migration branch October 7, 2025 14:01

		# TODO:
		raise InvalidParameterException("Fix this Exception message and type")

Uh oh!

Sns/v2 topic migration #13205

Sns/v2 topic migration #13205

Uh oh!

Conversation

baermat commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Uh oh!

github-actions bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results - Preflight, Unit

Uh oh!

github-actions bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results - Alternative Providers

Uh oh!

github-actions bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results (amd64) - Acceptance

Uh oh!

github-actions bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

github-actions bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results (amd64) - Integration, Bootstrap

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bentsku Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bentsku Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

localstack-bot commented Oct 6, 2025

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bentsku Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

bentsku Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

bentsku Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

baermat Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

bentsku Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

baermat commented Sep 29, 2025 •

edited

Loading

github-actions bot commented Sep 29, 2025 •

edited

Loading

github-actions bot commented Sep 29, 2025 •

edited

Loading

github-actions bot commented Sep 29, 2025 •

edited

Loading

github-actions bot commented Sep 29, 2025 •

edited

Loading

github-actions bot commented Sep 29, 2025 •

edited

Loading