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

feat: added selection of entropy crate #3776

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ShadowCurse
Copy link
Contributor

@ShadowCurse ShadowCurse commented Jun 2, 2023

Changes

Added ability to select between rand and
aws-lc-rs crates for entropy device.

If build with default command: cargo build everything will be build with aws-lc-sys.
If build with cargo build --no-default-features --features rand everything will be build with rand.

Reason

aws-lc-rs crate requires musl compatible c++ compiler which is not an easily obtainable dependency
Without the musl c++ toolchain compiling Firecracker gives the error:

  CMake Error at CMakeLists.txt:9 (enable_language):
    The CMAKE_C_COMPILER:

      musl-gcc

    is not a full path and was not found in the PATH.

    Tell CMake where to find the compiler by setting either the environment
    variable "CC" or the CMake cache entry CMAKE_C_COMPILER to the full path to
    the compiler, or to the compiler name if it is in the PATH.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet contribution quality standards.

  • This functionality cannot be added in rust-vmm.

@ShadowCurse ShadowCurse added the Type: Enhancement Indicates new feature requests label Jun 2, 2023
@ShadowCurse ShadowCurse self-assigned this Jun 2, 2023
@ShadowCurse ShadowCurse force-pushed the optional_rand_feature branch from 318f08a to 9634fb3 Compare June 2, 2023 15:28
Comment on lines 8 to 14
[features]
default = []
aws-lc-rs = ["dep:aws-lc-rs"]
rand = ["dep:rand"]
Copy link
Contributor

@JonathanWoollett-Light JonathanWoollett-Light Jun 2, 2023

Choose a reason for hiding this comment

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

One of aws-lc-rs or rand is required. Currently if someone doesn't configure this properly they will get a non-obvious error.

You can add a build.rs like:

fn main() {
    #[cfg(any(
        all(feature="aws-lc-rs", feature="rand"), // If both are enabled
        not(any(feature="aws-lc-rs", feature="rand")) // If neither are enabled
    )]
    compile_error!("Please enable the feature \"aws-lc-rs\" OR the feature \"rand\".");
}

This should also be added to firecracker and devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can add this. The problem though isn't that no features are enabled, but opposite. For some reason both of them are enabled at the same time and this causes problems.

@roypat
Copy link
Contributor

roypat commented Jun 5, 2023

I thought hat aws-lc-rs published a version that doesn't require the musl g++ chain 🤔

@ShadowCurse ShadowCurse force-pushed the optional_rand_feature branch 6 times, most recently from 13af016 to 5a94c57 Compare June 9, 2023 15:26
Added ability to select between `rand` and
`aws-lc-rs` crates for entropy device.

Signed-off-by: Egor Lazarchuk <[email protected]>
@ShadowCurse ShadowCurse force-pushed the optional_rand_feature branch from 5a94c57 to 9b7a1a3 Compare June 9, 2023 15:27
@bchalios
Copy link
Contributor

Why do we need rand?

@ShadowCurse
Copy link
Contributor Author

@bchalios Basically with rand you can compile firecracker only using rust tool chain. It makes development easier, because it can eliminate container requirement.

@bchalios
Copy link
Contributor

@bchalios Basically with rand you can compile firecracker only using rust tool chain. It makes development easier, because it can eliminate container requirement.

You should be able to do that with aws-lc-rs as well. Your only extra dependency is cmake.

@dm0-
Copy link
Contributor

dm0- commented Jun 22, 2023

I'd also like to have this feature to allow something other than aws-lc, although I didn't have any issue building it without musl. From the side of distro packaging, at least Fedora requires crypto libraries undergo additional review, and it might also require unbundling the C++ library from the Rust crate. Since rand looks like it's the only functionality being used, avoiding packaging yet another SSL library would greatly simplify everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants