imdsclient: add retry to fetch_token #1801
Conversation
sources/imdsclient/src/lib.rs
Outdated
| let mut token_attempts: u8 = 0; | ||
| let max_token_attempts: u8 = 2; | ||
| loop { | ||
| token_attempts += 1; | ||
| ensure!( | ||
| token_attempts <= max_token_attempts, | ||
| error::FailedFetchToken { token_attempts } | ||
| ); | ||
| if token_attempts > 1 { | ||
| time::sleep(Duration::from_secs(5)).await; | ||
| } |
There was a problem hiding this comment.
I'd like to align the number of retries and the length of the delay with fetch_imds - the requests are going to the same service and reaching the failure state in either function will likely result in a boot failure.
There was a problem hiding this comment.
The retries in fetch_imds are mostly to trigger token refreshes in the even the token is expired as opposed to IMDS unavailability (hence a shorter sleep). Each client initialization and fetch_imds attempt is going to fetch a new token. If IMDS is unreachable for whatever reason, the failure would occur during the fetch_token. Since we are adding retries to the token fetch now it might make sense to drop the max_imds_attempts from 3 to 2, but I wouldn't recommend aligning the sleep duration.
There was a problem hiding this comment.
Instead of dropping fetch_imds from 3 to 2, I bumped fetch_token from 2 to 3.
|
Once @bcressey 's concern is handled, lgtm |
This adds retry logic if imdsclient fails to fetch a session token. Also includes small changes to the fetch_imds retry logic, such as moving the sleep after the ensure imds_attempts <= max_imds_attempts and bumping the wait time from 100ms to 1s.
jpculp
force-pushed
the
add-imds-token-retry
branch
from
47f4f82 to
05f01a2
Compare
|
Description of changes:
This adds retry logic if
imdsclientfails to fetch a session token.Also includes small changes to the
fetch_imdsretry logic, such as movingthe sleep after the ensure
imds_attempts <= max_imds_attemptsand bumpingthe wait time from 100ms to 1s.
Testing done:
aws-k8s-1.20ami and launched instance.host-containers.admin.user-datacontained a base64-encoded block./.bottlerocket/host-containers/admin/user-datacontained JSON.sudo sheltieto verify root shell was still available.plutowith it's sub-commands to verify functionality.Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.