early-boot-config: Add guestinfo to available VMWare user data retrieval methods #1393

zmrow · 2021-03-15T22:53:13Z

Issue number:
Closes #1369

Description of changes:
The changes aren't vast here, but it's probably easiest to read the commits separately.

    early-boot-config: Rename CdRomDataProvider to VmwareDataProvider
    
    Given that the CdRomDataProvider contained VMware-specific logic and
    more will continue to be added to it, naming it as such makes it clearer
    to the caller and still allows for the underlying logic to be factored
    out in the future should we deem necessary.

    early-boot-config: Add `from_toml_str` to SettingsJson
    
    This adds a method to SettingsJson that takes in a string (expected to
    be TOML data), and attempts to parse it, remove the "settings" layer,
    and create a `SettingsJson` from it.  This logic was previously
    sprinkled around in a few places, creating multiple identical error
    variants, etc.

    early-boot-config: Add guestinfo to available VMWare retrieval methods
    
    This change adds an additional user data source to the VMWare provider;
    the guestinfo (host-guest) interface.  This source will be checked
    after the CD-ROM, and override any settings set via CD-ROM.

I do think that there's some refactoring to be done in this program, namely to separate "platform" from "data provider", but that will be done in another PR.

Testing done:

Boot an aws-k8s-1.17 AMI to ensure I didn't break anything over there
Boot a vmware-dev image with a CD-ROM attached containing an ovf-env.xml file w/ user data
Boot a vmware-dev image with a CD-ROM attached containing a TOML user-data file
In vSphere with guestinfo:
- Traverse the entire matrix of encodings base64, b64, B64, Base64 and gz+b64, Gz+B64, gzip+base64, and Gzip+Base64 with user data set and unset
- Ensure that raw TOML user data passed via guestinfo still works
- Ensure that settings passed via guestinfo override setting from an attached CD-ROM

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.

zmrow · 2021-03-15T23:02:24Z

^ Fixed up a failing test I missed... :)

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

sources/api/early-boot-config/Cargo.toml

zmrow · 2021-03-18T20:43:56Z

^ adds the platform dependency to Cargo.toml in early-boot-config. CI should all pass this time.

zmrow · 2021-03-19T21:39:19Z

^ un-nested the backdoor_get_bytes function per @webern

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

tjkirch · 2021-03-23T23:25:52Z

sources/api/early-boot-config/Cargo.toml

