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

Add retry strategy options to RemoteProviderConfig #2706

Closed
dmattia opened this issue Aug 25, 2021 · 8 comments
Closed

Add retry strategy options to RemoteProviderConfig #2706

dmattia opened this issue Aug 25, 2021 · 8 comments
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback.

Comments

@dmattia
Copy link
Contributor

dmattia commented Aug 25, 2021

Is your feature request related to a problem? Please describe.

Occasionally, the AWS metadata services will fail to fetch credentials. One common cause I've seen is the status code 429 TooManyRequestsException, which means too many requests are sent in a short time period and the client is rate limited.

The RemoteProviderConfig type allows passing a maxRetries value to each client, however, it does not give options for how those retries should occur.

Here's an image of a trace where I had a client get rate limited with maxRetries set to 5. Notice, all of the calls fail, as all of the calls are separated by under 1ms. If the metadata endpoint is failing because of rate limiting issues, it is unlikely to not be rate limited .5ms later.

Screen Shot 2021-08-24 at 9 42 10 PM

Describe the solution you'd like

I want to have an additional field to specify a retry strategy. This could be as simple as a retryDelay parameter that takes in how much time to wait for linear retries, or could be a set of fields that specifies if the retry should be exponential/linear, etc.

Describe alternatives you've considered

none

Additional context

none

@dmattia dmattia added the feature-request New feature or enhancement. May require GitHub community feedback. label Aug 25, 2021
@trivikr
Copy link
Member

trivikr commented Aug 25, 2021

@dmattia We have an existing option to set retry mode for clients. It's not documented in developer guide, so I'll create a request with writers. It is similar to AWS CLI retries

We will evaluate if this retry mode can be used for while fetching credentials.

In the mean time, I created two other reports based on your feature request:

Do you have information on the path of http request which is throwing 429 TooManyRequestsException?

@dmattia
Copy link
Contributor Author

dmattia commented Aug 25, 2021

Thanks @trivikr! We sure do, the full url from our trace is http://169.254.170.2/v2/credentials/<uuid_v4>.

@trivikr
Copy link
Member

trivikr commented Aug 25, 2021

The full url from our trace is http://169.254.170.2/v2/credentials<uuid_v4>.

Oh, you're using CMDS in a Fargate container and not IMDS.
Code for future reference:

if (process.env[ENV_CMDS_RELATIVE_URI]) {
return {
hostname: CMDS_IP,
path: process.env[ENV_CMDS_RELATIVE_URI],
};
}

I don't have context on CMDS, but using retry mode in those requests would help.
I'll revisit this issue when I get opportunity to deep dive into fromContainerMetadata implementation.

@trivikr
Copy link
Member

trivikr commented Aug 25, 2021

Do update this issue if you write your own custom fromContainerMetadata implementation with custom retries.

Existing implementation:

export const fromContainerMetadata = (init: RemoteProviderInit = {}): CredentialProvider => {
const { timeout, maxRetries } = providerConfigFromInit(init);
return () =>
retry(async () => {
const requestOptions = await getCmdsUri();
const credsResponse = JSON.parse(await requestFromEcsImds(timeout, requestOptions));
if (!isImdsCredentials(credsResponse)) {
throw new CredentialsProviderError("Invalid response received from instance metadata service.");
}
return fromImdsCredentials(credsResponse);
}, maxRetries);
};

You can write your own implementation and pass it while creating client:

import { FooClient } from "@aws-sdk/client-foo";

const client = new FooClient({
  credentials: fromContainerMetadataCustom({
    // custom params if required.
  })
});

@trivikr
Copy link
Member

trivikr commented Aug 26, 2021

@dmattia Are you making lot of concurrent calls, and facing this issue in v3.27.0?

If yes, we're going to push a fix in v3.28.0 on Friday 08/27
The fix can be viewed in PR #2716

If you're using v3.27.0, you can temporarily downgrade to v3.26.0 while fix is published.

@aldegoeij
Copy link

related to my issue on the SecurityHub client: #2741

@github-actions
Copy link

github-actions bot commented Sep 3, 2022

Greetings! We’re closing this issue because it has been open a long time and hasn’t been updated in a while and may not be getting the attention it deserves. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to comment or open a new issue.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Sep 3, 2022
@github-actions github-actions bot closed this as completed Sep 7, 2022
@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 Sep 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback.
Projects
None yet
Development

No branches or pull requests

3 participants