Skip to content

SNS: implement Content-Type logic for http/https target #11634

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

Jump to bottom
Merged
merged 1 commit into from
Oct 4, 2024
Merged

SNS: implement Content-Type logic for http/https target #11634

merged 1 commit into from
Oct 4, 2024

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Oct 3, 2024

Motivation

As reported with #11617, we did not properly use the configured Content-Type when sending Notification message type.

I've tried to implement some CRUD validation around Topic attributes, but it needs a lot of it, and everything lives in moto. Either we move away from it as most of it is mocked only and implement it in LocalStack, or we need to implement the feature there.

The fix/feature implemented here should be pretty solid, the only issue is that we can accept any format and users might not understand straight away why it doesn't work when the DeliveryPolicy they passed would fail when trying to set it, against AWS.

This page explains it pretty well: https://docs.aws.amazon.com/sns/latest/dg/sns-message-delivery-retries.html#creating-delivery-policy

Changes

  • for http and https targets, properly fetch the configured Content-Type to send.
  • add a CRUD test around Topic Attributes as I was initially thinking of improving it, but this would need too much time, so it is skipped for now

fixes #11617

@bentsku bentsku added aws:sns Amazon Simple Notification Service semver: patch Non-breaking changes which can be included in patch releases labels Oct 3, 2024
@bentsku bentsku self-assigned this Oct 3, 2024
@bentsku bentsku requested a review from baermat as a code owner October 3, 2024 23:03
@bentsku bentsku requested a review from cloutierMat October 3, 2024 23:03
bentsku
bentsku commented Oct 3, 2024
View reviewed changes
tests/aws/services/sns/test_sns.snapshot.json
"Accept-Encoding": "gzip,deflate",
"Connection": "connection",
"Content-Length": "content--length",
"Content-Type": "application/json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the important part of the fix: properly received content-type

tests/aws/services/sns/test_sns.snapshot.json
"Accept-Encoding": "gzip,deflate",
"Connection": "connection",
"Content-Length": "content--length",
"Content-Type": "text/plain; charset=UTF-8",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this shows that the SubscriptionConfirmation ignores the configured content-type

tests/aws/services/sns/test_sns.snapshot.json
"Accept-Encoding": "gzip,deflate",
"Connection": "connection",
"Content-Length": "content--length",
"Content-Type": "text/csv",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same, shows the fix

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 3, 2024

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 1m 34s ⏱️ - 38m 25s
2 109 tests  - 1 372  1 908 ✅  - 1 159  201 💤  - 213  0 ❌ ±0 
2 111 runs   - 1 372  1 908 ✅  - 1 159  203 💤  - 213  0 ❌ ±0 

Results for commit b28ca9a. ± Comparison against base commit 50b4063.

This pull request removes 1375 and adds 3 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.sns.test_sns.TestSNSSubscriptionHttp ‑ test_subscribe_external_http_endpoint_content_type[False]
tests.aws.services.sns.test_sns.TestSNSSubscriptionHttp ‑ test_subscribe_external_http_endpoint_content_type[True]
tests.aws.services.sns.test_sns.TestSNSTopicCrud ‑ test_topic_delivery_policy_crud
This pull request removes 214 skipped tests and adds 1 skipped test. 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.TestSNSTopicCrud ‑ test_topic_delivery_policy_crud

cloutierMat
cloutierMat approved these changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@cloutierMat cloutierMat left a comment

Choose a reason for hiding this comment

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

🚀 Nice fix. The testing is really great. Good attention to details to implement both subscription attributes and topic atributes!

@bentsku bentsku merged commit 3c1ce57 into master Oct 4, 2024
42 checks passed
@bentsku bentsku deleted the fix-sns-http-content-type branch October 4, 2024 09:06
macnev2013 pushed a commit that referenced this pull request Oct 22, 2024
@bentsku @macnev2013
SNS: implement Content-Type logic for http/https target (#11634)
bdf6e1c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloutierMat cloutierMat cloutierMat approved these changes

@baermat baermat Awaiting requested review from baermat baermat is a code owner

Assignees

@bentsku bentsku

Labels
aws:sns Amazon Simple Notification Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: SNS Topic does not respect Content-Type for Subscribed HTTP Endpoint
2 participants
@bentsku @cloutierMat