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

Use layers to deploy shared libraries to avoid bundler inconsistencies (fixes #42) #124

Merged
merged 11 commits into from
Sep 19, 2022

Conversation

NoxHarmonium
Copy link
Contributor

@NoxHarmonium NoxHarmonium commented Sep 16, 2022

Each example has been integration tested on a real deployment.

@NoxHarmonium
Copy link
Contributor Author

NoxHarmonium commented Sep 16, 2022

It seems like serverless can't even run package without AWS credentials if you use layers.

serverless/serverless#8187

This is incredibly annoying as I don't want to provide AWS credentials just to run a basic packaging test. The credentials would have to be accessible in PR builds and would be open to exploitation by someone opening a well crafted PR.

I guess I could provide extremely stripped down IAM credentials that can only read if a layer exists, but it is still a real pain.

? {}
: decoder.decode(response.Payload);

console.log({
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In aws-sdk v3 for whatever reason the payload is now base64 and isn't decoded automatically by the SDK.
We have to decode it ourself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we then have to log it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh oops I was looking at the wrong line. I've pushed a fix

@@ -0,0 +1,153 @@
// Thanks Datadog! https://github.com/DataDog/serverless-plugin-datadog/blob/9c9e8fc78b51f752df4e1958b938c82e561a20a3/src/env.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

future: would this be something that people writing other plugins like this one would find helpful to live in a micro lib?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reckon a library that tries to standardise writing plugins for serverless and provides helper functions like this might be useful.

It could provide the consistent well documented plugin API that serverless doesn't (with types etc.) Probably a lot of work to develop and maintain though. A micro lib with helper functions would probably be more reasonable.

@dspasojevic
Copy link
Contributor

LGTM

- This was the first attempt at fixing the serverless webpack warnings about ambiguous handlers. serverless webpack finds anything that looks like a handler (e.g. anything that ends with ts/js) so it detects type files as well
- Unfortunatly it still picks up map files so this did not fix the warning but I think it is better to organise the files separately anyway so I'm keeping the changes
- This fixes the warnings about ambiguous handlers as only the javascript files are copied to the staging area before bundling
- This might make it harder to get nice stack traces when an error occurs in lambda invocations, but the map files will still be under node_modules in a pinch. It was either this, or get the end user to add special config for serverless-webpack. We might need to revisit this in the future if it becomes an issue.
- Using the more mature 'serverless-plugin-warmup' library as a reference, rejigged the hooks to be more specific
- This allows smaller bundle sizes because we can just pull in the modules we need
- Reduces bundle size
- Although this was originally about severless-webpack warnings (#42) this ended up being a thorough shakeout of all the bundler issues. All the bundlers have documented workarounds where required now, and produce efficient lambda bundles.
- aws-sdk v3 is now used, which enables proper tree shaking
- It is annoying, but at the moment this is the easiest way to get around the requirement for credentials when packaging with layers
- See also: serverless/serverless#8187
@NoxHarmonium NoxHarmonium force-pushed the issue-42-webpack-bundler-warnings-wip branch from 75c49f0 to d890d7a Compare September 19, 2022 09:14
@NoxHarmonium
Copy link
Contributor Author

Fixed the build by giving the build job a super restricted IAM user into my sandbox environment.

I found out that we have to approve running workflows from new forks/PRs so that makes me more comfortable putting IAM credentials in the build job. Even if somehow the credentials were compromised, all someone could do is describe any stack starting with "adl-example-*" in my sandbox account.

I've left some notes in the build job definition about it (d890d7a)

I also raised an issue to use assume role to get credentials which is best practice (#126)

@NoxHarmonium NoxHarmonium merged commit 3849a3d into main Sep 19, 2022
@NoxHarmonium NoxHarmonium deleted the issue-42-webpack-bundler-warnings-wip branch September 19, 2022 09:35
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.

2 participants