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

unwanted context timeout added to imds requests #2571

Closed
2 tasks done
ndbaker1 opened this issue Mar 20, 2024 · 2 comments · Fixed by #2572
Closed
2 tasks done

unwanted context timeout added to imds requests #2571

ndbaker1 opened this issue Mar 20, 2024 · 2 comments · Fixed by #2572
Assignees
Labels
bug This issue is a bug.

Comments

@ndbaker1
Copy link

ndbaker1 commented Mar 20, 2024

Acknowledgements

Describe the bug

the middleware that is added to all imds api requests inserts a timeout on the provided context that is only 5 seconds long, which invalidates retries configured for longer.

previously brought up in #1247

Expected Behavior

after creating an imds.Client with a high number of retries I expect that when passing a context.TODO() into an imds api like GetUserData, all retries will be respected before returning an error.

Current Behavior

with the setup described above, the request errors with

operation error ec2imds: GetUserData, canceled, context deadline exceeded

Reproduction Steps

  1. follow repo in: imds.GetInstanceIdentityDocument doesn't honor retry options #1247

create an imds client with a retry greater than 5 seconds, and mock out the environment AWS_EC2_METADATA_SERVICE_ENDPOINT to get the api to fail

i := imds.New(imds.Options{
  Retryer: retry.NewStandard(func(so *retry.StandardOptions) {
    so.MaxAttempts = 15
    so.MaxBackoff = 1 * time.Second
  }),
})
resp, err := i.GetUserData(ctx, &imds.GetUserDataInput{})
if err != nil {
  fmt.Fatal(err)
}

Possible Solution

do not enforce this timeout when adding middleware for these api calls

Additional Information/Context

began discussion in PR awslabs/amazon-eks-ami#1732 for the purpose of initiating imds api calls before networking or imds may be available yet.

AWS Go SDK V2 Module Versions Used

1.24.1

Compiler and Version used

go version go1.22.1 linux/amd64

Operating System and version

AL2023

@ndbaker1 ndbaker1 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 20, 2024
@lucix-aws lucix-aws removed the needs-triage This issue or PR still needs to be triaged. label Mar 20, 2024
@lucix-aws
Copy link
Contributor

I can't fathom why we would have done this to begin with. It looks like there was a previous patch applied here to attempt to respect explicit context timeouts, but there's no way to just NOT have a timeout without removing it from the middleware stack.

It's too risky behaviorally to just rip it out, but I'm going to add a config switch to the client itself that lets you shut it off entirely.

Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants