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

MessageAttributes parsing from SNS #93

Open
AlexanderKudryavsky opened this issue Feb 1, 2021 · 14 comments
Open

MessageAttributes parsing from SNS #93

AlexanderKudryavsky opened this issue Feb 1, 2021 · 14 comments
Assignees
Labels
bug Something isn't working

Comments

@AlexanderKudryavsky
Copy link

Expected Behavior

If I receive messages with the correct attributes (__ SQS_S3__, SQS_GZIP) and with unwrapSns: true then it should parse my message back to its original form.

Current Behavior

If I receive a message in the form gzip or s3 then the message is not parsed properly.

Possible Solution

Incorrect path in the constructor of the class Message, must be added to if (opts.unwrapSns) - this.attributes = attributeUtils_1.parseMessageAttributes(unwrapped.MessageAttributes);
And in parseAttributeValue replace with this

const type = unparsedAttribute.DataType || unparsedAttribute.Type;
const stringValue = unparsedAttribute.StringValue || unparsedAttribute.Value;

Your Environment

  • Version used: 4.1.1
  • Node version: 10.19.0
@AlexanderKudryavsky AlexanderKudryavsky added the bug Something isn't working label Feb 1, 2021
@regevbr
Copy link
Contributor

regevbr commented Feb 1, 2021

Hi @AlexanderKudryavsky thanks for reporting the bug! Would you mind submitting a PR?

@regevbr
Copy link
Contributor

regevbr commented Feb 1, 2021

@AlexanderKudryavsky I am not sure what goes wrong here... If the queue messages are generated by SNS, then they will never by gzipped or stored in S3, you should probably not send messages yourself in the same queue an SNS can send messages in. If you do think this is a valid case, I don't see in AWS docs where the property MessageAttributes exists on the SNS message body. Can you please refer me to it? Same goes for the new way you suggest to parse attributes.

@AlexanderKudryavsky
Copy link
Author

AlexanderKudryavsky commented Feb 1, 2021

@regevbr If I send a message to the SNS through aws sdk then MessageAttribues get into the body
I need to send a message not only in SQS but also in SNS and I would like to use the c3 and gzip functionality of your package

@regevbr
Copy link
Contributor

regevbr commented Feb 1, 2021

Ok I understand, can you please share with me the docs that state it? Also can you please share how such a message looks like as a JSON output (remove sensitive data ofcourse)

@AlexanderKudryavsky
Copy link
Author

AlexanderKudryavsky commented Feb 1, 2021

@regevbr
https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/SQS.html#sendMessage-property
https://docs.aws.amazon.com/sns/latest/dg/sns-sqs-as-subscriber.html
Message:
{ "Type": "Notification", "MessageId": "ab63e60a-6348-540b-a838-be9b9e6b0148", "TopicArn": "arn:aws:sns:us-east-1:26363465:example", "Message": "{\"bucket\":\"my-bucket\",\"key\":\"TestCommand-2021-02-01T12:45:22+00:00\"}", "Timestamp": "2021-02-01T12:45:23.411Z", "SignatureVersion": "1", "Signature": "OufhMySV+SmYMfRSXGwcIz5Xee50Pa35SSgWXA7nvlxTp3e20Upvj6tosVhyH+SxbrxeigdvPz0ElIEczwmxnpav1P6Rewm8bOr7RK8l3R3i0DyJhnzrLxd9wgSdfgfdgdBXNAikn1pLbFoKgBTD7WVCiKEpl+mMPPf6eN1hVSX0cxe4InclSGU9/B4z/i4ZRk5sHnDZDNpDZaJZlMivv8nIE5bmu5lh0r5YlY7e4FxDxtw/sjouK48O+TGQc2U9ogt0cb0bNg4hdfpDWPG7RwRWaIYYmNk2vwUzsLQavyDTqzYcCZICXIO9j9dMGsPTRKsK8KmXm3hofmBvOqVSmviodtfjA==", "SigningCertURL": "https://sns.us-east-1.amazonaws.com/SimpleNotificationService-436436346346346346.pem", "UnsubscribeURL": "https://sns.us-east-1.amazonaws.com/?Action=Unsubscribe&SubscriptionArn=arn:aws:sns:us-east-1:4564564:example:1255a8a3-fecf-4a4c-ad99-4564543fgbb60", "MessageAttributes": { "__SQS_GZIP__": { "Type": "Number", "Value": "1" }, "__SQS_S3__": { "Type": "Number", "Value": "130" } } }

