Add imdsclient library for querying IMDS #1372

jpculp · 2021-03-09T00:51:18Z

Issue number:

N/A

Description of changes:

This adds an imdsclient library for tools and setting generators to leverage for fetching user-data, meta-data, and dynamic data from the AWS Instance Metadata Service.

shibaken was also updated by removing the stand-alone IMDS logic in favor of the shared imdsclient library.
Some of the log messages in shibaken were also tweaked for more clarity.

pluto and early-boot-config have also been updated by removing the stand-alone IMDS logic in favor of the shared imdsclient library.

Packages that need to be updated:

shibaken
pluto
early-boot-config

Issues:

Add testing (Add tests to imdsclient #1398)

Testing done:

Built aws-k8s-1.19 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.

Also built and launched vmware-k8s-1.20 (x86_64) and aws-ecs-1 (aarch64).
Verified that they booted correctly and connected to their respective clusters.

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.

jpculp · 2021-03-09T01:14:01Z

Seems like the examples in the Doc strings aren't tight enough to pass Doc-tests. Will post another revision.

sources/imdsclient/src/lib.rs

jpculp · 2021-03-09T01:45:47Z

sources/api/shibaken/src/main.rs

@@ -191,7 +158,7 @@ fn parse_args(args: env::Args) -> Result<Args> {
    }

    Ok(Args {
-        log_level: log_level.unwrap_or_else(|| LevelFilter::Info),
+        log_level: log_level.unwrap_or(LevelFilter::Info),


Since sundog only prints stderr in the event of an error, would it make sense for me to change default log level to Debug? It would make it easier to troubleshoot, but might be somewhat ugly if someone ran shibaken in a shell.

Do we not have enough information in error/warn/info to get a sense of the problem when it fails? It seems like we should be adding information there, not just promoting all of the debug messages, if you think there's not enough to understand the problem.

It's kind of an edge-case, but the current IMDS logic in Bottlerocket treats a 404 as an Ok(None) and that's what we want 99% of the time. However, in shibaken we decided to actually error out if it receives a 404 when it is collecting the data from the individual keys in IMDS. Since the imdsclient isn't returning an error on 404, some context is lost.

At an error level shibaken would return something like:

Error retrieving 'public-keys/1/openssh-key' from IMDS metadata

but the debug message would show the full uri like this:

(2) reqwest::async_impl::client: response '404 Not Found' for http://169.254.169.254/2020-10-27/meta-data/public-keys/1/openssh-key

I can update the error to say 404 for clarity, but I'm not quite sure if there is a simple way to pass the full uri. One option is to re-construct the uri via public constants, but I'm not a fan of that since it's not a guarantee that that was the uri that was reqwest'd

I don't think the full URI is super helpful, since there's only one thing it could possibly be, given you know the version you're running. And this case could seemingly only crop up if EC2 listed erroneous keys, or if the user changed the registered keys after the list was retrieved. I don't think it's possible to change the keys associated in EC2 while an instance is running, so I don't expect that particular issue to come up. The error seems fine to me...

I think the error message Error retrieving 'public-keys/1/openssh-key' from IMDS metadata is clear enough. If you really do have a reproducible case where you need to know whether it was a 404 or something else, then you are really in the weeds anyway and it seems reasonable to expect the user/developer to turn on debugging for something that deep.

zmrow

Posting what I have for now, will be back with a few more!

sources/imdsclient/build.rs

sources/imdsclient/README.md

sources/imdsclient/src/lib.rs

webern

Can tests be written with a mock server? You would need another constructor (which could be private) that takes the URLs. If this constructor were public, you could also use it in your doc example for a passing doc test. You could then implement Default for the real clients to use.

sources/imdsclient/src/lib.rs

jpculp · 2021-03-12T02:48:19Z

Removed examples from docstrings.
Made the ImdsClient struct mutable.
Added refresh_token function. When receiving a 401 response from IMDS, the token will be refreshed, and the IMDS fetch be re-attempted 1 time.
Tweaked README wording.
Removed extraneous generate_constants from build.rs

sources/imdsclient/src/lib.rs

jpculp · 2021-03-13T00:58:46Z

Add docstring to refresh_token
Tweaked ImdsData error in shibaken to be more visually consistent with ImdsClient error.

webern · 2021-03-15T20:29:09Z

sources/api/shibaken/src/main.rs

@@ -191,7 +158,7 @@ fn parse_args(args: env::Args) -> Result<Args> {
    }

    Ok(Args {
-        log_level: log_level.unwrap_or_else(|| LevelFilter::Info),
+        log_level: log_level.unwrap_or(LevelFilter::Info),


I think the error message Error retrieving 'public-keys/1/openssh-key' from IMDS metadata is clear enough. If you really do have a reproducible case where you need to know whether it was a 404 or something else, then you are really in the weeds anyway and it seems reasonable to expect the user/developer to turn on debugging for something that deep.

sources/imdsclient/src/lib.rs

etungsten

LGTM assuming all the existing comments are addressed.

sources/api/shibaken/src/main.rs

zmrow · 2021-03-15T23:10:32Z

sources/imdsclient/src/lib.rs

+    }
+
+    /// Helper to fetch `meta-data` targets from IMDS, preferring an override file if present.
+    pub fn fetch_metadata(


I'm a bit worried about this API since we're not handling these strings as URLs and we're not doing any handling of slashes, etc. Ideally we'd at least try to parse the string as a URL so we at least know it's valid.

I was going to rely on the str -> Url parse in Reqwest's get function, but it probably makes sense to parse the uri just before the loop.

sources/imdsclient/src/lib.rs

jpculp · 2021-03-16T18:37:11Z

Can tests be written with a mock server? You would need another constructor (which could be private) that takes the URLs. If this constructor were public, you could also use it in your doc example for a passing doc test. You could then implement Default for the real clients to use. -@webern

I've gone ahead and added an issue (#1398) for this so that we can revisit it at a later date.

jpculp · 2021-03-16T23:13:20Z

Replaced &str with AsRef<str> for public functions
Changed attempt from u16 to u8
Created max_attempts variable
Changed session token expired message from warn to info
Tweaked a description string in shibaken for a clearer log message.

zmrow

🔑

jpculp · 2021-05-07T21:28:59Z

Fixed and tested vmware and aws-ecs builds.

jpculp · 2021-05-12T16:11:40Z

Added tests to imdsclient.

jpculp · 2021-05-17T21:52:50Z

In collaboration with @webern, moved IMDS functions from shibaken, pluto, and early-boot-config, and replaced them with more abstracted functions like fetch_mac_addresses.
Replaced fetch_metadata / fetch_dynamic with fetch_string / fetch_bytes.
Made refresh_token private.
Removed requirement to always add a description value.
@webern added some additional tests for printable strings and parsing public keys.

webern

Nice refactor with new test code. 👍

sources/api/pluto/src/main.rs

sources/imdsclient/src/lib.rs

jpculp · 2021-05-20T16:51:58Z

Tweaked the two error messages from @webern's feedback.

sources/Cargo.lock

sources/imdsclient/src/lib.rs

jpculp · 2021-05-20T23:33:18Z

imdsclient

Changed number of retries.
Added 100ms delay between retries.
Added logic to retry on request timeouts (instead of just expired tokens).

This adds an imdsclient library for tools and setting generators to leverage for fetching user-data, meta-data, and dynamic data from the AWS Instance Metadata Service.

This removes the stand-alone IMDS logic in favor of the shared imdsclient library. Log messages were also tweaked for more clarity.

This removes the stand-alone IMDS logic in favor of the shared imdsclient library.

jpculp · 2021-05-21T19:03:56Z

imdsclient

Removed extraneous semicolon.

jpculp requested review from tjkirch, bcressey, zmrow and etungsten March 9, 2021 00:51

jpculp force-pushed the imdsclient-lib branch from 0418314 to c6e02ef Compare March 9, 2021 01:41

jpculp commented Mar 9, 2021

View reviewed changes

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

jpculp commented Mar 9, 2021

View reviewed changes

jpculp marked this pull request as draft March 9, 2021 01:53

zmrow reviewed Mar 9, 2021

View reviewed changes

webern reviewed Mar 9, 2021

View reviewed changes

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

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

jpculp force-pushed the imdsclient-lib branch from c6e02ef to 36546fa Compare March 12, 2021 02:40

jpculp commented Mar 12, 2021

View reviewed changes

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

jpculp force-pushed the imdsclient-lib branch from 36546fa to 69dad3c Compare March 13, 2021 00:56

jpculp marked this pull request as ready for review March 13, 2021 00:59

jpculp requested review from zmrow and webern March 13, 2021 00:59

webern reviewed Mar 15, 2021

View reviewed changes

etungsten approved these changes Mar 15, 2021

View reviewed changes

zmrow reviewed Mar 15, 2021

View reviewed changes

jpculp mentioned this pull request Mar 16, 2021

Add tests to imdsclient #1398

Closed

jpculp force-pushed the imdsclient-lib branch from 69dad3c to 7cc89b7 Compare March 16, 2021 23:09

jpculp requested review from zmrow and webern March 16, 2021 23:13

webern approved these changes Mar 17, 2021

View reviewed changes

zmrow approved these changes Mar 17, 2021

View reviewed changes

jpculp force-pushed the imdsclient-lib branch from a6af412 to ffb7270 Compare May 7, 2021 21:27

jpculp requested review from zmrow and etungsten May 7, 2021 21:48

jpculp force-pushed the imdsclient-lib branch from ffb7270 to b9faab9 Compare May 12, 2021 16:09

jpculp force-pushed the imdsclient-lib branch from b9faab9 to c3c4a2a Compare May 17, 2021 21:43

jpculp marked this pull request as ready for review May 17, 2021 22:05

webern approved these changes May 18, 2021

View reviewed changes

sources/api/pluto/src/main.rs Outdated Show resolved Hide resolved

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

etungsten approved these changes May 19, 2021

View reviewed changes

jpculp force-pushed the imdsclient-lib branch from c3c4a2a to 2c25ac0 Compare May 20, 2021 16:49

webern approved these changes May 20, 2021

View reviewed changes

bcressey reviewed May 20, 2021

View reviewed changes

sources/Cargo.lock Show resolved Hide resolved

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

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

jpculp force-pushed the imdsclient-lib branch from 2c25ac0 to 3459081 Compare May 20, 2021 23:30

jpculp requested a review from bcressey May 20, 2021 23:33

jpculp mentioned this pull request May 20, 2021

Conditionalize shibaken to not build for non-AWS variants #1590

Closed

webern approved these changes May 21, 2021

View reviewed changes

jpculp added 4 commits May 21, 2021 18:41

Add imdsclient library for querying IMDS

f9e755c

This adds an imdsclient library for tools and setting generators to leverage for fetching user-data, meta-data, and dynamic data from the AWS Instance Metadata Service.

shibaken: update to use imdsclient library

dfb7024

This removes the stand-alone IMDS logic in favor of the shared imdsclient library. Log messages were also tweaked for more clarity.

pluto: update to use imdsclient library

b0282c9

This removes the stand-alone IMDS logic in favor of the shared imdsclient library.

early-boot-config: update to use imdsclient library

2770773

This removes the stand-alone IMDS logic in favor of the shared imdsclient library.

jpculp force-pushed the imdsclient-lib branch from 3459081 to 2770773 Compare May 21, 2021 19:02

webern approved these changes May 21, 2021

View reviewed changes

bcressey approved these changes May 21, 2021

View reviewed changes

jpculp merged commit b25435d into bottlerocket-os:develop May 21, 2021

jpculp deleted the imdsclient-lib branch May 21, 2021 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add imdsclient library for querying IMDS #1372

Add imdsclient library for querying IMDS #1372

jpculp commented Mar 9, 2021 •

edited

Loading

jpculp commented Mar 9, 2021

jpculp Mar 9, 2021

tjkirch Mar 10, 2021

jpculp Mar 12, 2021

tjkirch Mar 12, 2021

webern Mar 15, 2021 •

edited

Loading

zmrow left a comment

webern left a comment •

edited

Loading

jpculp commented Mar 12, 2021

jpculp commented Mar 13, 2021

webern Mar 15, 2021 •

edited

Loading

etungsten left a comment

zmrow Mar 15, 2021

jpculp Mar 16, 2021

jpculp commented Mar 16, 2021

jpculp commented Mar 16, 2021

zmrow left a comment

jpculp commented May 7, 2021

jpculp commented May 12, 2021

jpculp commented May 17, 2021

webern left a comment

jpculp commented May 20, 2021

jpculp commented May 20, 2021

jpculp commented May 21, 2021

Add imdsclient library for querying IMDS #1372

Add imdsclient library for querying IMDS #1372

Conversation

jpculp commented Mar 9, 2021 • edited Loading

jpculp commented Mar 9, 2021

jpculp Mar 9, 2021

Choose a reason for hiding this comment

tjkirch Mar 10, 2021

Choose a reason for hiding this comment

jpculp Mar 12, 2021

Choose a reason for hiding this comment

tjkirch Mar 12, 2021

Choose a reason for hiding this comment

webern Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

zmrow left a comment

Choose a reason for hiding this comment

webern left a comment • edited Loading

Choose a reason for hiding this comment

jpculp commented Mar 12, 2021

jpculp commented Mar 13, 2021

webern Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

etungsten left a comment

Choose a reason for hiding this comment

zmrow Mar 15, 2021

Choose a reason for hiding this comment

jpculp Mar 16, 2021

Choose a reason for hiding this comment

jpculp commented Mar 16, 2021

jpculp commented Mar 16, 2021

zmrow left a comment

Choose a reason for hiding this comment

jpculp commented May 7, 2021

jpculp commented May 12, 2021

jpculp commented May 17, 2021

webern left a comment

Choose a reason for hiding this comment

jpculp commented May 20, 2021

jpculp commented May 20, 2021

jpculp commented May 21, 2021

jpculp commented Mar 9, 2021 •

edited

Loading

webern Mar 15, 2021 •

edited

Loading

webern left a comment •

edited

Loading

webern Mar 15, 2021 •

edited

Loading