imdsclient: better retries with tokio retry and timeout #1841

jpculp · 2021-11-24T23:33:20Z

Description of changes:

To mitigate things like closed connections, IMDSv2 token and data
fetches will continue to retry until success or until the timeout has
been reached.

The default timeout is 300s to match the ifup timeout in wicked, but can
be set manually using .with_timeout when creating a new ImdsClient.

Also moves the token fetch to just before the IMDS data fetch so
that building a new ImdsClient no longer needs an await.

refresh_token has been replaced by clear_token and write_token.

Testing done:
In addition to the unit testing...

Built aws-k8s-1.21 ami and launched instance.
Instance connected to eks cluster.
Connected to control container via ssm session.
Verified that host-containers.admin.user-data contained a base64-encoded block.
Connected to admin container via ssh.
Verified that /.bottlerocket/host-containers/admin/user-data contained JSON.
Ran sudo sheltie to verify root shell was still available.
Checked for failed systemd units.
Ran pluto with 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.

sources/imdsclient/src/lib.rs

jpculp · 2021-12-07T00:12:31Z

Reduced initial token retry duration from 1000ms to 500ms.
Reduced factor from 3 to 1.
Added comments to clarify where timings are coming from.

jpculp · 2021-12-07T00:15:37Z

Fixed incorrect test. (Correct test timeout was reduced from 10s in the initial commit to 2s)

sources/imdsclient/src/lib.rs

jpculp · 2021-12-10T19:55:03Z

In this latest push rather than just using tokio retry and timeout on the token fetch, they are used for fetching IMDS data as well. This introduces things like RwLock to allow for the token to be refreshed within the timeout/retry logic.

In a second commit I've also moved the token fetch out of ImdsClient::new() so that it no longer requires an await, but we can move that to a separate PR if folks are concerned it is out of scope.

sources/imdsclient/src/lib.rs

jpculp · 2021-12-10T23:33:57Z

Adjusted logic to avoid triggering a retry on first run.
Added comment about clone in read_token.
Start backoff with 250ms for even quicker retries.

sources/imdsclient/src/lib.rs

sources/imdsclient/README.md

zmrow · 2021-12-13T16:06:55Z

sources/imdsclient/src/lib.rs

    }