@@ -26,6 +27,10 @@ tokio = { version = "0.2", default-features = false, features = ["macros", "rt-t
 #tokio = { version = "0.3", default-features = false, features = ["macros", "rt-multi-thread"] }
 toml = "0.5"

+[target.'cfg(target_arch = "x86_64")'.dependencies]


So if someone is doing a cargo build (so, separate from #1404 solutions), what happens here? This dependency here would be skipped, but the code still references vmw_backdoor::bla, so I think it'd have compile errors about unknown references, which could be pretty confusing. ARM developer machines are getting more common now, and vmware-dev is our first "local" variant, so I don't think this is "weird" enough to ignore. Do we need to conditionalize the VmwareDataProvider on target_arch?

With #1431 in, this is a lot less important. I'm assuming we'd still have undefined references if cargo building this on a non-x86 host, so It'd be nice to do something to clean that up and make it clearer that it's not supported, but it's not as big an issue now since we'd expect the person to use cargo make to handle cross-compilation automatically.

tjkirch · 2021-03-23T23:29:34Z

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

            Ok(Some(s)) => output.push(s),
        }
+
+        // If we didn't find any user data in guestinfo, try CD-ROM


I'm curious what others think here. If I specified user data in two locations, I'd expect both of them to be obeyed (in some order, but that's a different problem) rather than one taking precedence and ignoring the other. Perhaps I have a common user data CD that I use in all launches, and then I use guestinfo for more specific settings in some launches, or whatever.

This seems good now, but I'm wondering where you think we should document this?

Creating an issue for that now. We can probably comment on it here, but I the "main" documentation should happen in the variant's top level README unless you think it belongs somewhere else.

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

sources/api/early-boot-config/Cargo.toml

Given that the CdRomDataProvider contained VMware-specific logic and more will continue to be added to it, naming it as such makes it clearer to the caller and still allows for the underlying logic to be factored out in the future should we deem necessary.

This adds a method to SettingsJson that takes in a string (expected to be TOML data), and attempts to parse it, remove the "settings" layer, and create a `SettingsJson` from it. This logic was previously sprinkled around in a few places, creating multiple identical error variants, etc.

This change adds an additional user data source to the VMWare provider; the guestinfo (host-guest) interface. This source will be checked after the CD-ROM, and override any settings set via CD-ROM.

zmrow · 2021-04-02T22:54:12Z

^ Addressed all the comments and updated the testing!

tjkirch · 2021-04-05T18:36:23Z

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

+        })?;
+
+        // Decompress the data if it's compressed
+        let decoded = expand_slice_maybe(&decoded_bytes).context(error::Decompression {


Here's another spot we could chain the OptionalCompressionReader and DecoderReader.

One note though - the error is no longer strictly related to decompression - it could fail base64 too - so I think we'd want a new variant that's a little more generic, maybe UserDataInput or something?

tjkirch · 2021-04-05T18:42:27Z

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

+            );
+        }
+
+        let json = SettingsJson::from_toml_str(&user_data_str, "user data from CD-ROM").context(


Suggested change

let json = SettingsJson::from_toml_str(&user_data_str, "user data from CD-ROM").context(

let json = SettingsJson::from_toml_str(&user_data_str, "user data from CD-ROM").with_context(||

nit: so we don't do the string conversion unnecessarily.

zmrow requested review from tjkirch, bcressey and webern March 15, 2021 22:53

zmrow force-pushed the guestinfo branch from 638cf61 to 3bd9312 Compare March 15, 2021 23:02

webern reviewed Mar 16, 2021

View reviewed changes

sources/api/early-boot-config/src/provider/vmware.rs Outdated Show resolved Hide resolved

tjkirch reviewed Mar 16, 2021

View reviewed changes

sources/api/early-boot-config/Cargo.toml Show resolved Hide resolved

zmrow force-pushed the guestinfo branch from 3bd9312 to faaa83d Compare March 18, 2021 20:42

zmrow force-pushed the guestinfo branch from faaa83d to 5f8b481 Compare March 19, 2021 21:38

webern approved these changes Mar 19, 2021

View reviewed changes

bcressey reviewed Mar 22, 2021

View reviewed changes

sources/api/early-boot-config/src/provider/vmware.rs Outdated Show resolved Hide resolved

sources/api/early-boot-config/src/provider/vmware.rs Show resolved Hide resolved

sources/api/early-boot-config/src/provider/vmware.rs Outdated Show resolved Hide resolved

tjkirch reviewed Mar 23, 2021

View reviewed changes

zmrow mentioned this pull request Mar 30, 2021

Add support for specifying a variant's supported architectures #1431

Merged

zmrow added 2 commits April 2, 2021 16:55

zmrow force-pushed the guestinfo branch from 5f8b481 to c8d7310 Compare April 2, 2021 22:39

early-boot-config: Add guestinfo to available VMWare retrieval methods

41aa3eb

This change adds an additional user data source to the VMWare provider; the guestinfo (host-guest) interface. This source will be checked after the CD-ROM, and override any settings set via CD-ROM.

zmrow force-pushed the guestinfo branch from c8d7310 to 41aa3eb Compare April 2, 2021 22:51

tjkirch approved these changes Apr 5, 2021

View reviewed changes

bcressey approved these changes Apr 5, 2021

View reviewed changes

zmrow merged commit 2fad46b into bottlerocket-os:develop Apr 5, 2021

zmrow deleted the guestinfo branch April 5, 2021 22:27

zmrow mentioned this pull request Apr 5, 2021

Variant documentation: Document the ordering of user data sources #1456

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

early-boot-config: Add guestinfo to available VMWare user data retrieval methods #1393

early-boot-config: Add guestinfo to available VMWare user data retrieval methods #1393

zmrow commented Mar 15, 2021 •

edited

Loading

zmrow commented Mar 15, 2021

zmrow commented Mar 18, 2021

zmrow commented Mar 19, 2021

tjkirch Mar 23, 2021

tjkirch Apr 5, 2021

tjkirch Mar 23, 2021

tjkirch Apr 5, 2021

zmrow Apr 5, 2021

zmrow commented Apr 2, 2021

tjkirch Apr 5, 2021

tjkirch Apr 5, 2021

	let json = SettingsJson::from_toml_str(&user_data_str, "user data from CD-ROM").context(
	let json = SettingsJson::from_toml_str(&user_data_str, "user data from CD-ROM").with_context(\|\|

early-boot-config: Add guestinfo to available VMWare user data retrieval methods #1393

early-boot-config: Add guestinfo to available VMWare user data retrieval methods #1393

Conversation

zmrow commented Mar 15, 2021 • edited Loading

zmrow commented Mar 15, 2021

zmrow commented Mar 18, 2021

zmrow commented Mar 19, 2021

tjkirch Mar 23, 2021

Choose a reason for hiding this comment

tjkirch Apr 5, 2021

Choose a reason for hiding this comment

tjkirch Mar 23, 2021

Choose a reason for hiding this comment

tjkirch Apr 5, 2021

Choose a reason for hiding this comment

zmrow Apr 5, 2021

Choose a reason for hiding this comment

zmrow commented Apr 2, 2021

tjkirch Apr 5, 2021

Choose a reason for hiding this comment

tjkirch Apr 5, 2021

Choose a reason for hiding this comment

zmrow commented Mar 15, 2021 •

edited

Loading