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

fix(property-provider): stop memoizing rejected provider #2680

Merged
merged 1 commit into from
Aug 17, 2021

Conversation

AllanZhengYP
Copy link
Contributor

@AllanZhengYP AllanZhengYP commented Aug 17, 2021

Resolves: #2659

Description

Previously memoize() cached the provider promise no matter whether the promise is rejected. When the credential provider throws, the client will keep throwing the credential error even after the underlying credential provider resumes.

With this change the memoize() only caches the successfully resolved credentials. If the credential provider errors out, it will retry the provider in next API call as no credential is available still.

Testing

Unit test, integration test

Additional context

JS-2740


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

expect(await memoized()).toStrictEqual(mockReturn);
expect(provider).toHaveBeenCalledTimes(1);
}
});

it("should always return the same promise", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This unit test is removed. We cannot return the same promise because the memoize() now needs to await the provider to resolve(). The unit test was introduced in #56 but with no explicit purpose. I have run the integration test to make sure it feature doesn't affect the SDK now.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@cbdcd9c). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2680   +/-   ##
=======================================
  Coverage        ?   58.87%           
=======================================
  Files           ?      521           
  Lines           ?    28945           
  Branches        ?     7175           
=======================================
  Hits            ?    17040           
  Misses          ?    11905           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbdcd9c...4e4bfad. Read the comment docs.

@aws-sdk-js-automation
Copy link

AWS CodeBuild CI Report

  • CodeBuild project: sdk-staging-test
  • Commit ID: 4e4bfad
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@github-actions
Copy link

github-actions bot commented Sep 2, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2021
@AllanZhengYP AllanZhengYP deleted the fix-memoize branch November 16, 2021 01:46
@aws aws unlocked this conversation Dec 3, 2021
@AllanZhengYP
Copy link
Contributor Author

We notice some customers are getting intermittent failures while fetching credentials from STS. This issue became more prevalent in US-EAST-1 region using Amazon EKS managed AWS Identity and Access Management (IAM) OpenID Connect (OIDC) external identity provider (IdP).

The client-side root cause of this issue is that when the SDK fetch the credentials coinsides the STS server-side errors, the SDK will incorrectly cache the errored response. Thus the SDK will continuously throw credential fetching errors even after the STS server-side is back up.

This code change fixes the issue(available since 3.27.0). If you have similar issue, please attempt to upgrade the SDK to the latest version. If the issue persists, please open a bug report to the support team.

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 20, 2021
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.

SNS publish issue with CredentialsProviderError
4 participants