From da1d513592c21c4458172490152ad8542e415f91 Mon Sep 17 00:00:00 2001 From: Paco Date: Thu, 6 Aug 2026 13:24:19 -0700 Subject: [PATCH 1/6] feat(acl-agent): add production-quality nebraska client module Add a self-contained `nebraska` module (crates/trident-acl-agent/src/nebraska) implementing the Nebraska/Omaha update protocol, scoped strictly to the protocol (no Trident gRPC, reboot, commit, or CLI concerns). It encodes in the type system the protocol invariants that otherwise fail silently, per the behavioural spec knowledge/topics/nebraska-client-protocol.md. Public API: Client (check_for_update / report_progress / complete_after_reboot / report_failure), CheckOutcome, UpdateOffer, ProgressEvent, MachineId, the AppStatus/UpdateCheckStatus response types, a Transport seam (ReqwestTransport), and NebraskaError. Invariants encoded: - Only the six whitelisted (eventtype,eventresult) pairs are constructible; no raw integers in the public API. - `track` is a Client field, so it cannot be omitted from any request. - error-updateInProgressOnInstance is a first-class CheckOutcome, and unknown status strings map to an Other catch-all rather than failing to parse. - MachineId is a validated, unbraced newtype. - Versions are semver::Version, not String. - Terminal events are emitted only via named methods, and completion uses the batched 3/2 + ping + updatecheck request. The all-or-nothing rule is a documented plain API (a cross-reboot commitment cannot be an in-process typestate); see nebraska/README.md for the rationale. Exposes the crate as a library (new lib.rs) so the module is reusable by a future TAA. Tests are hermetic (mock transport): wire-format, response parsing incl. the in-progress/unknown-status shapes, whitelist coverage, machine-id and semver handling. cargo fmt / clippy -D warnings clean. The existing agent is left unchanged; adoption is described in nebraska/README.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident-acl-agent/src/lib.rs | 8 + .../trident-acl-agent/src/nebraska/README.md | 80 +++ .../trident-acl-agent/src/nebraska/client.rs | 504 ++++++++++++++++++ .../trident-acl-agent/src/nebraska/error.rs | 47 ++ .../trident-acl-agent/src/nebraska/event.rs | 183 +++++++ crates/trident-acl-agent/src/nebraska/id.rs | 117 ++++ crates/trident-acl-agent/src/nebraska/mod.rs | 78 +++ .../trident-acl-agent/src/nebraska/status.rs | 143 +++++ .../src/nebraska/transport.rs | 48 ++ crates/trident-acl-agent/src/nebraska/wire.rs | 475 +++++++++++++++++ 10 files changed, 1683 insertions(+) create mode 100644 crates/trident-acl-agent/src/lib.rs create mode 100644 crates/trident-acl-agent/src/nebraska/README.md create mode 100644 crates/trident-acl-agent/src/nebraska/client.rs create mode 100644 crates/trident-acl-agent/src/nebraska/error.rs create mode 100644 crates/trident-acl-agent/src/nebraska/event.rs create mode 100644 crates/trident-acl-agent/src/nebraska/id.rs create mode 100644 crates/trident-acl-agent/src/nebraska/mod.rs create mode 100644 crates/trident-acl-agent/src/nebraska/status.rs create mode 100644 crates/trident-acl-agent/src/nebraska/transport.rs create mode 100644 crates/trident-acl-agent/src/nebraska/wire.rs diff --git a/crates/trident-acl-agent/src/lib.rs b/crates/trident-acl-agent/src/lib.rs new file mode 100644 index 0000000000..eb96512e1e --- /dev/null +++ b/crates/trident-acl-agent/src/lib.rs @@ -0,0 +1,8 @@ +//! Library surface for the `trident-acl-agent` crate. +//! +//! Currently this exposes the [`nebraska`] client module, a self-contained, +//! reusable implementation of the Nebraska/Omaha update protocol. It is usable +//! both by this crate's agent binary and by a future Trident ACL Agent that +//! orchestrates updates differently. + +pub mod nebraska; diff --git a/crates/trident-acl-agent/src/nebraska/README.md b/crates/trident-acl-agent/src/nebraska/README.md new file mode 100644 index 0000000000..bfae3fc4ae --- /dev/null +++ b/crates/trident-acl-agent/src/nebraska/README.md @@ -0,0 +1,80 @@ +# `nebraska` client module — design and adoption notes + +A self-contained Rust client for the [Nebraska](https://github.com/flatcar/nebraska) +update server (Omaha protocol), living in `crates/trident-acl-agent/src/nebraska/`. + +The authoritative behavioural spec is +`knowledge/topics/nebraska-client-protocol.md` in the `pacobot` repository. This +document only covers how the module maps that spec into a Rust API and how the +existing agent would adopt it. + +## Public API + +| Item | Purpose | +| --- | --- | +| `Client` | A client bound to one app + track + machine id. Methods: `check_for_update`, `report_progress`, `complete_after_reboot`, `report_failure`. | +| `CheckOutcome` | `UpToDate` \| `UpdateAvailable(UpdateOffer)` \| `UpdateInProgress`. The last models `error-updateInProgressOnInstance` as an expected outcome, not an error. | +| `UpdateOffer` | `{ version: semver::Version, package_url: Url }` — the resolved package URL (codebase joined with package name). | +| `ProgressEvent` | `DownloadStarted` \| `DownloadFinished` \| `Installed`. The only publicly constructible events; they map to the whitelisted wire pairs `13/1`, `14/1`, `800/1`. | +| `MachineId` | Validated, unbraced instance id. `from_uuid` / `new`. | +| `AppStatus`, `UpdateCheckStatus` | Response statuses with an `Other(String)` catch-all so unknown values never break parsing. | +| `Transport`, `ReqwestTransport` | The HTTP seam; injectable for hermetic tests. | +| `NebraskaError` | The module error type (`thiserror`). | + +Terminal events (`3/2` complete, `3/0` failure) are **not** public values — they +are emitted only through `complete_after_reboot` and `report_failure`, so they +always carry the correct request shape (e.g. the batched update-check that +completion requires). + +## How the invariants are encoded + +- **Whitelisted events only** — raw `(type, result)` integers are private; the + public vocabulary (`ProgressEvent` + the terminal methods) can only produce the + six accepted pairs. A unit test asserts this. +- **`track` mandatory** — a field of `Client`; no request can be built without it. +- **Unbraced, stable machine id** — `MachineId` rejects braced ids; `from_uuid` + uses Rust's unbraced `Display`. +- **`error-updateInProgressOnInstance` is expected** — surfaced as + `CheckOutcome::UpdateInProgress`; unknown statuses map to `Other`. +- **Real semver version** — the API takes `&semver::Version`, and offered + versions are parsed as semver. +- **All-or-nothing event reporting** — see the design decision below. + +## Design decision: invariant #2 (all-or-nothing) is a documented plain API, not a typestate + +Sending progress events commits the caller to a terminal event, and **the +terminal event fires after a reboot — in a different process** from the progress +events. No in-process typestate or RAII guard can span that boundary; worse, an +RAII "you didn't finish" guard would fire at the drop that happens *at* reboot, +which is exactly when completion must *not* be reported. A compile-time +"started ⇒ must-finish" is therefore structurally impossible here. + +Instead the property is encoded three ways that actually hold: + +1. Terminal events are not free-standing values; they are dedicated `Client` + methods, so a terminal cannot be sent in the wrong shape or context. +2. The only-whitelisted-pairs property is total, so no invalid event exists. +3. `complete_after_reboot` is the batched `3/2 + ping + updatecheck` request, + making the safe post-reboot path (which closes the wedge window) the easy one. + +Persisting "an update is in flight (previous X, target Y)" across the reboot is +the caller's responsibility — it is orchestration, deliberately out of this +module's scope — but the module makes the correct post-reboot call trivial. + +## Adopting this in the agent + +The current agent (`main.rs` + the ad-hoc `omaha` module) predates this module. +A future change would: + +1. Replace `omaha::send` / `query_and_fetch_document` / `report_event` with a + `nebraska::Client` built from the CLI args (`endpoint`, `appid`, `track`) and a + `MachineId` derived from `IdSource`. +2. Map the poll loop's results onto `CheckOutcome` (the agent already distinguishes + no-update / in-progress / available). +3. In `--events full`, call `report_progress` around the Trident stage/finalize, + persist the in-flight state to `/var`, and after the reboot call + `complete_after_reboot` (with retry) as the first request. +4. Delete the `omaha` module once nothing references it. + +This module contains no Trident gRPC, reboot, commit, or CLI logic, so that +adoption is purely at the protocol seam. diff --git a/crates/trident-acl-agent/src/nebraska/client.rs b/crates/trident-acl-agent/src/nebraska/client.rs new file mode 100644 index 0000000000..b8f5d1689e --- /dev/null +++ b/crates/trident-acl-agent/src/nebraska/client.rs @@ -0,0 +1,504 @@ +//! The high-level [`Client`] for talking to a Nebraska server. + +use log::{debug, trace}; +use semver::Version; +use url::Url; + +use super::{ + error::NebraskaError, + event::{ProgressEvent, TerminalEvent}, + id::MachineId, + transport::{ReqwestTransport, Transport}, + wire::{self, App}, +}; + +/// The outcome of an update check. +/// +/// `UpdateInProgress` is a first-class outcome rather than an error because +/// Nebraska returns it on **every** poll between the first progress event and +/// the terminal event; it is expected server behaviour (protocol spec §4 and +/// §7 trap 5). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CheckOutcome { + /// No update is available; the instance is up to date. + UpToDate, + + /// An update is available. + UpdateAvailable(UpdateOffer), + + /// Nebraska reports an update is already in progress for this instance. + /// Expected while an update is mid-flight; the caller should keep polling + /// (or, post-reboot, report completion) rather than treat it as an error. + UpdateInProgress, +} + +/// An offered update: the version and the fully-resolved package URL. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct UpdateOffer { + /// The version being offered. + pub version: Version, + + /// The absolute URL of the update package, resolved by joining the + /// response's `codebase` with the package `name`. + pub package_url: Url, +} + +/// A client for a single Nebraska app on a single track. +/// +/// The client bundles the immutable request identity — endpoint, app id, +/// `track`, and [`MachineId`]. Because these are required to construct the +/// client and every request flows through it, two protocol invariants hold +/// structurally: `track` is present on every request including event-only ones +/// (protocol spec §7 trap 4), and the machine id is always a validated, unbraced +/// value (§7 trap 2). +/// +/// # Event ordering and the all-or-nothing rule +/// +/// Emitting a [progress event](Client::report_progress) is a **commitment** to +/// eventually emit a terminal event: leaving an instance in a progress state +/// wedges it permanently, with no server-side self-heal (protocol spec §3). The +/// terminal event is sent *after the reboot* — i.e. from a different process — +/// so this cannot be enforced at compile time; instead the terminal operations +/// are exposed as dedicated, hard-to-forget methods +/// ([`complete_after_reboot`](Client::complete_after_reboot) and +/// [`report_failure`](Client::report_failure)), and the caller is responsible +/// for persisting enough state across the reboot to make that call. +/// +/// A client that sends **no** events at all is always safe: Nebraska self-heals +/// the instance to Complete on the next check at the new version. Prefer that +/// over sending a partial sequence. +pub struct Client { + endpoint: Url, + app_id: String, + track: String, + machine_id: MachineId, + transport: T, +} + +impl Client { + /// Creates a client using the default blocking `reqwest` transport. + /// + /// `endpoint` should be the Nebraska update URL (typically ending in + /// `/v1/update/`, with the trailing slash preserved). + pub fn new( + endpoint: Url, + app_id: impl Into, + track: impl Into, + machine_id: MachineId, + ) -> Self { + Self::with_transport(endpoint, app_id, track, machine_id, ReqwestTransport::new()) + } +} + +impl Client { + /// Creates a client with an explicit [`Transport`], primarily for testing. + pub fn with_transport( + endpoint: Url, + app_id: impl Into, + track: impl Into, + machine_id: MachineId, + transport: T, + ) -> Self { + Self { + endpoint, + app_id: app_id.into(), + track: track.into(), + machine_id, + transport, + } + } + + /// Checks for an available update, reporting `current_version` as the + /// instance's current version. + /// + /// `current_version` **must be the real version** and valid semver: a client + /// reporting `0.0.0` is offered an update on every poll forever, and a + /// non-semver version fails instance registration server-side (protocol spec + /// §8). + pub fn check_for_update( + &self, + current_version: &Version, + ) -> Result { + let app = self.app(current_version).with_update_check(); + let response = self.send(app)?; + self.interpret_check(response) + } + + /// Reports a [`ProgressEvent`] for an in-flight update. + /// + /// Only valid after a successful [`check_for_update`](Client::check_for_update) + /// has caused Nebraska to grant the update (Nebraska rejects events from an + /// instance it has never seen; protocol spec §7 trap 5). Emitting a progress + /// event commits the caller to eventually reporting a terminal event — see + /// the [type docs](Client). + pub fn report_progress( + &self, + current_version: &Version, + event: ProgressEvent, + ) -> Result<(), NebraskaError> { + let app = self.app(current_version).with_event(event.wire()); + let response = self.send(app)?; + self.require_app_present(&response)?; + Ok(()) + } + + /// Reports successful completion after the reboot, in the single batched + /// request Nebraska expects: a terminal `complete` event plus a `` + /// plus an ``. + /// + /// Nebraska processes the event before the update check within one request, + /// so this both moves the instance to Complete and returns a clean + /// `noupdate` in one round trip — closing the window in which a bare + /// post-reboot poll would hit `error-updateInProgressOnInstance` (protocol + /// spec §4). This is the terminal event that discharges the commitment made + /// by [`report_progress`](Client::report_progress), and it must be retried + /// until it lands: losing it wedges the instance permanently. + /// + /// `previous_version` is the version the instance was on before the update; + /// `current_version` is the (new) version now running. + pub fn complete_after_reboot( + &self, + previous_version: &Version, + current_version: &Version, + ) -> Result { + let app = self + .app(current_version) + .with_event(TerminalEvent::Completed.wire()) + .with_previous_version(previous_version.to_string()) + .with_ping() + .with_update_check(); + let response = self.send(app)?; + // The instance should now be Complete; a still-in-progress status means + // the completion did not take and the caller should retry. + self.interpret_check(response) + } + + /// Reports a failed update (terminal `3/0`), which moves the instance to + /// Error, clears `update_in_progress`, and re-arms it so a subsequent check + /// can grant again (protocol spec §6). This is the "reset and retry" path + /// for a wedged or failed update. + pub fn report_failure( + &self, + previous_version: &Version, + current_version: &Version, + ) -> Result<(), NebraskaError> { + let app = self + .app(current_version) + .with_event(TerminalEvent::Failed.wire()) + .with_previous_version(previous_version.to_string()); + let response = self.send(app)?; + self.require_app_present(&response)?; + Ok(()) + } + + /// Builds the base `` for a request, carrying the client identity. + fn app(&self, version: &Version) -> App { + App::new( + self.app_id.clone(), + version.to_string(), + self.track.clone(), + self.machine_id.to_string(), + ) + } + + /// Serializes, sends, and parses a request/response round-trip. + fn send(&self, app: App) -> Result { + let request = wire::request_for(app); + let body = request + .to_xml() + .map_err(|e| NebraskaError::Serialize(e.to_string()))?; + trace!( + "Nebraska request to '{}':\n{}", + self.endpoint, + String::from_utf8_lossy(&body) + ); + let text = self.transport.post_xml(&self.endpoint, &body)?; + trace!("Nebraska response:\n{text}"); + wire::parse_response(&text).map_err(NebraskaError::Parse) + } + + /// Locates this client's app in a response, validating it is present and + /// that its id matches. + fn app_response<'r>( + &self, + response: &'r wire::Response, + ) -> Result<&'r wire::AppResponse, NebraskaError> { + match response.apps.as_slice() { + [] => Err(NebraskaError::UnexpectedResponse( + "response contained no app".to_string(), + )), + [app] => { + if app.app_id != self.app_id { + return Err(NebraskaError::UnexpectedResponse(format!( + "response app id '{}' does not match requested '{}'", + app.app_id, self.app_id + ))); + } + Ok(app) + } + apps => Err(NebraskaError::UnexpectedResponse(format!( + "expected exactly one app in response, found {}", + apps.len() + ))), + } + } + + /// Validates the app is present (used by event requests, which have no + /// update-check to interpret). + fn require_app_present(&self, response: &wire::Response) -> Result<(), NebraskaError> { + self.app_response(response).map(|_| ()) + } + + /// Interprets a response that carries an update check into a [`CheckOutcome`]. + fn interpret_check(&self, response: wire::Response) -> Result { + let app = self.app_response(&response)?; + + if app.status.is_update_in_progress() { + debug!("Nebraska reports an update already in progress for this instance"); + return Ok(CheckOutcome::UpdateInProgress); + } + + if !app.status.is_ok() { + return Err(NebraskaError::ServerError(app.status.to_string())); + } + + let update_check = app.update_check.as_ref().ok_or_else(|| { + NebraskaError::UnexpectedResponse("app response missing updatecheck".to_string()) + })?; + + if update_check.status.is_no_update() { + return Ok(CheckOutcome::UpToDate); + } + + if !update_check.status.is_update_available() { + return Err(NebraskaError::ServerError(update_check.status.to_string())); + } + + let offer = self.build_offer(update_check)?; + Ok(CheckOutcome::UpdateAvailable(offer)) + } + + /// Builds an [`UpdateOffer`] from a positive update-check response. + fn build_offer( + &self, + update_check: &wire::UpdateCheckResponse, + ) -> Result { + let manifest = update_check.manifest.as_ref().ok_or_else(|| { + NebraskaError::UnexpectedResponse("update available but no manifest".to_string()) + })?; + + let version = Version::parse(&manifest.version).map_err(|e| { + NebraskaError::UnexpectedResponse(format!( + "offered version '{}' is not valid semver: {e}", + manifest.version + )) + })?; + + let codebase = update_check + .urls + .as_ref() + .and_then(|u| u.urls.first()) + .ok_or_else(|| { + NebraskaError::UnexpectedResponse( + "update available but no codebase URL".to_string(), + ) + })?; + + let packages = manifest + .packages + .as_ref() + .map(|p| p.packages.as_slice()) + .unwrap_or(&[]); + let package = match packages { + [package] => package, + [] => { + return Err(NebraskaError::UnexpectedResponse( + "update available but no package listed".to_string(), + )) + } + many => { + return Err(NebraskaError::UnexpectedResponse(format!( + "expected exactly one package, found {}", + many.len() + ))) + } + }; + + // Join the codebase (which must end in a trailing slash) with the + // package name to get the absolute package URL; do not otherwise rewrite + // it. + let package_url = codebase.codebase.join(&package.name).map_err(|e| { + NebraskaError::UnexpectedResponse(format!( + "failed to join codebase '{}' with package '{}': {e}", + codebase.codebase, package.name + )) + })?; + + Ok(UpdateOffer { + version, + package_url, + }) + } +} + +#[cfg(test)] +mod tests { + use std::cell::RefCell; + + use super::*; + + /// A canned transport: records the last request body and returns a fixed + /// response, so client logic is exercised without a network. + struct MockTransport { + response: String, + last_body: RefCell>, + } + + impl MockTransport { + fn new(response: impl Into) -> Self { + Self { + response: response.into(), + last_body: RefCell::new(None), + } + } + } + + impl Transport for MockTransport { + fn post_xml(&self, _endpoint: &Url, body: &[u8]) -> Result { + *self.last_body.borrow_mut() = Some(String::from_utf8_lossy(body).into_owned()); + Ok(self.response.clone()) + } + } + + fn client_with(response: &str) -> Client { + Client::with_transport( + Url::parse("https://nebraska.example/v1/update/").unwrap(), + "app-1", + "stable", + MachineId::new("mid-1").unwrap(), + MockTransport::new(response), + ) + } + + const OFFER: &str = r#" + + + + + + + + + + + "#; + + #[test] + fn check_returns_offer_with_joined_url() { + let client = client_with(OFFER); + let outcome = client + .check_for_update(&Version::new(3, 0, 20260731)) + .unwrap(); + match outcome { + CheckOutcome::UpdateAvailable(offer) => { + assert_eq!(offer.version, Version::new(3, 0, 20260803)); + assert_eq!( + offer.package_url.as_str(), + "http://192.168.122.1:8080/acl-3.0.20260803.cosi" + ); + } + other => panic!("expected an update offer, got {other:?}"), + } + } + + #[test] + fn check_reports_no_update() { + let client = client_with( + r#""#, + ); + assert_eq!( + client + .check_for_update(&Version::new(3, 0, 20260803)) + .unwrap(), + CheckOutcome::UpToDate + ); + } + + #[test] + fn check_maps_update_in_progress() { + let client = client_with( + r#""#, + ); + assert_eq!( + client + .check_for_update(&Version::new(3, 0, 20260803)) + .unwrap(), + CheckOutcome::UpdateInProgress + ); + } + + #[test] + fn check_wrong_app_id_is_error() { + let client = client_with( + r#""#, + ); + let err = client.check_for_update(&Version::new(1, 0, 0)).unwrap_err(); + assert!( + matches!(err, NebraskaError::UnexpectedResponse(_)), + "got {err:?}" + ); + } + + #[test] + fn report_progress_sends_track_and_event() { + let client = client_with( + r#""#, + ); + client + .report_progress( + &Version::new(3, 0, 20260731), + ProgressEvent::DownloadStarted, + ) + .unwrap(); + let body = client.transport.last_body.borrow().clone().unwrap(); + assert!(body.contains(r#"track="stable""#), "{body}"); + assert!( + body.contains(r#""#, + ); + let outcome = client + .complete_after_reboot(&Version::new(3, 0, 20260731), &Version::new(3, 0, 20260803)) + .unwrap(); + assert_eq!(outcome, CheckOutcome::UpToDate); + let body = client.transport.last_body.borrow().clone().unwrap(); + assert!( + body.contains(r#""#, + ); + client + .report_failure(&Version::new(3, 0, 20260731), &Version::new(3, 0, 20260803)) + .unwrap(); + let body = client.transport.last_body.borrow().clone().unwrap(); + assert!( + body.contains(r#"`, so the client cannot detect the mistake from +//! the response (protocol spec §2 and §7 trap 1). To make that class of bug +//! impossible, this module never exposes raw integers: callers work with typed +//! events, and the mapping to wire values is private and total over the +//! whitelist. +//! +//! The events also split into two kinds with very different consequences +//! (protocol spec §3): +//! +//! - **Progress** events ([`ProgressEvent`]) are informational. Sending them is +//! a *commitment* to also send a terminal event, because leaving an instance +//! in a progress state (Downloading/Downloaded/Installed) wedges it +//! permanently. +//! - **Terminal** events move the instance to a final state. They are not +//! exposed as free-standing values precisely so a caller cannot send one in +//! the wrong shape or context; they are emitted only through the dedicated +//! [`Client`](crate::nebraska::Client) methods +//! (`complete_after_reboot`, `report_failure`). + +/// The `eventtype`/`eventresult` wire pair for an Omaha event. +/// +/// Constructed only by this module, and only ever with whitelisted values. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) struct WirePair { + pub(super) event_type: u16, + pub(super) event_result: u8, +} + +/// A progress event reported while an update is being applied. +/// +/// These correspond to the intermediate Nebraska instance states. Emitting any +/// of them commits the caller to eventually reporting a terminal event (success +/// or failure); see the module docs and protocol spec §3. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ProgressEvent { + /// Staging of the update has begun. Wire `(13, 1)` → Nebraska `Downloading`. + DownloadStarted, + /// Staging of the update has finished. Wire `(14, 1)` → Nebraska `Downloaded`. + DownloadFinished, + /// The update has been finalized and the new slot armed. Wire `(800, 1)` → + /// Nebraska `Installed`. + Installed, +} + +impl ProgressEvent { + /// The whitelisted wire pair for this progress event. + pub(super) fn wire(self) -> WirePair { + match self { + ProgressEvent::DownloadStarted => WirePair { + event_type: 13, + event_result: 1, + }, + ProgressEvent::DownloadFinished => WirePair { + event_type: 14, + event_result: 1, + }, + ProgressEvent::Installed => WirePair { + event_type: 800, + event_result: 1, + }, + } + } + + /// A short human-readable label, suitable for operator-facing logging. + pub fn label(self) -> &'static str { + match self { + ProgressEvent::DownloadStarted => "download started", + ProgressEvent::DownloadFinished => "download finished", + ProgressEvent::Installed => "installed", + } + } +} + +/// A terminal event, moving the instance to a final state. +/// +/// Not publicly constructible: terminal events are emitted only by the +/// [`Client`](crate::nebraska::Client) so they always carry the correct +/// surrounding request shape (e.g. the batched update-check that completion +/// requires). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum TerminalEvent { + /// The update completed successfully after reboot. Wire `(3, 2)` (the + /// better-tested "success + reboot" branch for non-Flatcar apps) → Nebraska + /// `Complete`. + Completed, + /// The update failed. Wire `(3, 0)` → Nebraska `Error`; clears + /// `update_in_progress` and re-arms the instance (protocol spec §6). + Failed, +} + +impl TerminalEvent { + pub(super) fn wire(self) -> WirePair { + match self { + TerminalEvent::Completed => WirePair { + event_type: 3, + event_result: 2, + }, + TerminalEvent::Failed => WirePair { + event_type: 3, + event_result: 0, + }, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The exact set of pairs seeded in Nebraska's `event_type` table. + const WHITELIST: &[(u16, u8)] = &[(3, 0), (3, 1), (3, 2), (13, 1), (14, 1), (800, 1)]; + + fn is_whitelisted(p: WirePair) -> bool { + WHITELIST.contains(&(p.event_type, p.event_result)) + } + + #[test] + fn progress_events_are_whitelisted() { + for ev in [ + ProgressEvent::DownloadStarted, + ProgressEvent::DownloadFinished, + ProgressEvent::Installed, + ] { + assert!(is_whitelisted(ev.wire()), "{ev:?} -> {:?}", ev.wire()); + } + } + + #[test] + fn terminal_events_are_whitelisted() { + for ev in [TerminalEvent::Completed, TerminalEvent::Failed] { + assert!(is_whitelisted(ev.wire()), "{ev:?} -> {:?}", ev.wire()); + } + } + + #[test] + fn wire_values_match_spec() { + assert_eq!( + ProgressEvent::DownloadStarted.wire(), + WirePair { + event_type: 13, + event_result: 1 + } + ); + assert_eq!( + ProgressEvent::DownloadFinished.wire(), + WirePair { + event_type: 14, + event_result: 1 + } + ); + assert_eq!( + ProgressEvent::Installed.wire(), + WirePair { + event_type: 800, + event_result: 1 + } + ); + assert_eq!( + TerminalEvent::Completed.wire(), + WirePair { + event_type: 3, + event_result: 2 + } + ); + assert_eq!( + TerminalEvent::Failed.wire(), + WirePair { + event_type: 3, + event_result: 0 + } + ); + } +} diff --git a/crates/trident-acl-agent/src/nebraska/id.rs b/crates/trident-acl-agent/src/nebraska/id.rs new file mode 100644 index 0000000000..71188b327f --- /dev/null +++ b/crates/trident-acl-agent/src/nebraska/id.rs @@ -0,0 +1,117 @@ +//! The [`MachineId`] newtype: a Nebraska instance identity. + +use std::fmt::{self, Display, Formatter}; + +use uuid::Uuid; + +use super::error::NebraskaError; + +/// A Nebraska instance identifier. +/// +/// `machineid` is the **primary key** of an instance in Nebraska +/// (protocol spec §7 trap 3), so it carries two invariants that fail *silently* +/// when violated, which is why this is a validated newtype rather than a bare +/// `String`: +/// +/// 1. **It must not be brace-formatted.** Nebraska filters instance ids matching +/// `{8-4-4-4-12}` out of both the instance list and the group statistics as +/// "fake instances" (protocol spec §7 trap 2). A client using a braced id is +/// invisible in the UI while appearing to work perfectly over the wire. +/// [`MachineId::new`] rejects such values; [`MachineId::from_uuid`] relies on +/// Rust's [`Uuid`] `Display`, which is hyphenated and unbraced. +/// +/// 2. **It must be stable across the update reboot.** If it changes, the old +/// instance is left behind in whatever state it was in — permanently wedged +/// if intermediate events had been sent — and a new instance appears with no +/// history. Stability is the caller's responsibility (e.g. deriving it from a +/// machine id that lives on a partition that survives the A/B swap); this +/// type only guarantees the format. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct MachineId(String); + +impl MachineId { + /// Builds a [`MachineId`] from an arbitrary string, rejecting values that + /// would be filtered out by Nebraska. + /// + /// # Errors + /// + /// Returns [`NebraskaError::InvalidRequest`] if the value is empty or is a + /// brace-wrapped UUID (`{...}`), which Nebraska treats as a fake instance. + pub fn new(id: impl Into) -> Result { + let id = id.into(); + if id.is_empty() { + return Err(NebraskaError::InvalidRequest( + "machine id must not be empty".to_string(), + )); + } + if is_braced(&id) { + return Err(NebraskaError::InvalidRequest(format!( + "machine id '{id}' is brace-wrapped; Nebraska filters braced ids out of the UI \ + and group statistics" + ))); + } + Ok(Self(id)) + } + + /// Builds a [`MachineId`] from a [`Uuid`], using its hyphenated, unbraced + /// representation. This is always valid. + pub fn from_uuid(uuid: Uuid) -> Self { + // `Uuid`'s `Display` is hyphenated and unbraced, which is exactly what + // Nebraska expects; construct directly to skip the (impossible) failure. + Self(uuid.to_string()) + } + + /// Returns the id as a string slice. + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl Display for MachineId { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } +} + +/// Returns whether the value is a brace-wrapped id (`{...}`), the shape Nebraska +/// filters out. +fn is_braced(id: &str) -> bool { + id.starts_with('{') && id.ends_with('}') +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn from_uuid_is_unbraced() { + let uuid = Uuid::parse_str("b187c502-8d4d-9b6f-91f7-cbd2e6a10225").unwrap(); + let id = MachineId::from_uuid(uuid); + assert_eq!(id.as_str(), "b187c502-8d4d-9b6f-91f7-cbd2e6a10225"); + assert!(!id.as_str().starts_with('{')); + } + + #[test] + fn new_accepts_plain_id() { + let id = MachineId::new("b187c502-8d4d-9b6f-91f7-cbd2e6a10225").unwrap(); + assert_eq!(id.as_str(), "b187c502-8d4d-9b6f-91f7-cbd2e6a10225"); + } + + #[test] + fn new_rejects_braced_uuid() { + let err = MachineId::new("{b187c502-8d4d-9b6f-91f7-cbd2e6a10225}").unwrap_err(); + assert!( + matches!(err, NebraskaError::InvalidRequest(_)), + "got {err:?}" + ); + } + + #[test] + fn new_rejects_empty() { + let err = MachineId::new("").unwrap_err(); + assert!( + matches!(err, NebraskaError::InvalidRequest(_)), + "got {err:?}" + ); + } +} diff --git a/crates/trident-acl-agent/src/nebraska/mod.rs b/crates/trident-acl-agent/src/nebraska/mod.rs new file mode 100644 index 0000000000..08059ea569 --- /dev/null +++ b/crates/trident-acl-agent/src/nebraska/mod.rs @@ -0,0 +1,78 @@ +//! A client for the [Nebraska](https://github.com/flatcar/nebraska) update +//! server, speaking the Omaha protocol. +//! +//! This module is scoped strictly to the Nebraska/Omaha protocol: building and +//! sending update checks and events, and interpreting the responses. It knows +//! nothing about Trident, reboots, commits, or the update orchestration around +//! it — that separation is deliberate. +//! +//! # Why the API looks the way it does +//! +//! The Omaha protocol as Nebraska implements it has several invariants that +//! **fail silently** when violated — a client can appear to work over the wire +//! while leaving the fleet's state permanently wrong. This module encodes those +//! invariants in the type system so they cannot be violated by accident. The +//! authoritative behavioural reference is the protocol spec at +//! `knowledge/topics/nebraska-client-protocol.md` in the `pacobot` repository; +//! the docs below cite its sections. In summary: +//! +//! - **Only six `(eventtype, eventresult)` pairs are accepted**; anything else +//! is silently discarded. Raw integers never appear in the public API — see +//! [`ProgressEvent`] and the private wire mapping (spec §2). +//! - **`track` is mandatory on every request**, including event-only ones. It +//! is a field of [`Client`], so it cannot be omitted (spec §7 trap 4). +//! - **`error-updateInProgressOnInstance` is expected, not fatal.** It is +//! modelled as [`CheckOutcome::UpdateInProgress`], and unknown status strings +//! never break parsing (spec §4, §7 trap 1). +//! - **The machine id must be unbraced and stable.** See [`MachineId`] +//! (spec §7 traps 2, 3). +//! - **Event reporting is all-or-nothing.** Sending progress events commits the +//! caller to a terminal event that fires after a reboot; the terminal +//! operations are dedicated methods on [`Client`] rather than free-standing +//! values (spec §3). +//! +//! # Example +//! +//! ```no_run +//! use semver::Version; +//! use url::Url; +//! use trident_acl_agent::nebraska::{Client, CheckOutcome, MachineId, ProgressEvent}; +//! +//! # fn demo() -> Result<(), Box> { +//! let client = Client::new( +//! Url::parse("https://nebraska.example/v1/update/")?, +//! "6d10cf97-443f-4542-8479-b9fdb44c9588", +//! "stable", +//! MachineId::from_uuid(uuid::Uuid::new_v4()), +//! ); +//! +//! let current = Version::new(3, 0, 20260731); +//! match client.check_for_update(¤t)? { +//! CheckOutcome::UpToDate => {} +//! CheckOutcome::UpdateInProgress => {} +//! CheckOutcome::UpdateAvailable(offer) => { +//! // (drive the update via Trident, out of this module's scope) +//! client.report_progress(¤t, ProgressEvent::DownloadStarted)?; +//! // ... download finished, installed, then reboot ... +//! // After the reboot, from a fresh process running the new version: +//! client.complete_after_reboot(¤t, &offer.version)?; +//! } +//! } +//! # Ok(()) +//! # } +//! ``` + +mod client; +mod error; +mod event; +mod id; +mod status; +mod transport; +mod wire; + +pub use client::{CheckOutcome, Client, UpdateOffer}; +pub use error::NebraskaError; +pub use event::ProgressEvent; +pub use id::MachineId; +pub use status::{AppStatus, UpdateCheckStatus}; +pub use transport::{ReqwestTransport, Transport}; diff --git a/crates/trident-acl-agent/src/nebraska/status.rs b/crates/trident-acl-agent/src/nebraska/status.rs new file mode 100644 index 0000000000..8507f1002e --- /dev/null +++ b/crates/trident-acl-agent/src/nebraska/status.rs @@ -0,0 +1,143 @@ +//! Response status types, modelled so that unknown values never break parsing. +//! +//! Two Nebraska behaviours drive the design here (protocol spec §4 and §7): +//! +//! - `error-updateInProgressOnInstance` is returned on **every** update check +//! between the first progress event and the terminal event. It is expected, +//! not a fault, and must be handled distinctly. +//! - Nebraska may return status strings a given client does not know about. A +//! status enum without a catch-all would turn a normal response into a hard +//! parse failure, so every status type here has an `Other` variant. + +use std::fmt::{self, Display, Formatter}; + +use serde::Deserialize; + +/// Status of an `` element in a Nebraska response. +#[derive(Debug, Clone, PartialEq, Eq, Deserialize)] +pub enum AppStatus { + /// The app resolved successfully. + #[serde(rename = "ok")] + Ok, + + /// An update is already in progress for this instance. Returned on every + /// poll between the first progress event and the terminal event; expected, + /// not fatal (protocol spec §4). + #[serde(rename = "error-updateInProgressOnInstance")] + UpdateInProgress, + + /// Any other (including unknown) status string, preserved verbatim so that + /// an unrecognised value can never cause a parse failure. + #[serde(untagged)] + Other(String), +} + +impl AppStatus { + /// Whether this status represents success (`ok`). + pub fn is_ok(&self) -> bool { + matches!(self, AppStatus::Ok) + } + + /// Whether this is the expected "update already in progress" status, which a + /// correct client tolerates rather than treating as an error. + pub fn is_update_in_progress(&self) -> bool { + matches!(self, AppStatus::UpdateInProgress) + } +} + +impl Display for AppStatus { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + match self { + AppStatus::Ok => f.write_str("ok"), + AppStatus::UpdateInProgress => f.write_str("error-updateInProgressOnInstance"), + AppStatus::Other(s) => f.write_str(s), + } + } +} + +/// Status of an `` element in a Nebraska response. +#[derive(Debug, Clone, PartialEq, Eq, Deserialize)] +pub enum UpdateCheckStatus { + /// An update is available. + #[serde(rename = "ok")] + Ok, + + /// No update is available. + #[serde(rename = "noupdate")] + NoUpdate, + + /// An internal server error. In normal operation this accompanies the + /// app-level `error-updateInProgressOnInstance` and is therefore expected + /// during an in-flight update (protocol spec §4). + #[serde(rename = "error-internal")] + ErrorInternal, + + /// Any other (including unknown) status string, preserved verbatim. + #[serde(untagged)] + Other(String), +} + +impl UpdateCheckStatus { + /// Whether an update is available (`ok`). + pub fn is_update_available(&self) -> bool { + matches!(self, UpdateCheckStatus::Ok) + } + + /// Whether the check reported no update (`noupdate`). + pub fn is_no_update(&self) -> bool { + matches!(self, UpdateCheckStatus::NoUpdate) + } +} + +impl Display for UpdateCheckStatus { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + match self { + UpdateCheckStatus::Ok => f.write_str("ok"), + UpdateCheckStatus::NoUpdate => f.write_str("noupdate"), + UpdateCheckStatus::ErrorInternal => f.write_str("error-internal"), + UpdateCheckStatus::Other(s) => f.write_str(s), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn app_status_known_values() { + assert_eq!(serde_plain_from(r#""ok""#), AppStatus::Ok,); + assert_eq!( + serde_plain_from(r#""error-updateInProgressOnInstance""#), + AppStatus::UpdateInProgress, + ); + } + + #[test] + fn app_status_unknown_does_not_fail() { + // The critical property: an unrecognised status must deserialize into + // `Other`, never error. + let status = serde_plain_from(r#""error-somethingBrandNew""#); + assert_eq!( + status, + AppStatus::Other("error-somethingBrandNew".to_string()) + ); + assert!(!status.is_ok()); + assert!(!status.is_update_in_progress()); + } + + #[test] + fn update_check_status_unknown_does_not_fail() { + let status: UpdateCheckStatus = serde_json::from_str(r#""error-brandNew""#).unwrap(); + assert_eq!( + status, + UpdateCheckStatus::Other("error-brandNew".to_string()) + ); + } + + /// Helper: deserialize an `AppStatus` from a JSON string literal (serde data + /// model is shared with XML attribute deserialization). + fn serde_plain_from(s: &str) -> AppStatus { + serde_json::from_str(s).unwrap() + } +} diff --git a/crates/trident-acl-agent/src/nebraska/transport.rs b/crates/trident-acl-agent/src/nebraska/transport.rs new file mode 100644 index 0000000000..57a799d982 --- /dev/null +++ b/crates/trident-acl-agent/src/nebraska/transport.rs @@ -0,0 +1,48 @@ +//! The [`Transport`] abstraction over the HTTP round-trip to Nebraska. +//! +//! Abstracting the transport keeps the protocol logic in +//! [`Client`](crate::nebraska::Client) hermetically testable — unit tests inject +//! a canned transport and never touch the network — while the production path +//! uses a blocking `reqwest` client. + +use url::Url; + +use super::error::NebraskaError; + +/// Performs the HTTP POST of an Omaha request body and returns the response body. +/// +/// Implementors should POST `body` to `endpoint` with an XML content type and +/// return the response text, mapping failures to [`NebraskaError::Transport`] +/// (connection failures) or [`NebraskaError::Http`] (non-success status). +pub trait Transport { + /// POSTs `body` to `endpoint` and returns the response body as a string. + fn post_xml(&self, endpoint: &Url, body: &[u8]) -> Result; +} + +/// The default [`Transport`], backed by a blocking `reqwest` client. +#[derive(Debug, Default)] +pub struct ReqwestTransport { + client: reqwest::blocking::Client, +} + +impl ReqwestTransport { + /// Creates a new transport with a default `reqwest` client. + pub fn new() -> Self { + Self::default() + } +} + +impl Transport for ReqwestTransport { + fn post_xml(&self, endpoint: &Url, body: &[u8]) -> Result { + self.client + .post(endpoint.as_str()) + .header("Content-Type", "application/xml") + .body(body.to_vec()) + .send() + .map_err(|e| NebraskaError::Transport(e.to_string()))? + .error_for_status() + .map_err(|e| NebraskaError::Http(e.to_string()))? + .text() + .map_err(|e| NebraskaError::Http(e.to_string())) + } +} diff --git a/crates/trident-acl-agent/src/nebraska/wire.rs b/crates/trident-acl-agent/src/nebraska/wire.rs new file mode 100644 index 0000000000..c49b2a0f59 --- /dev/null +++ b/crates/trident-acl-agent/src/nebraska/wire.rs @@ -0,0 +1,475 @@ +//! Private serde types for the Omaha request/response XML wire format. +//! +//! These are an implementation detail of the [`nebraska`](crate::nebraska) +//! module and are never exposed publicly; callers interact with the higher-level +//! [`Client`](crate::nebraska::Client) API. Keeping them private means the +//! protocol's invariants (whitelisted events, mandatory `track`, unbraced +//! machine id) can only be satisfied through the validated builders. + +use quick_xml::{ + events::{BytesDecl, Event}, + Writer, +}; +use serde::{Deserialize, Serialize}; +use url::Url; +use uuid::Uuid; + +use super::{ + event::WirePair, + status::{AppStatus, UpdateCheckStatus}, +}; + +const OMAHA_PROTOCOL: &str = "3.0"; +const XML_VERSION: &str = "1.0"; +const XML_ENCODING: &str = "UTF-8"; + +/// Serializes an `ismachine`-style boolean as the string `"1"` or `"0"`, as the +/// Omaha protocol requires. +fn bool_as_num(value: &bool, serializer: S) -> Result +where + S: serde::Serializer, +{ + serializer.serialize_str(if *value { "1" } else { "0" }) +} + +/// An outgoing Omaha ``. +#[derive(Debug, Serialize)] +pub(super) struct Request { + #[serde(rename = "@protocol")] + protocol: &'static str, + + #[serde(rename = "@version")] + version: &'static str, + + #[serde(rename = "@ismachine", serialize_with = "bool_as_num")] + is_machine: bool, + + #[serde(rename = "@sessionid")] + session_id: Uuid, + + #[serde(rename = "os")] + os: Os, + + #[serde(rename = "app")] + app: App, +} + +impl Request { + /// Builds a request carrying a single app. + pub(super) fn new(app: App) -> Self { + Self { + protocol: OMAHA_PROTOCOL, + version: env!("CARGO_PKG_VERSION"), + is_machine: true, + session_id: Uuid::new_v4(), + os: Os::current(), + app, + } + } + + /// Serializes the request to UTF-8 XML bytes, including the XML declaration. + pub(super) fn to_xml(&self) -> Result, quick_xml::SeError> { + let mut buf = Vec::new(); + let mut writer = Writer::new(&mut buf); + writer.write_event(Event::Decl(BytesDecl::new( + XML_VERSION, + Some(XML_ENCODING), + None, + )))?; + writer.write_serializable("request", self)?; + Ok(buf) + } +} + +/// The `` element. Informational; Nebraska keys update decisions off the +/// `` attribute rather than this. +#[derive(Debug, Serialize)] +pub(super) struct Os { + #[serde(rename = "@platform")] + platform: &'static str, + + #[serde(rename = "@version")] + version: String, + + #[serde(rename = "@arch")] + arch: &'static str, +} + +impl Os { + fn current() -> Self { + Self { + platform: "linux", + version: String::new(), + arch: arch_str(), + } + } +} + +/// Returns the Omaha architecture string for the current build target. +fn arch_str() -> &'static str { + if cfg!(target_arch = "aarch64") { + "arm64" + } else { + "amd64" + } +} + +/// The `` element of a request. +#[derive(Debug, Serialize)] +pub(super) struct App { + #[serde(rename = "@appid")] + app_id: String, + + #[serde(rename = "@version")] + version: String, + + #[serde(rename = "@track")] + track: String, + + #[serde(rename = "@machineid")] + machine_id: String, + + #[serde(rename = "@previousversion", skip_serializing_if = "Option::is_none")] + previous_version: Option, + + #[serde(rename = "updatecheck", skip_serializing_if = "Option::is_none")] + update_check: Option, + + #[serde(rename = "ping", skip_serializing_if = "Option::is_none")] + ping: Option, + + #[serde(rename = "event", skip_serializing_if = "Vec::is_empty")] + events: Vec, +} + +impl App { + /// Creates a new `` with the mandatory identity fields. `track` is a + /// required parameter here — the type cannot be built without it — which is + /// how the module guarantees `track` is present on every request, including + /// event-only ones (protocol spec §7 trap 4). + pub(super) fn new(app_id: String, version: String, track: String, machine_id: String) -> Self { + Self { + app_id, + version, + track, + machine_id, + previous_version: None, + update_check: None, + ping: None, + events: Vec::new(), + } + } + + pub(super) fn with_previous_version(mut self, previous: String) -> Self { + self.previous_version = Some(previous); + self + } + + pub(super) fn with_update_check(mut self) -> Self { + self.update_check = Some(UpdateCheck); + self + } + + pub(super) fn with_ping(mut self) -> Self { + self.ping = Some(Ping { active: 1 }); + self + } + + pub(super) fn with_event(mut self, pair: WirePair) -> Self { + self.events.push(EventElement { + event_type: pair.event_type, + event_result: pair.event_result, + }); + self + } + + /// Sets the `` attribute (informational). Kept in sync with the + /// app version by the client. + pub(super) fn os_version(&self) -> &str { + &self.version + } +} + +#[derive(Debug, Serialize)] +pub(super) struct UpdateCheck; + +#[derive(Debug, Serialize)] +pub(super) struct Ping { + #[serde(rename = "@active")] + active: u8, +} + +#[derive(Debug, Serialize)] +pub(super) struct EventElement { + #[serde(rename = "@eventtype")] + event_type: u16, + + #[serde(rename = "@eventresult")] + event_result: u8, +} + +/// Builds a request from an app, setting the `` from the app version +/// so the two agree. +pub(super) fn request_for(app: App) -> Request { + let mut request = Request::new(app); + request.os.version = request.app.os_version().to_string(); + request +} + +// --------------------------------------------------------------------------- +// Response types +// --------------------------------------------------------------------------- + +/// An incoming Omaha ``. +#[derive(Debug, Deserialize)] +pub(super) struct Response { + #[serde(default, rename = "app")] + pub(super) apps: Vec, +} + +#[derive(Debug, Deserialize)] +pub(super) struct AppResponse { + #[serde(rename = "@appid")] + pub(super) app_id: String, + + #[serde(rename = "@status")] + pub(super) status: AppStatus, + + #[serde(default, rename = "updatecheck")] + pub(super) update_check: Option, +} + +#[derive(Debug, Deserialize)] +pub(super) struct UpdateCheckResponse { + #[serde(rename = "@status")] + pub(super) status: UpdateCheckStatus, + + #[serde(default, rename = "urls")] + pub(super) urls: Option, + + #[serde(rename = "manifest")] + pub(super) manifest: Option, +} + +#[derive(Debug, Deserialize)] +pub(super) struct Urls { + #[serde(default, rename = "url")] + pub(super) urls: Vec, +} + +#[derive(Debug, Deserialize)] +pub(super) struct UrlElement { + #[serde(rename = "@codebase")] + pub(super) codebase: Url, +} + +#[derive(Debug, Deserialize)] +pub(super) struct Manifest { + #[serde(rename = "@version")] + pub(super) version: String, + + #[serde(default, rename = "packages")] + pub(super) packages: Option, +} + +#[derive(Debug, Deserialize)] +pub(super) struct Packages { + #[serde(default, rename = "package")] + pub(super) packages: Vec, +} + +#[derive(Debug, Deserialize)] +pub(super) struct Package { + #[serde(rename = "@name")] + pub(super) name: String, +} + +/// Parses a Nebraska response body. +pub(super) fn parse_response(body: &str) -> Result { + let deserializer = &mut quick_xml::de::Deserializer::from_str(body); + serde_path_to_error::deserialize(deserializer).map_err(|e| e.to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn xml_of(app: App) -> String { + String::from_utf8(request_for(app).to_xml().unwrap()).unwrap() + } + + #[test] + fn update_check_request_shape() { + let app = App::new( + "app-1".into(), + "3.0.20260731".into(), + "stable".into(), + "mid-1".into(), + ) + .with_update_check(); + let xml = xml_of(app); + + assert!(xml.contains(r#"protocol="3.0""#), "{xml}"); + assert!(xml.contains(r#"ismachine="1""#), "{xml}"); + assert!(xml.contains(r#"appid="app-1""#), "{xml}"); + assert!(xml.contains(r#"version="3.0.20260731""#), "{xml}"); + assert!(xml.contains(r#"track="stable""#), "{xml}"); + assert!(xml.contains(r#"machineid="mid-1""#), "{xml}"); + assert!(xml.contains(" that must not break parsing. + let body = r#" + + + + + + + + + + + + + + "#; + let resp = parse_response(body).unwrap(); + assert_eq!(resp.apps.len(), 1); + let app = &resp.apps[0]; + assert_eq!(app.app_id, "6d10cf97-443f-4542-8479-b9fdb44c9588"); + assert!(app.status.is_ok()); + let uc = app.update_check.as_ref().unwrap(); + assert!(uc.status.is_update_available()); + assert_eq!(uc.manifest.as_ref().unwrap().version, "3.0.20260803"); + assert_eq!( + uc.urls.as_ref().unwrap().urls[0].codebase.as_str(), + "http://192.168.122.1:8080/" + ); + assert_eq!( + uc.manifest + .as_ref() + .unwrap() + .packages + .as_ref() + .unwrap() + .packages[0] + .name, + "acl-3.0.20260803.cosi" + ); + } + + #[test] + fn parse_noupdate() { + let body = r#" + + + + + + "#; + let resp = parse_response(body).unwrap(); + let uc = resp.apps[0].update_check.as_ref().unwrap(); + assert!(uc.status.is_no_update()); + } + + #[test] + fn parse_update_in_progress() { + // The shape returned on every poll between the first progress event and + // the terminal one. Must parse cleanly (app status Other-mapped, check + // status error-internal), never panic. + let body = r#" + + + + + + "#; + let resp = parse_response(body).unwrap(); + assert!(resp.apps[0].status.is_update_in_progress()); + assert_eq!( + resp.apps[0].update_check.as_ref().unwrap().status, + UpdateCheckStatus::ErrorInternal + ); + } + + #[test] + fn parse_unknown_status_does_not_fail() { + let body = r#" + + + + + + "#; + let resp = parse_response(body).unwrap(); + assert_eq!( + resp.apps[0].status, + AppStatus::Other("error-madeUpStatus".to_string()) + ); + } +} From 25dc52072a96d2c39a04c3c47e705c38d3b70e53 Mon Sep 17 00:00:00 2001 From: Paco Date: Thu, 6 Aug 2026 13:38:24 -0700 Subject: [PATCH 2/6] =?UTF-8?q?feat(acl-agent):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20retry=20classifier,=20async=20note,=20omaha=20super?= =?UTF-8?q?session?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the nebraska module review: - Add NebraskaError::is_retryable() to distinguish transient (transport/HTTP) failures from permanent protocol errors, so a caller retrying the post-reboot completion loops only on retryable errors and never spins on a permanent one (the inverse of the gRPC-UNIMPLEMENTED-retried-30s bug). complete_after_reboot's rustdoc now states loudly that it MUST be retried, and that losing it wedges the instance permanently; retry policy stays the caller's. - Document that the sync Transport is deliberate and that an async transport is a non-breaking addition (Client is generic; an AsyncTransport + async client can be added alongside), so the seam does not force a future rewrite. - Make the omaha module's supersession by nebraska explicit: a module-level note with the call-to-call migration mapping (no #[deprecated] since the agent still depends on it and the crate builds with -D warnings). - Emit the batched completion elements in logical order (event → ping → updatecheck) and assert that order in a test, so a refactor cannot silently split or reorder the request that closes the wedge window. cargo fmt / clippy --all-targets -D warnings clean; 26 module unit tests + doctest. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../trident-acl-agent/src/nebraska/README.md | 23 +++++++++++ .../trident-acl-agent/src/nebraska/client.rs | 15 ++++++- .../trident-acl-agent/src/nebraska/error.rs | 39 +++++++++++++++++++ .../src/nebraska/transport.rs | 17 ++++++++ crates/trident-acl-agent/src/nebraska/wire.rs | 22 +++++++++-- crates/trident-acl-agent/src/omaha/mod.rs | 18 +++++++++ 6 files changed, 128 insertions(+), 6 deletions(-) diff --git a/crates/trident-acl-agent/src/nebraska/README.md b/crates/trident-acl-agent/src/nebraska/README.md index bfae3fc4ae..877d5904c3 100644 --- a/crates/trident-acl-agent/src/nebraska/README.md +++ b/crates/trident-acl-agent/src/nebraska/README.md @@ -61,6 +61,29 @@ Persisting "an update is in flight (previous X, target Y)" across the reboot is the caller's responsibility — it is orchestration, deliberately out of this module's scope — but the module makes the correct post-reboot call trivial. +## Retry of the terminal event is the caller's, but the module makes the distinction visible + +`complete_after_reboot` is **not** retried internally. Retry policy is the +caller's — it lives alongside the cross-reboot state the caller must already +persist, and baking a policy into a protocol module tends to fight whatever the +caller has. But because losing the terminal event wedges the instance +permanently, the module makes the retry decision unmissable: + +- `complete_after_reboot`'s rustdoc states, in plain terms, that the call must be + retried until it succeeds and why. +- `NebraskaError::is_retryable()` classifies transient (transport/HTTP) failures + from permanent (protocol) ones, so the caller can loop while retryable and stop + on a permanent error — avoiding the inverse bug (spinning on a permanent + failure) that bit the gRPC commit path. + +## Blocking transport today; async is a non-breaking addition + +`Transport` is synchronous, matching the current agent. A future async TAA must +not call a blocking HTTP client inside its Tokio runtime. Supporting async does +**not** require changing this API: `Client` is generic over the transport, so an +`AsyncTransport` trait plus a thin async client can be added *alongside* the sync +ones without breaking them. See the `transport` module docs for the full note. + ## Adopting this in the agent The current agent (`main.rs` + the ad-hoc `omaha` module) predates this module. diff --git a/crates/trident-acl-agent/src/nebraska/client.rs b/crates/trident-acl-agent/src/nebraska/client.rs index b8f5d1689e..3346ecf520 100644 --- a/crates/trident-acl-agent/src/nebraska/client.rs +++ b/crates/trident-acl-agent/src/nebraska/client.rs @@ -151,8 +151,19 @@ impl Client { /// `noupdate` in one round trip — closing the window in which a bare /// post-reboot poll would hit `error-updateInProgressOnInstance` (protocol /// spec §4). This is the terminal event that discharges the commitment made - /// by [`report_progress`](Client::report_progress), and it must be retried - /// until it lands: losing it wedges the instance permanently. + /// by [`report_progress`](Client::report_progress). + /// + /// # This call MUST be retried until it succeeds + /// + /// The first network call immediately after a reboot routinely fails while + /// DNS and routing settle. **Losing this terminal event wedges the instance + /// permanently** — there is no server-side self-heal, timer, or REST reset + /// (protocol spec §3, §6). This module deliberately does not bake in a retry + /// policy (that is the caller's to own, alongside the cross-reboot state it + /// must already persist), but the caller is responsible for retrying: loop + /// with a bounded backoff while [`NebraskaError::is_retryable`] holds, and + /// give up only on a permanent error. See [`report_failure`](Client::report_failure) + /// for the recovery path if completion genuinely cannot be reported. /// /// `previous_version` is the version the instance was on before the update; /// `current_version` is the (new) version now running. diff --git a/crates/trident-acl-agent/src/nebraska/error.rs b/crates/trident-acl-agent/src/nebraska/error.rs index d39597b0a0..276e3ed6e1 100644 --- a/crates/trident-acl-agent/src/nebraska/error.rs +++ b/crates/trident-acl-agent/src/nebraska/error.rs @@ -45,3 +45,42 @@ pub enum NebraskaError { #[error("Nebraska reported error status: {0}")] ServerError(String), } + +impl NebraskaError { + /// Whether this error is transient and the request is worth retrying. + /// + /// This distinction matters most for the post-reboot completion report: the + /// first network call after a reboot routinely fails while DNS and routing + /// settle, and losing the terminal event **wedges the instance permanently** + /// (protocol spec §3, §7). A caller retrying that report should loop while + /// `is_retryable()` holds (with a bounded backoff), and stop on a permanent + /// error rather than spinning on it — the inverse mistake (retrying a + /// permanent failure) is just as damaging. + /// + /// Transport and HTTP failures are treated as transient; protocol-level + /// failures (serialization, parse, unexpected response, server error status, + /// invalid request) are permanent. + pub fn is_retryable(&self) -> bool { + matches!(self, NebraskaError::Transport(_) | NebraskaError::Http(_)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn transient_errors_are_retryable() { + assert!(NebraskaError::Transport("connection refused".into()).is_retryable()); + assert!(NebraskaError::Http("502 Bad Gateway".into()).is_retryable()); + } + + #[test] + fn permanent_errors_are_not_retryable() { + assert!(!NebraskaError::InvalidRequest("bad".into()).is_retryable()); + assert!(!NebraskaError::Serialize("x".into()).is_retryable()); + assert!(!NebraskaError::Parse("x".into()).is_retryable()); + assert!(!NebraskaError::UnexpectedResponse("x".into()).is_retryable()); + assert!(!NebraskaError::ServerError("error-osnotsupported".into()).is_retryable()); + } +} diff --git a/crates/trident-acl-agent/src/nebraska/transport.rs b/crates/trident-acl-agent/src/nebraska/transport.rs index 57a799d982..3fd751b7c0 100644 --- a/crates/trident-acl-agent/src/nebraska/transport.rs +++ b/crates/trident-acl-agent/src/nebraska/transport.rs @@ -4,6 +4,23 @@ //! [`Client`](crate::nebraska::Client) hermetically testable — unit tests inject //! a canned transport and never touch the network — while the production path //! uses a blocking `reqwest` client. +//! +//! # Blocking today; async is a non-breaking addition +//! +//! [`Transport`] is intentionally **synchronous**, matching the current agent +//! (which is otherwise sync and only enters a Tokio runtime for its Trident gRPC +//! call). A future async TAA that drives Trident over `tonic`/`tokio` must not +//! call a blocking HTTP client from within the async runtime, as that stalls the +//! executor. +//! +//! Supporting that does **not** require changing this API. Because +//! [`Client`](crate::nebraska::Client) is generic over the transport, an async +//! variant can be introduced *alongside* the sync one — a separate +//! `AsyncTransport` trait and a thin async client wrapper — without modifying or +//! breaking [`Transport`], [`ReqwestTransport`], or the existing `Client` +//! surface. The sync path is the right default now; the async path is additive +//! when a caller needs it. Until then, an async caller can also simply wrap a +//! sync call in `tokio::task::spawn_blocking`. use url::Url; diff --git a/crates/trident-acl-agent/src/nebraska/wire.rs b/crates/trident-acl-agent/src/nebraska/wire.rs index c49b2a0f59..81ff1f15dd 100644 --- a/crates/trident-acl-agent/src/nebraska/wire.rs +++ b/crates/trident-acl-agent/src/nebraska/wire.rs @@ -132,14 +132,19 @@ pub(super) struct App { #[serde(rename = "@previousversion", skip_serializing_if = "Option::is_none")] previous_version: Option, - #[serde(rename = "updatecheck", skip_serializing_if = "Option::is_none")] - update_check: Option, + // Child elements are declared — and therefore serialized — in the order + // Nebraska logically processes them: events first, then the ping, then the + // update check. Nebraska actually processes events before the update check + // regardless of XML order (protocol spec §4), but emitting them in this + // order keeps the batched post-reboot request self-documenting. + #[serde(rename = "event", skip_serializing_if = "Vec::is_empty")] + events: Vec, #[serde(rename = "ping", skip_serializing_if = "Option::is_none")] ping: Option, - #[serde(rename = "event", skip_serializing_if = "Vec::is_empty")] - events: Vec, + #[serde(rename = "updatecheck", skip_serializing_if = "Option::is_none")] + update_check: Option, } impl App { @@ -377,6 +382,15 @@ mod tests { assert!(xml.contains(r#"active="1""#), "{xml}"); assert!(xml.contains("` → +//! `nebraska::Client::check_for_update` +//! - `report_event` for a progress event → `nebraska::Client::report_progress` +//! - the batched post-reboot completion → `nebraska::Client::complete_after_reboot` +//! - a failure/reset event → `nebraska::Client::report_failure` use log::{debug, trace}; use url::Url; From 3bfd0603b6991cef819055ba31818c05365abd5e Mon Sep 17 00:00:00 2001 From: Paco Date: Thu, 6 Aug 2026 14:13:30 -0700 Subject: [PATCH 3/6] docs(nebraska): remove internal references; self-contained docs; trim README MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prepare the module for a public repo: - Remove all references to the internal knowledge-base doc (repo name and file path) from mod.rs and the README — these named internal material a public reader cannot resolve. - Replace every "protocol spec §N" citation across the module with the reasoning stated inline, so each doc comment stands on its own. The facts are observable behaviour of the open-source Nebraska server; only the internal pointer is removed. - Replace demo-specific values in examples and tests (real app id, POC hostname, 192.168.122.x addresses, date-stamped build versions, package name) with obviously-generic ones (example-app, updates.example.com, 1.0.0/2.0.0). - Rewrite the README to be short and usage-focused: a brief intro plus worked examples for polling, the full event sequence with retry, and wedge recovery. No behavioural change. cargo fmt / clippy --all-targets -D warnings clean; 26 module tests + doctest pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../trident-acl-agent/src/nebraska/README.md | 212 +++++++++--------- .../trident-acl-agent/src/nebraska/client.rs | 78 +++---- .../trident-acl-agent/src/nebraska/error.rs | 12 +- .../trident-acl-agent/src/nebraska/event.rs | 15 +- crates/trident-acl-agent/src/nebraska/id.rs | 25 +-- crates/trident-acl-agent/src/nebraska/mod.rs | 40 ++-- .../trident-acl-agent/src/nebraska/status.rs | 6 +- crates/trident-acl-agent/src/nebraska/wire.rs | 41 ++-- 8 files changed, 215 insertions(+), 214 deletions(-) diff --git a/crates/trident-acl-agent/src/nebraska/README.md b/crates/trident-acl-agent/src/nebraska/README.md index 877d5904c3..6e784f317d 100644 --- a/crates/trident-acl-agent/src/nebraska/README.md +++ b/crates/trident-acl-agent/src/nebraska/README.md @@ -1,103 +1,113 @@ -# `nebraska` client module — design and adoption notes +# `nebraska` client module A self-contained Rust client for the [Nebraska](https://github.com/flatcar/nebraska) -update server (Omaha protocol), living in `crates/trident-acl-agent/src/nebraska/`. - -The authoritative behavioural spec is -`knowledge/topics/nebraska-client-protocol.md` in the `pacobot` repository. This -document only covers how the module maps that spec into a Rust API and how the -existing agent would adopt it. - -## Public API - -| Item | Purpose | -| --- | --- | -| `Client` | A client bound to one app + track + machine id. Methods: `check_for_update`, `report_progress`, `complete_after_reboot`, `report_failure`. | -| `CheckOutcome` | `UpToDate` \| `UpdateAvailable(UpdateOffer)` \| `UpdateInProgress`. The last models `error-updateInProgressOnInstance` as an expected outcome, not an error. | -| `UpdateOffer` | `{ version: semver::Version, package_url: Url }` — the resolved package URL (codebase joined with package name). | -| `ProgressEvent` | `DownloadStarted` \| `DownloadFinished` \| `Installed`. The only publicly constructible events; they map to the whitelisted wire pairs `13/1`, `14/1`, `800/1`. | -| `MachineId` | Validated, unbraced instance id. `from_uuid` / `new`. | -| `AppStatus`, `UpdateCheckStatus` | Response statuses with an `Other(String)` catch-all so unknown values never break parsing. | -| `Transport`, `ReqwestTransport` | The HTTP seam; injectable for hermetic tests. | -| `NebraskaError` | The module error type (`thiserror`). | - -Terminal events (`3/2` complete, `3/0` failure) are **not** public values — they -are emitted only through `complete_after_reboot` and `report_failure`, so they -always carry the correct request shape (e.g. the batched update-check that -completion requires). - -## How the invariants are encoded - -- **Whitelisted events only** — raw `(type, result)` integers are private; the - public vocabulary (`ProgressEvent` + the terminal methods) can only produce the - six accepted pairs. A unit test asserts this. -- **`track` mandatory** — a field of `Client`; no request can be built without it. -- **Unbraced, stable machine id** — `MachineId` rejects braced ids; `from_uuid` - uses Rust's unbraced `Display`. -- **`error-updateInProgressOnInstance` is expected** — surfaced as - `CheckOutcome::UpdateInProgress`; unknown statuses map to `Other`. -- **Real semver version** — the API takes `&semver::Version`, and offered - versions are parsed as semver. -- **All-or-nothing event reporting** — see the design decision below. - -## Design decision: invariant #2 (all-or-nothing) is a documented plain API, not a typestate - -Sending progress events commits the caller to a terminal event, and **the -terminal event fires after a reboot — in a different process** from the progress -events. No in-process typestate or RAII guard can span that boundary; worse, an -RAII "you didn't finish" guard would fire at the drop that happens *at* reboot, -which is exactly when completion must *not* be reported. A compile-time -"started ⇒ must-finish" is therefore structurally impossible here. - -Instead the property is encoded three ways that actually hold: - -1. Terminal events are not free-standing values; they are dedicated `Client` - methods, so a terminal cannot be sent in the wrong shape or context. -2. The only-whitelisted-pairs property is total, so no invalid event exists. -3. `complete_after_reboot` is the batched `3/2 + ping + updatecheck` request, - making the safe post-reboot path (which closes the wedge window) the easy one. - -Persisting "an update is in flight (previous X, target Y)" across the reboot is -the caller's responsibility — it is orchestration, deliberately out of this -module's scope — but the module makes the correct post-reboot call trivial. - -## Retry of the terminal event is the caller's, but the module makes the distinction visible - -`complete_after_reboot` is **not** retried internally. Retry policy is the -caller's — it lives alongside the cross-reboot state the caller must already -persist, and baking a policy into a protocol module tends to fight whatever the -caller has. But because losing the terminal event wedges the instance -permanently, the module makes the retry decision unmissable: - -- `complete_after_reboot`'s rustdoc states, in plain terms, that the call must be - retried until it succeeds and why. -- `NebraskaError::is_retryable()` classifies transient (transport/HTTP) failures - from permanent (protocol) ones, so the caller can loop while retryable and stop - on a permanent error — avoiding the inverse bug (spinning on a permanent - failure) that bit the gRPC commit path. - -## Blocking transport today; async is a non-breaking addition - -`Transport` is synchronous, matching the current agent. A future async TAA must -not call a blocking HTTP client inside its Tokio runtime. Supporting async does -**not** require changing this API: `Client` is generic over the transport, so an -`AsyncTransport` trait plus a thin async client can be added *alongside* the sync -ones without breaking them. See the `transport` module docs for the full note. - -## Adopting this in the agent - -The current agent (`main.rs` + the ad-hoc `omaha` module) predates this module. -A future change would: - -1. Replace `omaha::send` / `query_and_fetch_document` / `report_event` with a - `nebraska::Client` built from the CLI args (`endpoint`, `appid`, `track`) and a - `MachineId` derived from `IdSource`. -2. Map the poll loop's results onto `CheckOutcome` (the agent already distinguishes - no-update / in-progress / available). -3. In `--events full`, call `report_progress` around the Trident stage/finalize, - persist the in-flight state to `/var`, and after the reboot call - `complete_after_reboot` (with retry) as the first request. -4. Delete the `omaha` module once nothing references it. - -This module contains no Trident gRPC, reboot, commit, or CLI logic, so that -adoption is purely at the protocol seam. +update server (Omaha protocol). Scoped strictly to the protocol — no Trident +gRPC, reboot, commit, or CLI logic — so it is reusable by any update agent. + +The API encodes the protocol's silently-failing invariants in the type system: +only whitelisted events are constructible, `track` cannot be omitted, the machine +id is a validated unbraced newtype, versions are `semver::Version`, and +`error-updateInProgressOnInstance` is a normal outcome rather than an error. The +rationale for each is documented inline on the relevant type. + +## Usage + +### Poll for an update + +```rust,no_run +use semver::Version; +use url::Url; +use trident_acl_agent::nebraska::{Client, CheckOutcome, MachineId}; + +# fn main() -> Result<(), Box> { +let client = Client::new( + Url::parse("https://updates.example.com/v1/update/")?, // trailing slash matters + "example-app", + "stable", + MachineId::from_uuid(uuid::Uuid::new_v4()), +); + +let current = Version::new(1, 0, 0); +match client.check_for_update(¤t)? { + CheckOutcome::UpToDate => println!("no update"), + CheckOutcome::UpdateInProgress => println!("update already in progress"), + CheckOutcome::UpdateAvailable(offer) => { + println!("update to {} at {}", offer.version, offer.package_url); + } +} +# Ok(()) +# } +``` + +### Report the full update sequence (with events) + +Sending progress events is a **commitment** to send a terminal event: leaving an +instance in a progress state wedges it permanently. The terminal event is sent +*after the reboot*, from a fresh process, so the caller must persist the +in-flight state across the reboot and retry the completion until it lands. + +```rust,no_run +use semver::Version; +use url::Url; +use trident_acl_agent::nebraska::{Client, CheckOutcome, MachineId, ProgressEvent}; + +# fn main() -> Result<(), Box> { +# let client = Client::new( +# Url::parse("https://updates.example.com/v1/update/")?, +# "example-app", "stable", MachineId::from_uuid(uuid::Uuid::new_v4())); +let current = Version::new(1, 0, 0); + +if let CheckOutcome::UpdateAvailable(offer) = client.check_for_update(¤t)? { + // Before/after each stage of the update (driven elsewhere), report progress: + client.report_progress(¤t, ProgressEvent::DownloadStarted)?; + // ... stage the update ... + client.report_progress(¤t, ProgressEvent::DownloadFinished)?; + // ... finalize ... + client.report_progress(¤t, ProgressEvent::Installed)?; + + // Persist { previous: current, target: offer.version } somewhere durable, + // then reboot. After the reboot, from a fresh process on the new version: + let previous = current; + let now_running = offer.version; + loop { + match client.complete_after_reboot(&previous, &now_running) { + Ok(_) => break, + // Retry only transient failures; a permanent one will never succeed. + Err(e) if e.is_retryable() => continue, + Err(e) => return Err(e.into()), + } + } +} +# Ok(()) +# } +``` + +### Recover a wedged instance + +If completion cannot be reported, `report_failure` moves the instance to Error +and re-arms it so a later check can grant again: + +```rust,no_run +# use semver::Version; +# use url::Url; +# use trident_acl_agent::nebraska::{Client, MachineId}; +# fn main() -> Result<(), Box> { +# let client = Client::new( +# Url::parse("https://updates.example.com/v1/update/")?, +# "example-app", "stable", MachineId::from_uuid(uuid::Uuid::new_v4())); +client.report_failure(&Version::new(1, 0, 0), &Version::new(2, 0, 0))?; +# Ok(()) +# } +``` + +## Testing + +`Transport` abstracts the HTTP round-trip, so the client is testable without a +network by injecting a canned implementation via `Client::with_transport`. + +## Notes + +- The `Transport` is synchronous today; an async transport can be added + alongside it without breaking this API (see the `transport` module docs). +- This module supersedes the crate's older ad-hoc `omaha` module; the agent's + migration to it is a separate change. diff --git a/crates/trident-acl-agent/src/nebraska/client.rs b/crates/trident-acl-agent/src/nebraska/client.rs index 3346ecf520..82c0083ddb 100644 --- a/crates/trident-acl-agent/src/nebraska/client.rs +++ b/crates/trident-acl-agent/src/nebraska/client.rs @@ -16,8 +16,7 @@ use super::{ /// /// `UpdateInProgress` is a first-class outcome rather than an error because /// Nebraska returns it on **every** poll between the first progress event and -/// the terminal event; it is expected server behaviour (protocol spec §4 and -/// §7 trap 5). +/// the terminal event; it is expected server behaviour. #[derive(Debug, Clone, PartialEq, Eq)] pub enum CheckOutcome { /// No update is available; the instance is up to date. @@ -49,14 +48,16 @@ pub struct UpdateOffer { /// `track`, and [`MachineId`]. Because these are required to construct the /// client and every request flows through it, two protocol invariants hold /// structurally: `track` is present on every request including event-only ones -/// (protocol spec §7 trap 4), and the machine id is always a validated, unbraced -/// value (§7 trap 2). +/// (Nebraska resolves the group from `track` before processing events, so an +/// omitted track silently drops them), and the machine id is always a validated, +/// unbraced value (Nebraska hides braced ids from its UI and statistics). /// /// # Event ordering and the all-or-nothing rule /// /// Emitting a [progress event](Client::report_progress) is a **commitment** to /// eventually emit a terminal event: leaving an instance in a progress state -/// wedges it permanently, with no server-side self-heal (protocol spec §3). The +/// wedges it permanently, because Nebraska's self-heal path only triggers from +/// the `UpdateGranted` state and nothing resets instance status on a timer. The /// terminal event is sent *after the reboot* — i.e. from a different process — /// so this cannot be enforced at compile time; instead the terminal operations /// are exposed as dedicated, hard-to-forget methods @@ -113,8 +114,7 @@ impl Client { /// /// `current_version` **must be the real version** and valid semver: a client /// reporting `0.0.0` is offered an update on every poll forever, and a - /// non-semver version fails instance registration server-side (protocol spec - /// §8). + /// non-semver version fails instance registration server-side. pub fn check_for_update( &self, current_version: &Version, @@ -127,10 +127,9 @@ impl Client { /// Reports a [`ProgressEvent`] for an in-flight update. /// /// Only valid after a successful [`check_for_update`](Client::check_for_update) - /// has caused Nebraska to grant the update (Nebraska rejects events from an - /// instance it has never seen; protocol spec §7 trap 5). Emitting a progress - /// event commits the caller to eventually reporting a terminal event — see - /// the [type docs](Client). + /// has caused Nebraska to grant the update: Nebraska rejects events from an + /// instance it has never seen. Emitting a progress event commits the caller + /// to eventually reporting a terminal event — see the [type docs](Client). pub fn report_progress( &self, current_version: &Version, @@ -149,21 +148,21 @@ impl Client { /// Nebraska processes the event before the update check within one request, /// so this both moves the instance to Complete and returns a clean /// `noupdate` in one round trip — closing the window in which a bare - /// post-reboot poll would hit `error-updateInProgressOnInstance` (protocol - /// spec §4). This is the terminal event that discharges the commitment made - /// by [`report_progress`](Client::report_progress). + /// post-reboot poll would hit `error-updateInProgressOnInstance`. This is + /// the terminal event that discharges the commitment made by + /// [`report_progress`](Client::report_progress). /// /// # This call MUST be retried until it succeeds /// /// The first network call immediately after a reboot routinely fails while /// DNS and routing settle. **Losing this terminal event wedges the instance - /// permanently** — there is no server-side self-heal, timer, or REST reset - /// (protocol spec §3, §6). This module deliberately does not bake in a retry - /// policy (that is the caller's to own, alongside the cross-reboot state it - /// must already persist), but the caller is responsible for retrying: loop - /// with a bounded backoff while [`NebraskaError::is_retryable`] holds, and - /// give up only on a permanent error. See [`report_failure`](Client::report_failure) - /// for the recovery path if completion genuinely cannot be reported. + /// permanently** — there is no server-side self-heal, timer, or REST reset. + /// This module deliberately does not bake in a retry policy (that is the + /// caller's to own, alongside the cross-reboot state it must already + /// persist), but the caller is responsible for retrying: loop with a bounded + /// backoff while [`NebraskaError::is_retryable`] holds, and give up only on a + /// permanent error. See [`report_failure`](Client::report_failure) for the + /// recovery path if completion genuinely cannot be reported. /// /// `previous_version` is the version the instance was on before the update; /// `current_version` is the (new) version now running. @@ -186,8 +185,8 @@ impl Client { /// Reports a failed update (terminal `3/0`), which moves the instance to /// Error, clears `update_in_progress`, and re-arms it so a subsequent check - /// can grant again (protocol spec §6). This is the "reset and retry" path - /// for a wedged or failed update. + /// can grant again. This is the "reset and retry" path for a wedged or + /// failed update. pub fn report_failure( &self, previous_version: &Version, @@ -396,9 +395,9 @@ mod tests { - - - + + + @@ -407,15 +406,13 @@ mod tests { #[test] fn check_returns_offer_with_joined_url() { let client = client_with(OFFER); - let outcome = client - .check_for_update(&Version::new(3, 0, 20260731)) - .unwrap(); + let outcome = client.check_for_update(&Version::new(1, 0, 0)).unwrap(); match outcome { CheckOutcome::UpdateAvailable(offer) => { - assert_eq!(offer.version, Version::new(3, 0, 20260803)); + assert_eq!(offer.version, Version::new(2, 0, 0)); assert_eq!( offer.package_url.as_str(), - "http://192.168.122.1:8080/acl-3.0.20260803.cosi" + "https://updates.example.com/os-image-2.0.0.cosi" ); } other => panic!("expected an update offer, got {other:?}"), @@ -428,9 +425,7 @@ mod tests { r#""#, ); assert_eq!( - client - .check_for_update(&Version::new(3, 0, 20260803)) - .unwrap(), + client.check_for_update(&Version::new(2, 0, 0)).unwrap(), CheckOutcome::UpToDate ); } @@ -441,9 +436,7 @@ mod tests { r#""#, ); assert_eq!( - client - .check_for_update(&Version::new(3, 0, 20260803)) - .unwrap(), + client.check_for_update(&Version::new(2, 0, 0)).unwrap(), CheckOutcome::UpdateInProgress ); } @@ -466,10 +459,7 @@ mod tests { r#""#, ); client - .report_progress( - &Version::new(3, 0, 20260731), - ProgressEvent::DownloadStarted, - ) + .report_progress(&Version::new(1, 0, 0), ProgressEvent::DownloadStarted) .unwrap(); let body = client.transport.last_body.borrow().clone().unwrap(); assert!(body.contains(r#"track="stable""#), "{body}"); @@ -485,7 +475,7 @@ mod tests { r#""#, ); let outcome = client - .complete_after_reboot(&Version::new(3, 0, 20260731), &Version::new(3, 0, 20260803)) + .complete_after_reboot(&Version::new(1, 0, 0), &Version::new(2, 0, 0)) .unwrap(); assert_eq!(outcome, CheckOutcome::UpToDate); let body = client.transport.last_body.borrow().clone().unwrap(); @@ -493,7 +483,7 @@ mod tests { body.contains(r#""#, ); client - .report_failure(&Version::new(3, 0, 20260731), &Version::new(3, 0, 20260803)) + .report_failure(&Version::new(1, 0, 0), &Version::new(2, 0, 0)) .unwrap(); let body = client.transport.last_body.borrow().clone().unwrap(); assert!( diff --git a/crates/trident-acl-agent/src/nebraska/error.rs b/crates/trident-acl-agent/src/nebraska/error.rs index 276e3ed6e1..a9cfd6a35d 100644 --- a/crates/trident-acl-agent/src/nebraska/error.rs +++ b/crates/trident-acl-agent/src/nebraska/error.rs @@ -10,9 +10,9 @@ use thiserror::Error; /// - An update already being in progress for this instance /// (`error-updateInProgressOnInstance`) is surfaced as /// [`CheckOutcome::UpdateInProgress`](crate::nebraska::CheckOutcome::UpdateInProgress), -/// not an error. See the protocol spec, §4. +/// not an error. /// - An unrecognised status string never fails parsing; it is preserved in an -/// `Other` variant. See the protocol spec, §7 trap 1. +/// `Other` variant. #[derive(Debug, Error)] pub enum NebraskaError { /// The provided value could not be used to build a valid Omaha request. @@ -52,10 +52,10 @@ impl NebraskaError { /// This distinction matters most for the post-reboot completion report: the /// first network call after a reboot routinely fails while DNS and routing /// settle, and losing the terminal event **wedges the instance permanently** - /// (protocol spec §3, §7). A caller retrying that report should loop while - /// `is_retryable()` holds (with a bounded backoff), and stop on a permanent - /// error rather than spinning on it — the inverse mistake (retrying a - /// permanent failure) is just as damaging. + /// (there is no server-side self-heal from that state). A caller retrying + /// that report should loop while `is_retryable()` holds (with a bounded + /// backoff), and stop on a permanent error rather than spinning on it — the + /// inverse mistake (retrying a permanent failure) is just as damaging. /// /// Transport and HTTP failures are treated as transient; protocol-level /// failures (serialization, parse, unexpected response, server error status, diff --git a/crates/trident-acl-agent/src/nebraska/event.rs b/crates/trident-acl-agent/src/nebraska/event.rs index 9de3075152..fa91230ddf 100644 --- a/crates/trident-acl-agent/src/nebraska/event.rs +++ b/crates/trident-acl-agent/src/nebraska/event.rs @@ -9,13 +9,11 @@ //! //! An event with any other pair is **silently discarded** — Nebraska still //! returns ``, so the client cannot detect the mistake from -//! the response (protocol spec §2 and §7 trap 1). To make that class of bug -//! impossible, this module never exposes raw integers: callers work with typed -//! events, and the mapping to wire values is private and total over the -//! whitelist. +//! the response. To make that class of bug impossible, this module never +//! exposes raw integers: callers work with typed events, and the mapping to +//! wire values is private and total over the whitelist. //! -//! The events also split into two kinds with very different consequences -//! (protocol spec §3): +//! The events also split into two kinds with very different consequences: //! //! - **Progress** events ([`ProgressEvent`]) are informational. Sending them is //! a *commitment* to also send a terminal event, because leaving an instance @@ -40,7 +38,7 @@ pub(super) struct WirePair { /// /// These correspond to the intermediate Nebraska instance states. Emitting any /// of them commits the caller to eventually reporting a terminal event (success -/// or failure); see the module docs and protocol spec §3. +/// or failure); see the module docs. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ProgressEvent { /// Staging of the update has begun. Wire `(13, 1)` → Nebraska `Downloading`. @@ -94,7 +92,8 @@ pub(super) enum TerminalEvent { /// `Complete`. Completed, /// The update failed. Wire `(3, 0)` → Nebraska `Error`; clears - /// `update_in_progress` and re-arms the instance (protocol spec §6). + /// `update_in_progress` and re-arms the instance so a later check can grant + /// again. Failed, } diff --git a/crates/trident-acl-agent/src/nebraska/id.rs b/crates/trident-acl-agent/src/nebraska/id.rs index 71188b327f..bae1d70b82 100644 --- a/crates/trident-acl-agent/src/nebraska/id.rs +++ b/crates/trident-acl-agent/src/nebraska/id.rs @@ -8,17 +8,16 @@ use super::error::NebraskaError; /// A Nebraska instance identifier. /// -/// `machineid` is the **primary key** of an instance in Nebraska -/// (protocol spec §7 trap 3), so it carries two invariants that fail *silently* -/// when violated, which is why this is a validated newtype rather than a bare -/// `String`: +/// `machineid` is the **primary key** of an instance in Nebraska, so it carries +/// two invariants that fail *silently* when violated, which is why this is a +/// validated newtype rather than a bare `String`: /// /// 1. **It must not be brace-formatted.** Nebraska filters instance ids matching /// `{8-4-4-4-12}` out of both the instance list and the group statistics as -/// "fake instances" (protocol spec §7 trap 2). A client using a braced id is -/// invisible in the UI while appearing to work perfectly over the wire. -/// [`MachineId::new`] rejects such values; [`MachineId::from_uuid`] relies on -/// Rust's [`Uuid`] `Display`, which is hyphenated and unbraced. +/// "fake instances". A client using a braced id is invisible in the UI while +/// appearing to work perfectly over the wire. [`MachineId::new`] rejects such +/// values; [`MachineId::from_uuid`] relies on Rust's [`Uuid`] `Display`, +/// which is hyphenated and unbraced. /// /// 2. **It must be stable across the update reboot.** If it changes, the old /// instance is left behind in whatever state it was in — permanently wedged @@ -85,21 +84,21 @@ mod tests { #[test] fn from_uuid_is_unbraced() { - let uuid = Uuid::parse_str("b187c502-8d4d-9b6f-91f7-cbd2e6a10225").unwrap(); + let uuid = Uuid::parse_str("12345678-1234-4234-8234-1234567890ab").unwrap(); let id = MachineId::from_uuid(uuid); - assert_eq!(id.as_str(), "b187c502-8d4d-9b6f-91f7-cbd2e6a10225"); + assert_eq!(id.as_str(), "12345678-1234-4234-8234-1234567890ab"); assert!(!id.as_str().starts_with('{')); } #[test] fn new_accepts_plain_id() { - let id = MachineId::new("b187c502-8d4d-9b6f-91f7-cbd2e6a10225").unwrap(); - assert_eq!(id.as_str(), "b187c502-8d4d-9b6f-91f7-cbd2e6a10225"); + let id = MachineId::new("12345678-1234-4234-8234-1234567890ab").unwrap(); + assert_eq!(id.as_str(), "12345678-1234-4234-8234-1234567890ab"); } #[test] fn new_rejects_braced_uuid() { - let err = MachineId::new("{b187c502-8d4d-9b6f-91f7-cbd2e6a10225}").unwrap_err(); + let err = MachineId::new("{12345678-1234-4234-8234-1234567890ab}").unwrap_err(); assert!( matches!(err, NebraskaError::InvalidRequest(_)), "got {err:?}" diff --git a/crates/trident-acl-agent/src/nebraska/mod.rs b/crates/trident-acl-agent/src/nebraska/mod.rs index 08059ea569..1b84ebbeae 100644 --- a/crates/trident-acl-agent/src/nebraska/mod.rs +++ b/crates/trident-acl-agent/src/nebraska/mod.rs @@ -11,25 +11,27 @@ //! The Omaha protocol as Nebraska implements it has several invariants that //! **fail silently** when violated — a client can appear to work over the wire //! while leaving the fleet's state permanently wrong. This module encodes those -//! invariants in the type system so they cannot be violated by accident. The -//! authoritative behavioural reference is the protocol spec at -//! `knowledge/topics/nebraska-client-protocol.md` in the `pacobot` repository; -//! the docs below cite its sections. In summary: +//! invariants in the type system so they cannot be violated by accident: //! -//! - **Only six `(eventtype, eventresult)` pairs are accepted**; anything else -//! is silently discarded. Raw integers never appear in the public API — see -//! [`ProgressEvent`] and the private wire mapping (spec §2). -//! - **`track` is mandatory on every request**, including event-only ones. It -//! is a field of [`Client`], so it cannot be omitted (spec §7 trap 4). -//! - **`error-updateInProgressOnInstance` is expected, not fatal.** It is -//! modelled as [`CheckOutcome::UpdateInProgress`], and unknown status strings -//! never break parsing (spec §4, §7 trap 1). -//! - **The machine id must be unbraced and stable.** See [`MachineId`] -//! (spec §7 traps 2, 3). +//! - **Only six `(eventtype, eventresult)` pairs are accepted** by Nebraska; +//! any other pair is silently discarded (the server still returns +//! ``). Raw integers never appear in the public API — see +//! [`ProgressEvent`] and the private wire mapping. +//! - **`track` is mandatory on every request**, including event-only ones: +//! Nebraska resolves the group from `track` before processing events, so +//! omitting it silently drops them. It is a field of [`Client`], so it cannot +//! be omitted. +//! - **`error-updateInProgressOnInstance` is expected, not fatal.** Nebraska +//! returns it on every update check between the first progress event and the +//! terminal one; it is modelled as [`CheckOutcome::UpdateInProgress`], and +//! unknown status strings never break parsing (see [`AppStatus`]). +//! - **The machine id must be unbraced and stable.** Nebraska filters +//! brace-wrapped ids out of its UI and statistics, and uses the id as the +//! instance primary key. See [`MachineId`]. //! - **Event reporting is all-or-nothing.** Sending progress events commits the //! caller to a terminal event that fires after a reboot; the terminal //! operations are dedicated methods on [`Client`] rather than free-standing -//! values (spec §3). +//! values. //! //! # Example //! @@ -38,15 +40,15 @@ //! use url::Url; //! use trident_acl_agent::nebraska::{Client, CheckOutcome, MachineId, ProgressEvent}; //! -//! # fn demo() -> Result<(), Box> { +//! # fn example() -> Result<(), Box> { //! let client = Client::new( -//! Url::parse("https://nebraska.example/v1/update/")?, -//! "6d10cf97-443f-4542-8479-b9fdb44c9588", +//! Url::parse("https://updates.example.com/v1/update/")?, +//! "example-app", //! "stable", //! MachineId::from_uuid(uuid::Uuid::new_v4()), //! ); //! -//! let current = Version::new(3, 0, 20260731); +//! let current = Version::new(1, 0, 0); //! match client.check_for_update(¤t)? { //! CheckOutcome::UpToDate => {} //! CheckOutcome::UpdateInProgress => {} diff --git a/crates/trident-acl-agent/src/nebraska/status.rs b/crates/trident-acl-agent/src/nebraska/status.rs index 8507f1002e..f8d0aa8198 100644 --- a/crates/trident-acl-agent/src/nebraska/status.rs +++ b/crates/trident-acl-agent/src/nebraska/status.rs @@ -1,6 +1,6 @@ //! Response status types, modelled so that unknown values never break parsing. //! -//! Two Nebraska behaviours drive the design here (protocol spec §4 and §7): +//! Two Nebraska behaviours drive the design here: //! //! - `error-updateInProgressOnInstance` is returned on **every** update check //! between the first progress event and the terminal event. It is expected, @@ -22,7 +22,7 @@ pub enum AppStatus { /// An update is already in progress for this instance. Returned on every /// poll between the first progress event and the terminal event; expected, - /// not fatal (protocol spec §4). + /// not fatal. #[serde(rename = "error-updateInProgressOnInstance")] UpdateInProgress, @@ -68,7 +68,7 @@ pub enum UpdateCheckStatus { /// An internal server error. In normal operation this accompanies the /// app-level `error-updateInProgressOnInstance` and is therefore expected - /// during an in-flight update (protocol spec §4). + /// during an in-flight update. #[serde(rename = "error-internal")] ErrorInternal, diff --git a/crates/trident-acl-agent/src/nebraska/wire.rs b/crates/trident-acl-agent/src/nebraska/wire.rs index 81ff1f15dd..5aa1657ee0 100644 --- a/crates/trident-acl-agent/src/nebraska/wire.rs +++ b/crates/trident-acl-agent/src/nebraska/wire.rs @@ -135,8 +135,8 @@ pub(super) struct App { // Child elements are declared — and therefore serialized — in the order // Nebraska logically processes them: events first, then the ping, then the // update check. Nebraska actually processes events before the update check - // regardless of XML order (protocol spec §4), but emitting them in this - // order keeps the batched post-reboot request self-documenting. + // regardless of XML order, but emitting them in this order keeps the batched + // post-reboot request self-documenting. #[serde(rename = "event", skip_serializing_if = "Vec::is_empty")] events: Vec, @@ -151,7 +151,8 @@ impl App { /// Creates a new `` with the mandatory identity fields. `track` is a /// required parameter here — the type cannot be built without it — which is /// how the module guarantees `track` is present on every request, including - /// event-only ones (protocol spec §7 trap 4). + /// event-only ones (Nebraska resolves the group from `track` before + /// processing events, so omitting it silently drops them). pub(super) fn new(app_id: String, version: String, track: String, machine_id: String) -> Self { Self { app_id, @@ -307,7 +308,7 @@ mod tests { fn update_check_request_shape() { let app = App::new( "app-1".into(), - "3.0.20260731".into(), + "1.0.0".into(), "stable".into(), "mid-1".into(), ) @@ -317,13 +318,13 @@ mod tests { assert!(xml.contains(r#"protocol="3.0""#), "{xml}"); assert!(xml.contains(r#"ismachine="1""#), "{xml}"); assert!(xml.contains(r#"appid="app-1""#), "{xml}"); - assert!(xml.contains(r#"version="3.0.20260731""#), "{xml}"); + assert!(xml.contains(r#"version="1.0.0""#), "{xml}"); assert!(xml.contains(r#"track="stable""#), "{xml}"); assert!(xml.contains(r#"machineid="mid-1""#), "{xml}"); assert!(xml.contains(" that must not break parsing. + // A representative positive update-check response, including the empty + // element that must not break parsing. let body = r#" - + - - + + - + @@ -415,14 +416,14 @@ mod tests { let resp = parse_response(body).unwrap(); assert_eq!(resp.apps.len(), 1); let app = &resp.apps[0]; - assert_eq!(app.app_id, "6d10cf97-443f-4542-8479-b9fdb44c9588"); + assert_eq!(app.app_id, "example-app"); assert!(app.status.is_ok()); let uc = app.update_check.as_ref().unwrap(); assert!(uc.status.is_update_available()); - assert_eq!(uc.manifest.as_ref().unwrap().version, "3.0.20260803"); + assert_eq!(uc.manifest.as_ref().unwrap().version, "2.0.0"); assert_eq!( uc.urls.as_ref().unwrap().urls[0].codebase.as_str(), - "http://192.168.122.1:8080/" + "https://updates.example.com/" ); assert_eq!( uc.manifest @@ -433,7 +434,7 @@ mod tests { .unwrap() .packages[0] .name, - "acl-3.0.20260803.cosi" + "os-image-2.0.0.cosi" ); } From 9aea139d39375baa2077d4c554b8f79f6f9bc42d Mon Sep 17 00:00:00 2001 From: Paco Date: Thu, 6 Aug 2026 14:18:04 -0700 Subject: [PATCH 4/6] fix(nebraska): treat HTTP 501 and 4xx as permanent in is_retryable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nebraska returns 501 Not Implemented when an Omaha secret is configured and the client's URL lacks it — a permanent client misconfiguration. Under the previous classification (all HTTP errors retryable) a bounded-retry caller would spin on it. Carry the HTTP status code on NebraskaError::Http and treat 4xx and 501 as permanent, other 5xx as transient. Carrying the code also makes it available for programmatic use, which the flattened String discarded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../trident-acl-agent/src/nebraska/error.rs | 59 ++++++++++++++++--- .../src/nebraska/transport.rs | 10 +++- 2 files changed, 59 insertions(+), 10 deletions(-) diff --git a/crates/trident-acl-agent/src/nebraska/error.rs b/crates/trident-acl-agent/src/nebraska/error.rs index a9cfd6a35d..3be4772747 100644 --- a/crates/trident-acl-agent/src/nebraska/error.rs +++ b/crates/trident-acl-agent/src/nebraska/error.rs @@ -27,9 +27,16 @@ pub enum NebraskaError { #[error("failed to send request to Nebraska: {0}")] Transport(String), - /// The Nebraska server returned a non-success HTTP status. - #[error("Nebraska returned an HTTP error: {0}")] - Http(String), + /// The Nebraska server returned a non-success HTTP status. Carries the + /// status code (when known) so retry logic can distinguish transient + /// server errors from permanent ones. + #[error("Nebraska returned an HTTP error: {message}")] + Http { + /// The HTTP status code, if the failure carried one. + status: Option, + /// The underlying error message. + message: String, + }, /// The response body could not be parsed as an Omaha response. #[error("failed to parse Nebraska response: {0}")] @@ -57,11 +64,29 @@ impl NebraskaError { /// backoff), and stop on a permanent error rather than spinning on it — the /// inverse mistake (retrying a permanent failure) is just as damaging. /// - /// Transport and HTTP failures are treated as transient; protocol-level - /// failures (serialization, parse, unexpected response, server error status, - /// invalid request) are permanent. + /// Transport failures are always transient. HTTP failures are transient only + /// for server-side 5xx errors *other than* `501 Not Implemented`: Nebraska + /// returns 501 when an Omaha secret is configured and the client's URL lacks + /// it, which is a permanent client misconfiguration that must not be retried. + /// 4xx are likewise permanent. All protocol-level failures (serialization, + /// parse, unexpected response, server error status, invalid request) are + /// permanent. pub fn is_retryable(&self) -> bool { - matches!(self, NebraskaError::Transport(_) | NebraskaError::Http(_)) + match self { + NebraskaError::Transport(_) => true, + // A 5xx other than 501 is a transient server/infrastructure error; + // 4xx and 501 are permanent. A missing status (e.g. a body-read + // failure) is treated as transient. + NebraskaError::Http { + status: Some(code), .. + } => (500..600).contains(code) && *code != 501, + NebraskaError::Http { status: None, .. } => true, + NebraskaError::InvalidRequest(_) + | NebraskaError::Serialize(_) + | NebraskaError::Parse(_) + | NebraskaError::UnexpectedResponse(_) + | NebraskaError::ServerError(_) => false, + } } } @@ -69,10 +94,28 @@ impl NebraskaError { mod tests { use super::*; + fn http(status: Option) -> NebraskaError { + NebraskaError::Http { + status, + message: "http error".to_string(), + } + } + #[test] fn transient_errors_are_retryable() { assert!(NebraskaError::Transport("connection refused".into()).is_retryable()); - assert!(NebraskaError::Http("502 Bad Gateway".into()).is_retryable()); + assert!(http(Some(502)).is_retryable()); + assert!(http(Some(503)).is_retryable()); + assert!(http(None).is_retryable()); + } + + #[test] + fn permanent_http_errors_are_not_retryable() { + // 501 = Nebraska rejecting a wrong/missing Omaha secret in the URL. + assert!(!http(Some(501)).is_retryable()); + // 4xx are client errors and permanent. + assert!(!http(Some(400)).is_retryable()); + assert!(!http(Some(404)).is_retryable()); } #[test] diff --git a/crates/trident-acl-agent/src/nebraska/transport.rs b/crates/trident-acl-agent/src/nebraska/transport.rs index 3fd751b7c0..2a1df1fc48 100644 --- a/crates/trident-acl-agent/src/nebraska/transport.rs +++ b/crates/trident-acl-agent/src/nebraska/transport.rs @@ -58,8 +58,14 @@ impl Transport for ReqwestTransport { .send() .map_err(|e| NebraskaError::Transport(e.to_string()))? .error_for_status() - .map_err(|e| NebraskaError::Http(e.to_string()))? + .map_err(|e| NebraskaError::Http { + status: e.status().map(|s| s.as_u16()), + message: e.to_string(), + })? .text() - .map_err(|e| NebraskaError::Http(e.to_string())) + .map_err(|e| NebraskaError::Http { + status: None, + message: e.to_string(), + }) } } From 29a86666d09bcd46b8320cc7c358dd68641af085 Mon Sep 17 00:00:00 2001 From: Paco Date: Fri, 7 Aug 2026 00:56:43 -0700 Subject: [PATCH 5/6] feat(nebraska): complete_after_reboot retries by default; add single-attempt variant Losing the post-reboot terminal event wedges the instance permanently, so retrying it is a correctness requirement rather than a quality-of-service choice. Make the safe behaviour the default: complete_after_reboot now retries retryable failures with a bounded exponential backoff, and callers no longer hand-roll the loop. try_complete_after_reboot is the single-attempt escape hatch for callers that own their own scheduler (e.g. an existing poll loop). The retry classification and a sensible default policy live in the library (where the protocol knowledge is); a caller opts out only when it has a reason to. Adds a small internal RetryPolicy + retry helper with unit tests (first-success, transient-then-success, permanent-not-retried, exhaustion) and a test that the single-attempt variant makes exactly one call. Simplifies the README example to a single call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../trident-acl-agent/src/nebraska/README.md | 16 +- .../trident-acl-agent/src/nebraska/client.rs | 211 ++++++++++++++++-- 2 files changed, 198 insertions(+), 29 deletions(-) diff --git a/crates/trident-acl-agent/src/nebraska/README.md b/crates/trident-acl-agent/src/nebraska/README.md index 6e784f317d..bd8cfe2c37 100644 --- a/crates/trident-acl-agent/src/nebraska/README.md +++ b/crates/trident-acl-agent/src/nebraska/README.md @@ -66,17 +66,15 @@ if let CheckOutcome::UpdateAvailable(offer) = client.check_for_update(¤t)? client.report_progress(¤t, ProgressEvent::Installed)?; // Persist { previous: current, target: offer.version } somewhere durable, - // then reboot. After the reboot, from a fresh process on the new version: + // then reboot. After the reboot, from a fresh process on the new version, + // report completion. This blocks while it retries transient failures (a + // reboot's first network call often fails while DNS settles); losing it + // would wedge the instance permanently, which is why it retries by default. let previous = current; let now_running = offer.version; - loop { - match client.complete_after_reboot(&previous, &now_running) { - Ok(_) => break, - // Retry only transient failures; a permanent one will never succeed. - Err(e) if e.is_retryable() => continue, - Err(e) => return Err(e.into()), - } - } + client.complete_after_reboot(&previous, &now_running)?; + // A caller with its own scheduler can use `try_complete_after_reboot` + // instead and drive the retry on its own cadence. } # Ok(()) # } diff --git a/crates/trident-acl-agent/src/nebraska/client.rs b/crates/trident-acl-agent/src/nebraska/client.rs index 82c0083ddb..e50acb2763 100644 --- a/crates/trident-acl-agent/src/nebraska/client.rs +++ b/crates/trident-acl-agent/src/nebraska/client.rs @@ -1,6 +1,8 @@ //! The high-level [`Client`] for talking to a Nebraska server. -use log::{debug, trace}; +use std::{thread, time::Duration}; + +use log::{debug, trace, warn}; use semver::Version; use url::Url; @@ -42,6 +44,59 @@ pub struct UpdateOffer { pub package_url: Url, } +/// The bounded exponential-backoff policy used by +/// [`Client::complete_after_reboot`] when retrying transient failures. +/// +/// It is intentionally bounded so that a persistently-unreachable server cannot +/// hang startup forever, but generous, because the call it guards must land to +/// avoid permanently wedging the instance. +#[derive(Debug, Clone, Copy)] +struct RetryPolicy { + /// Maximum number of attempts (including the first). + max_attempts: u32, + /// Backoff before the second attempt. + initial_backoff: Duration, + /// Upper bound on the (doubling) backoff. + max_backoff: Duration, +} + +impl Default for RetryPolicy { + fn default() -> Self { + Self { + max_attempts: 8, + initial_backoff: Duration::from_millis(500), + max_backoff: Duration::from_secs(5), + } + } +} + +/// Runs `op`, retrying while it returns a [retryable](NebraskaError::is_retryable) +/// error, with an exponential backoff bounded by `policy`. Returns the first +/// `Ok`, or the last error once attempts are exhausted or a permanent error is +/// hit. Blocks the current thread between attempts. +fn retry( + policy: RetryPolicy, + mut op: impl FnMut() -> Result, +) -> Result { + let mut backoff = policy.initial_backoff; + let mut attempt = 1; + loop { + match op() { + Ok(value) => return Ok(value), + Err(e) if e.is_retryable() && attempt < policy.max_attempts => { + warn!( + "retryable Nebraska error (attempt {attempt}/{}): {e}", + policy.max_attempts + ); + thread::sleep(backoff); + backoff = (backoff * 2).min(policy.max_backoff); + attempt += 1; + } + Err(e) => return Err(e), + } + } +} + /// A client for a single Nebraska app on a single track. /// /// The client bundles the immutable request identity — endpoint, app id, @@ -141,28 +196,35 @@ impl Client { Ok(()) } - /// Reports successful completion after the reboot, in the single batched - /// request Nebraska expects: a terminal `complete` event plus a `` - /// plus an ``. + /// Reports successful completion after the reboot, retrying transient + /// failures automatically. /// - /// Nebraska processes the event before the update check within one request, - /// so this both moves the instance to Complete and returns a clean - /// `noupdate` in one round trip — closing the window in which a bare - /// post-reboot poll would hit `error-updateInProgressOnInstance`. This is - /// the terminal event that discharges the commitment made by - /// [`report_progress`](Client::report_progress). + /// This sends the single batched request Nebraska expects: a terminal + /// `complete` event plus a `` plus an ``. Nebraska + /// processes the event before the update check within one request, so this + /// both moves the instance to Complete and returns a clean `noupdate` in one + /// round trip — closing the window in which a bare post-reboot poll would hit + /// `error-updateInProgressOnInstance`. This is the terminal event that + /// discharges the commitment made by [`report_progress`](Client::report_progress). /// - /// # This call MUST be retried until it succeeds + /// # Why this retries by default /// /// The first network call immediately after a reboot routinely fails while - /// DNS and routing settle. **Losing this terminal event wedges the instance - /// permanently** — there is no server-side self-heal, timer, or REST reset. - /// This module deliberately does not bake in a retry policy (that is the - /// caller's to own, alongside the cross-reboot state it must already - /// persist), but the caller is responsible for retrying: loop with a bounded - /// backoff while [`NebraskaError::is_retryable`] holds, and give up only on a - /// permanent error. See [`report_failure`](Client::report_failure) for the - /// recovery path if completion genuinely cannot be reported. + /// DNS and routing settle, and **losing this terminal event wedges the + /// instance permanently** — there is no server-side self-heal, timer, or REST + /// reset. Retrying is therefore a correctness requirement, not a + /// quality-of-service choice, so it is the default here: this method retries + /// [retryable](NebraskaError::is_retryable) failures with a bounded + /// exponential backoff (a handful of attempts over a few tens of seconds) and + /// returns the last error only once transient retries are exhausted or a + /// permanent error occurs. + /// + /// This blocks the calling thread while retrying. A caller that has its own + /// scheduler (e.g. an existing poll loop) and would rather re-attempt on its + /// own cadence should use [`try_complete_after_reboot`](Client::try_complete_after_reboot) + /// instead and drive the retry itself. If completion genuinely cannot be + /// reported, see [`report_failure`](Client::report_failure) for the recovery + /// path. /// /// `previous_version` is the version the instance was on before the update; /// `current_version` is the (new) version now running. @@ -170,6 +232,29 @@ impl Client { &self, previous_version: &Version, current_version: &Version, + ) -> Result { + retry(RetryPolicy::default(), || { + self.try_complete_after_reboot(previous_version, current_version) + }) + } + + /// Reports successful completion after the reboot in a **single attempt**, + /// without retrying. + /// + /// This is the non-retrying variant of + /// [`complete_after_reboot`](Client::complete_after_reboot); prefer that + /// method unless you are driving retries yourself. + /// + /// Because losing the terminal event wedges the instance permanently, a + /// caller using this variant **must** retry the call itself while the + /// returned error [is retryable](NebraskaError::is_retryable) (with a bounded + /// backoff), giving up only on a permanent error. See + /// [`complete_after_reboot`](Client::complete_after_reboot) for the rationale + /// and [`report_failure`](Client::report_failure) for the recovery path. + pub fn try_complete_after_reboot( + &self, + previous_version: &Version, + current_version: &Version, ) -> Result { let app = self .app(current_version) @@ -353,7 +438,7 @@ impl Client { #[cfg(test)] mod tests { - use std::cell::RefCell; + use std::cell::{Cell, RefCell}; use super::*; @@ -502,4 +587,90 @@ mod tests { "{body}" ); } + + /// A zero-backoff policy so the retry-loop tests do not actually sleep. + fn fast_policy() -> RetryPolicy { + RetryPolicy { + max_attempts: 4, + initial_backoff: Duration::ZERO, + max_backoff: Duration::ZERO, + } + } + + #[test] + fn retry_returns_first_success_without_retrying() { + let calls = Cell::new(0); + let result: Result = retry(fast_policy(), || { + calls.set(calls.get() + 1); + Ok(42) + }); + assert_eq!(result.unwrap(), 42); + assert_eq!(calls.get(), 1); + } + + #[test] + fn retry_retries_transient_then_succeeds() { + let calls = Cell::new(0); + let result: Result = retry(fast_policy(), || { + calls.set(calls.get() + 1); + if calls.get() < 3 { + Err(NebraskaError::Transport("dns not ready".into())) + } else { + Ok(7) + } + }); + assert_eq!(result.unwrap(), 7); + assert_eq!(calls.get(), 3); + } + + #[test] + fn retry_does_not_retry_permanent_error() { + let calls = Cell::new(0); + let result: Result = retry(fast_policy(), || { + calls.set(calls.get() + 1); + Err(NebraskaError::UnexpectedResponse("bad".into())) + }); + assert!(matches!(result, Err(NebraskaError::UnexpectedResponse(_)))); + assert_eq!(calls.get(), 1, "a permanent error must not be retried"); + } + + #[test] + fn retry_gives_up_after_max_attempts() { + let calls = Cell::new(0); + let result: Result = retry(fast_policy(), || { + calls.set(calls.get() + 1); + Err(NebraskaError::Transport("still down".into())) + }); + assert!(matches!(result, Err(NebraskaError::Transport(_)))); + assert_eq!(calls.get(), 4, "should stop at max_attempts"); + } + + #[test] + fn try_complete_after_reboot_is_single_attempt() { + // A transport that always fails transiently: the non-retrying variant + // must call it exactly once and surface the retryable error. + struct AlwaysFails { + calls: Cell, + } + impl Transport for AlwaysFails { + fn post_xml(&self, _endpoint: &Url, _body: &[u8]) -> Result { + self.calls.set(self.calls.get() + 1); + Err(NebraskaError::Transport("down".into())) + } + } + let client = Client::with_transport( + Url::parse("https://nebraska.example/v1/update/").unwrap(), + "app-1", + "stable", + MachineId::new("mid-1").unwrap(), + AlwaysFails { + calls: Cell::new(0), + }, + ); + let err = client + .try_complete_after_reboot(&Version::new(1, 0, 0), &Version::new(2, 0, 0)) + .unwrap_err(); + assert!(err.is_retryable()); + assert_eq!(client.transport.calls.get(), 1); + } } From 0d2317324158e837351cdd4199dfc0f007ee181e Mon Sep 17 00:00:00 2001 From: Paco Date: Fri, 7 Aug 2026 13:29:17 -0700 Subject: [PATCH 6/6] feat(nebraska): surface package hash and size on UpdateOffer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously UpdateOffer exposed only version + package_url and dropped the package hash Nebraska sends. Parse and surface it: add package_hash (PackageHash { sha1, sha256 }) and package_size to UpdateOffer, and capture the hash/hash_sha256/size attributes at the wire layer. The hash is documented clearly as a hash of the package *file* (Nebraska sends a base64 SHA-1, optionally SHA-256), for integrity-checking the download — not a hash of any content embedded within the package. An unparseable size is treated as absent rather than failing the offer. Adds tests for hash+size present and absent, and asserts the wire layer captures them. cargo fmt / clippy --all-targets -D warnings clean; 33 module tests + doctest. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../trident-acl-agent/src/nebraska/README.md | 5 ++ .../trident-acl-agent/src/nebraska/client.rs | 66 ++++++++++++++++++- crates/trident-acl-agent/src/nebraska/mod.rs | 2 +- crates/trident-acl-agent/src/nebraska/wire.rs | 35 ++++++---- 4 files changed, 94 insertions(+), 14 deletions(-) diff --git a/crates/trident-acl-agent/src/nebraska/README.md b/crates/trident-acl-agent/src/nebraska/README.md index bd8cfe2c37..28632f78a7 100644 --- a/crates/trident-acl-agent/src/nebraska/README.md +++ b/crates/trident-acl-agent/src/nebraska/README.md @@ -33,6 +33,11 @@ match client.check_for_update(¤t)? { CheckOutcome::UpdateInProgress => println!("update already in progress"), CheckOutcome::UpdateAvailable(offer) => { println!("update to {} at {}", offer.version, offer.package_url); + // The package file's hash (base64 SHA-1, plus SHA-256 when present) is + // available for integrity-checking the downloaded artifact: + if let Some(hash) = &offer.package_hash { + println!("expected file sha1: {}", hash.sha1); + } } } # Ok(()) diff --git a/crates/trident-acl-agent/src/nebraska/client.rs b/crates/trident-acl-agent/src/nebraska/client.rs index e50acb2763..af0b5b8ca5 100644 --- a/crates/trident-acl-agent/src/nebraska/client.rs +++ b/crates/trident-acl-agent/src/nebraska/client.rs @@ -33,7 +33,8 @@ pub enum CheckOutcome { UpdateInProgress, } -/// An offered update: the version and the fully-resolved package URL. +/// An offered update: the version, the fully-resolved package URL, and the +/// package's hash and size as reported by Nebraska. #[derive(Debug, Clone, PartialEq, Eq)] pub struct UpdateOffer { /// The version being offered. @@ -42,6 +43,34 @@ pub struct UpdateOffer { /// The absolute URL of the update package, resolved by joining the /// response's `codebase` with the package `name`. pub package_url: Url, + + /// The hash of the package *file* as reported by Nebraska. + /// + /// **This is a hash of the package file, not of any content inside it.** + /// Nebraska's `hash` attribute is a base64-encoded SHA-1 of the file (with + /// an optional SHA-256). It is provided for integrity checking of the + /// downloaded artifact; note in particular that it is **not** the same as a + /// hash of a manifest or other content embedded within the package, so it + /// cannot be used where such an inner hash is required. `None` if the + /// response carried no hash. + pub package_hash: Option, + + /// The package size in bytes, as reported by Nebraska, if present. + pub package_size: Option, +} + +/// The hash(es) of an update package file, as reported by Nebraska. +/// +/// Both values are base64-encoded and hash the package *file* (not its +/// contents). Nebraska always populates the SHA-1 `sha1` field; `sha256` is +/// present only when the package was registered with one. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PackageHash { + /// Base64-encoded SHA-1 of the package file. + pub sha1: String, + + /// Base64-encoded SHA-256 of the package file, when Nebraska provides it. + pub sha256: Option, } /// The bounded exponential-backoff policy used by @@ -429,9 +458,20 @@ impl Client { )) })?; + let package_hash = package.hash.as_ref().map(|sha1| PackageHash { + sha1: sha1.clone(), + sha256: package.hash_sha256.clone(), + }); + + // Size is a string on the wire; surface it as a number when it parses, + // and treat an unparseable size as absent rather than failing the offer. + let package_size = package.size.as_ref().and_then(|s| s.parse::().ok()); + Ok(UpdateOffer { version, package_url, + package_hash, + package_size, }) } } @@ -482,7 +522,7 @@ mod tests { - + @@ -499,11 +539,33 @@ mod tests { offer.package_url.as_str(), "https://updates.example.com/os-image-2.0.0.cosi" ); + assert_eq!( + offer.package_hash, + Some(PackageHash { + sha1: "AAAAAAAAAAAAAAAAAAAAAAAAAAA=".to_string(), + sha256: None, + }) + ); + assert_eq!(offer.package_size, Some(1024)); } other => panic!("expected an update offer, got {other:?}"), } } + #[test] + fn check_offer_without_hash_is_none() { + let client = client_with( + r#""#, + ); + match client.check_for_update(&Version::new(1, 0, 0)).unwrap() { + CheckOutcome::UpdateAvailable(offer) => { + assert_eq!(offer.package_hash, None); + assert_eq!(offer.package_size, None); + } + other => panic!("expected an offer, got {other:?}"), + } + } + #[test] fn check_reports_no_update() { let client = client_with( diff --git a/crates/trident-acl-agent/src/nebraska/mod.rs b/crates/trident-acl-agent/src/nebraska/mod.rs index 1b84ebbeae..fb58e21760 100644 --- a/crates/trident-acl-agent/src/nebraska/mod.rs +++ b/crates/trident-acl-agent/src/nebraska/mod.rs @@ -72,7 +72,7 @@ mod status; mod transport; mod wire; -pub use client::{CheckOutcome, Client, UpdateOffer}; +pub use client::{CheckOutcome, Client, PackageHash, UpdateOffer}; pub use error::NebraskaError; pub use event::ProgressEvent; pub use id::MachineId; diff --git a/crates/trident-acl-agent/src/nebraska/wire.rs b/crates/trident-acl-agent/src/nebraska/wire.rs index 5aa1657ee0..a2f83df8eb 100644 --- a/crates/trident-acl-agent/src/nebraska/wire.rs +++ b/crates/trident-acl-agent/src/nebraska/wire.rs @@ -288,6 +288,19 @@ pub(super) struct Packages { pub(super) struct Package { #[serde(rename = "@name")] pub(super) name: String, + + /// The package hash as sent by Nebraska: base64-encoded SHA-1 of the package + /// *file*. Optional because not every response carries it. + #[serde(default, rename = "@hash")] + pub(super) hash: Option, + + /// The optional SHA-256 package hash, base64-encoded, when present. + #[serde(default, rename = "@hash_sha256")] + pub(super) hash_sha256: Option, + + /// The package size in bytes, as a string in the wire format. + #[serde(default, rename = "@size")] + pub(super) size: Option, } /// Parses a Nebraska response body. @@ -425,17 +438,17 @@ mod tests { uc.urls.as_ref().unwrap().urls[0].codebase.as_str(), "https://updates.example.com/" ); - assert_eq!( - uc.manifest - .as_ref() - .unwrap() - .packages - .as_ref() - .unwrap() - .packages[0] - .name, - "os-image-2.0.0.cosi" - ); + let pkg = &uc + .manifest + .as_ref() + .unwrap() + .packages + .as_ref() + .unwrap() + .packages[0]; + assert_eq!(pkg.name, "os-image-2.0.0.cosi"); + assert_eq!(pkg.hash.as_deref(), Some("AAAAAAAAAAAAAAAAAAAAAAAAAAA=")); + assert_eq!(pkg.size.as_deref(), Some("368420864")); } #[test]