-    async fn new_impl(imds_base_uri: String) -> Result<Self> {
+    fn new_impl(imds_base_uri: String) -> Result<Self> {


We're not allowing the user to override imds_base_uri anywhere, so this method probably doesn't need to take it as an argument.

new_impl is just for unit testing so that we can point the client to the server httptest creates.

sources/imdsclient/src/lib.rs

zmrow · 2021-12-13T16:58:41Z

sources/imdsclient/src/lib.rs

    pub enum Error {
        #[snafu(display("Response '{}' from '{}': {}", get_status_code(source), uri, source))]
        BadResponse { uri: String, source: reqwest::Error },

+        #[snafu(display("Fetched IMDSv2 session token"))]


Should this say something about the token being empty? Or am I reading this wrong?

This was the token being fetched because it was empty, but actually this error isn't used anymore so I've removed it in this latest push.

zmrow · 2021-12-13T17:03:45Z

sources/imdsclient/src/lib.rs

@@ -422,6 +474,9 @@ mod error {
        #[snafu(display("Response was not UTF-8: {}", source))]
        NonUtf8Response { source: std::string::FromUtf8Error },

+        #[snafu(display("IMDSv2 session token was refreshed."))]


This message is a little confusing to me. The error is UnauthorizedTokenRefreshed, but the message says the token was refreshed. Maybe I'm missing some context here.

My original line of thinking was the token was refreshed because the query returned a 401, but I went ahead and renamed this to TokenRefreshed.

zmrow · 2021-12-13T17:05:06Z

sources/imdsclient/src/lib.rs

+        #[snafu(display("Failed to fetch IMDSv2 session token"))]
+        FailedFetchToken,
+
+        #[snafu(display("Failed to read token within ImdsClient"))]


Nit: Since these are "internal" implementation detail errors, let's make the message a bit clearer. Something like "Failed to read token from RwLock..."? (I'm not sold on that wording in particular)

sources/api/early-boot-config/src/provider/aws.rs

To mitigate things like closed connections, IMDSv2 token and data fetches will continue to retry until success or until the timeout has been reached. The default timeout is 300s to match the ifup timeout in wicked, but can be set manually using `.with_timeout` when creating a new ImdsClient.

jpculp · 2021-12-13T22:20:55Z

Replaced Result<Self> with Self in ImdsClient::new() (and added Default for clippy).
Made the 300s retry_timeout a constant (RETRY_TIMEOUT_SECS).
Renamed UnauthorizedTokenRefreshed to TokenRefreshed to be less confusing.
Removed outdated comment about fetching the token on instantiation.
Other small fixes for increased clarity.

sources/imdsclient/src/lib.rs

This moves the token fetch to just before the IMDS data fetch so that building a new ImdsClient no longer needs an await, nor does it return a result. `refresh_token` has been replaced by `clear_token` and `write_token`.

jpculp · 2021-12-14T02:05:45Z

Implemented @bcressey's suggestion to clear the token on 401/Unauthorized.
Fixed some comment tweaks I missed in the previous push.

jpculp requested review from tjkirch, bcressey and cbgbt November 24, 2021 23:33

tjkirch reviewed Nov 25, 2021

View reviewed changes

jpculp force-pushed the imds-token-tokio-retry branch from d26b2bb to 942cb17 Compare December 7, 2021 00:07

jpculp force-pushed the imds-token-tokio-retry branch from 942cb17 to 1b48686 Compare December 7, 2021 00:12

jpculp requested a review from tjkirch December 7, 2021 00:19

tjkirch reviewed Dec 7, 2021

View reviewed changes

sources/imdsclient/src/lib.rs Outdated Show resolved Hide resolved

sources/imdsclient/src/lib.rs Show resolved Hide resolved

tjkirch approved these changes Dec 7, 2021

View reviewed changes

sources/imdsclient/src/lib.rs Outdated Show resolved Hide resolved

jpculp force-pushed the imds-token-tokio-retry branch from 1b48686 to 36e86d5 Compare December 7, 2021 00:55

webern approved these changes Dec 7, 2021

View reviewed changes

sources/imdsclient/src/lib.rs Show resolved Hide resolved

jpculp force-pushed the imds-token-tokio-retry branch from 36e86d5 to b14eab7 Compare December 10, 2021 19:42

jpculp changed the title ~~imdsclient: better session token retries with tokio retry and timeout~~ imdsclient: better retries with tokio retry and timeout Dec 10, 2021

jpculp requested a review from webern December 10, 2021 19:55

jpculp commented Dec 10, 2021

View reviewed changes

sources/imdsclient/src/lib.rs Outdated Show resolved Hide resolved

cbgbt reviewed Dec 10, 2021

View reviewed changes

sources/imdsclient/src/lib.rs Outdated Show resolved Hide resolved

sources/imdsclient/src/lib.rs Show resolved Hide resolved

jpculp force-pushed the imds-token-tokio-retry branch from b14eab7 to 739e999 Compare December 10, 2021 23:30

jpculp requested a review from cbgbt December 10, 2021 23:34

cbgbt reviewed Dec 13, 2021

View reviewed changes

sources/imdsclient/src/lib.rs Outdated Show resolved Hide resolved

zmrow reviewed Dec 13, 2021

View reviewed changes

webern approved these changes Dec 13, 2021

View reviewed changes

jpculp force-pushed the imds-token-tokio-retry branch from 739e999 to c6bc712 Compare December 13, 2021 22:14

jpculp requested review from cbgbt and zmrow December 13, 2021 22:21

jpculp requested a review from webern December 13, 2021 22:21

webern approved these changes Dec 13, 2021

View reviewed changes

cbgbt approved these changes Dec 14, 2021

View reviewed changes

bcressey reviewed Dec 14, 2021

View reviewed changes

sources/imdsclient/src/lib.rs Outdated Show resolved Hide resolved

imdsclient: move token fetch out ImdsClient::new()

5e5200e

This moves the token fetch to just before the IMDS data fetch so that building a new ImdsClient no longer needs an await, nor does it return a result. `refresh_token` has been replaced by `clear_token` and `write_token`.

jpculp force-pushed the imds-token-tokio-retry branch from c6bc712 to 5e5200e Compare December 14, 2021 02:02

jpculp requested a review from bcressey December 14, 2021 02:06

bcressey approved these changes Dec 14, 2021

View reviewed changes

zmrow approved these changes Dec 14, 2021

View reviewed changes

cbgbt approved these changes Dec 15, 2021

View reviewed changes

jpculp merged commit 4db4b5a into bottlerocket-os:develop Dec 15, 2021

jpculp deleted the imds-token-tokio-retry branch December 15, 2021 01:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imdsclient: better retries with tokio retry and timeout #1841

imdsclient: better retries with tokio retry and timeout #1841

jpculp commented Nov 24, 2021 •

edited

Loading

jpculp commented Dec 7, 2021

jpculp commented Dec 7, 2021

jpculp commented Dec 10, 2021

jpculp commented Dec 10, 2021

zmrow Dec 13, 2021

jpculp Dec 13, 2021

zmrow Dec 13, 2021

jpculp Dec 13, 2021

zmrow Dec 13, 2021

jpculp Dec 13, 2021

zmrow Dec 13, 2021

jpculp commented Dec 13, 2021

jpculp commented Dec 14, 2021

imdsclient: better retries with tokio retry and timeout #1841

imdsclient: better retries with tokio retry and timeout #1841

Conversation

jpculp commented Nov 24, 2021 • edited Loading

jpculp commented Dec 7, 2021

jpculp commented Dec 7, 2021

jpculp commented Dec 10, 2021

jpculp commented Dec 10, 2021

zmrow Dec 13, 2021

Choose a reason for hiding this comment

jpculp Dec 13, 2021

Choose a reason for hiding this comment

zmrow Dec 13, 2021

Choose a reason for hiding this comment

jpculp Dec 13, 2021

Choose a reason for hiding this comment

zmrow Dec 13, 2021

Choose a reason for hiding this comment

jpculp Dec 13, 2021

Choose a reason for hiding this comment

zmrow Dec 13, 2021

Choose a reason for hiding this comment

jpculp commented Dec 13, 2021

jpculp commented Dec 14, 2021

jpculp commented Nov 24, 2021 •

edited

Loading