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

feat(credential-provider-node): use dynamic import for credential providers #5698

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Jan 19, 2024

This re-stages #5677 which was merged too early.

  • integ

  • e2e

  • e2e legacy

  • test Node.js platform target bundlers (esbuild, webpack)

@kuhe kuhe requested a review from a team as a code owner January 19, 2024 18:38
@kuhe kuhe force-pushed the feat/dynamic-import branch 2 times, most recently from 9dee15c to 65e75a2 Compare January 23, 2024 20:17
@kuhe kuhe merged commit 1452cd4 into aws:main Jan 24, 2024
3 checks passed
@kuhe kuhe deleted the feat/dynamic-import branch January 24, 2024 13:57
@mskonf
Copy link

mskonf commented Jan 25, 2024

@kuhe I am not sure if this is the right place to mention this:
But we are using AWS SDK along with our code deployed on AWS Lambda, Node Version 19x and 20x both, and after the latest release we are facing issues maybe due to this change, while the code is bundled using webpack.

2024-01-25T08:09:47.489Z	85c2f086-1ee8-4b53-bbc7-7389da5df30d	ERROR	Error: Cannot find module '../564/index.js'
Require stack:
- /var/task/listEvents/index.js
- /var/runtime/index.mjs
    at Module._resolveFilename (node:internal/modules/cjs/loader:1144:15)
    at Module._load (node:internal/modules/cjs/loader:985:27)
    at Module.require (node:internal/modules/cjs/loader:1235:19)
    at require (node:internal/modules/helpers:176:18)
    at __webpack_require__.f.require (/var/task/listEvents/index.js:34787:28)
    at /var/task/listEvents/index.js:34726:40
    at Array.reduce (<anonymous>)
    at __webpack_require__.e (/var/task/listEvents/index.js:34725:67)
    at fromSSO.fromSSO (/var/task/listEvents/index.js:21092:59)
    at /var/task/listEvents/index.js:25522:39 {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/var/task/listEvents/index.js', '/var/runtime/index.mjs' ]
}

The fromEnv function from @aws/credential-provider-node package is not present in the Lambda runtime for the other AWS Packages to consume...

Please let me know if there is a more appropriate place to post the issue...

@kuhe
Copy link
Contributor Author

kuhe commented Jan 25, 2024

@mskonf could you create a new bug report with instructions on how to reproduce the issue?

@mskonf
Copy link

mskonf commented Jan 26, 2024

@mskonf could you create a new bug report with instructions on how to reproduce the issue?

Sure

@mskonf
Copy link

mskonf commented Jan 26, 2024

@kuhe have created the issue. This is my first time creating an issue, I have included all the context to the best of my ablity. Please let me know if I have left something out and if you need additional context on the same!
Thanks

@perpil
Copy link

perpil commented Jan 26, 2024

Not sure if this is the best place to comment, but I think this change was made in response to #5516 which is locked so I can't update it.

In the lambda environment this didn't result in the performance gains in coldstarts I was expecting. I updated my test repo: https://github.com/perpil/sso-coldstart-benchmark-cdk to use 3.499

The most surprising thing to me was the delta between the new provider and the new provider when the sso packages are excluded from the bundle so they would be loaded from disk. I would expect the performance to be the same because the sso packages shouldn't be loaded, but there was a 285 ms delta seemingly indicating those packages are still getting used and being loaded from disk.

If I figure out what is going on, should I follow up here?

@kuhe
Copy link
Contributor Author

kuhe commented Jan 26, 2024

#5516 has been re-opened, please discuss there since this is a closed PR.

@aws aws locked as resolved and limited conversation to collaborators Jan 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants