From 5ebf3ebd35851cebdab4898b74fd0cf25028e868 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 28 Aug 2026 16:19:56 -0400 Subject: [PATCH] Model the Daytona snapshot source as an enum `DaytonaSnapshotSettings` carried two independent `Option`s (`image` and `dockerfile`) that every consumer had to re-validate. Replace them with a single `source: DaytonaSnapshotSource { Image, Dockerfile }` so the both-set and neither-set states are unrepresentable at the sandbox layer. This removes four unreachable error arms in `canonical_manifest` and `create_snapshot_params`, the `.filter(...)` guard in `initialize`, and the presence guard in `daytona_config_from_environment`. The mutual-exclusion rule now lives only in fabro-config, which owns the `image.docker` / `image.dockerfile` keys the old messages named. Merge `ImageSnapshotManifest` into `SnapshotManifest` via a flattened `SourceManifest` enum. The dockerfile case serializes to the same bytes as before, so existing snapshot names are unchanged; the pinned identity test still passes. Pin the image-case identity as well so a future manifest change cannot silently orphan image snapshots. Fold `validate_daytona_image_settings` into the existing Daytona arm of `validate_provider_capabilities`; both callers already invoke it right after `resolve_environment_fields`, so error order is unchanged. Co-Authored-By: Claude Fable 5 --- lib/components/fabro-sandbox/src/config.rs | 18 +- .../fabro-sandbox/src/daytona/mod.rs | 164 +++++++----------- .../fabro-sandbox/src/from_environment.rs | 57 +++--- .../tests/it/daytona_integration.rs | 16 +- .../fabro-config/src/resolve/environment.rs | 29 ++-- 5 files changed, 124 insertions(+), 160 deletions(-) diff --git a/lib/components/fabro-sandbox/src/config.rs b/lib/components/fabro-sandbox/src/config.rs index d6cd1b179..ec946a537 100644 --- a/lib/components/fabro-sandbox/src/config.rs +++ b/lib/components/fabro-sandbox/src/config.rs @@ -117,11 +117,19 @@ pub enum DockerfileSource { Path { path: String }, } +/// Where a custom Daytona snapshot is built from. +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +pub enum DaytonaSnapshotSource { + /// A pullable image reference such as `ubuntu:24.04`. + Image(String), + /// A Dockerfile that Daytona builds into the snapshot. + Dockerfile(DockerfileSource), +} + #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] pub struct DaytonaSnapshotSettings { - pub cpu: Option, - pub memory: Option, - pub disk: Option, - pub image: Option, - pub dockerfile: Option, + pub cpu: Option, + pub memory: Option, + pub disk: Option, + pub source: DaytonaSnapshotSource, } diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 7a9e4efd4..66173286f 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -133,7 +133,7 @@ pub const REQUIRED_DAYTONA_PERMISSIONS: &[Permissions] = &[ pub use crate::config::{ DaytonaNetwork, DaytonaSettings as DaytonaConfig, - DaytonaSnapshotSettings as DaytonaSnapshotConfig, DockerfileSource, + DaytonaSnapshotSettings as DaytonaSnapshotConfig, DaytonaSnapshotSource, DockerfileSource, }; pub mod snapshot_identity { @@ -142,7 +142,7 @@ pub mod snapshot_identity { use sha2::{Digest, Sha256}; use uuid::Uuid; - use super::{DaytonaSnapshotConfig, DockerfileSource}; + use super::{DaytonaSnapshotConfig, DaytonaSnapshotSource, DockerfileSource}; const IDENTITY_VERSION: u8 = 1; const PROVIDER: &str = "daytona"; @@ -150,27 +150,27 @@ pub mod snapshot_identity { type HmacSha256 = Hmac; + /// The snapshot source as it appears in the identity manifest. Each + /// variant flattens into a single `"": ""` entry. #[derive(Serialize)] - struct SnapshotManifest<'a> { - identity_version: u8, - provider: &'static str, - tenant: &'static str, - dockerfile_sha256: &'a str, - cpu: Option, - memory_gb: Option, - disk_gb: Option, - entrypoint: Option<&'static str>, + #[serde(rename_all = "snake_case")] + enum SourceManifest<'a> { + DockerfileSha256(String), + Image(&'a str), } #[derive(Serialize)] - struct ImageSnapshotManifest<'a> { + struct SnapshotManifest<'a> { identity_version: u8, provider: &'static str, tenant: &'static str, - image: &'a str, + #[serde(flatten)] + source: SourceManifest<'a>, cpu: Option, memory_gb: Option, disk_gb: Option, + /// Nothing sets an entrypoint yet. The field stays because removing + /// it would rename every existing snapshot under `IDENTITY_VERSION` 1. entrypoint: Option<&'static str>, } @@ -186,49 +186,26 @@ pub mod snapshot_identity { } fn canonical_manifest(config: &DaytonaSnapshotConfig) -> crate::Result> { - let dockerfile = match (&config.image, &config.dockerfile) { - (Some(image), None) => { - return serde_json::to_vec(&ImageSnapshotManifest { - identity_version: IDENTITY_VERSION, - provider: PROVIDER, - tenant: TENANT, - image, - cpu: config.cpu, - memory_gb: config.memory, - disk_gb: config.disk, - entrypoint: None, - }) - .map_err(|err| { - crate::Error::context("Failed to serialize Daytona snapshot identity", err) - }); + let source = match &config.source { + DaytonaSnapshotSource::Image(image) => SourceManifest::Image(image), + DaytonaSnapshotSource::Dockerfile(DockerfileSource::Inline(text)) => { + SourceManifest::DockerfileSha256(hex::encode(Sha256::digest(text.as_bytes()))) } - (Some(_), Some(_)) => { - return Err(crate::Error::message( - "Daytona custom snapshots accept either image.docker or image.dockerfile, not both", - )); - } - (None, None) => { - return Err(crate::Error::message( - "Daytona custom snapshots require image.docker or image.dockerfile", - )); - } - (None, Some(DockerfileSource::Inline(text))) => text.as_str(), - (None, Some(DockerfileSource::Path { .. })) => { + DaytonaSnapshotSource::Dockerfile(DockerfileSource::Path { .. }) => { return Err(crate::Error::message( "Daytona snapshot dockerfile path should have been resolved to inline content before sandbox creation", )); } }; - let dockerfile_sha256 = hex::encode(Sha256::digest(dockerfile.as_bytes())); let manifest = SnapshotManifest { - identity_version: IDENTITY_VERSION, - provider: PROVIDER, - tenant: TENANT, - dockerfile_sha256: &dockerfile_sha256, - cpu: config.cpu, - memory_gb: config.memory, - disk_gb: config.disk, - entrypoint: None, + identity_version: IDENTITY_VERSION, + provider: PROVIDER, + tenant: TENANT, + source, + cpu: config.cpu, + memory_gb: config.memory, + disk_gb: config.disk, + entrypoint: None, }; serde_json::to_vec(&manifest).map_err(|err| { crate::Error::context("Failed to serialize Daytona snapshot identity", err) @@ -240,26 +217,16 @@ fn create_snapshot_params( name: &str, config: &DaytonaSnapshotConfig, ) -> crate::Result { - let image = match (&config.image, &config.dockerfile) { - (Some(image), None) => daytona_sdk::ImageSource::Name(image.clone()), - (None, Some(DockerfileSource::Inline(dockerfile))) => { + let image = match &config.source { + DaytonaSnapshotSource::Image(image) => daytona_sdk::ImageSource::Name(image.clone()), + DaytonaSnapshotSource::Dockerfile(DockerfileSource::Inline(dockerfile)) => { daytona_sdk::ImageSource::Custom(daytona_sdk::DockerImage::from_dockerfile(dockerfile)) } - (None, Some(DockerfileSource::Path { .. })) => { + DaytonaSnapshotSource::Dockerfile(DockerfileSource::Path { .. }) => { return Err(crate::Error::message(format!( "Snapshot '{name}': dockerfile path should have been resolved to inline content before sandbox creation" ))); } - (Some(_), Some(_)) => { - return Err(crate::Error::message(format!( - "Snapshot '{name}': image.docker and image.dockerfile cannot both be configured" - ))); - } - (None, None) => { - return Err(crate::Error::message(format!( - "Snapshot '{name}' does not exist and no image or dockerfile was provided to create it" - ))); - } }; Ok(daytona_sdk::CreateSnapshotParams { @@ -1511,12 +1478,7 @@ impl Sandbox for DaytonaSandbox { }); let init_start = Instant::now(); - let params = if let Some(snap_cfg) = self - .config - .snapshot - .as_ref() - .filter(|snapshot| snapshot.image.is_some() || snapshot.dockerfile.is_some()) - { + let params = if let Some(snap_cfg) = self.config.snapshot.as_ref() { let api_key = self.api_key.as_deref().ok_or_else(|| { self.fail_init( init_start, @@ -3695,11 +3657,10 @@ mod tests { #[test] fn computed_snapshot_identity_is_deterministic_and_keyed() { let config = DaytonaSnapshotConfig { - cpu: Some(2), - memory: Some(4), - disk: Some(10), - image: None, - dockerfile: Some(DockerfileSource::Inline( + cpu: Some(2), + memory: Some(4), + disk: Some(10), + source: DaytonaSnapshotSource::Dockerfile(DockerfileSource::Inline( "FROM ubuntu:24.04\nRUN apt-get update".to_string(), )), }; @@ -3722,17 +3683,18 @@ mod tests { #[test] fn computed_snapshot_identity_changes_for_generation_inputs() { let base = DaytonaSnapshotConfig { - cpu: Some(2), - memory: Some(4), - disk: Some(10), - image: None, - dockerfile: Some(DockerfileSource::Inline("FROM ubuntu:24.04".to_string())), + cpu: Some(2), + memory: Some(4), + disk: Some(10), + source: DaytonaSnapshotSource::Dockerfile(DockerfileSource::Inline( + "FROM ubuntu:24.04".to_string(), + )), }; let base_name = snapshot_identity::snapshot_name("dtn_secret", &base).unwrap(); let cases = [ DaytonaSnapshotConfig { - dockerfile: Some(DockerfileSource::Inline( + source: DaytonaSnapshotSource::Dockerfile(DockerfileSource::Inline( "FROM ubuntu:24.04\n# roll cache".to_string(), )), ..base.clone() @@ -3760,11 +3722,10 @@ mod tests { #[test] fn computed_snapshot_identity_excludes_raw_dockerfile_and_key_material() { let config = DaytonaSnapshotConfig { - cpu: None, - memory: None, - disk: None, - image: None, - dockerfile: Some(DockerfileSource::Inline( + cpu: None, + memory: None, + disk: None, + source: DaytonaSnapshotSource::Dockerfile(DockerfileSource::Inline( "FROM private.example.com/secret-image\nRUN echo raw-secret".to_string(), )), }; @@ -3780,31 +3741,29 @@ mod tests { #[test] fn computed_snapshot_identity_changes_for_image_reference() { let config = DaytonaSnapshotConfig { - cpu: Some(2), - memory: Some(4), - disk: Some(10), - image: Some("ubuntu:24.04".to_string()), - dockerfile: None, + cpu: Some(2), + memory: Some(4), + disk: Some(10), + source: DaytonaSnapshotSource::Image("ubuntu:24.04".to_string()), }; let first = snapshot_identity::snapshot_name("dtn_secret", &config).unwrap(); let changed = snapshot_identity::snapshot_name("dtn_secret", &DaytonaSnapshotConfig { - image: Some("ubuntu:24.10".to_string()), + source: DaytonaSnapshotSource::Image("ubuntu:24.10".to_string()), ..config }) .unwrap(); + assert_eq!(first, "fabro-5d23a023-d7ff-8d68-b3ca-e6286f4211d9"); assert_ne!(first, changed); - assert!(!first.contains("ubuntu")); } #[test] fn snapshot_creation_uses_named_image_source() { let config = DaytonaSnapshotConfig { - cpu: Some(2), - memory: Some(4), - disk: Some(10), - image: Some("ubuntu:24.04".to_string()), - dockerfile: None, + cpu: Some(2), + memory: Some(4), + disk: Some(10), + source: DaytonaSnapshotSource::Image("ubuntu:24.04".to_string()), }; let params = create_snapshot_params("fabro-test", &config).unwrap(); @@ -3824,11 +3783,12 @@ mod tests { async fn ensure_snapshot_uses_computed_snapshot_name_for_daytona_api_calls() { let api_key = "dtn_secret"; let snapshot = DaytonaSnapshotConfig { - cpu: Some(2), - memory: Some(4), - disk: Some(10), - image: None, - dockerfile: Some(DockerfileSource::Inline("FROM ubuntu:24.04".to_string())), + cpu: Some(2), + memory: Some(4), + disk: Some(10), + source: DaytonaSnapshotSource::Dockerfile(DockerfileSource::Inline( + "FROM ubuntu:24.04".to_string(), + )), }; let computed_name = snapshot_identity::snapshot_name(api_key, &snapshot).unwrap(); let server = MockServer::start_async().await; diff --git a/lib/components/fabro-sandbox/src/from_environment.rs b/lib/components/fabro-sandbox/src/from_environment.rs index b6178e4eb..1689ddc83 100644 --- a/lib/components/fabro-sandbox/src/from_environment.rs +++ b/lib/components/fabro-sandbox/src/from_environment.rs @@ -15,7 +15,8 @@ use fabro_types::settings::run::{ #[cfg(feature = "daytona")] use crate::config::{ - DaytonaNetwork, DaytonaSnapshotSettings, DockerfileSource as SandboxDockerfileSource, + DaytonaNetwork, DaytonaSnapshotSettings, DaytonaSnapshotSource, + DockerfileSource as SandboxDockerfileSource, }; #[cfg(feature = "daytona")] use crate::daytona::DaytonaConfig; @@ -28,30 +29,30 @@ pub fn daytona_config_from_environment( settings: &RunEnvironmentSettings, clone: &RunCloneSettings, ) -> DaytonaConfig { - let dockerfile = settings - .image - .dockerfile - .as_ref() - .map(|dockerfile| match dockerfile { - ResolvedDockerfileSource::Inline(text) => SandboxDockerfileSource::Inline(text.clone()), - ResolvedDockerfileSource::Path { path } => { - SandboxDockerfileSource::Path { path: path.clone() } - } - }); - let snapshot = (settings.image.docker.is_some() || dockerfile.is_some()).then(|| { - DaytonaSnapshotSettings { - cpu: settings.resources.cpu, - memory: settings - .resources - .memory - .map(|size| size_to_gb_i32(size.as_bytes())), - disk: settings - .resources - .disk - .map(|size| size_to_gb_i32(size.as_bytes())), - image: settings.image.docker.clone(), - dockerfile, - } + // fabro-config rejects Daytona environments that set both image.docker + // and image.dockerfile. If both still arrive here, the image wins, which + // matches how the Docker provider treats the pair. + let source = match (&settings.image.docker, &settings.image.dockerfile) { + (Some(image), _) => Some(DaytonaSnapshotSource::Image(image.clone())), + (None, Some(ResolvedDockerfileSource::Inline(text))) => Some( + DaytonaSnapshotSource::Dockerfile(SandboxDockerfileSource::Inline(text.clone())), + ), + (None, Some(ResolvedDockerfileSource::Path { path })) => Some( + DaytonaSnapshotSource::Dockerfile(SandboxDockerfileSource::Path { path: path.clone() }), + ), + (None, None) => None, + }; + let snapshot = source.map(|source| DaytonaSnapshotSettings { + cpu: settings.resources.cpu, + memory: settings + .resources + .memory + .map(|size| size_to_gb_i32(size.as_bytes())), + disk: settings + .resources + .disk + .map(|size| size_to_gb_i32(size.as_bytes())), + source, }); DaytonaConfig { @@ -260,8 +261,10 @@ mod tests { let config = daytona_config_from_environment(&settings, &RunCloneSettings::default()); let snapshot = config.snapshot.expect("image should configure a snapshot"); - assert_eq!(snapshot.image.as_deref(), Some("ubuntu:24.04")); - assert!(snapshot.dockerfile.is_none()); + assert_eq!( + snapshot.source, + DaytonaSnapshotSource::Image("ubuntu:24.04".to_string()) + ); assert_eq!(snapshot.cpu, Some(2)); } } diff --git a/lib/components/fabro-workflow/tests/it/daytona_integration.rs b/lib/components/fabro-workflow/tests/it/daytona_integration.rs index 0fd36836b..407500c9d 100644 --- a/lib/components/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/components/fabro-workflow/tests/it/daytona_integration.rs @@ -403,13 +403,15 @@ async fn daytona_snapshot_sandbox() { let config = DaytonaConfig { auto_stop_interval: Some(60), snapshot: Some(DaytonaSnapshotConfig { - cpu: Some(2), - memory: Some(4), - disk: Some(10), - image: None, - dockerfile: Some(fabro_sandbox::daytona::DockerfileSource::Inline( - "FROM ubuntu:22.04\nRUN apt-get update && apt-get install -y ripgrep".to_string(), - )), + cpu: Some(2), + memory: Some(4), + disk: Some(10), + source: fabro_sandbox::daytona::DaytonaSnapshotSource::Dockerfile( + fabro_sandbox::daytona::DockerfileSource::Inline( + "FROM ubuntu:22.04\nRUN apt-get update && apt-get install -y ripgrep" + .to_string(), + ), + ), }), ..DaytonaConfig::default() }; diff --git a/lib/foundation/fabro-config/src/resolve/environment.rs b/lib/foundation/fabro-config/src/resolve/environment.rs index cf13eade3..3f84ee421 100644 --- a/lib/foundation/fabro-config/src/resolve/environment.rs +++ b/lib/foundation/fabro-config/src/resolve/environment.rs @@ -81,7 +81,6 @@ fn resolve_environment_fields( labels: layer.labels.clone().into_inner(), env: layer.env.clone().into_inner(), }; - validate_daytona_image_settings(&environment, path, errors); environment } @@ -201,23 +200,6 @@ fn dockerfile_source(dockerfile: &EnvironmentDockerfileLayer) -> DockerfileSourc } } -fn validate_daytona_image_settings( - environment: &EnvironmentSettings, - path: &str, - errors: &mut Vec, -) { - if environment.provider == EnvironmentProvider::Daytona - && environment.image.docker.is_some() - && environment.image.dockerfile.is_some() - { - errors.push(ResolveError::Invalid { - path: format!("{path}.image"), - reason: "daytona environments accept either image.docker or image.dockerfile, not both" - .to_string(), - }); - } -} - fn validate_provider_capabilities( environment: &EnvironmentSettings, path: &str, @@ -246,6 +228,15 @@ fn validate_provider_capabilities( }); } } - EnvironmentProvider::Daytona => {} + EnvironmentProvider::Daytona => { + if environment.image.docker.is_some() && environment.image.dockerfile.is_some() { + errors.push(ResolveError::Invalid { + path: format!("{path}.image"), + reason: "daytona environments accept either image.docker or image.dockerfile, \ + not both" + .to_string(), + }); + } + } } }