fix SNS cross-account issues with Subscriptions #10819

bentsku · 2024-05-14T17:27:24Z

Motivation

Follow up from #10788

We now had issues around accessing subscriptions made by another account for SetSubscriptionAttributes, which has extensive validations of the FilterPolicy field.
The issue was the same as before, Moto was not saving the subscription in the right backend for the topic account.

We were currently blocked for #10691 because the validation of filter policies containing $or was incorrect, so it was the right opportunity to move the validation inside LocalStack, and remove usage of moto around SNS Subscriptions. We were already saving and using the Subscription data from our store only, and used moto for additional validation.

As of today, Topic is still stored in moto and extended in LocalStack, and mocked operations around A2P (SMS, and Platform Applications/Endpoint, related to mobile notifications) are still done by moto.

Changes

migrated all the tests related to FilterPolicy in its own file as it was becoming consequent, and we added a few more.
- the TestSNSFilterPolicyConditions class in test_sns_filter_policy.py contains the new tests
move the code related to Filter Policy to its own file, to group the actual filter policy matcher and the validator
remove usage of call_moto and call_moto_with_request in the SNS provider
sneaked a small parity fix for HTTP subscriptions (returning pending confirmation instead of the ARN if the subscription is still pending confirmation, except if ReturnSubscriptionArn is set to true).
- added test_http_subscription_response in test_sns.py

github-actions · 2024-05-14T18:24:30Z

LocalStack Community integration with Pro

2 files ±0 2 suites ±0 1h 37m 31s ⏱️ +21s
2 950 tests +8 2 648 ✅ +7 302 💤 +1 0 ❌ ±0
2 952 runs +8 2 648 ✅ +7 304 💤 +1 0 ❌ ±0

Results for commit 02d9552. ± Comparison against base commit cac2d03.

This pull request removes 12 and adds 20 tests. Note that renamed tests count towards both.

tests.aws.services.sns.test_sns.TestSNSFilter ‑ test_exists_filter_policy
tests.aws.services.sns.test_sns.TestSNSFilter ‑ test_exists_filter_policy_attributes_array
tests.aws.services.sns.test_sns.TestSNSFilter ‑ test_filter_policy
tests.aws.services.sns.test_sns.TestSNSFilter ‑ test_filter_policy_for_batch
tests.aws.services.sns.test_sns.TestSNSFilter ‑ test_filter_policy_on_message_body[False]
tests.aws.services.sns.test_sns.TestSNSFilter ‑ test_filter_policy_on_message_body[True]
tests.aws.services.sns.test_sns.TestSNSFilter ‑ test_filter_policy_on_message_body_array_attributes
tests.aws.services.sns.test_sns.TestSNSFilter ‑ test_filter_policy_on_message_body_array_of_object_attributes
tests.aws.services.sns.test_sns.TestSNSFilter ‑ test_filter_policy_on_message_body_dot_attribute
tests.aws.services.sns.test_sns.TestSNSFilter ‑ test_set_subscription_filter_policy_scope
…

tests.aws.services.sns.test_sns.TestSNSSubscriptionHttp ‑ test_http_subscription_response
tests.aws.services.sns.test_sns_filter_policy.TestSNSFilterPolicyAttributes ‑ test_exists_filter_policy
tests.aws.services.sns.test_sns_filter_policy.TestSNSFilterPolicyAttributes ‑ test_exists_filter_policy_attributes_array
tests.aws.services.sns.test_sns_filter_policy.TestSNSFilterPolicyAttributes ‑ test_filter_policy
tests.aws.services.sns.test_sns_filter_policy.TestSNSFilterPolicyBody ‑ test_filter_policy_for_batch
tests.aws.services.sns.test_sns_filter_policy.TestSNSFilterPolicyBody ‑ test_filter_policy_on_message_body[False]
tests.aws.services.sns.test_sns_filter_policy.TestSNSFilterPolicyBody ‑ test_filter_policy_on_message_body[True]
tests.aws.services.sns.test_sns_filter_policy.TestSNSFilterPolicyBody ‑ test_filter_policy_on_message_body_array_attributes
tests.aws.services.sns.test_sns_filter_policy.TestSNSFilterPolicyBody ‑ test_filter_policy_on_message_body_array_of_object_attributes
tests.aws.services.sns.test_sns_filter_policy.TestSNSFilterPolicyBody ‑ test_filter_policy_on_message_body_dot_attribute
…

♻️ This comment has been updated with latest results.

cloutierMat

Lots of work in there, congrats for keeping all that easily readable and adding a lot of meaningful tests.

I added most of my comments before realising that most of the SubscriptionFilter class was a refactor... Some of them might not be as relevant to your current task then.

