Skip to content
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.

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

fix SNS publish_batch passing MessageDeduplicationId to SQS queue #6660

Merged
merged 1 commit into from
Aug 19, 2022

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Aug 12, 2022

This PR fixes #6657

We only checked ContentBasedDeduplication settings in our tests for publish_batch.
We would not pass the MessageDeduplicationId to the SQS queue, thus launching this exception:

2022-08-12T14:00:05.973  INFO --- [   asgi_gw_1] localstack.services.sns.provider : Unable to forward SNS message to SQS: An error occurred (InvalidParameterValue) when calling the SendMessage operation (reached max retries: 0): The Queue should either have ContentBasedDeduplication enabled or MessageDeduplicationId provided explicitly  Traceback (most recent call last):
  File "/Users/benjaminsimon/Projects/localstack/localstack/localstack/services/sns/provider.py", line 984, in message_to_subscriber
    sqs_client.send_message(
  File "/Users/benjaminsimon/Projects/localstack/localstack-ext/.venv/lib/python3.10/site-packages/botocore/client.py", line 508, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/Users/benjaminsimon/Projects/localstack/localstack-ext/.venv/lib/python3.10/site-packages/botocore/client.py", line 915, in _make_api_call
    raise error_class(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (InvalidParameterValue) when calling the SendMessage operation (reached max retries: 0): The Queue should either have ContentBasedDeduplication enabled or MessageDeduplicationId provided explicitly 

This reinforces the need for a refactoring of the SNS provider.
We are not directly enforcing the deduplication in SNS, but instead piggy back on SQS.
The behaviour of SNS is a bit different, see: https://docs.aws.amazon.com/sns/latest/dg/fifo-message-dedup.html, with a time window of 5 minutes. This will need to be implemented in a follow up iteration.

@bentsku bentsku requested a review from baermat August 12, 2022 12:02
@bentsku bentsku temporarily deployed to localstack-ext-tests August 12, 2022 12:02 Inactive
@bentsku bentsku changed the title fix passing MessageDeduplicationId to SQS queue fix SNS publish_batch passing MessageDeduplicationId to SQS queue Aug 12, 2022
@coveralls
Copy link

coveralls commented Aug 12, 2022

Coverage Status

Coverage decreased (-0.001%) to 91.568% when pulling 06229d0 on fix/6657 into 9613a83 on master.

@github-actions
Copy link

github-actions bot commented Aug 12, 2022

LocalStack integration with Pro

       3 files         3 suites   1h 6m 15s ⏱️
1 205 tests 1 164 ✔️ 41 💤 0
1 586 runs  1 514 ✔️ 72 💤 0

Results for commit 938dad2.

♻️ This comment has been updated with latest results.

@bentsku bentsku temporarily deployed to localstack-ext-tests August 16, 2022 20:26 Inactive
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 👍 sorry about the delayed review
Always nice to see when a TODO is removed (even when it get's replaced by a new one^^)

@bentsku bentsku temporarily deployed to localstack-ext-tests August 18, 2022 17:30 Inactive
@bentsku bentsku temporarily deployed to localstack-ext-tests August 19, 2022 09:08 Inactive
@bentsku bentsku temporarily deployed to localstack-ext-tests August 19, 2022 13:09 Inactive
@bentsku bentsku temporarily deployed to localstack-ext-tests August 19, 2022 13:22 Inactive
@bentsku bentsku merged commit 58326f9 into master Aug 19, 2022
@bentsku bentsku deleted the fix/6657 branch August 19, 2022 15:50
@localstack localstack locked and limited conversation to collaborators Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SNS FIFO topic to SQS FIFO queue does not seem to work
3 participants