-
Notifications
You must be signed in to change notification settings - Fork 34
fix(spur-net): honor requested image architecture #438
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
c6335be
d6b61f3
1f36a38
18c2a04
01063f3
8206790
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,24 @@ struct Manifest { | |
| // v1 compat: some registries return "fsLayers" instead | ||
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
| struct ManifestList { | ||
| manifests: Vec<ManifestEntry>, | ||
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
| struct ManifestEntry { | ||
| digest: String, | ||
| #[serde(default)] | ||
| platform: Option<Platform>, | ||
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
| struct Platform { | ||
| architecture: String, | ||
| os: String, | ||
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
| struct LayerDescriptor { | ||
| digest: String, | ||
|
|
@@ -101,31 +119,42 @@ pub fn parse_image_ref(image: &str) -> ImageRef { | |
| /// Pull an image from a registry and create a squashfs file. | ||
| /// | ||
| /// Returns the path to the squashfs file. | ||
| pub async fn pull_image(image: &str, output_dir: &Path) -> anyhow::Result<PathBuf> { | ||
| pub async fn pull_image(image: &str, output_dir: &Path, arch: &str) -> anyhow::Result<PathBuf> { | ||
| let image_ref = parse_image_ref(image); | ||
| info!( | ||
| registry = %image_ref.registry, | ||
| repository = %image_ref.repository, | ||
| tag = %image_ref.tag, | ||
| architecture = arch, | ||
| "pulling image" | ||
| ); | ||
|
|
||
| let sanitized = sanitize_name(image); | ||
| let sqsh_path = output_dir.join(format!("{}.sqsh", sanitized)); | ||
| let arch_path = sqsh_path.with_extension("sqsh.arch"); | ||
|
|
||
| if sqsh_path.exists() { | ||
| info!(path = %sqsh_path.display(), "image already exists"); | ||
| return Ok(sqsh_path); | ||
| let cached_arch = std::fs::read_to_string(&arch_path).ok(); | ||
| if cached_architecture_matches(cached_arch.as_deref(), arch) { | ||
| info!(path = %sqsh_path.display(), architecture = arch, "image already exists"); | ||
| return Ok(sqsh_path); | ||
| } | ||
| info!(path = %sqsh_path.display(), architecture = arch, "replacing image for requested architecture"); | ||
| } | ||
|
|
||
| std::fs::create_dir_all(output_dir)?; | ||
|
|
||
| // Create temp directory for rootfs assembly | ||
| let tmp_dir = output_dir.join(format!(".pulling_{}", sanitized)); | ||
| if tmp_dir.exists() { | ||
| std::fs::remove_dir_all(&tmp_dir)?; | ||
| } | ||
| let rootfs_dir = tmp_dir.join("rootfs"); | ||
| let staged_sqsh_path = tmp_dir.join("image.sqsh"); | ||
| let staged_arch_path = tmp_dir.join("image.sqsh.arch"); | ||
| std::fs::create_dir_all(&rootfs_dir)?; | ||
|
|
||
| let result = pull_and_extract(&image_ref, &rootfs_dir).await; | ||
| let result = pull_and_extract(&image_ref, &rootfs_dir, arch).await; | ||
| if let Err(e) = &result { | ||
| let _ = std::fs::remove_dir_all(&tmp_dir); | ||
| return Err(anyhow::anyhow!("{}", e)); | ||
|
|
@@ -136,7 +165,7 @@ pub async fn pull_image(image: &str, output_dir: &Path) -> anyhow::Result<PathBu | |
| let mksquashfs_result = std::process::Command::new("mksquashfs") | ||
| .args([ | ||
| rootfs_dir.to_str().unwrap(), | ||
| sqsh_path.to_str().unwrap(), | ||
| staged_sqsh_path.to_str().unwrap(), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cleaned this up by passing the filesystem paths directly to |
||
| "-noappend", | ||
| "-comp", | ||
| "zstd", | ||
|
|
@@ -165,8 +194,20 @@ pub async fn pull_image(image: &str, output_dir: &Path) -> anyhow::Result<PathBu | |
| } | ||
| } | ||
|
|
||
| // Clean up temp dir | ||
| let finalize_result = (|| -> anyhow::Result<()> { | ||
| std::fs::write(&staged_arch_path, oci_architecture(arch))?; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The sidecar is written as the requested arch unconditionally. On the single-manifest path (a registry returning an image manifest directly rather than an index), no platform verification runs, so the recorded arch could be wrong and defeat the rebuild-on-arch-change guard. Verifying the fetched manifest's platform (or documenting the limitation) would close this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed by validating direct manifests through their OCI config blob before layer extraction. The config |
||
| std::fs::rename(&staged_sqsh_path, &sqsh_path) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The install is two separate renames (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. First-time installs now publish the sidecar before exposing the payload. Replacements invalidate the old sidecar before publishing the new payload, then publish the new sidecar last, so an interruption forces a cache miss rather than pairing a payload with stale metadata. Both paths have filesystem-level regression coverage. |
||
| .with_context(|| format!("failed to install image at {}", sqsh_path.display()))?; | ||
| std::fs::rename(&staged_arch_path, &arch_path).with_context(|| { | ||
| format!( | ||
| "failed to record image architecture at {}", | ||
| arch_path.display() | ||
| ) | ||
| })?; | ||
| Ok(()) | ||
| })(); | ||
| let _ = std::fs::remove_dir_all(&tmp_dir); | ||
| finalize_result?; | ||
|
|
||
| let size = std::fs::metadata(&sqsh_path).map(|m| m.len()).unwrap_or(0); | ||
| info!( | ||
|
|
@@ -179,7 +220,11 @@ pub async fn pull_image(image: &str, output_dir: &Path) -> anyhow::Result<PathBu | |
| } | ||
|
|
||
| /// Download manifest and layers, extract to rootfs directory. | ||
| async fn pull_and_extract(image_ref: &ImageRef, rootfs_dir: &Path) -> anyhow::Result<()> { | ||
| async fn pull_and_extract( | ||
| image_ref: &ImageRef, | ||
| rootfs_dir: &Path, | ||
| arch: &str, | ||
| ) -> anyhow::Result<()> { | ||
| let client = reqwest::Client::builder().user_agent("spur/0.1").build()?; | ||
|
|
||
| // Get auth token | ||
|
|
@@ -234,6 +279,7 @@ async fn pull_and_extract(image_ref: &ImageRef, rootfs_dir: &Path) -> anyhow::Re | |
| ®istry_url, | ||
| image_ref, | ||
| token.as_deref(), | ||
| arch, | ||
| ) | ||
| .await?; | ||
| index | ||
|
|
@@ -515,50 +561,51 @@ async fn get_auth_token( | |
| Ok(None) | ||
| } | ||
|
|
||
| /// Resolve a manifest list (multi-arch) to a single amd64/linux manifest. | ||
| async fn resolve_manifest_list( | ||
| client: &reqwest::Client, | ||
| body: &str, | ||
| registry_url: &str, | ||
| image_ref: &ImageRef, | ||
| token: Option<&str>, | ||
| ) -> anyhow::Result<Manifest> { | ||
| #[derive(Deserialize)] | ||
| struct ManifestList { | ||
| manifests: Vec<ManifestEntry>, | ||
| } | ||
| #[derive(Deserialize)] | ||
| struct ManifestEntry { | ||
| digest: String, | ||
| #[serde(default)] | ||
| platform: Option<Platform>, | ||
| } | ||
| #[derive(Deserialize)] | ||
| struct Platform { | ||
| architecture: String, | ||
| os: String, | ||
| fn oci_architecture(arch: &str) -> &str { | ||
| match arch { | ||
| "x86_64" => "amd64", | ||
| "aarch64" => "arm64", | ||
| "x86" => "386", | ||
| arch => arch, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: the normalization fallthrough passes unknown arches through unchanged; combined with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That limitation remains intentional here. The issue contract is architecture selection, while arm v6/v7 support needs a variant input and normalization contract rather than silently choosing the first |
||
| } | ||
| } | ||
|
|
||
| fn cached_architecture_matches(cached_arch: Option<&str>, requested_arch: &str) -> bool { | ||
| cached_arch.is_some_and(|arch| arch.trim() == oci_architecture(requested_arch)) | ||
| } | ||
|
|
||
| fn manifest_digest_for_arch(body: &str, arch: &str) -> anyhow::Result<String> { | ||
| let list: ManifestList = serde_json::from_str(body).context("failed to parse manifest list")?; | ||
| let oci_arch = oci_architecture(arch); | ||
|
|
||
| // Find linux/amd64 | ||
| let entry = list | ||
| .manifests | ||
| list.manifests | ||
| .iter() | ||
| .find(|m| { | ||
| m.platform | ||
| .find(|manifest| { | ||
| manifest | ||
| .platform | ||
| .as_ref() | ||
| .map(|p| p.architecture == "amd64" && p.os == "linux") | ||
| .unwrap_or(false) | ||
| .is_some_and(|platform| platform.architecture == oci_arch && platform.os == "linux") | ||
| }) | ||
| .or_else(|| list.manifests.first()) | ||
| .ok_or_else(|| anyhow::anyhow!("no linux/amd64 manifest found in manifest list"))?; | ||
| .map(|manifest| manifest.digest.clone()) | ||
| .ok_or_else(|| anyhow::anyhow!("no linux/{arch} manifest found in manifest list")) | ||
| } | ||
|
|
||
| debug!(digest = %entry.digest, "resolved manifest list to platform manifest"); | ||
| /// Resolve a manifest list (multi-arch) to a single Linux platform manifest. | ||
| async fn resolve_manifest_list( | ||
| client: &reqwest::Client, | ||
| body: &str, | ||
| registry_url: &str, | ||
| image_ref: &ImageRef, | ||
| token: Option<&str>, | ||
| arch: &str, | ||
| ) -> anyhow::Result<Manifest> { | ||
| let digest = manifest_digest_for_arch(body, arch)?; | ||
|
|
||
| debug!(digest = %digest, architecture = arch, "resolved manifest list to platform manifest"); | ||
|
|
||
| let url = format!( | ||
| "{}/v2/{}/manifests/{}", | ||
| registry_url, image_ref.repository, entry.digest | ||
| registry_url, image_ref.repository, digest | ||
| ); | ||
| let mut req = client.get(&url).header( | ||
| ACCEPT, | ||
|
|
@@ -644,6 +691,19 @@ mod tests { | |
|
|
||
| use super::*; | ||
|
|
||
| const MULTI_ARCH_MANIFEST: &str = r#"{ | ||
| "manifests": [ | ||
| { | ||
| "digest": "sha256:amd64", | ||
| "platform": { "architecture": "amd64", "os": "linux" } | ||
| }, | ||
| { | ||
| "digest": "sha256:arm64", | ||
| "platform": { "architecture": "arm64", "os": "linux" } | ||
| } | ||
| ] | ||
| }"#; | ||
|
|
||
| #[test] | ||
| fn test_decode_registry_auth_b64_valid() { | ||
| // echo -n 'alice:secret' | base64 -w0 | ||
|
|
@@ -684,6 +744,42 @@ mod tests { | |
| assert_eq!(super::decode_registry_auth_b64(&enc).as_deref(), Some(cred)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_manifest_digest_for_requested_arch() { | ||
| assert_eq!( | ||
| manifest_digest_for_arch(MULTI_ARCH_MANIFEST, "arm64").unwrap(), | ||
| "sha256:arm64" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_manifest_digest_normalizes_rust_arch_names() { | ||
| assert_eq!( | ||
| manifest_digest_for_arch(MULTI_ARCH_MANIFEST, "x86_64").unwrap(), | ||
| "sha256:amd64" | ||
| ); | ||
| assert_eq!( | ||
| manifest_digest_for_arch(MULTI_ARCH_MANIFEST, "aarch64").unwrap(), | ||
| "sha256:arm64" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_cached_architecture_must_match_requested_arch() { | ||
| assert!(cached_architecture_matches(Some("arm64\n"), "aarch64")); | ||
| assert!(!cached_architecture_matches(Some("amd64"), "arm64")); | ||
| assert!(!cached_architecture_matches(None, "arm64")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_manifest_digest_error_includes_requested_arch() { | ||
| let error = manifest_digest_for_arch(MULTI_ARCH_MANIFEST, "riscv64").unwrap_err(); | ||
| assert_eq!( | ||
| error.to_string(), | ||
| "no linux/riscv64 manifest found in manifest list" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_dockerhub_official() { | ||
| let r = parse_image_ref("ubuntu:22.04"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1109,7 +1109,7 @@ pub fn container_init( | |
| /// umoci, or enroot. Only needs mksquashfs (squashfs-tools). | ||
| pub async fn import_image(uri: &str) -> anyhow::Result<PathBuf> { | ||
| let dir = image_dir(); | ||
| spur_net::pull_image(uri, &dir).await | ||
| spur_net::pull_image(uri, &dir, std::env::consts::ARCH).await | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Requested arch is honored on the pull path, but
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a separate execution-time validation boundary. This PR keeps daemon-side imports host-native and stays focused on honoring the requested architecture during import; changing |
||
| } | ||
|
|
||
| /// List imported images. | ||
|
|
@@ -1144,6 +1144,7 @@ pub fn remove_image(name: &str) -> anyhow::Result<()> { | |
| bail!("image '{}' not found", name); | ||
| } | ||
| std::fs::remove_file(&path)?; | ||
| let _ = std::fs::remove_file(path.with_extension("sqsh.arch")); | ||
| info!(name, "image removed"); | ||
| Ok(()) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: the
.pulling_<name>temp dir is deterministic per image, so two concurrent pulls of the same image race — and this unconditionalremove_dir_allcan wipe an in-flight pull's working tree. A PID/uuid suffix would make it safe. Low likelihood for interactive imports.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shared staging directory is gone. Each pull now uses a UUID-suffixed directory, and same-image pulls are serialized through the cache recheck and final publication, so concurrent imports cannot delete or interleave one another's artifacts.