-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(docker): add support for sandboxes from docker archives #2361
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 all commits
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 |
|---|---|---|
|
|
@@ -2020,6 +2020,10 @@ pub async fn sandbox_create( | |
| let tag = build_from_dockerfile(&dockerfile, &context, gateway_name).await?; | ||
| Some(tag) | ||
| } | ||
| ResolvedSource::DockerArchive { path } => { | ||
| let tag = load_from_docker_archive(&path, gateway_name).await?; | ||
| Some(tag) | ||
| } | ||
| } | ||
| } | ||
| None => None, | ||
|
|
@@ -2517,13 +2521,16 @@ enum ResolvedSource { | |
| dockerfile: PathBuf, | ||
| context: PathBuf, | ||
| }, | ||
| /// A Docker archive (`.tar` or `.tar.gz`) to load into the local daemon. | ||
| DockerArchive { path: PathBuf }, | ||
| } | ||
|
|
||
| /// Classify the `--from` value into an image reference or a Dockerfile that | ||
| /// needs building. | ||
| /// Classify the `--from` value into an image reference, a Dockerfile that | ||
| /// needs building, or a Docker archive that needs loading. | ||
| /// | ||
| /// Resolution order: | ||
| /// 1. Existing file whose name contains "Dockerfile" → build from file. | ||
| /// 1b. Existing file with `.tar`/`.tar.gz`/`.tgz` extension → load archive. | ||
| /// 2. Existing directory that contains a `Dockerfile` → build from directory. | ||
|
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. Question: Why are two "Dockerfile" cases now separated by a archive case? Should the explicit Dockerfile and Dockerfile parent folder cases not be treated the same way? |
||
| /// 3. Missing explicit local paths → local error, not image pull. | ||
|
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. Out-of-scope: What are "Missing explicit local paths"? |
||
| /// 4. Value contains `/`, `:`, or `.` → treat as a full image reference. | ||
|
|
@@ -2548,9 +2555,17 @@ fn resolve_from(value: &str) -> Result<ResolvedSource> { | |
| }); | ||
| } | ||
|
|
||
| if filename_looks_like_docker_archive(path) { | ||
| let archive_path = path | ||
| .canonicalize() | ||
| .into_diagnostic() | ||
| .wrap_err_with(|| format!("failed to resolve path: {}", path.display()))?; | ||
| return Ok(ResolvedSource::DockerArchive { path: archive_path }); | ||
| } | ||
|
|
||
| if value_looks_like_local_source(value) { | ||
| return Err(miette::miette!( | ||
| "local --from file is not a Dockerfile: {}", | ||
| "local --from file is not a Dockerfile or Docker archive (.tar/.tar.gz/.tgz): {}", | ||
| path.display() | ||
| )); | ||
| } | ||
|
|
@@ -2589,7 +2604,7 @@ fn resolve_from(value: &str) -> Result<ResolvedSource> { | |
| if value_looks_like_local_source(value) { | ||
| return Err(miette::miette!( | ||
| "local --from path does not exist: {}\n\ | ||
| Use an existing Dockerfile, a directory containing Dockerfile, or a container image reference.", | ||
| Use an existing Dockerfile, directory containing Dockerfile, Docker archive (.tar/.tar.gz/.tgz), or a container image reference.", | ||
| path.display() | ||
| )); | ||
| } | ||
|
|
@@ -2610,6 +2625,19 @@ fn filename_looks_like_dockerfile(path: &Path) -> bool { | |
| lower.contains("dockerfile") || lower.ends_with(".dockerfile") | ||
|
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. Out of scope, but the |
||
| } | ||
|
|
||
| fn filename_looks_like_docker_archive(path: &Path) -> bool { | ||
| let name = path | ||
| .file_name() | ||
| .map(|n| n.to_string_lossy()) | ||
| .unwrap_or_default(); | ||
| let lower = name.to_lowercase(); | ||
| // `.tar.gz` has extension `gz`, so check the compound suffix with ends_with. | ||
| lower.ends_with(".tar.gz") | ||
| || Path::new(&*lower) | ||
| .extension() | ||
| .is_some_and(|ext| ext == "tar" || ext == "tgz") | ||
|
Comment on lines
+2635
to
+2638
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. Question: Why use Also we already have a path? Can't we call |
||
| } | ||
|
|
||
| fn value_looks_like_local_source(value: &str) -> bool { | ||
| value_is_explicit_local_path(value) || value_looks_like_bare_dockerfile_name(value) | ||
| } | ||
|
|
@@ -2689,6 +2717,43 @@ async fn build_from_dockerfile( | |
| Ok(tag) | ||
| } | ||
|
|
||
| /// Load a Docker archive and return the image tag. | ||
| /// | ||
| /// Local-gateway only — same constraint as Dockerfile sources. | ||
| async fn load_from_docker_archive(archive_path: &Path, gateway_name: &str) -> Result<String> { | ||
| let metadata = get_gateway_metadata(gateway_name); | ||
| if !dockerfile_sources_supported_for_gateway(metadata.as_ref()) { | ||
| return Err(miette!( | ||
| "local Docker archive sources are only supported for local gateways; gateway '{}' is remote", | ||
| gateway_name | ||
| )); | ||
| } | ||
|
|
||
| eprintln!( | ||
| "Loading Docker archive {} for gateway '{}'", | ||
| archive_path.display().to_string().cyan(), | ||
| gateway_name, | ||
| ); | ||
| eprintln!(); | ||
|
|
||
| let mut on_log = |msg: String| { | ||
| eprintln!(" {msg}"); | ||
| }; | ||
|
|
||
| let tag = openshell_bootstrap::build::load_docker_archive(archive_path, &mut on_log).await?; | ||
|
|
||
| eprintln!(); | ||
| eprintln!( | ||
| "{} Image {} is available in the local Docker daemon for gateway '{}'.", | ||
| "✓".green().bold(), | ||
| tag.cyan(), | ||
| gateway_name, | ||
| ); | ||
| eprintln!(); | ||
|
|
||
| Ok(tag) | ||
| } | ||
|
|
||
| /// Load sandbox policy YAML. | ||
| /// | ||
| /// Resolution order: `--policy` flag > `OPENSHELL_SANDBOX_POLICY` env var. | ||
|
|
@@ -8737,8 +8802,8 @@ mod tests { | |
| .expect("failed to canonicalize context") | ||
| ); | ||
| } | ||
| super::ResolvedSource::Image(image) => { | ||
| panic!("expected Dockerfile source, got image {image}"); | ||
| other => { | ||
| panic!("expected Dockerfile source, got {other:?}"); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -8763,12 +8828,105 @@ mod tests { | |
|
|
||
| match resolve_from(image_ref).expect("expected image source") { | ||
| super::ResolvedSource::Image(image) => assert_eq!(image, image_ref), | ||
| super::ResolvedSource::Dockerfile { .. } => { | ||
| panic!("expected image ref, got Dockerfile source"); | ||
| other => { | ||
| panic!("expected image ref, got {other:?}"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_from_classifies_tar_archive() { | ||
| let temp = tempfile::tempdir().expect("failed to create tempdir"); | ||
| let archive = temp.path().join("image.tar"); | ||
| fs::write(&archive, b"fake tar content").expect("failed to write archive"); | ||
|
|
||
| match resolve_from(archive.to_str().expect("temp path is not UTF-8")) | ||
| .expect("expected DockerArchive source") | ||
| { | ||
| super::ResolvedSource::DockerArchive { path } => { | ||
| assert_eq!( | ||
| path, | ||
| archive | ||
| .canonicalize() | ||
| .expect("failed to canonicalize archive") | ||
| ); | ||
| } | ||
| other => panic!("expected DockerArchive source, got {other:?}"), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_from_classifies_tar_gz_archive() { | ||
| let temp = tempfile::tempdir().expect("failed to create tempdir"); | ||
| let archive = temp.path().join("image.tar.gz"); | ||
| fs::write(&archive, b"fake tar.gz content").expect("failed to write archive"); | ||
|
|
||
| match resolve_from(archive.to_str().expect("temp path is not UTF-8")) | ||
| .expect("expected DockerArchive source") | ||
| { | ||
| super::ResolvedSource::DockerArchive { path } => { | ||
| assert_eq!( | ||
| path, | ||
| archive | ||
| .canonicalize() | ||
| .expect("failed to canonicalize archive") | ||
| ); | ||
| } | ||
| other => panic!("expected DockerArchive source, got {other:?}"), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_from_classifies_tgz_archive() { | ||
| let temp = tempfile::tempdir().expect("failed to create tempdir"); | ||
| let archive = temp.path().join("image.tgz"); | ||
| fs::write(&archive, b"fake tgz content").expect("failed to write archive"); | ||
|
|
||
| match resolve_from(archive.to_str().expect("temp path is not UTF-8")) | ||
| .expect("expected DockerArchive source") | ||
| { | ||
| super::ResolvedSource::DockerArchive { path } => { | ||
| assert_eq!( | ||
| path, | ||
| archive | ||
| .canonicalize() | ||
| .expect("failed to canonicalize archive") | ||
| ); | ||
| } | ||
| other => panic!("expected DockerArchive source, got {other:?}"), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_from_rejects_missing_tar_archive() { | ||
| let temp = tempfile::tempdir().expect("failed to create tempdir"); | ||
| let missing = temp.path().join("missing.tar"); | ||
|
|
||
| let err = resolve_from(missing.to_str().expect("temp path is not UTF-8")) | ||
| .expect_err("expected missing archive to be rejected"); | ||
|
|
||
| assert!( | ||
| err.to_string().contains("local --from path does not exist"), | ||
| "unexpected error: {err}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn filename_looks_like_docker_archive_detects_extensions() { | ||
| use super::filename_looks_like_docker_archive; | ||
| assert!(filename_looks_like_docker_archive(Path::new("image.tar"))); | ||
| assert!(filename_looks_like_docker_archive(Path::new( | ||
| "image.tar.gz" | ||
| ))); | ||
| assert!(filename_looks_like_docker_archive(Path::new("image.tgz"))); | ||
| assert!(filename_looks_like_docker_archive(Path::new("IMAGE.TAR"))); | ||
| assert!(filename_looks_like_docker_archive(Path::new( | ||
| "my-image.TAR.GZ" | ||
| ))); | ||
| assert!(!filename_looks_like_docker_archive(Path::new("Dockerfile"))); | ||
| assert!(!filename_looks_like_docker_archive(Path::new("image.zip"))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn dockerfile_sources_are_rejected_for_remote_gateways() { | ||
| let metadata = GatewayMetadata { | ||
|
|
||
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.
Split into: A single heading like "Existing file" followed by two points indicating the different sources.