diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index cccc8c85..3f2b49d0 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -2,6 +2,7 @@ use rhai::{Engine, Scope}; pub mod defs; pub mod oi; +pub mod reserved; pub mod runtime; pub mod sysconst; pub mod system; diff --git a/crates/core/src/oi/handler/ingresses.rs b/crates/core/src/oi/handler/ingresses.rs index e6a90f7d..f850f017 100644 --- a/crates/core/src/oi/handler/ingresses.rs +++ b/crates/core/src/oi/handler/ingresses.rs @@ -203,6 +203,9 @@ pub(crate) fn create_site_ingress( ctx: &RequestCtx, ) -> HandlerResult { validate_hostname(¶ms.hostname)?; + // r[impl namespace.reserved] + crate::reserved::check_site_ingress_name(¶ms.name) + .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e.to_string()))?; if matches!(params.tls_provider, TlsProvider::Tailscale) { return Err(OiError::new( ErrorCode::RequirementsInvalid, diff --git a/crates/core/src/oi/handler/ingresses/tests.rs b/crates/core/src/oi/handler/ingresses/tests.rs index 71205525..c248ce39 100644 --- a/crates/core/src/oi/handler/ingresses/tests.rs +++ b/crates/core/src/oi/handler/ingresses/tests.rs @@ -316,3 +316,19 @@ fn discovery_status_and_refresh_without_tailscale() { assert_eq!(code, "requirements_invalid"); assert!(msg.contains("not configured"), "{msg}"); } + +// r[verify namespace.reserved] +// The Tailscale provider disables whatever row holds this name, so a manual +// ingress created under it was permanently stale. +#[test] +fn reserved_site_ingress_name_is_rejected_at_creation() { + let oi = TestOi::new(); + let (code, message) = oi + .call( + "/ingresses/site/create", + json!({ "name": "tailscale", "hostname": "example.com" }), + ) + .unwrap_err(); + assert_eq!(code, "requirements_invalid"); + assert!(message.contains("reserved"), "message: {message}"); +} diff --git a/crates/core/src/oi/handler/volumes.rs b/crates/core/src/oi/handler/volumes.rs index 786a7047..bbbf8c98 100644 --- a/crates/core/src/oi/handler/volumes.rs +++ b/crates/core/src/oi/handler/volumes.rs @@ -122,6 +122,10 @@ pub(crate) fn restore_held( })?, }; + // r[impl namespace.reserved] + crate::reserved::check_site_volume_name(&target_name) + .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e.to_string()))?; + let target_for_lookup = target_name.clone(); let existing = state .db @@ -277,6 +281,10 @@ pub(crate) fn create_site_volume( ) -> HandlerResult { use crate::runtime::site_volumes::{SiteVolumeDef, SiteVolumeKind}; + // r[impl namespace.reserved] + crate::reserved::check_site_volume_name(¶ms.name) + .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e.to_string()))?; + let kind = match params.kind.as_str() { "managed" => SiteVolumeKind::Managed, "bind" => { @@ -517,6 +525,10 @@ pub(crate) fn snapshot_site_volume( ) -> HandlerResult { use crate::runtime::site_volumes::{SiteVolumeDef, SiteVolumeKind}; + // r[impl namespace.reserved] + crate::reserved::check_site_volume_name(¶ms.name) + .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e.to_string()))?; + let (source, source_path) = parse_source_vol_id(¶ms.source, state) .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e))?; @@ -576,6 +588,10 @@ pub(crate) fn promote_site_volume( ) -> HandlerResult { use crate::runtime::site_volumes::{SiteVolumeDef, SiteVolumeKind}; + // r[impl namespace.reserved] + crate::reserved::check_site_volume_name(¶ms.name) + .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e.to_string()))?; + let source_name = params.source.clone(); let source_def = state .db diff --git a/crates/core/src/oi/handler/volumes/tests.rs b/crates/core/src/oi/handler/volumes/tests.rs index feb20d79..1fa31292 100644 --- a/crates/core/src/oi/handler/volumes/tests.rs +++ b/crates/core/src/oi/handler/volumes/tests.rs @@ -415,3 +415,26 @@ fn declared_external_volumes_guard_unmap() { assert_eq!(code, "requirements_invalid"); assert!(msg.contains("declared by app"), "{msg}"); } + +// r[verify namespace.reserved] +// The daemon deletes every site volume under `backup-snap-` at startup, so an +// operator volume created under that prefix was destroyed on the next restart. +#[test] +fn reserved_prefix_is_rejected_at_creation() { + let oi = TestOi::new(); + let (code, message) = oi + .call( + "/volumes/site/create", + json!({ "name": "backup-snap-archive", "kind": "managed" }), + ) + .unwrap_err(); + assert_eq!(code, "requirements_invalid"); + assert!(message.contains("reserved"), "message: {message}"); + + // A name that merely contains the prefix is the operator's. + oi.call( + "/volumes/site/create", + json!({ "name": "my-backup-snap-thing", "kind": "managed" }), + ) + .expect("only the claimed prefix is reserved"); +} diff --git a/crates/core/src/reserved.rs b/crates/core/src/reserved.rs new file mode 100644 index 00000000..241c8784 --- /dev/null +++ b/crates/core/src/reserved.rs @@ -0,0 +1,109 @@ +//! Names Seedling grants itself inside namespaces operators also use. +//! +//! Site volumes and site ingresses are operator-facing namespaces, and the +//! daemon takes names in both: `backup-snap-*` for the transient snapshots a +//! backup run creates, and `tailscale` for the ingress the discovery provider +//! maintains. Both are then recognised later *by name*, and both have a +//! destructive consumer — startup deletes everything under the snapshot +//! prefix, and the provider disables whatever row holds the ingress name. +//! Nothing stopped an operator creating an object with either name first. +//! +//! Two halves are needed and neither suffices alone. Reservation at creation +//! stops new collisions but cannot repair one that already exists, and does +//! not protect against a future code path that forgets to ask. So the +//! destructive consumers also match on *recorded ownership*: the startup +//! sweep skips names present in `site_volumes`, and the Tailscale provider +//! acts only on rows whose source is its own discovery. Reservation makes +//! collisions impossible going forward; ownership checks make them harmless +//! regardless of history. +//! +//! The constants lived apart before — `backup_execution.rs` and +//! `tailscale.rs` knew nothing about each other or about the creation +//! handlers — which is exactly how the gap opened. + +use seedling_protocol::names::{SiteIngressName, SiteVolumeName}; + +/// Site-volume name prefixes the daemon claims. +/// +/// Startup deletes every site volume under these, so an operator volume that +/// happened to match was destroyed on the next daemon start. +pub const RESERVED_SITE_VOLUME_PREFIXES: &[&str] = + &[crate::runtime::backup_execution::SNAPSHOT_NAME_PREFIX]; + +/// Site-ingress names the daemon claims. +pub const RESERVED_SITE_INGRESS_NAMES: &[&str] = + &[crate::runtime::tailscale::TAILSCALE_INGRESS_NAME]; + +/// A name that belongs to the daemon. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ReservedName { + pub name: String, + pub reason: String, +} + +impl std::fmt::Display for ReservedName { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{} is reserved: {}", self.name, self.reason) + } +} + +/// Reject a site-volume name the daemon claims. +/// +/// Creation only — never update or delete. An operator with a legacy +/// `backup-snap-*` volume must still be able to manage it out of existence. +// r[impl namespace.reserved] +pub fn check_site_volume_name(name: &SiteVolumeName) -> Result<(), ReservedName> { + for prefix in RESERVED_SITE_VOLUME_PREFIXES { + if name.as_str().starts_with(prefix) { + return Err(ReservedName { + name: name.as_str().to_owned(), + reason: format!( + "site volume names beginning {prefix:?} are used by backup runs and are \ + deleted at daemon startup" + ), + }); + } + } + Ok(()) +} + +/// Reject a site-ingress name the daemon claims. +/// +/// Creation only, for the same reason as above. +// r[impl namespace.reserved] +pub fn check_site_ingress_name(name: &SiteIngressName) -> Result<(), ReservedName> { + if RESERVED_SITE_INGRESS_NAMES.contains(&name.as_str()) { + return Err(ReservedName { + name: name.as_str().to_owned(), + reason: "this site ingress name is maintained by a discovery provider".to_owned(), + }); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + // r[verify namespace.reserved] + #[test] + fn snapshot_prefix_is_reserved() { + let reserved = SiteVolumeName::new("backup-snap-archive").unwrap(); + assert!(check_site_volume_name(&reserved).is_err()); + let ordinary = SiteVolumeName::new("archive").unwrap(); + assert!(check_site_volume_name(&ordinary).is_ok()); + // A name that merely contains the prefix is fine — only the claim on + // the front of the namespace is the daemon's. + let contains = SiteVolumeName::new("my-backup-snap-thing").unwrap(); + assert!(check_site_volume_name(&contains).is_ok()); + } + + // r[verify namespace.reserved] + #[test] + fn tailscale_ingress_name_is_reserved() { + let reserved = SiteIngressName::new("tailscale").unwrap(); + assert!(check_site_ingress_name(&reserved).is_err()); + let ordinary = SiteIngressName::new("tailscale-manual").unwrap(); + assert!(check_site_ingress_name(&ordinary).is_ok()); + } +} diff --git a/crates/core/src/runtime/history.rs b/crates/core/src/runtime/history.rs index da9ef2b0..37ee2ce1 100644 --- a/crates/core/src/runtime/history.rs +++ b/crates/core/src/runtime/history.rs @@ -78,6 +78,23 @@ pub fn find_instance(db: &Db, id: InstanceId) -> rusqlite::Result rusqlite::Result> { + let mut stmt = db + .conn + .prepare("SELECT DISTINCT display_name FROM resource_instances WHERE app = ?1")?; + let rows = stmt.query_map(params![app], |row| row.get::<_, String>(0))?; + rows.collect() +} + // r[impl identity.components] pub fn find_instances_for_group( db: &Db, diff --git a/crates/core/src/runtime/history/tests.rs b/crates/core/src/runtime/history/tests.rs index de6720a6..e756dabe 100644 --- a/crates/core/src/runtime/history/tests.rs +++ b/crates/core/src/runtime/history/tests.rs @@ -709,3 +709,36 @@ fn delete_instance_clears_observations_and_faults_atomically() { assert_eq!(obs_count, 0, "observations deleted"); assert_eq!(fault_count, 0, "faults deleted"); } + +// r[verify app.uninstall.scope] +// `seedling-{app}-` is not prefix-free: both app names and resource names may +// contain hyphens, so uninstalling `app` matched every unit belonging to a +// sibling called `app-db`, and the retry branch reset and stopped that +// healthy sibling's units every tick while the uninstall never completed. +#[test] +fn display_names_are_scoped_to_their_own_app() { + let db = Db::open_in_memory().expect("open"); + + let instance = |app: &str, name: &str| ResourceInstance { + id: InstanceId::generate(), + app: AppName::new(app).unwrap(), + kind: ResourceKind::Deployment, + name: Some(name.to_owned()), + variant: InstanceVariant::Singleton, + display_name: format!("{app}-{name}"), + }; + + let app = instance("app", "web"); + let sibling = instance("app-db", "web"); + insert_instance(&db, &app).expect("insert app"); + insert_instance(&db, &sibling).expect("insert sibling"); + + let names = display_names_for_app(&db, &AppName::new("app").unwrap()).expect("query"); + assert_eq!(names, vec!["app-web".to_owned()]); + // The sibling's unit is `seedling-app-db-web.service`, which starts with + // `seedling-app-` — the exact match is what excludes it. + assert!( + format!("seedling-{}.service", sibling.display_name).starts_with("seedling-app-"), + "precondition: the sibling really does match the old prefix" + ); +} diff --git a/crates/core/src/runtime/tailscale.rs b/crates/core/src/runtime/tailscale.rs index cee251cb..c242e476 100644 --- a/crates/core/src/runtime/tailscale.rs +++ b/crates/core/src/runtime/tailscale.rs @@ -27,7 +27,7 @@ pub const DEFAULT_SOCKET_PATH: &str = "/var/run/tailscale/tailscaled.sock"; /// Operator-visible name we use for the discovered Tailscale site ingress. /// Matches the provider name so listings read naturally. -const TAILSCALE_INGRESS_NAME: &str = "tailscale"; +pub(crate) const TAILSCALE_INGRESS_NAME: &str = "tailscale"; /// How often the provider polls tailscaled for the current identity. const DEFAULT_POLL_INTERVAL: Duration = Duration::from_secs(60); @@ -334,10 +334,24 @@ impl TailscaleProvider { fn mark_existing_stale(&self, stale: bool) { self.db.call(move |db| { let name = SiteIngressName::new_unchecked(TAILSCALE_INGRESS_NAME); - if let Ok(Some(_)) = site_ingresses::get(db, &name) - && let Err(e) = site_ingresses::set_stale(db, &name, stale) - { - warn!(error = %e, "tailscale: failed to update stale flag"); + // r[impl namespace.reserved] — act on the row this provider owns, + // not on whatever holds the name. A manual ingress an operator + // created before the name was reserved was disabled here on the + // provider's first bad poll, and never re-enabled. + match site_ingresses::get(db, &name) { + Ok(Some(def)) if !def.source.is_discovered() => { + warn!( + "tailscale: a manually-created site ingress holds the name \ + {TAILSCALE_INGRESS_NAME:?}; leaving it alone" + ); + } + Ok(Some(_)) => { + if let Err(e) = site_ingresses::set_stale(db, &name, stale) { + warn!(error = %e, "tailscale: failed to update stale flag"); + } + } + Ok(None) => {} + Err(e) => warn!(error = %e, "tailscale: failed to look up the discovered ingress"), } }); } @@ -435,9 +449,19 @@ fn upsert_discovered_row( // case, which matches the design intent: a node-id change is "the // host identity changed" and attachments shouldn't silently follow. let name = SiteIngressName::new_unchecked(TAILSCALE_INGRESS_NAME); - if let Some(existing) = site_ingresses::get(db, &name)? - && existing.source.is_discovered() - { + // r[impl namespace.reserved] — a manual row under this name is the + // operator's. Falling through to `create` collided on the primary key + // every poll, so the provider livelocked and the operator's row stayed + // stale forever. + if let Some(existing) = site_ingresses::get(db, &name)? { + if !existing.source.is_discovered() { + warn!( + "tailscale: a manually-created site ingress holds the name \ + {TAILSCALE_INGRESS_NAME:?}; not creating the discovered one. Rename or remove \ + it to let discovery take over." + ); + return Ok(()); + } site_ingresses::delete(db, &name)?; info!( stale_node_id = ?match &existing.source { diff --git a/crates/core/src/system/reconcile.rs b/crates/core/src/system/reconcile.rs index bd058c6d..b0017c1e 100644 --- a/crates/core/src/system/reconcile.rs +++ b/crates/core/src/system/reconcile.rs @@ -28,7 +28,12 @@ use crate::{ stopped, }, system::{ - System, actuator::Actuator, caddy, observer::Observer, resolver, types::DataPlaneRules, + System, + actuator::Actuator, + caddy, + observer::Observer, + resolver, + types::{DataPlaneRules, UnitSummary}, }, }; @@ -442,6 +447,21 @@ pub struct Reconciler { site_resolver: Option>, } +/// Units that could belong to `app`, judged only by the shape of their names. +/// +/// This is a candidate enumeration and never a decision — `seedling-{app}-` is +/// not prefix-free, so `app-db`'s units land here too. It exists for the one +/// question the recorded identities cannot answer when there are none of them: +/// is there anything at all that might still be this app's? +// r[impl app.uninstall.scope] +fn units_possibly_of<'a>(units: &'a [UnitSummary], app: &AppName) -> Vec<&'a UnitSummary> { + let prefix = format!("seedling-{app}-"); + units + .iter() + .filter(|u| u.name.starts_with(&prefix)) + .collect() +} + impl Reconciler { #[expect( clippy::too_many_arguments, @@ -1593,8 +1613,92 @@ impl Reconciler { if !running.is_empty() { continue; } - let unit_prefix = format!("seedling-{}-", app.name); - match self.driver.process.list_units(&unit_prefix).await { + // r[impl app.uninstall.scope] — match units against the + // identities the registry recorded, not against a name prefix. + // `seedling-{app}-` is not prefix-free: uninstalling `app` matched + // every unit of a sibling called `app-db`, and the retry branch + // then reset and stopped that healthy sibling's units every five + // seconds while this uninstall never completed. + let app_name_for_query = app.name.clone(); + let expected: HashSet = match self.db.call(move |db| { + crate::runtime::history::display_names_for_app(db, &app_name_for_query) + }) { + Ok(names) => names + .into_iter() + .map(|display_name| format!("seedling-{display_name}.service")) + .collect(), + // Fail safe: an empty expected set makes every unit invisible, + // which would read as "teardown finished" and delete the + // registry rows while the units are still loaded. Wait for a + // tick where the identities can actually be read. + Err(e) => { + warn!( + app = %app.name, + "uninstall: could not load recorded instance names; \ + not advancing teardown this tick: {e}" + ); + continue; + } + }; + // An empty recorded set is ambiguous in exactly the way this + // change is about. It is the truth for an app that was registered + // but never scheduled, and it is also what a GC sweep leaves + // behind if it reaps the rows while units are still loaded — and + // an empty `expected` filters every unit away, which the branch + // below reads as "teardown finished". Distinguish the two by + // asking whether anything that could be ours is still loaded. + // `seedling-{app}-` is not prefix-free, so it may over-match a + // sibling; over-matching only makes this refuse to conclude, + // which is the safe direction. + if expected.is_empty() { + match self.driver.process.list_units("seedling-").await { + Ok(units) if !units_possibly_of(&units, &app.name).is_empty() => { + tracing::error!( + app = %app.name, + "uninstall: units may still be loaded for this app but no recorded \ + identities remain to match them against; not advancing teardown" + ); + let app_name_owned = app.name.clone(); + self.db.call(move |db| { + let _ = crate::runtime::faults::file_once( + db, + &crate::runtime::faults::FaultKey::new( + &app_name_owned, + "uninstall_unidentifiable_units", + "", + ), + &crate::runtime::faults::FaultMeta::default(), + "units remain under this app's prefix but no recorded instance \ + identities remain to match them against, so teardown cannot be \ + safely completed", + ); + }); + continue; + } + Ok(_) => {} + Err(e) => { + warn!( + app = %app.name, + "uninstall: could not list units to corroborate an empty recorded \ + set; not advancing teardown this tick: {e}" + ); + continue; + } + } + } + // The prefix scan only enumerates candidates; the decision is the + // exact match against `expected`. + match self + .driver + .process + .list_units("seedling-") + .await + .map(|units| { + units + .into_iter() + .filter(|u| expected.contains(&u.name)) + .collect::>() + }) { Ok(units) if units.is_empty() => { let app_name_owned = app.name.clone(); let phase_handle = Arc::clone(&app.phase_handle); @@ -1612,6 +1716,14 @@ impl Reconciler { ) { warn!(app = %app_name_owned, "failed to clean up resource instances during uninstall: {e}"); } + // The other half of `uninstall_unidentifiable_units`: + // teardown finishing is the only thing that resolves + // it, and it must not outlive the app it describes. + let _ = crate::runtime::faults::clear_faults_by_kind( + db, + &app_name_owned, + "uninstall_unidentifiable_units", + ); }); // i[impl event.types] // Uninstall is reconciler-driven and therefore emits no @@ -1652,6 +1764,54 @@ impl Reconciler { #[cfg(test)] mod tests { + + fn unit(name: &str) -> UnitSummary { + UnitSummary { + name: name.to_owned(), + state: Default::default(), + } + } + + // r[verify app.uninstall.scope] + #[test] + fn no_recorded_identities_with_units_still_loaded_is_not_teardown_finished() { + let units = [ + unit("seedling-app-web.service"), + unit("seedling-other-web.service"), + ]; + let app = AppName::new_unchecked("app"); + assert!( + !units_possibly_of(&units, &app).is_empty(), + "with no recorded identities left, a loaded unit under the app's own prefix is the \ + only remaining evidence that teardown has not finished; treating the empty record \ + set as completion deletes the rows while the unit is still loaded" + ); + } + + // r[verify app.uninstall.scope] + #[test] + fn no_recorded_identities_and_no_units_is_genuinely_nothing_to_tear_down() { + let units = [unit("seedling-other-web.service")]; + let app = AppName::new_unchecked("app"); + assert!( + units_possibly_of(&units, &app).is_empty(), + "an app registered but never scheduled has no rows and no units, and must still be \ + able to finish uninstalling" + ); + } + + // r[verify app.uninstall.scope] + #[test] + fn the_candidate_scan_may_over_match_a_sibling_but_only_towards_caution() { + let units = [unit("seedling-app-db-web.service")]; + let app = AppName::new_unchecked("app"); + assert!( + !units_possibly_of(&units, &app).is_empty(), + "`seedling-app-` also matches `app-db`; over-matching here only withholds a \ + completion, never stops a sibling's unit — the decision itself is still the exact \ + match against recorded identity" + ); + } use std::collections::HashMap; use crate::runtime::registry::{RegistryError, ScaledGroup}; diff --git a/crates/core/src/system/translate/proxy.rs b/crates/core/src/system/translate/proxy.rs index 099f00d1..b274a59c 100644 --- a/crates/core/src/system/translate/proxy.rs +++ b/crates/core/src/system/translate/proxy.rs @@ -4,6 +4,7 @@ use std::{ }; use ipnet::Ipv6Net; +use sha2::{Digest, Sha256}; use crate::{ defs::ingress::IngressDef, @@ -77,12 +78,47 @@ pub fn instance_ipv6(node_prefix: &Ipv6Net, instance: &ResourceInstance) -> Ipv6 /// Derives the pod network /64 prefix for a pod instance. /// -/// Prefix layout: `fd5e:edXX:XXXX:KKUU::/64` — identical to `instance_ipv6` -/// but with the interface ID (bytes 8–15) zeroed. +/// Prefix layout: `fd5e:edXX:XXXX:KKSS::/64`, where `KK` is the resource kind +/// and `SS` is derived from the instance's **full** identity. +/// +/// `SS` used to be `uuid[0]`, which is *zero* for static Jobs — their +/// `InstanceId` is nil. Every static Job on the node, in every app, therefore +/// derived the identical /64, and netavark rejects the second network on a +/// duplicate subnet. Hashing the full identity (app, kind, name, whole UUID) +/// removes that deterministic collision: two distinct Jobs now differ even +/// when both their UUIDs are nil. +/// +/// It does not fix the *probabilistic* half. `SS` is still eight bits, so +/// scaled replicas of one deployment birthday-collide at the same rate as +/// before. Widening it means taking byte 6 as well, and byte 6 is the kind +/// discriminant that keeps pod /64s disjoint from the service /128 space and +/// from the `fffe` infrastructure addresses — not something to overload +/// without deciding what those namespaces mean. The real fix is allocation +/// rather than derivation: an `instance_id → subnet id` table with a unique +/// index, allocated at first actuation and freed at GC, which needs a +/// database handle in what is currently a pure translate layer. Left as the +/// next step rather than half-done. +/// +/// Changing the derivation re-homes each instance at its next pod recreation, +/// when its per-instance network is torn down and remade; no flag day. +// r[impl infra.pod.subnet] pub fn pod_network_prefix(node_prefix: &Ipv6Net, instance: &ResourceInstance) -> Ipv6Net { - let addr = instance_ipv6(node_prefix, instance); - let mut bytes = addr.octets(); - bytes[8..].fill(0); + debug_assert_eq!(node_prefix.prefix_len(), 48, "node prefix must be /48"); + + let mut hasher = Sha256::new(); + hasher.update(instance.app.as_str().as_bytes()); + hasher.update([0]); + hasher.update([instance.kind as u8]); + hasher.update([0]); + hasher.update(instance.name.as_deref().unwrap_or("").as_bytes()); + hasher.update([0]); + hasher.update(instance.id.0.as_bytes()); + let digest = hasher.finalize(); + + let mut bytes = [0u8; 16]; + bytes[..6].copy_from_slice(&node_prefix.network().octets()[..6]); + bytes[6] = instance.kind as u8; + bytes[7] = digest[0]; Ipv6Net::new(Ipv6Addr::from(bytes), 64).expect("64 is a valid IPv6 prefix length") } @@ -309,14 +345,45 @@ mod tests { assert_eq!(&octets[8..], &[0u8; 8]); } + // r[verify infra.pod.subnet] + // The pod /64 no longer shares its low byte with the instance address — + // that byte is a hash of the full identity now — but it keeps the kind + // discriminant, which is what holds pod /64s disjoint from the service + // /128 space and the `fffe` infrastructure addresses. #[test] - fn pod_prefix_matches_instance_address_upper_64() { + fn pod_prefix_keeps_the_kind_discriminant() { let prefix = test_prefix(); let instance = make_instance(ResourceKind::Job); - let addr = instance_ipv6(&prefix, &instance); let net = pod_network_prefix(&prefix, &instance); - // The /64 network address must match the first 8 bytes of the instance address - assert_eq!(&addr.octets()[..8], &net.network().octets()[..8]); + let addr = instance_ipv6(&prefix, &instance); + assert_eq!(net.network().octets()[6], ResourceKind::Job as u8); + assert_eq!(&addr.octets()[..7], &net.network().octets()[..7]); + } + + // r[verify infra.pod.subnet] + // Static Jobs carry a nil InstanceId, so deriving the subnet from + // `uuid[0]` gave every static Job on the node — across every app — the + // same /64, and netavark refuses the second network on a duplicate + // subnet. + #[test] + fn static_jobs_with_nil_ids_get_distinct_subnets() { + let prefix = test_prefix(); + let nil = crate::runtime::identity::InstanceId(uuid::Uuid::nil()); + let job = |app: &str, name: &str| ResourceInstance { + id: nil, + app: seedling_protocol::names::AppName::new(app).unwrap(), + kind: ResourceKind::Job, + name: Some(name.to_owned()), + variant: crate::runtime::identity::InstanceVariant::Singleton, + display_name: format!("{app}-job-{name}"), + }; + + let a = pod_network_prefix(&prefix, &job("alpha", "migrate")); + let b = pod_network_prefix(&prefix, &job("beta", "migrate")); + let c = pod_network_prefix(&prefix, &job("alpha", "vacuum")); + + assert_ne!(a, b, "same job name in different apps must not collide"); + assert_ne!(a, c, "different jobs in one app must not collide"); } #[test] diff --git a/crates/daemon/src/main.rs b/crates/daemon/src/main.rs index 44dd69ba..f8cb0cb6 100644 --- a/crates/daemon/src/main.rs +++ b/crates/daemon/src/main.rs @@ -616,10 +616,83 @@ async fn main() { let prefix = seedling_core::runtime::backup_execution::SNAPSHOT_NAME_PREFIX; match driver_ref.volume_store.list_sites_with_prefix(prefix) { Ok(orphans) if !orphans.is_empty() => { - tracing::warn!( - count = orphans.len(), - "found orphaned backup-execution snapshots from a previous run; removing" - ); + // r[impl namespace.reserved] — a DB row is the record that an + // operator owns this name. Creation rejects the prefix now, + // but that cannot help a volume created before the check + // existed, and this sweep deletes on sight. + /// Why a snapshot-prefixed volume is not an orphan to remove. + /// A definite "the operator owns this" and a "we could not + /// tell" both keep the volume, but they are not the same + /// answer and must not be reported as one. + enum Keep { + Registered, + UnparseableName(String), + OwnershipUnknown(String), + } + + let candidates: Vec = + orphans.iter().map(|n| n.as_str().to_owned()).collect(); + let keeps: std::collections::HashMap = db.call(move |conn| { + candidates + .into_iter() + .filter_map(|name| { + let reason = match seedling_protocol::names::SiteVolumeName::new(&name) + { + Ok(parsed) => { + match seedling_core::runtime::site_volumes::get(conn, &parsed) { + Ok(Some(_)) => Keep::Registered, + Ok(None) => return None, + // A destructive sweep must be + // conservative: "we could not tell" is + // not "not owned". + Err(e) => Keep::OwnershipUnknown(e.to_string()), + } + } + // Also a "could not tell": a name Seedling + // could not have created is not a snapshot of + // ours to delete. + Err(e) => Keep::UnparseableName(e.to_string()), + }; + Some((name, reason)) + }) + .collect() + }); + let orphans: Vec<_> = orphans + .into_iter() + .filter(|name| match keeps.get(name.as_str()) { + None => true, + Some(Keep::Registered) => { + tracing::info!( + volume = %name, + "a registered site volume uses the backup snapshot prefix; \ + leaving it alone" + ); + false + } + Some(Keep::UnparseableName(e)) => { + tracing::warn!( + volume = %name, + "a snapshot-prefixed path is not a valid site volume name, so \ + it is not a snapshot of ours; leaving it alone: {e}" + ); + false + } + Some(Keep::OwnershipUnknown(e)) => { + tracing::warn!( + volume = %name, + "could not determine ownership of a snapshot-prefixed site \ + volume; leaving it alone: {e}" + ); + false + } + }) + .collect(); + if !orphans.is_empty() { + tracing::warn!( + count = orphans.len(), + "found orphaned backup-execution snapshots from a previous run; removing" + ); + } for name in &orphans { tracing::info!(snapshot = %name, "removing orphaned backup snapshot"); tokio::task::block_in_place(|| { diff --git a/docs/spec/runtime.md b/docs/spec/runtime.md index bb00aa1e..f6358bf2 100644 --- a/docs/spec/runtime.md +++ b/docs/spec/runtime.md @@ -1184,6 +1184,22 @@ Some internal operations (for example [backup.list](#r--backup.list), [backup.re > A fault on one resource must not prevent the reconciler from continuing to manage other resources. > The faulted resource is excluded from active reconciliation until the fault is resolved. +> r[namespace.reserved] +> The runtime grants itself names inside namespaces operators also use: site-volume names beginning `backup-snap-`, and the site-ingress name `tailscale`. +> Creating a site volume or site ingress under a reserved name is rejected; the restriction applies to creation only, so an object that predates the reservation can still be renamed or removed. +> Reservation alone is not sufficient: a component that deletes or disables objects it believes are its own must identify them by what the runtime recorded — a registered site volume is never deleted by the snapshot sweep, and a manually created site ingress is never disabled or replaced by a discovery provider — regardless of what the object is named. + +> r[app.uninstall.scope] +> Uninstalling an app affects only resources belonging to that app. +> Resources are identified by the identities the runtime recorded for them, not by the shape of their names: names are not required to be prefix-free, so one app's name may be a prefix of another's. +> A scan by name prefix is permitted only to enumerate candidates that are then matched exactly against recorded identity. +> Having no recorded identities is not on its own evidence that teardown has finished: where the runtime cannot rule out that resources of the app are still present, it must not declare the app uninstalled, and must raise a fault that clears when teardown does complete. + +> r[infra.pod.subnet] +> No two concurrently running pod instances may share a network prefix. +> A prefix must therefore not be derived from a part of an instance's identity that distinct instances can share: in particular, instances carrying no unique identifier of their own must still be distinguished by the rest of their identity. +> Deriving a prefix, however widely, does not on its own satisfy this requirement — only allocating prefixes and recording the allocation makes it a guarantee rather than a probability. + # Resource Identity > r[identity.stable]