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

Add conditional compilation to early-boot-config #1288

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

zmrow
Copy link
Contributor

@zmrow zmrow commented Jan 21, 2021

Issue number:
Related to #1218

Description of changes:
This change adds a simple solution for conditionally compiling
early-boot-config based on the current variant. As part of the change,
local file handling (which was previously broken) is split into its own
trait implementation and only compiled into the program for "aws-dev"
variants. We also add the ability to read a mounted cdrom for user
data.

Most of the code in main.rs has been moved to modules, but the logic is still very largely the same.

A separate PR will contain the relevant mount units to correctly mount a CD-ROM.

Testing done:

  • Built and ran the aws-dev variant both with and without local files. The system boots and correctly uses local files if they exist.
  • Build the aws-k8s-1.17 variant and made sure it correctly fetches user data from IMDS on a new instance.
  • Built and ran a fake variant (based on aws-dev) locally to test the cd-rom user data capability. Both xml and user-data files worked as expected.

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
Copy link
Contributor Author

zmrow commented Jan 22, 2021

⏫ Fix the conflicts with the base branch

@zmrow
Copy link
Contributor Author

zmrow commented Jan 26, 2021

The above force push should address outstanding comments.

The tl;dr is: "removed any odd conditional compilation in error handling."

After multiple offline conversations I think there's a much better solution in place for error handling here. The PlatformDataProvider trait was changed to return Box<std::error::Error which eliminates the error type that was previously defined in that module. Doing so eliminates all of the conditional compilation that was previously converting different provider's errors to a single error type so main.rs could handle it. Now main has a single error variant with a source of Box<std::error::Error>. Boom! Roasted!

@tjkirch brought up the valid point that we should probably separate "platform" from "data provider", and let the platform (AWS, VMWare, etc) choose the provider (local file, IMDS, etc.). I'll open an issue separately; it shouldn't block this PR.

sources/api/early-boot-config/src/provider.rs Outdated Show resolved Hide resolved
@zmrow
Copy link
Contributor Author

zmrow commented Jan 26, 2021

^ Address @etungsten 's comment

@webern
Copy link
Contributor

webern commented Jan 26, 2021

trait was changed to return Box<std::error::Error which eliminates the error type that was previously defined in that module.

Yes, I agree that is fine. The wrapper that I wrote basically amounts to implementing Error for Box<dyn Error>. That would probably be nicer if a this were a library to be used externally, but here we don't care that our error type isn't an Error.

@zmrow zmrow mentioned this pull request Jan 27, 2021
Comment on lines 30 to 45
// Build a list of user data files that exist
let mut user_data_files = Self::USER_DATA_FILENAMES
.iter()
.map(|filename| Path::new(Self::CD_ROM_MOUNT).join(filename))
.filter(|file| file.exists());

// If no files exist, return
let user_data_file = match user_data_files.next() {
Some(file) => file,
None => return Ok(None),
};

// There should only be 1 file, if more exist something is wonky
ensure!(
user_data_files.next().is_none(),
error::UserDataFileCount {
location: Self::CD_ROM_MOUNT
}
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for me, all three of these comments are about "what" which is pretty obvious. A "why" comment could be helpful for building a list of the files, since it's not obvious why there wouldn't be a canonical file name.

sources/api/early-boot-config/src/provider/cdrom.rs Outdated Show resolved Hide resolved
sources/api/early-boot-config/src/provider/cdrom.rs Outdated Show resolved Hide resolved
sources/api/early-boot-config/src/provider/cdrom.rs Outdated Show resolved Hide resolved
sources/api/early-boot-config/src/provider/cdrom.rs Outdated Show resolved Hide resolved
This change adds a simple solution for conditionally compiling
early-boot-config based on the current variant.  As part of the change,
local file handling (which was previously broken) is split into its own
trait implementation and only compiled into the program for "aws-dev"
variants.  We also add the ability to read a mounted cdrom for user
data.
@zmrow
Copy link
Contributor Author

zmrow commented Jan 28, 2021

^ Addressed @bcressey 's comments

@zmrow zmrow merged commit fdadb17 into bottlerocket-os:develop Jan 29, 2021
@zmrow zmrow deleted the ebc_refactor branch January 29, 2021 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants