10000 use snapshots for SNS tests by bentsku · Pull Request #6601 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

use snapshots for SNS tests #6601

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

Merged
merged 5 commits into from
Aug 9, 2022
Merged

use snapshots for SNS tests #6601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3318c00
started snapshot tests for SNS
bentsku Aug 2, 2022
41d4935
fix change
bentsku Aug 4, 2022
9fe2785
add SNS transformer utility, move some SQS into SNS one
bentsku Aug 5, 2022
5f6f684
fix SNS error messages parity
bentsku Aug 5, 2022
0883522
regenerate snapshots with ordering
bentsku Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions localstack/services/sns/provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,15 @@ def publish_batch(
raise InvalidParameterException(
"Invalid parameter: The MessageGroupId parameter is required for FIFO topics"
)
moto_sns_backend = moto_sns_backends[context.region]
if moto_sns_backend.get_topic(arn=topic_arn).content_based_deduplication == "false":
if not all( 8000
["MessageDeduplicationId" in entry for entry in publish_batch_request_entries]
):
raise InvalidParameterException(
"Invalid parameter: The topic should either have ContentBasedDeduplication enabled or MessageDeduplicationId provided explicitly",
)

response = {"Successful": [], "Failed": []}
for entry in publish_batch_request_entries:
message_id = str(uuid.uuid4())
Expand Down Expand Up @@ -499,9 +508,7 @@ def set_subscription_attributes(
) -> None:
sub = get_subscription_by_arn(subscription_arn)
if not sub:
raise NotFoundException(
f"Unable to find subscription for given ARN: {subscription_arn}"
)
raise NotFoundException("Subscription does not exist")
sub[attribute_name] = attribute_value

def confirm_subscription(
Expand Down Expand Up @@ -663,7 +670,7 @@ def publish(
if topic_arn and ".fifo" in topic_arn:
if not message_group_id:
raise InvalidParameterException(
"The MessageGroupId parameter is required for FIFO topics",
"Invalid parameter: The MessageGroupId parameter is required for FIFO topics",
)
moto_sns_backend = moto_sns_backends[context.region]
if moto_sns_backend.get_topic(arn=topic_arn).content_based_deduplication == "false":
Expand Down Expand Up @@ -730,7 +737,7 @@ def subscribe(
)
if ".fifo" in endpoint and ".fifo" not in topic_arn:
raise InvalidParameterException(
"FIFO SQS Queues can not be subscribed to standard SNS topics"
"Invalid parameter: Invalid parameter: Endpoint Reason: FIFO SQS Queues can not be subscribed to standard SNS topics"
)
moto_response = call_moto(context)
subscription_arn = moto_response.get("SubscriptionArn")
Expand Down Expand Up @@ -1167,6 +1174,7 @@ def create_sns_message_body(
"Timestamp": timestamp_millis(),
"SignatureVersion": "1",
# TODO Add a more sophisticated solution with an actual signature
# check KMS for providing real cert and how to serve them
# Hardcoded
"Signature": "EXAMPLEpH+..",
"SigningCertURL": "https://sns.us-east-1.amazonaws.com/SimpleNotificationService-0000000000000000000000.pem",
Expand Down
47 changes: 36 additions & 11 deletions localstack/testing/snapshots/transformer_utility.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,17 +176,42 @@ def sqs_api():
TransformerUtility.key_value("SenderId"),
TransformerUtility.jsonpath("$..MessageAttributes.RequestID.StringValue", "request-id"),
KeyValueBasedTransformer(_resource_name_transformer, "resource"),
KeyValueBasedTransformer(_signing_cert_url_token_transformer, replacement="token"),
]

@staticmethod
def sns_api():
"""
:return: array with Transformers, for sns api.
"""
return [
TransformerUtility.key_value("ReceiptHandle"),
TransformerUtility.key_value("SequenceNumber"), # this might need to be in SQS
TransformerUtility.key_value(
"Signature", value_replacement="<signature>", reference_replacement=False
),
Comment on lines +189 to +191
Copy link
Member

Choose a reason for hiding this comment

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

should we maybe just use the default value_replacement?

Copy link
Member

Choose a reason for hiding this comment

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

with the reference_replacement set to false, the default value would be signature. I think it's fine to use <signature> here as it indicates at the first sight that the value was replaced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that was my issue, the fact that the <[...]> were missing was troubling me, so I added it manually!

# the body of SNS messages contains a timestamp, need to ignore the hash
TransformerUtility.key_value("MD5OfBody", "<md5-hash>", reference_replacement=False),
Copy link
Member

Choose a reason for hiding this comment

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

should we not maybe leave this in per default? or are you having issues with bodies containing data that is dynamically generated by the tests? (like passing ARNs through a message body).

Copy link
Member

Choose a reason for hiding this comment

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

+1
If the md5 hash matches in most cases, we should not always exclude it. Is this the case for SNS?
Ideally we would only exclude md5-hashes for individual test cases with a note why it has to be replaced (e.g. includes timestamp).

Copy link
Contributor Author
@bentsku bentsku Aug 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The MD5 hash almost never matches: the body of the SNS message always contains a timestamp, and a signature.
It only matches when RawDelivery is set to True, as the body is the string message passed. I had to manually add the MD5 transformer to almost every test. I thought the MD5 hash could just be checked with a unit test.
Otherwise, I had an over engineered way to do, which was to delete the MD5 transformer from the sns_api with a fixture that I would add to the test.
What do you think we should do in this case?

Copy link
Member

Choose a reason for hiding this comment

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

alright :) i think in that case, having unittests should be fine.
alternatively we could also create another api-helper, which excludes the md5-transformer and use this one for the RawDelivery messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it could work as well. The issue also that I didn't think about is that in some tests, we have both RawDelivery and regular in the same test. So I have to ignore the hash. Maybe I will manually check the MD5 of raw messages in the integration tests? As it's AWS validated, it will check the behaviour when we regenerate the snapshots

Copy link
Member

Choose a reason for hiding this comment

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

I see :) yes, sounds good. but I think it would also be fine to cover the md5-checksum in unittests as you suggested before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The md5 is part of SQS, and is covered inside the integration tests there, there shouldn't be any regression on that side.

# this can interfere in ARN with the accountID
TransformerUtility.key_value(
"SenderId", value_replacement="<sender-id>", reference_replacement=False
),
KeyValueBasedTransformer(
_sns_pem_file_token_transformer, replacement="signing-cert-file"
_sns_pem_file_token_transformer,
replacement="signing-cert-file",
),
# replaces the domain in "UnsubscribeURL"
TransformerUtility.regex(
re.compile(
r"(?<=UnsubscribeURL[\"|']:\s[\"|'])(https?.*?)(?=/\?Action=Unsubscribe)"
),
# replaces the domain in "UnsubscribeURL" URL (KeyValue won't work as it replaces reference, and if
# replace_reference is False, then it replaces the whole key
# this will be able to use a KeyValue based once we provide a certificate for message signing in SNS
# a match must be made case-insensitive because the key casing is different from lambda notifications
RegexTransformer(
r"(?<=(?i)UnsubscribeURL[\"|']:\s[\"|'])(https?.*?)(?=/\?Action=Unsubscribe)",
replacement="<unsubscribe-domain>",
),
KeyValueBasedTransformer(_resource_name_transformer, "resource"),
# add a special transformer with 'resource' replacement for SubscriptionARN in UnsubscribeURL
KeyValueBasedTransformer(
_sns_unsubscribe_url_subscription_arn_transformer, replacement="resource"
),
]

@staticmethod
Expand Down Expand Up @@ -220,15 +245,15 @@ def secretsmanager_secret_id_arn(create_secret_res: CreateSecretResponse, index:


def _sns_pem_file_token_transformer(key: str, val: str) -> str:
if isinstance(val, str) and key == "SigningCertURL":
pattern = re.compile(r".*SimpleNotificationService-(.*)?\.pem")
if isinstance(val, str) and key.lower() == "SigningCertURL".lower():
pattern = re.compile(r".*SimpleNotificationService-(.*\.pem)")
match = re.match(pattern, val)
if match:
return match.groups()[0]


def _signing_cert_url_token_transformer(key: str, val: str) -> str:
if isinstance(val, str) and key == "UnsubscribeURL":
def _sns_unsubscribe_url_subscription_arn_transformer(key: str, val: str) -> str:
if isinstance(val, str) and key.lower() == "UnsubscribeURL".lower():
pattern = re.compile(r".*(?<=\?Action=Unsubscribe&SubscriptionArn=).*:(.*)")
match = re.match(pattern, val)
if match:
Expand Down
9 changes: 2 additions & 7 deletions tests/integration/s3/test_s3_notifications_sns.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,9 @@ def test_object_created_put(
snapshot,
):
snapshot.add_transformer(snapshot.transform.sqs_api())
snapshot.add_transformer(snapshot.transform.sns_api())
snapshot.add_transformer(snapshot.transform.s3_api())
snapshot.add_transformer(
snapshot.transform.jsonpath(
"$..Signature",
"signature",
reference_replacement=False,
),
)

bucket_name = s3_create_bucket()
topic_arn = sns_create_topic()["TopicArn"]
queue_url = sqs_create_queue()
Expand Down
14 changes: 7 additions & 7 deletions tests/integration/s3/test_s3_notifications_sns.snapshot.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"tests/integration/s3/test_s3_notifications_sns.py::TestS3NotificationsToSns::test_object_created_put": {
"recorded-date": "08-08-2022, 17:51:20",
"recorded-date": "09-08-2022, 11:38:05",
"recorded-content": {
"receive_messages": {
"messages": [
Expand Down Expand Up @@ -44,14 +44,14 @@
]
},
"MessageId": "<uuid:1>",
"Signature": "signature",
"Signature": "<signature>",
"SignatureVersion": "1",
"SigningCertURL": "https://sns.<region>.amazonaws.com/SimpleNotificationService-<signing-cert-file:1>.pem",
"SigningCertURL": "https://sns.<region>.amazonaws.com/SimpleNotificationService-<signing-cert-file:1>",
"Subject": "Amazon S3 Notification",
"Timestamp": "date",
"TopicArn": "arn:aws:sns:<region>:111111111111:<resource:2>",
"Type": "Notification",
"UnsubscribeURL": "<unsubscribe-domain>/?Action=Unsubscribe&SubscriptionArn=arn:aws:sns:<region>:111111111111:<resource:2>:<token:1>"
"UnsubscribeURL": "<unsubscribe-domain>/?Action=Unsubscribe&SubscriptionArn=arn:aws:sns:<region>:111111111111:<resource:2>:<resource:3>"
},
{
"Message": {
Expand Down Expand Up @@ -93,14 +93,14 @@
]
},
"MessageId": "<uuid:2>",
"Signature": "signature",
"Signature": "<signature>",
"SignatureVersion": "1",
"SigningCertURL": "https://sns.<region>.amazonaws.com/SimpleNotificationService-<signing-cert-file:1>.pem",
"SigningCertURL": "https://sns.<region>.amazonaws.com/SimpleNotificationService-<signing-cert-file:1>",
"Subject": "Amazon S3 Notification",
"Timestamp": "date",
"TopicArn": "arn:aws:sns:<region>:111111111111:<resource:2>",
"Type": "Notification",
"UnsubscribeURL": "<unsubscribe-domain>/?Action=Unsubscribe&SubscriptionArn=arn:aws:sns:<region>:111111111111:<resource:2>:<token:1>"
"UnsubscribeURL": "<unsubscribe-domain>/?Action=Unsubscribe&SubscriptionArn=arn:aws:sns:<region>:111111111111:<resource:2>:<resource:3>"
}
]
}
Expand Down
Loading
0