@AlexanderKudryavsky
Copy link
Author

My SQS queue is subscribed to SNS and messages come from there

@regevbr
Copy link
Contributor

regevbr commented Feb 1, 2021

The docs you sent does not support what you are saying is going on.
I see here that the message attributes you are sending when you publish SNS message are of the exact same structure that is currently expected. Can you please share with me how do you send a message to SNS?
I think a better approach here is to make the tool be aware if messages can be not originated from SNS, and in that case, we should add a message attribute to it stating that it is a regular message, then we need to skip the unwrapping all together. It doesn't make sense for you to mimic an SNS message when you send a regular message... it will surely break down in the future if we will need to rely somehow on a new field that SNS passes...

@AlexanderKudryavsky
Copy link
Author

this.sns = new SNS();


const params = {
    Message: stringMessage,
    TopicArn: SNS_TOPIC_ARN,
    MessageAttributes: {
                    __SQS_S3__: {
                        DataType: 'Number',
                        StringValue: `${compressedMessageSize}`
                    },
                    __SQS_GZIP__: {
                        DataType: 'Number',
                        StringValue: '1'
                    }
                }
};

return this.sns.publish(params).promise();

If you look at the example of message, you can see that this structure is not expected, in opts.msg.MessageAttributes there will be undefined

@regevbr
Copy link
Contributor

regevbr commented Feb 1, 2021

@AlexanderKudryavsky I see but what is strange here is that the message attributes inside the unrwapped message has a different structure.
Also why would you send those attributes manually? They are internal properties that should be handled automatically by the Squiss tool...

@AlexanderKudryavsky
Copy link
Author

@regevbr

  1. The structure apparently changes aws sdk
  2. I send a message not through squiss, but through sns (aws sdk) squiss only listens,I added attributes so that squiss would understand what it is dealing with and how to process it

@regevbr
Copy link
Contributor

regevbr commented Feb 1, 2021

https://stackoverflow.com/questions/44238656/how-to-add-sqs-message-attributes-in-sns-subscription answers describe the same strange scenario here. They do give a tip though - you can set some property called sendRaw on the subscription then the message is not wrapped...
In any case, I'm convinced that the way the message appears as you describe it is the correct one. that being said, I still don't understand why you are sending internal properties like that?

@regevbr
Copy link
Contributor

regevbr commented Feb 1, 2021

@regevbr

  1. The structure apparently changes aws sdk
  2. I send a message not through squiss, but through sns (aws sdk) squiss only listens,I added attributes so that squiss would understand what it is dealing with and how to process it

I understand that, but I'm asking in what scenario you will pass manually the GZIP flag?

@AlexanderKudryavsky
Copy link
Author

I check the message size, if it is more than 64 KB, then I do gzip and add an attribute, if the gzip size is more than 256 KB, then I send it to s3 and add the s3 attribute, squiss does not support sending to SNS, so I add the attributes myself

@regevbr
Copy link
Contributor

regevbr commented Feb 1, 2021

Ok I understand. This is not recommended because the zip algorithm can change but I understand your need now. Can I ask though why not send the message to SQS then? why do you need SNS in that case? You can just send directly to the SQS queue using the tool...

I added my review comments to the PR so we can continue the discussions there.
If you want, I can welcome another PR that will also add a way to send SNS messages :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants