fix: use default credentials provider for all provided SDK clients #1303
Conversation
|
@msailes as you are closest to Snapstart and the original change it would be great if you could help us convince ourselves this is the best way to deal with this. IMHO it would also be nice if we can add an E2E test for this - we have the framework for it now running in the repo, and @roamingthings has given us a reproducible deployable test case |
|
I have to add that I have a support ticket open to understand if the behavior of the runtime (cold-start after timeout) is intentional. |
|
Hey @roamingthings to keep you posted - we're asking around internally to make sure that the the default provider chain is the best way to handle this. Concretely this means making sure that we don't take a big performance hit when we work through it, and that there isn't some other best practice we should be employing. We're also having a bit of a look to see if we can add to our E2E tests to catch these sort of issues in the future. |
8000
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #1303 +/- ##
============================================
+ Coverage 79.19% 79.65% +0.46%
Complexity 639 639
============================================
Files 73 73
Lines 2360 2345 -15
Branches 258 253 -5
============================================
- Hits 1869 1868 -1
+ Misses 410 397 -13
+ Partials 81 80 -1
☔ View full report in Codecov by Sentry. |
|
@scottgerring Thank you for keeping me updated. At least support told me that this is the way to go. I'm curious what you will find out. Since there is a workaround for this issue I'm not in a hurry. I like the idea of having E2E tests in place for something like this. It has been a short night when I discovered this in production. |
|
Thank you @roamingthings I confirm that with these changes there is no noticeable performance difference, you can find a summary of my tests below: Version 1.16.1Version 1.17.0-SNAPSHOT |
...ools-core/src/main/java/software/amazon/lambda/powertools/core/internal/LambdaConstants.java
Show resolved
Hide resolved
|
For reference this describes the behavior of the runtime when an error like a timeout occurs during the invoke phase. This matches the observed behavior. |
|
@mriccia thanks for the data! Looks like we can make this change without fearing a performance regression. |
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
LGTM, thank you for spotting this issue and for writing this PR, @roamingthings!
Issue #, if available: #1302
Description of changes:
This pull request removes the conditional configuration of the credentials provider for all SDK clients that are created when no other SDK client has been provided by the application. As a consequence these SDK clients will always use the credentials provider chain that is used by SDK clients by default.
This change is necessary because in some situations a Lambda function that is published using SnapStart will be re-initialized by the runtime with its invocation type set to
on-demand. As a consequence the SDK client would be configured to only use theEnvironmentVariableCredentialsProvider. The next time the client is used an error will be thrown since the credentials are not provided differently than without SnapStart.For more details please see #1302 and http://github.com/roamingthings/sdk-client-snapstart-error-poc
Checklist
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.