From 7451c6dbdc31e03b073f035fba7f971d217fdd3a Mon Sep 17 00:00:00 2001 From: Chet Nichols III Date: Thu, 23 Jul 2026 05:01:14 -0700 Subject: [PATCH] test: consolidate firmware artifact resolution cases Make the shared resolver table the owner of files[] selection, location precedence, and error coverage. Keep machine-controller coverage focused on adapter behavior and Scout path handling, then add whitespace normalization cases at the shared boundary. This supports https://github.com/NVIDIA/infra-controller/issues/3974 Signed-off-by: Chet Nichols III --- crates/firmware/src/artifact_resolution.rs | 269 +++++++---- .../src/handler/firmware_artifact.rs | 454 ++++++------------ 2 files changed, 337 insertions(+), 386 deletions(-) diff --git a/crates/firmware/src/artifact_resolution.rs b/crates/firmware/src/artifact_resolution.rs index 8983f53c5a..84ab076073 100644 --- a/crates/firmware/src/artifact_resolution.rs +++ b/crates/firmware/src/artifact_resolution.rs @@ -95,99 +95,175 @@ pub fn resolve_files_firmware_artifact( #[cfg(test)] mod tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, check_cases}; use model::firmware::FirmwareFileArtifact; use super::*; - #[test] - fn resolve_files_firmware_artifact_uses_url_as_remote_source() { - let firmware_cache_directory = Path::new("/mnt/persistence/fw/download-cache"); - let firmware = firmware_with_files(vec![FirmwareFileArtifact { - filename: None, - url: Some("https://firmware.example.invalid/path/fw.bin".to_string()), - sha256: "abc123".to_string(), - }]); - - let artifact = resolve_files_firmware_artifact(firmware_cache_directory, &firmware, 0) - .unwrap() - .unwrap(); - - assert!(artifact.local_path.starts_with(firmware_cache_directory)); - assert_eq!(artifact.local_path.file_name().unwrap(), "fw.bin"); - assert_eq!( - artifact.source, - ResolvedFirmwareArtifactSource::Remote { - url: "https://firmware.example.invalid/path/fw.bin".to_string(), - sha256: "abc123".to_string(), - } - ); - } - - #[test] - fn resolve_files_firmware_artifact_uses_filename_as_local_source() { - let firmware = firmware_with_files(vec![FirmwareFileArtifact { - filename: Some("/opt/carbide/firmware/fw.bin".to_string()), - url: None, - sha256: "abc123".to_string(), - }]); - - let artifact = resolve_files_firmware_artifact(Path::new("/cache"), &firmware, 0) - .unwrap() - .unwrap(); - - assert_eq!( - artifact.local_path, - PathBuf::from("/opt/carbide/firmware/fw.bin") - ); - assert_eq!(artifact.source, ResolvedFirmwareArtifactSource::Local); - } + const CACHE_DIRECTORY: &str = "/mnt/persistence/fw/download-cache"; - #[test] - fn resolve_files_firmware_artifact_gives_url_precedence_over_filename() { - let firmware_cache_directory = Path::new("/mnt/persistence/fw/download-cache"); - let firmware = firmware_with_files(vec![FirmwareFileArtifact { - filename: Some("/opt/carbide/firmware/local.bin".to_string()), - url: Some("https://firmware.example.invalid/remote.bin".to_string()), - sha256: "abc123".to_string(), - }]); - - let artifact = resolve_files_firmware_artifact(firmware_cache_directory, &firmware, 0) - .unwrap() - .unwrap(); - - assert!(artifact.local_path.starts_with(firmware_cache_directory)); - assert_eq!(artifact.local_path.file_name().unwrap(), "remote.bin"); - assert_eq!( - artifact.source, - ResolvedFirmwareArtifactSource::Remote { - url: "https://firmware.example.invalid/remote.bin".to_string(), - sha256: "abc123".to_string(), - } - ); + struct ResolutionInput { + files: Vec, + pos: u32, } #[test] - fn resolve_files_firmware_artifact_returns_none_without_files() { - let firmware = FirmwareEntry::standard("1.0"); - - let artifact = resolve_files_firmware_artifact(Path::new("/cache"), &firmware, 0).unwrap(); - - assert_eq!(artifact, None); - } + fn resolve_files_firmware_artifact_cases() { + let remote_url = "https://firmware.example.invalid/path/fw.bin"; + let second_url = "https://firmware.example.invalid/second.bin"; - #[test] - fn resolve_files_firmware_artifact_rejects_url_without_filename() { - let firmware = firmware_with_files(vec![FirmwareFileArtifact { - filename: None, - url: Some("https://firmware.example.invalid/".to_string()), - sha256: "abc123".to_string(), - }]); - - let error = resolve_files_firmware_artifact(Path::new("/cache"), &firmware, 0) - .unwrap_err() - .to_string(); - - assert!(error.contains("URL does not include a filename")); + check_cases( + [ + Case { + scenario: "no files", + input: ResolutionInput { + files: Vec::new(), + pos: 0, + }, + expect: Yields(None), + }, + Case { + scenario: "URL only", + input: ResolutionInput { + files: vec![file(None, Some(remote_url), "abc123")], + pos: 0, + }, + expect: Yields(Some(remote_artifact(remote_url, "abc123"))), + }, + Case { + scenario: "filename only", + input: ResolutionInput { + files: vec![file( + Some("/opt/carbide/firmware/fw.bin"), + None, + "abc123", + )], + pos: 0, + }, + expect: Yields(Some(local_artifact("/opt/carbide/firmware/fw.bin"))), + }, + Case { + scenario: "URL takes precedence over filename", + input: ResolutionInput { + files: vec![file( + Some("/opt/carbide/firmware/local.bin"), + Some(remote_url), + "abc123", + )], + pos: 0, + }, + expect: Yields(Some(remote_artifact(remote_url, "abc123"))), + }, + Case { + scenario: "requested index selects matching artifact", + input: ResolutionInput { + files: vec![ + file( + Some("/opt/carbide/firmware/first.bin"), + Some("https://firmware.example.invalid/first.bin"), + "first-sha", + ), + file( + Some("/opt/carbide/firmware/second.bin"), + Some(second_url), + "second-sha", + ), + ], + pos: 1, + }, + expect: Yields(Some(remote_artifact(second_url, "second-sha"))), + }, + Case { + scenario: "requested index is out of range", + input: ResolutionInput { + files: vec![file( + Some("/opt/carbide/firmware/fw.bin"), + None, + "abc123", + )], + pos: 1, + }, + expect: FailsWith( + "firmware version 1.0 has no files[] artifact at index 1".to_string(), + ), + }, + Case { + scenario: "URL has no filename", + input: ResolutionInput { + files: vec![file( + None, + Some("https://firmware.example.invalid/"), + "abc123", + )], + pos: 0, + }, + expect: FailsWith( + "firmware version 1.0 files[] artifact at index 0 URL does not include a filename" + .to_string(), + ), + }, + Case { + scenario: "filename and URL are missing", + input: ResolutionInput { + files: vec![file(None, None, "abc123")], + pos: 0, + }, + expect: FailsWith( + "firmware version 1.0 files[] artifact at index 0 has no filename or URL" + .to_string(), + ), + }, + Case { + scenario: "surrounding URL whitespace is trimmed", + input: ResolutionInput { + files: vec![file( + None, + Some(" https://firmware.example.invalid/trimmed.bin \n"), + "abc123", + )], + pos: 0, + }, + expect: Yields(Some(remote_artifact( + "https://firmware.example.invalid/trimmed.bin", + "abc123", + ))), + }, + Case { + scenario: "surrounding filename whitespace is trimmed", + input: ResolutionInput { + files: vec![file( + Some(" \t/opt/carbide/firmware/trimmed.bin "), + None, + "abc123", + )], + pos: 0, + }, + expect: Yields(Some(local_artifact( + "/opt/carbide/firmware/trimmed.bin", + ))), + }, + Case { + scenario: "blank filename and URL are missing", + input: ResolutionInput { + files: vec![file(Some(" \t "), Some(" \n "), "abc123")], + pos: 0, + }, + expect: FailsWith( + "firmware version 1.0 files[] artifact at index 0 has no filename or URL" + .to_string(), + ), + }, + ], + |ResolutionInput { files, pos }| { + resolve_files_firmware_artifact( + Path::new(CACHE_DIRECTORY), + &firmware_with_files(files), + pos, + ) + .map_err(|error| error.to_string()) + }, + ); } fn firmware_with_files(files: Vec) -> FirmwareEntry { @@ -197,4 +273,29 @@ mod tests { ..FirmwareEntry::default() } } + + fn file(filename: Option<&str>, url: Option<&str>, sha256: &str) -> FirmwareFileArtifact { + FirmwareFileArtifact { + filename: filename.map(str::to_string), + url: url.map(str::to_string), + sha256: sha256.to_string(), + } + } + + fn remote_artifact(url: &str, sha256: &str) -> ResolvedFirmwareArtifact { + ResolvedFirmwareArtifact { + local_path: firmware_cache_filename(Path::new(CACHE_DIRECTORY), url).unwrap(), + source: ResolvedFirmwareArtifactSource::Remote { + url: url.to_string(), + sha256: sha256.to_string(), + }, + } + } + + fn local_artifact(path: &str) -> ResolvedFirmwareArtifact { + ResolvedFirmwareArtifact { + local_path: PathBuf::from(path), + source: ResolvedFirmwareArtifactSource::Local, + } + } } diff --git a/crates/machine-controller/src/handler/firmware_artifact.rs b/crates/machine-controller/src/handler/firmware_artifact.rs index 3a353040d0..e2086c8743 100644 --- a/crates/machine-controller/src/handler/firmware_artifact.rs +++ b/crates/machine-controller/src/handler/firmware_artifact.rs @@ -104,340 +104,190 @@ fn firmware_artifact_url( mod tests { use std::path::PathBuf; + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, check_cases}; + use super::*; const FIRMWARE_DOWNLOAD_CACHE_DIRECTORY: &str = "/mnt/persistence/fw/download-cache"; + const FIRMWARE_DIRECTORY: &str = "/opt/nico/firmware"; + const PXE_PUBLIC_BASE_URL: &str = "http://carbide-pxe.forge:8080/"; - #[test] - fn resolves_files_artifact_with_url_and_filename() { - let firmware = firmware_with_files(vec![FirmwareFileArtifact { - filename: Some("/opt/nico/firmware/fw.bin".to_string()), - url: Some("https://firmware.example.invalid/fw.bin".to_string()), - sha256: "abc123".to_string(), - }]); - - let artifact = - resolve_firmware_artifact(Path::new(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY), &firmware, 0) - .unwrap(); - - assert!( - artifact - .local_path - .starts_with(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY) - ); - assert_eq!(artifact.local_path.file_name().unwrap(), "fw.bin"); - assert_eq!( - artifact.source, - ResolvedFirmwareArtifactSource::Remote { - url: "https://firmware.example.invalid/fw.bin".to_string(), - sha256: "abc123".to_string(), - } - ); + struct ResolutionInput { + firmware: FirmwareEntry, + pos: u32, } - #[test] - fn derives_files_artifact_filename_from_url_when_filename_is_missing() { - let firmware = firmware_with_files(vec![FirmwareFileArtifact { - filename: None, - url: Some("https://firmware.example.invalid/path/fw_image.bin".to_string()), - sha256: "abc123".to_string(), - }]); - - let artifact = - resolve_firmware_artifact(Path::new(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY), &firmware, 0) - .unwrap(); - - assert!( - artifact - .local_path - .starts_with(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY) - ); - assert_eq!(artifact.local_path.file_name().unwrap(), "fw_image.bin"); - assert_eq!( - artifact - .local_path - .parent() - .unwrap() - .file_name() - .unwrap() - .to_string_lossy() - .len(), - 64 - ); + struct ScoutInput { + filename: Option<&'static str>, + url: Option<&'static str>, } - #[test] - fn resolves_files_artifact_by_index() { - let firmware = firmware_with_files(vec![ - FirmwareFileArtifact { - filename: Some("/opt/nico/firmware/first.bin".to_string()), - url: Some("https://firmware.example.invalid/first.bin".to_string()), - sha256: "first-sha".to_string(), - }, - FirmwareFileArtifact { - filename: Some("/opt/nico/firmware/second.bin".to_string()), - url: Some("https://firmware.example.invalid/second.bin".to_string()), - sha256: "second-sha".to_string(), - }, - ]); - - let artifact = - resolve_firmware_artifact(Path::new(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY), &firmware, 1) - .unwrap(); - - assert!( - artifact - .local_path - .starts_with(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY) - ); - assert_eq!(artifact.local_path.file_name().unwrap(), "second.bin"); - assert_eq!( - artifact.source, - ResolvedFirmwareArtifactSource::Remote { - url: "https://firmware.example.invalid/second.bin".to_string(), - sha256: "second-sha".to_string(), - } - ); + #[derive(Debug, PartialEq, Eq)] + enum ComparableError { + Generic(String), + Other(String), } #[test] - fn resolves_files_artifact_without_url_as_local_file() { - let firmware = firmware_with_files(vec![FirmwareFileArtifact { - filename: Some("/opt/nico/firmware/fw.bin".to_string()), - url: None, - sha256: "abc123".to_string(), - }]); - - let artifact = - resolve_firmware_artifact(Path::new(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY), &firmware, 0) - .unwrap(); - - assert_eq!( - artifact.local_path, - PathBuf::from("/opt/nico/firmware/fw.bin") - ); - assert_eq!(artifact.source, ResolvedFirmwareArtifactSource::Local); - } - - #[test] - fn files_artifact_index_out_of_range_is_an_error() { - let firmware = firmware_with_files(vec![FirmwareFileArtifact { - filename: Some("/opt/nico/firmware/fw.bin".to_string()), - url: None, - sha256: "abc123".to_string(), - }]); - - let error = - resolve_firmware_artifact(Path::new(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY), &firmware, 1) - .unwrap_err(); - - assert!( - error - .to_string() - .contains("has no files[] artifact at index 1") - ); - } - - #[test] - fn files_artifact_without_filename_or_url_is_an_error() { - let firmware = firmware_with_files(vec![FirmwareFileArtifact { - filename: None, - url: None, - sha256: "abc123".to_string(), - }]); - - let error = - resolve_firmware_artifact(Path::new(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY), &firmware, 0) - .unwrap_err(); - - assert!(error.to_string().contains("has no filename or URL")); - } - - #[test] - fn files_artifact_url_without_filename_is_an_error() { - let firmware = firmware_with_files(vec![FirmwareFileArtifact { - filename: None, - url: Some("https://firmware.example.invalid/".to_string()), - sha256: "abc123".to_string(), - }]); - - let error = - resolve_firmware_artifact(Path::new(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY), &firmware, 0) - .unwrap_err(); - - assert!( - error - .to_string() - .contains("URL does not include a filename") + fn resolve_firmware_artifact_cases() { + check_cases( + [ + Case { + scenario: "shared files artifact delegates successfully", + input: ResolutionInput { + firmware: firmware_with_files(vec![file( + Some("/opt/nico/firmware/fw.bin"), + None, + )]), + pos: 0, + }, + expect: Yields(local_artifact("/opt/nico/firmware/fw.bin")), + }, + Case { + scenario: "shared resolver error maps to generic error", + input: ResolutionInput { + firmware: firmware_with_files(vec![file(None, None)]), + pos: 0, + }, + expect: FailsWith(ComparableError::Generic( + "firmware version 1.0 files[] artifact at index 0 has no filename or URL" + .to_string(), + )), + }, + Case { + scenario: "legacy artifact selects indexed filename and remains local", + input: ResolutionInput { + firmware: FirmwareEntry { + version: "1.0".to_string(), + filenames: vec![ + "/opt/nico/firmware/first.bin".to_string(), + "/opt/nico/firmware/second.bin".to_string(), + ], + url: Some("https://firmware.example.invalid/legacy.bin".to_string()), + checksum: Some("legacy-sha".to_string()), + ..FirmwareEntry::default() + }, + pos: 1, + }, + expect: Yields(local_artifact("/opt/nico/firmware/second.bin")), + }, + ], + |ResolutionInput { firmware, pos }| { + resolve_firmware_artifact( + Path::new(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY), + &firmware, + pos, + ) + .map_err(comparable_error) + }, ); } #[test] - fn legacy_firmware_ignores_top_level_url_and_resolves_as_local_source() { - let firmware = FirmwareEntry { - version: "1.0".to_string(), - filename: None, - filenames: vec![ - "/opt/nico/firmware/first.bin".to_string(), - "/opt/nico/firmware/second.bin".to_string(), + fn resolve_scout_file_artifact_cases() { + check_cases( + [ + Case { + scenario: "URL takes precedence over filename", + input: ScoutInput { + filename: Some("/opt/nico/firmware/nvidia/fw.bin"), + url: Some("https://firmware.example.invalid/fw.bin"), + }, + expect: Yields(scout_artifact( + "https://firmware.example.invalid/fw.bin", + )), + }, + Case { + scenario: "filename becomes PXE public URL", + input: ScoutInput { + filename: Some("/opt/nico/firmware/nvidia/fw.bin"), + url: None, + }, + expect: Yields(scout_artifact( + "http://carbide-pxe.forge:8080/public/firmware/nvidia/fw.bin", + )), + }, + Case { + scenario: "filename and URL are missing", + input: ScoutInput { + filename: None, + url: None, + }, + expect: FailsWith(ComparableError::Generic( + "scout firmware artifact has no filename or URL".to_string(), + )), + }, + Case { + scenario: "same-prefix sibling is outside firmware directory", + input: ScoutInput { + filename: Some( + "/opt/nico/firmware2/nvidia/dgxh100/cx7/cx7.bin", + ), + url: None, + }, + expect: FailsWith(ComparableError::Generic( + "firmware artifact path /opt/nico/firmware2/nvidia/dgxh100/cx7/cx7.bin is outside firmware directory /opt/nico/firmware" + .to_string(), + )), + }, + Case { + scenario: "parent traversal is unsafe", + input: ScoutInput { + filename: Some("/opt/nico/firmware/../cx7.bin"), + url: None, + }, + expect: FailsWith(ComparableError::Generic( + "firmware artifact path /opt/nico/firmware/../cx7.bin contains unsafe path components" + .to_string(), + )), + }, ], - url: Some("https://firmware.example.invalid/legacy.bin".to_string()), - checksum: Some("legacy-sha".to_string()), - ..FirmwareEntry::default() - }; - - let artifact = - resolve_firmware_artifact(Path::new(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY), &firmware, 1) - .unwrap(); - - assert_eq!( - artifact.local_path, - PathBuf::from("/opt/nico/firmware/second.bin") + |ScoutInput { filename, url }| { + resolve_scout_file_artifact( + PXE_PUBLIC_BASE_URL, + Path::new(FIRMWARE_DIRECTORY), + &file(filename, url), + ) + .map_err(comparable_error) + }, ); - assert_eq!(artifact.source, ResolvedFirmwareArtifactSource::Local); } - #[test] - fn legacy_firmware_without_url_resolves_as_local_source() { - let firmware = FirmwareEntry { + fn firmware_with_files(files: Vec) -> FirmwareEntry { + FirmwareEntry { version: "1.0".to_string(), - filename: Some("/opt/nico/firmware/fw.bin".to_string()), - url: None, - checksum: None, + files, ..FirmwareEntry::default() - }; - - let artifact = - resolve_firmware_artifact(Path::new(FIRMWARE_DOWNLOAD_CACHE_DIRECTORY), &firmware, 0) - .unwrap(); - - assert_eq!( - artifact.local_path, - PathBuf::from("/opt/nico/firmware/fw.bin") - ); - assert_eq!(artifact.source, ResolvedFirmwareArtifactSource::Local); - } - - #[test] - fn resolve_scout_file_artifact_uses_direct_url_when_url_and_filename_are_set() { - let artifact = FirmwareFileArtifact { - filename: Some("/opt/nico/firmware/nvidia/fw.bin".to_string()), - url: Some("https://firmware.example.invalid/fw.bin".to_string()), - sha256: "abc123".to_string(), - }; - - let file_artifact = resolve_scout_file_artifact( - "http://carbide-pxe.forge:8080", - Path::new("/opt/nico/firmware"), - &artifact, - ) - .expect("artifact should resolve"); - - assert_eq!(file_artifact.url, "https://firmware.example.invalid/fw.bin"); - assert_eq!(file_artifact.sha256, "abc123"); + } } - #[test] - fn resolve_scout_file_artifact_uses_pxe_url_for_filename_without_url() { - let artifact = FirmwareFileArtifact { - filename: Some("/opt/nico/firmware/nvidia/fw.bin".to_string()), - url: None, + fn file(filename: Option<&str>, url: Option<&str>) -> FirmwareFileArtifact { + FirmwareFileArtifact { + filename: filename.map(str::to_string), + url: url.map(str::to_string), sha256: "abc123".to_string(), - }; - - let file_artifact = resolve_scout_file_artifact( - "http://carbide-pxe.forge:8080/", - Path::new("/opt/nico/firmware"), - &artifact, - ) - .expect("artifact should resolve"); - - assert_eq!( - file_artifact.url, - "http://carbide-pxe.forge:8080/public/firmware/nvidia/fw.bin" - ); - assert_eq!(file_artifact.sha256, "abc123"); + } } - #[test] - fn resolve_scout_file_artifact_uses_direct_url_without_filename() { - let artifact = FirmwareFileArtifact { - filename: None, - url: Some("https://firmware.example.invalid/fw.bin".to_string()), - sha256: "abc123".to_string(), - }; - - let file_artifact = resolve_scout_file_artifact( - "http://carbide-pxe.forge:8080", - Path::new("/opt/nico/firmware"), - &artifact, - ) - .expect("artifact should resolve"); - - assert_eq!(file_artifact.url, "https://firmware.example.invalid/fw.bin"); - assert_eq!(file_artifact.sha256, "abc123"); + fn local_artifact(path: &str) -> ResolvedFirmwareArtifact { + ResolvedFirmwareArtifact { + local_path: PathBuf::from(path), + source: ResolvedFirmwareArtifactSource::Local, + } } - #[test] - fn resolve_scout_file_artifact_requires_filename_or_url() { - let artifact = FirmwareFileArtifact { - filename: None, - url: None, + fn scout_artifact(url: &str) -> FileArtifact { + FileArtifact { + url: url.to_string(), sha256: "abc123".to_string(), - }; - - let error = resolve_scout_file_artifact( - "http://carbide-pxe.forge:8080", - Path::new("/opt/nico/firmware"), - &artifact, - ) - .unwrap_err(); - - assert!( - error - .to_string() - .contains("scout firmware artifact has no filename or URL") - ); - } - - #[test] - fn resolve_scout_file_artifact_rejects_unsafe_filename_paths() { - let unsafe_cases = [ - ( - // A sibling directory with the same string prefix is not inside the firmware root. - "/opt/nico/firmware2/nvidia/dgxh100/cx7/cx7.bin", - "is outside firmware directory /opt/nico/firmware", - ), - ( - // A path under the firmware root still cannot traverse back out with `..`. - "/opt/nico/firmware/../cx7.bin", - "contains unsafe path components", - ), - ]; - - for (filename, expected_error) in unsafe_cases { - let artifact = FirmwareFileArtifact { - filename: Some(filename.to_string()), - url: None, - sha256: "abc123".to_string(), - }; - - let error = resolve_scout_file_artifact( - "http://carbide-pxe.forge:8080", - Path::new("/opt/nico/firmware"), - &artifact, - ) - .unwrap_err(); - - assert!(error.to_string().contains(expected_error)); } } - fn firmware_with_files(files: Vec) -> FirmwareEntry { - FirmwareEntry { - version: "1.0".to_string(), - files, - ..FirmwareEntry::default() + fn comparable_error(error: StateHandlerError) -> ComparableError { + match error { + StateHandlerError::GenericError(error) => ComparableError::Generic(error.to_string()), + error => ComparableError::Other(error.to_string()), } } }