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

Deprecate 'from_sns' #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ashleyprimo
Copy link

@ashleyprimo ashleyprimo commented Feb 28, 2024

Context

This PR is (at-least initially-hence draft) intended to be a discussion, as I do not fully understand why from_sns is needed, as far as I can tell the payload format when S3->SNS->SQS architecture is deployed, the payload resembles the following:

{
    "Records": [
      {
        "eventVersion": "2.1",
        "eventSource": "aws:s3",
        "awsRegion": "obfuscated",
        "eventTime": "2024-01-01T00:00:00.000Z",
        "eventName": "ObjectCreated:Put",
        "userIdentity": {
          "principalId": "AWS:obfuscated"
        },
        "requestParameters": {
          "sourceIPAddress": "obfuscated"
        },
        "responseElements": {
          "x-amz-request-id": "obfuscated",
          "x-amz-id-2": "obfuscated"
        },
        "s3": {
          "s3SchemaVersion": "1.0",
          "configurationId": "obfuscated",
          "bucket": {
            "name": "obfuscated",
            "ownerIdentity": {
              "principalId": "obfuscated"
            },
            "arn": "arn:aws:s3:obfuscated"
          },
          "object": {
            "key": "obfuscated",
            "size": 0,
            "eTag": "obfuscated",
            "versionId": "obfuscated",
            "sequencer": "obfuscated"
          }
        }
      }
    ]
  }

Which is inline with https://docs.aws.amazon.com/AmazonS3/latest/userguide/notification-content-structure.html

So, attempting to payload = JSON.parse(payload['Message']) if @from_sns will fail as of course, the JSON structure does not match that expectation.

There are number of issues I have seen, related to this:

Why removal?

I do not currently understand how this payload would ever be nested within payload['Message']; though if there is a case where this would be the case - we should improve documentation to reflect this, and probably the naming of the variable.

Currently, from_sns needs to equal false even when the data does originate from SNS - contrary to the comment in code Whether the event is processed though an SNS to SQS. (S3>SNS>SQS = true |S3>SQS=false) (and inference made in documentation)

@cherweg maybe you will know if I am missing something here?

@ashleyprimo ashleyprimo marked this pull request as draft February 28, 2024 17:23
@christianherweg0807
Copy link
Collaborator

At time of writing the code SNS did not support raw message body delivery.

(https://docs.aws.amazon.com/sns/latest/dg/sns-large-payload-raw-message-delivery.html)

So a Message delivered from S3-> SNS to SQS had an additional Header. The Original message was in payload['Message'].

Thank you for your idea…

@ashleyprimo
Copy link
Author

ashleyprimo commented Feb 28, 2024

At time of writing the code SNS did not support raw message body delivery.

(https://docs.aws.amazon.com/sns/latest/dg/sns-large-payload-raw-message-delivery.html)

So a Message delivered from S3-> SNS to SQS had an additional Header. The Original message was in payload['Message'].

Thank you for your idea…

I see, that makes a lot of sense. I did spend some time searching to try understand, knew there must've been a reason why you added it. 😁

I guess if it's possible that someone might turn this feature off voluntarily (for whatever) reason we can add a special flag to support that use case.

Maybe sns_raw_payload_delivery_disabled with a default set to false?

Happy to alter the PR to add this.

@ashleyprimo ashleyprimo changed the title Remove 'from_sns' Deprecate 'from_sns' Feb 29, 2024
Signed-off-by: Ashley Primo <ashley.primo@transferwise.com>
@ashleyprimo ashleyprimo marked this pull request as ready for review February 29, 2024 09:07
@ashleyprimo
Copy link
Author

How is this latest commit @christianherweg0807

@ashleyprimo
Copy link
Author

Bump @cherweg / @christianherweg0807 - would be great to get this, or a derivative solution in mainstream to make configuration nicer. 🙂

@cherweg
Copy link
Owner

cherweg commented Apr 12, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants