feat(sdk)!: typed VmId replaces raw String for process builders#5
Open
feat(sdk)!: typed VmId replaces raw String for process builders#5
Conversation
Firecracker's --id flag panics on any character outside `c == '-' || c.is_alphanumeric()` or byte length outside 1..=64, per utils::validators::validate_instance_id. Callers previously had to sanitize on their own, and a bad id turned into a child process abort rather than a recoverable error. Add a VmId newtype that encodes that exact rule at the SDK boundary. Two constructors: `new` (strict, Result<_, VmIdError>) and `from_sanitized` (infallible projection, always valid). FirecrackerProcessBuilder::id, JailerProcessBuilder::new, and the bundled runtime's jailer_builder now take VmId. fc-cli surfaces VmIdError to the user via invalid_input. Workspace version bumped to 0.3.0 to reflect the breaking change.
There was a problem hiding this comment.
Pull request overview
This PR introduces a typed VmId into fc-sdk to enforce Firecracker’s --id validation rules in the SDK, preventing child-process panics from invalid identifiers and making ID handling explicit across builders and the CLI.
Changes:
- Added
fc_sdk::VmId+VmIdErrorwith strict validation (new) and deterministic sanitization (from_sanitized). - Updated process builders (Firecracker/Jailer) and bundled runtime options to take
VmIdinstead ofString(breaking API change). - Updated
fc-clito validate--idinput and surface validation errors cleanly.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/runtime/bundled.rs |
Updates bundled jailer builder API to require VmId and adjusts tests accordingly. |
fc-sdk/src/vm_id.rs |
Adds the new VmId type, error model, sanitization/validation logic, and unit tests. |
fc-sdk/src/process.rs |
Switches builder APIs/fields from String IDs to VmId and updates docs/tests. |
fc-sdk/src/lib.rs |
Exposes the new vm_id module and re-exports VmId/VmIdError. |
fc-sdk/src/error.rs |
Adds InvalidVmId variant and From<VmIdError> conversion for ergonomic ? usage. |
fc-cli/src/main.rs |
Validates CLI-provided IDs via VmId::new and returns InvalidInput errors on failure. |
Cargo.toml |
Bumps workspace version to 0.3.0 to reflect the breaking API change. |
Cargo.lock |
Updates workspace crate versions to 0.3.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+101
to
+113
| let sanitized: String = input | ||
| .as_ref() | ||
| .chars() | ||
| .map(|c| if is_valid_char(c) { c } else { '-' }) | ||
| .collect(); | ||
|
|
||
| let truncated = truncate_to_bytes(&sanitized, MAX_LEN); | ||
| let final_id = if truncated.is_empty() { | ||
| SANITIZE_FALLBACK.to_owned() | ||
| } else { | ||
| truncated.to_owned() | ||
| }; | ||
|
|
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Firecracker's
--idflag is validated againstutils::validators::validate_instance_id(byte length 1..=64, chars matchingc == '-' || c.is_alphanumeric()). Passing a bad id makes the child process panic at startup — callers previously had no way to catch that ahead of time and had to roll their own sanitizer.This PR promotes that rule into the SDK as a typed
VmId, so callers either validate upfront or get a Firecracker-compatible identifier by construction.fc_sdk::VmIdinfc-sdk/src/vm_id.rswith two constructors:VmId::new(s) -> Result<VmId, VmIdError>— strict, error variants mirror upstream'sValidatorError(InvalidChar { c, position },InvalidLen { length, min, max }) so messages line up with what a user would see from the Firecracker binary itself.VmId::from_sanitized(s) -> VmId— infallible projection. Non-(alphanumeric-or-hyphen) chars become-, result truncated to 64 bytes at a char boundary, empty result falls back to"vm". Deterministic.VmId(breaking):FirecrackerProcessBuilder::id,JailerProcessBuilder::new, and the bundledBundledRuntimeOptions::jailer_builderall consume a pre-validatedVmId.fc-clisurfacesVmIdErrorthroughinvalid_inputso the user's CLI input is validated and reported cleanly.Test plan
cargo test -p fc-sdk— 16 unit tests (allvm_idscenarios incl. multi-byte UTF-8 truncation) + 7 doc-tests pass.cargo test --workspace— all member crates green.cargo clippy --workspace --all-targets -- -D warnings— clean.cargo fmt --check— clean.inst_<uuid>-style identifier with an underscore passed to.id(VmId::from_sanitized(...))produces a live firecracker process (--id inst-<uuid>), noInvalidCharpanic, API reportsRunning.