Sns/v2 apply subscribe changes #13226

baermat · 2025-10-06T10:59:47Z

Motivation

This PR applies the changes of #13095 on top of #13205. It also adds subscription attribute functions, as well as confirm_subscription.

closes PNX-78, closes PNX-76, closes PNX-95

Changes

apply initital changes of subscribe on top of the migrated topic logic
add subscription_attribute methods
reactive subscription crud tests

What's left to do:

Reset the base to master (right now it's set for an ease of review)

github-actions · 2025-10-06T11:19:20Z

Test Results - Preflight, Unit

22 337 tests +45 20 587 ✅ +38 15m 35s ⏱️ -1s
1 suites ± 0 1 750 💤 + 7
1 files ± 0 0 ❌ ± 0

Results for commit 8a097e6. ± Comparison against base commit 19c5bf0.

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

tests.unit.test_common.TestCommon ‑ test_is_number
tests.unit.test_moto ‑ test_service_exception_translator_context_manager

tests.unit.aws.handlers.test_service.TestServiceExceptionSerializer ‑ test_moto_exception_is_parsed
tests.unit.aws.test_service_router ‑ test_service_router_works_for_every_service[bedrock-agentcore-control-rest-json-SynchronizeGatewayTargets]
tests.unit.aws.test_service_router ‑ test_service_router_works_for_every_service[bedrock-agentcore-rest-json-BatchCreateMemoryRecords]
tests.unit.aws.test_service_router ‑ test_service_router_works_for_every_service[bedrock-agentcore-rest-json-BatchDeleteMemoryRecords]
tests.unit.aws.test_service_router ‑ test_service_router_works_for_every_service[bedrock-agentcore-rest-json-BatchUpdateMemoryRecords]
tests.unit.aws.test_service_router ‑ test_service_router_works_for_every_service[bedrock-agentcore-rest-json-CompleteResourceTokenAuth]
tests.unit.aws.test_service_router ‑ test_service_router_works_for_every_service[bedrock-agentcore-rest-json-GetAgentCard]
tests.unit.aws.test_service_router ‑ test_service_router_works_for_every_service[bedrock-agentcore-rest-json-StopRuntimeSession]
tests.unit.aws.test_service_router ‑ test_service_router_works_for_every_service[memorydb-json-DescribeMultiRegionParameterGroups]
tests.unit.aws.test_service_router ‑ test_service_router_works_for_every_service[memorydb-json-DescribeMultiRegionParameters]
…

♻️ This comment has been updated with latest results.

github-actions · 2025-10-06T11:26:02Z

Test Results - Alternative Providers

154 tests 30 ✅ 33s ⏱️
1 suites 124 💤
1 files 0 ❌

Results for commit 8a097e6.

♻️ This comment has been updated with latest results.

github-actions · 2025-10-06T11:27:57Z

Test Results (amd64) - Acceptance

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

Results for commit 8a097e6. ± Comparison against base commit 19c5bf0.

♻️ This comment has been updated with latest results.

github-actions · 2025-10-06T11:39:20Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 59m 15s ⏱️ - 58m 50s
2 849 tests - 1 953 2 724 ✅ - 1 740 125 💤 - 213 0 ❌ ±0
2 851 runs - 1 953 2 724 ✅ - 1 740 127 💤 - 213 0 ❌ ±0

Results for commit 8a097e6. ± Comparison against base commit 19c5bf0.

This pull request removes 1978 and adds 25 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.TestApigatewayIntegration ‑ test_create_integration_with_vpc_link
tests.aws.services.events.test_events_inputs.TestInputTransformer ‑ test_input_transformer_nested_keys_replacement["Command is <valid>!"]
tests.aws.services.s3.test_s3.TestS3 ‑ test_create_bucket_aws_global
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_check_not_opted_out
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_check_opted_out
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_check_opted_out_invalid
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_confirm_subscription
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_creating_subscription
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_creating_subscription_with_attributes
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_delete_subscriptions_on_delete_topic
…

This pull request removes 225 skipped tests and adds 13 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.TestSNSSubscriptionCrudV2 ‑ test_check_not_opted_out
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_check_opted_out
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_check_opted_out_invalid
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_confirm_subscription
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_creating_subscription_with_attributes
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_delete_subscriptions_on_delete_topic
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_get_subscription_attributes_error_not_exists
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_list_opted_out
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_opt_in
tests.aws.services.sns.test_sns.TestSNSSubscriptionCrudV2 ‑ test_set_subscription_attributes
…

♻️ This comment has been updated with latest results.

github-actions · 2025-10-06T11:46:16Z

Test Results (amd64) - Integration, Bootstrap

5 files 5 suites 1h 27m 26s ⏱️
2 873 tests 2 751 ✅ 122 💤 0 ❌
2 879 runs 2 751 ✅ 128 💤 0 ❌

Results for commit 8a097e6.

♻️ This comment has been updated with latest results.

bentsku

Nice update! We can enable many more tests, and this looks great!

I've added a few comments that are mostly all related to one change that would be required: removing the topic_subscriptions attribute from the store, and making that part of the topic itself. Once this is addressed and all the related changes are done, I think we're good to merge that one!

There's also the failing test we need to have a look at, not sure what happened, but the snapshot is now broken.

This looks great, it's nice to see so much core logic already ported 🚀

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

tests/aws/services/sns/test_sns.snapshot.json

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

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

bentsku · 2025-10-06T15:00:43Z

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

+
+        store = self.get_store(account_id=parsed_topic_arn["account"], region=context.region)
+
+        if topic_arn not in store.topic_subscriptions:


nit: this should now be if topic_arn not in store.topics

We should try to unify usage of topics around the codebase and avoid having multiple attributes for the same resource

could be (for future usage in this same function)

if topic_arn not in store.topics: raise NotFoundException("Topic does not exist") topic_subscriptions = store.topics[topic_arn]["subscriptions"]

and then topic_subscriptions can be used through this function

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

bentsku · 2025-10-06T15:37:22Z

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

+        with contextlib.suppress(ValueError):
+            store.topic_subscriptions[subscription["TopicArn"]].remove(subscription_arn)


here this will be adapted to remove it from the store.topics[subscription["TopicArn"]]["subscriptions"] instead

bentsku · 2025-10-06T15:39:06Z

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

+                # TODO: add topic attributes once they are ported from moto to LocalStack
+                # topic_attributes=vars(self._get_topic(topic_arn, context)),


nit: I believe those should now be present in the topic, right?

yes, but as I was not yet sure about the PublishContext and the exact effects of the code, I would rather do this together with publish

makes sense 👍 I believe it will be enough to pass the topic["attributes"], let's check it later! 👍

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

baermat · 2025-10-13T07:57:30Z

Most important change since the last review @bentsku is that the subscription objects are now directly referenced in the topic (not just their arns where you then have to check the store again. They can still live outside topics because they are stored in "subscriptions" as well, this just saves this one level of lookup)

bentsku

Almost good to go! Sorry for the multiple iterations on this. I think we should keep the topic's subscriptions as only a list of ARN, and store the data in only one store attributes and not have the same data stored in 2 places (see my comment for more context on this).

I can also see all the new tests being added but not validated, is it worth adding them now, or should it be its own PR? As they don't add anything yet as they still need to be validated. I'm also fine leaving them in if it makes your life easier to avoid future conflicts. We probably need to mark them as needs_fixing and not unknown then, unknown was more of a "transitive" marker when we started adding markers to existing tests.

Otherwise, the rest looks good! 👍

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

bentsku · 2025-10-13T12:59:28Z

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

+        store.subscriptions[subscription_arn] = subscription
+
+        topic_subscriptions.append(subscription)


this works currently because the references are shared: both store.subscriptions[susbcription_arn] and store.topics[topic_arn][subscription_index] share the same reference to the underlying Subscription object.

but when restoring state, we're going to go from serialized state back to Python object, and the framework will create 2 different objects that time, and set_subscription_attributes is not going to work anymore because you're only going to modify one object that is not shared with the other.

I think it might be easier to keep only the subscription ARN in the topic's subscriptions to avoid the issue. I think it is fine to have both look ups: you check if the subscription is in the topic's subscriptions first, then you fetch the subscription from the store subscriptions.

Thank you for clarifying this, I wondered this exact point but assumed that the reference would be kept. I find it interesting to be honest that this doesn't work (my assumption was that a lot of stuff could easily break if object identity is not respected on restore). Something for a coffee chat at some point :D Will change it back

tests/aws/services/sns/test_sns.py

bentsku · 2025-10-13T13:15:22Z

tests/aws/services/sns/test_sns.py

+        resp = aws_client.sns.list_subscriptions_by_topic(TopicArn=topic_arn)
+        assert "NextToken" in resp
+
+    @pytest.mark.skip(reason="TODO")


question: are all those new tests straight ported from Moto, with no AWS validation? is it worth having them in the same PR, or should they be in a different PR and being validated at the same time?

I would like to leave them in if that's alright with you, I created a linear ticket so this doesn't get lost and marked them all as "needs fixing"

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

This reverts commit a1a499a.

bentsku

LGTM! Thanks for addressing all the comments, this looks perfect now 👌

bentsku · 2025-10-15T15:18:20Z

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

        # subscribe calls do nothing (subscribe is idempotent), except if its attributes are different.
        for existing_topic_subscription in topic_subscriptions:
-            if existing_topic_subscription.get("Endpoint") == endpoint:
+            sub = store.subscriptions.get(existing_topic_subscription, {})


praise: very nice change!

baermat changed the base branch from main to sns/v2-topic-migration October 6, 2025 11:01

baermat added semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes labels Oct 6, 2025

baermat marked this pull request as ready for review October 6, 2025 11:30

baermat requested a review from bentsku as a code owner October 6, 2025 11:30

bentsku requested changes Oct 6, 2025

View reviewed changes

Base automatically changed from sns/v2-topic-migration to main October 7, 2025 14:01

baermat force-pushed the sns/v2-apply-subscribe-changes branch from 0196e37 to d37f9d5 Compare October 7, 2025 14:02

baermat added 16 commits October 7, 2025 16:16

working test

7e08a4e

working test

0ce7c58

working test

131eb8c

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

cdce122

add moto tests

737411d

add todo

cf07bc1

work in review suggestions

080e6b4

work in review suggestions

151d8fe

merge existing PR with topic migration

60170de

working test

5a61c76

add getsubattr, working test

ab6a8b7

add setsubattr, working test

b325761

working test

b64a739

working test

ee15177

working test

5977b38

working test

9a488f1

baermat added 8 commits October 7, 2025 16:18

working test

bcfa6ec

working test

aadb7e3

working until test_double_subscription

02b6f18

fix sub_invalid_sms

c7d4ce9

rebase

71a1b9f

WIP

c4c5665

WIP

58010ff

Wip

aa2e024

baermat force-pushed the sns/v2-apply-subscribe-changes branch from d37f9d5 to aa2e024 Compare October 7, 2025 14:29

baermat added 4 commits October 7, 2025 16:31

remove snapshot for paging test

5920e13

skip tests for follow up

bce98e9

fix list_topics pagination

2dadac1

save whole subscriptions with topic

a1a499a

baermat requested review from bentsku and removed request for bentsku October 9, 2025 11:45

fix test

9e1b90b

baermat requested a review from bentsku October 13, 2025 07:52

remove old commented code

b4770eb

bentsku reviewed Oct 13, 2025

View reviewed changes

baermat added 2 commits October 15, 2025 13:29

Revert "save whole subscriptions with topic"

4f3e678

This reverts commit a1a499a.

work in review suggestions

8a097e6

baermat requested a review from bentsku October 15, 2025 14:07

bentsku approved these changes Oct 15, 2025

View reviewed changes

baermat merged commit 0279b16 into main Oct 15, 2025
61 of 62 checks passed

baermat deleted the sns/v2-apply-subscribe-changes branch October 15, 2025 16:17

baermat mentioned this pull request Oct 22, 2025

Sns/v2 verify subscribe operations #13095

Closed

1 task


		store = self.get_store(account_id=parsed_topic_arn["account"], region=context.region)

		if topic_arn not in store.topic_subscriptions:

		with contextlib.suppress(ValueError):
		store.topic_subscriptions[subscription["TopicArn"]].remove(subscription_arn)

		# TODO: add topic attributes once they are ported from moto to LocalStack
		# topic_attributes=vars(self._get_topic(topic_arn, context)),

		store.subscriptions[subscription_arn] = subscription

		topic_subscriptions.append(subscription)

Uh oh!

Sns/v2 apply subscribe changes #13226

Sns/v2 apply subscribe changes #13226

Uh oh!

Conversation

baermat commented Oct 6, 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 Oct 6, 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 Oct 6, 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 Oct 6, 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 Oct 6, 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 Oct 6, 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

baermat commented Oct 13, 2025

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bentsku Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

baermat commented Oct 6, 2025 •

edited

Loading

github-actions bot commented Oct 6, 2025 •

edited

Loading

github-actions bot commented Oct 6, 2025 •

edited

Loading

github-actions bot commented Oct 6, 2025 •

edited

Loading

github-actions bot commented Oct 6, 2025 •

edited

Loading

github-actions bot commented Oct 6, 2025 •

edited

Loading

bentsku Oct 13, 2025 •

edited

Loading

baermat Oct 15, 2025 •

edited

Loading