Switch to AWS SDK v3 #146
Switch to AWS SDK v3 #146
Conversation
|
I don't understand - this still uses require, which is no longer available in Node.JS 18 - how does this fix #144 ? |
|
NodeJS 18.x does still support |
|
I tried using this is one of my projects but ran into issues with this line: https://github.com/arithmetric/aws-lambda-ses-forwarder/pull/146/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R206
|
First off, thank you for your reply! Second, I guess that means #144 is incorrect in the assertion that NodeJS no longer supports require. I did a bit more reading and figured out that our issue (the people reporting #144) is that following the same steps available on several articles about settings this up (and indeed the same steps mentioned in the repository), AWS creates an index file with the extension .mjs by default, which uses the ECMA Script Module system, which no longer supports require nor exports: https://nodejs.org/api/esm.html#no-require-exports-or-moduleexports - which explains why I am intermittently seeing "require is not defined in ES module scope, you can use import instead" and "exports is not defined in ES module scope". I guess "ES module scope" should have been my clue here, but I am a novice at Node - this is the first time I've setup a production system that is actually using Node for execution. I changed the extension to .CJS to use CommonJS instead (changing it to .JS still runs as an ES Module since that is Lambda's default module system - strangely the CloudWatch logs even still refer to a not-actually-existent index.mjs when using the .JS extension), and now I'm seeing an access denied error when fetching the email from the bucket, but I reckon I can figure that out. :) Maybe it would be a good idea for the index.js file in this repository to be renamed to index.cjs given that Lambda defaults to ECMA otherwise. In its current state, even uploading the script directly from a ZIP of the repository creates a non-functional result. |
|
I think that #144 is poorly worded. Both Node.js 16.x and 18.x support both ESM (file extension: .mjs) and CommonJS (file extension: .cjs) modules. The issue that this PR addresses (and the issue that I think #144 was trying to describe) is that the AWS Lambda Node.js 16.x and earlier runtimes have the AWS SDK v2 installed, whereas the AWS Lambda Node.js 18.x runtimes (and presumably all future runtimes) ship the AWS SDK v3. The reason that this package won't work on AWS Lambda Node.js 18.x runtimes is that it currently uses the AWS SDK v2 runtime, not the v3 runtime. It has nothing to do with ESM vs. CommonJS support. See https://aws.amazon.com/blogs/compute/node-js-18-x-runtime-now-available-in-aws-lambda/. |
|
Okay, I think I understand after reading a little more closely: You are saying it isn't relevant to the PR... not that it isn't necessary to be aware of in order for the script to function. #144 shouldn't really make mention of the require statement at all, since that is not actually an effect of the change they are referring to. Would it make sense to make a separate PR to rename the index file so that the correct module system is used by default? I imagine at one point in time CommonJS was the default for .JS files and is no longer. |
|
No changes are needed regarding the module system. |
|
Just to clarify - I'm sorry if this is getting grating and I appreciate any insight you have as I am, as stated, new to Node and have been trying to set this up on AWS just following the steps in the repository's readme - you are saying this should work with CJS or ESM? I don't understand then why I only get these errors when running your version of the index script if I save the index as .mjs or .js, but not when I save it as .cjs:
I suppose there must be something else at play here that I don't understand? |
|
If you copy the file and save it as If you copy the file and save it as If you copy the file and save it as |
|
Thank you for clarifyng that this is written in CJS syntax and does not work with ESM also. As far as package.json... I actually have no package.json in my Lambda function at all, since there is none by default (although I have read that if you create one, it will be used), and the steps in the readme for this repo don't mention creating one. Additionally, I read in the AWS documentation that ESM is used by default (although I believe that was not the case at the time of this project's inception), and I reckon that default configuration is behind-the-scenes.
Are you running this in Lambda yourself? Since this is specifically for AWS Lambda, where ESM is used by default with no package.json or any other configuration, that is why I maintain that renaming the index to .cjs seems appropriate, or otherwise maybe at least making mention of the fact that the script is written in CJS syntax... although changing the extension would both imply that for people who are in-the-know and make it work out-of-the-box for noobs like me. |
No, I don't think that's correct. In the absence of a package.json file, AWS Lambda Node.js runtime will treat files with the extension I am not copying this file. I am importing it in a larger project and bundling it using esbuild. |
|
Thank you again for the reply. For whatever reason, when I create new functions, they seem to be configured for ESM by default. Maybe there is some sort of global configuration somewhere I could have set to prefer ESM? I did try making a new function to see if I could spot some configuration option for scripting module that I could have possibly missed, and I did scour the documentation looking for proof of my claim here. I won't bother you with more on the topic after this, but this is what I found - hopefully I'm not beating a dead horse here - just trying to get to the bottom of the discrepancy: I created a new Lambda function from scratch to see if I missed something and had selected ECMA somehow when I initially made my function. I only changed the name field to "test" while making no other changes whatsoever, and it created a function with an index.mjs. I double checked during the creation step, which is only a single page, and found no configuration options for ESM vs CJS under the Advanced section nor anywhere else. I reckon that if I changed the extension to .js it would still see the file as .mjs, like happened with my instance of this email script. As far as the AWS documentation goes, different articles seem (to me at least, although I might be misinterpreting them) to be contradictory... This documentation from 2022 seems to support exactly what you're saying about CommonJS being default. It mentions making changes to the package.json or changing the file extension to use ESM: https://aws.amazon.com/blogs/compute/using-node-js-es-modules-and-top-level-await-in-aws-lambda/ But the Developer Guide for AWS SDK3 seems to me like it suggests ESM is default: "The AWS SDK for JavaScript code examples are written in ECMAScript 6 (ES6). ES6 brings new syntax and new features to make your code more modern and readable, and do more." In any case, thank you for replying and offering your insight... I'll hush and see if something I've overlooked becomes obvious. 😵💫 |
|
nice. I just was going to submit a PR for this exact thing. @lpsinger any reason you went with the v2 ses client rather than the v3? also, when assigning |
There is no SES v3 API. There are two SES APIs: SES and SESv2. AWS has two JavaScript SDKs: v2 and v3. Amazon SES has two APIs: v1 and v2. This patch uses the newer of the two SES APIs, SESv2, and it uses the newer (and actively developed and supported) SDKs, v3.
I don't know, because I am not currently using S3 to store emails at all (see #1025). |
I pushed a possible fix for that in the latest commit, 9a5281b. |
Ah ok. I naively thought that @aws-sdk/client-sesv2 was from aws sdk v2, but it is the latest version for the ses client, while @aws-sdk/client-ses is actually the older version. Got it. I saw 9a5281b -- that is essentially what I've done as well and should work. EDIT: @lpsinger with only specifying the raw email blob, is SES not going to email all recipients of the original email? FWIW - I don't know a way around this in SES Client v2. It seems that the raw email blob is obeyed and the |
|
The script now runs for me without error after that last change... at least initially (see below). I had been getting a permissions error, which I realized was the result of me creating a new bucket during troubleshooting and never setting the permissions, but after correcting that I was still having issues until the last change. I updated the index.js and also emptied my S3 bucket, since initially I thought the issue below was the script re-processing the existing test e-mails I had sent, although I don't actually think now that this was the case. Now when the function runs, initially in the logs it seems to be working - I can even see "Sending e-mail via SES" with my original address and desired destination address, and a message saying sendRawEmail() successful, but I am seeing the steps repeated over, and over, and over again, and it is creating many, many copies of the same e-mail in my S3 bucket. Additionally, I am not actually receiving any copies of the e-mail ever despite that it says the send was successful. At the very end of the log, after many iterations, the sendRawEmail() step returns an error saying the header is too long (I am guessing it becomes longer with every iteration?) followed by another error about the email send failing and an invoke error before the script actually stops running. If anyone has any suggestions as to what I could have done wrong or what might be happening, I would really appreciate it. I have been trying to get this set up for a couple weeks now. 😿 |
|
@dekkeravesque here's code that should work -- I had the same issues with the code from this PR. It seems that aws ses v2, when taking raw email (which we really need to do in order to do attachment support) does not override the To/From in the raw email from the specified "Destination" and "FromEmailAddress" fields. https://gist.github.com/mylesboone/b6113f8dd74617d27f54e5d0b8598ff7 |
|
@mylesboone Ahhh, I saw what you said about emails going to recipients on the original message instead of those specified in the configuration section of the script as a potential effect of this limitation, but for some reason it did not occur to me that my duplication problem was a manifestation of that exact issue, although now that you point it out, it makes total sense... The emails are just coming back to the original recipient, where they are then forwarded again, and then again, etc, etc... So it is functioning now... just not the way I'd like it to. 🐱 Thank you for your help! |
|
@mylesboone Hooray, that worked! Thank you so much! 🐱 |
|
@lpsinger Thanks for this contribution! I like the idea to migrate to AWS SDK v3. I'll review this PR further, but making this change will not work out-of-the-box on Node.js 16 and earlier runtimes, so this will need to be documented as a breaking change, and this project will then have a version that works with Node.js 16 and one for Node.js 18. Note: I've described another way to run the current version of the code with AWS SDK v2 on the Node.js 18 Lambda runtime here: #144 (comment) |
|
@arithmetric this pr does not behave correctly in its current form. https://gist.github.com/mylesboone/b6113f8dd74617d27f54e5d0b8598ff7 will give a working index.js -- I have not created a PR because I haven't looked at tests, etc. |
|
@mylesboone, would you comment inline in this PR to indicate what changes are necessary? |
|
@lpsinger does the code in this PR work for you? Here's a diff between this PR (on the left) and known working code (on the right): https://www.diffchecker.com/EUueM4hx/ |
|
@mylesboone, do you know how to comment on lines of code in a PR? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#adding-comments-to-a-pull-request |
@lpsinger I'm not sure if your comment was made sincerely or not. I'll give you the benefit of doubt and assume it was made sincerely. Yes, I do know how to make comments on PRs in GitHub. Unfortunately a bulk of the changes in this PR make for code that behaves erroneously. The nature of the PR in its current state would mean rewriting most of it in comments, which is not a good use case for comments. I've given a working alternative in a gist that could be referenced for rectifying behavior. @arithmetric I'll go ahead and put in a working pull request. I'd suggest closing this PR in its current state. |
Yes, it was. I'm sorry if I offended, that was not my intention! I look forward to seeing your PR. |
|
We've been using this in production for several months now in https://github.com/nasa-gcn/gcn.nasa.gov/blob/main/app/email-incoming/support/index.ts. |
Fixes #144.