The only missing part I see is the lack of support for "event":[{"anything-but": {"prefix": "order-"}}]. Let me know if you feel this is better suited for a follow up work.

cloutierMat · 2024-05-14T20:17:46Z

localstack/services/sns/filter.py

+ self, filter_policy: dict, message_attributes: dict
+ ):
+ for criteria, conditions in filter_policy.items():
+ if not self._evaluate_filter_policy_conditions_on_attribute(


How big can the message attributes get? Could we transform it into a dict to share the self._evaluate_nested_filter_policy_on_dict implementation?

In the end it might not be worth it, since we would need to loop over it twice and it would take a bit extra memory, but reusing the implementation could make it easier to maintain?

Hmm, the biggest difference is that MessageAttributes can only be on one level, they can't have nested dict. So the logic is much simpler there. I think we could maybe join the 2 implementations, but not sure if worth it. You technically can have objects inside the message for MessageAttributes, but the behavior is undefined from the documentation. We could have a look in the follow up, as you said, this was just moved around and will be updated with the next PR #10691

cloutierMat · 2024-05-14T21:00:59Z

localstack/services/sns/filter.py

+ # the remaining conditions require the value to not be None
+ return False
+ elif anything_but := condition.get("anything-but"):
+ return value not in anything_but


"anything-but" can also be a "prefix" https://docs.aws.amazon.com/sns/latest/dg/string-value-matching.html#string-anything-but-matching-prefix

Yes! This is not supported yet, and we also do not support suffix and equals-ignore-case, which will be supported by #10691. I could try to sneak the anything-but into prefix in there as well, but kept this as minimal as I could be, as the PR got big pretty quickly.

localstack/services/sns/filter.py

cloutierMat · 2024-05-14T21:27:40Z

localstack/services/sns/filter.py

+
+ if operator not in (">", ">="):
+ raise InvalidParameterException(
+ f"{self.error_prefix}FilterPolicy: Too many elements in numeric expression"


Really? That is the error that aws will return if you place the < before >? That sounds silly! 🤣

Yeah it's a bit weird, the errors are not too expressive I'd say 😅

bentsku self-assigned this May 14, 2024

bentsku added aws:sns Amazon Simple Notification Service semver: patch Non-breaking changes which can be included in patch releases labels May 14, 2024

bentsku added this to the 3.5 milestone May 14, 2024

bentsku requested review from baermat and cloutierMat May 14, 2024 19:34

bentsku marked this pull request as ready for review May 14, 2024 19:43

bentsku requested a review from thrau as a code owner May 14, 2024 19:43

cloutierMat requested changes May 14, 2024

View reviewed changes

bentsku added 7 commits May 15, 2024 14:58

migrate tests

cf709fb

add test + start migrating

417e392

add $or test, to be skipped

a5a44b3

move SNS subscriptions in localstack

df1a433

add cross account test for sub attrs

342b530

regenerate missing snapshot

46b48b4

PR comments

02d9552

bentsku force-pushed the fix-sns-subs-cross-account branch from 24b28e4 to 02d9552 Compare May 15, 2024 12:58

bentsku requested a review from cloutierMat May 15, 2024 12:59

cloutierMat approved these changes May 15, 2024

View reviewed changes

bentsku merged commit 9d80628 into master May 15, 2024
30 checks passed

bentsku deleted the fix-sns-subs-cross-account branch May 15, 2024 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix SNS cross-account issues with Subscriptions #10819

fix SNS cross-account issues with Subscriptions #10819

bentsku commented May 14, 2024 •

edited

github-actions bot commented May 14, 2024 •

edited

cloutierMat left a comment

cloutierMat May 14, 2024

bentsku May 15, 2024

cloutierMat May 14, 2024

bentsku May 15, 2024 •

edited

cloutierMat May 14, 2024

bentsku May 15, 2024

fix SNS cross-account issues with Subscriptions #10819

fix SNS cross-account issues with Subscriptions #10819

Conversation

bentsku commented May 14, 2024 • edited

Motivation

Changes

github-actions bot commented May 14, 2024 • edited

LocalStack Community integration with Pro

cloutierMat left a comment

Choose a reason for hiding this comment

cloutierMat May 14, 2024

Choose a reason for hiding this comment

bentsku May 15, 2024

Choose a reason for hiding this comment

cloutierMat May 14, 2024

Choose a reason for hiding this comment

bentsku May 15, 2024 • edited

Choose a reason for hiding this comment

cloutierMat May 14, 2024

Choose a reason for hiding this comment

bentsku May 15, 2024

Choose a reason for hiding this comment

bentsku commented May 14, 2024 •

edited

github-actions bot commented May 14, 2024 •

edited

bentsku May 15, 2024 •

edited