diff --git a/Cargo.lock b/Cargo.lock index c78a95d..9139e2c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1140,7 +1140,7 @@ checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe" [[package]] name = "openpxe" -version = "0.4.63" +version = "0.4.64" dependencies = [ "anyhow", "axum", @@ -1162,7 +1162,7 @@ dependencies = [ [[package]] name = "openpxe-core" -version = "0.4.63" +version = "0.4.64" dependencies = [ "anyhow", "bcrypt", @@ -1181,7 +1181,7 @@ dependencies = [ [[package]] name = "openpxe-dhcp-proxy" -version = "0.4.63" +version = "0.4.64" dependencies = [ "anyhow", "bytes", @@ -1195,7 +1195,7 @@ dependencies = [ [[package]] name = "openpxe-http-api" -version = "0.4.63" +version = "0.4.64" dependencies = [ "anyhow", "axum", @@ -1226,7 +1226,7 @@ dependencies = [ [[package]] name = "openpxe-ipxe-assets" -version = "0.4.63" +version = "0.4.64" dependencies = [ "openpxe-core", "rust-embed", @@ -1236,7 +1236,7 @@ dependencies = [ [[package]] name = "openpxe-iso-store" -version = "0.4.63" +version = "0.4.64" dependencies = [ "anyhow", "bcrypt", @@ -1260,7 +1260,7 @@ dependencies = [ [[package]] name = "openpxe-tftp" -version = "0.4.63" +version = "0.4.64" dependencies = [ "anyhow", "bytes", @@ -1274,7 +1274,7 @@ dependencies = [ [[package]] name = "openpxe-webui" -version = "0.4.63" +version = "0.4.64" [[package]] name = "parking_lot" diff --git a/Cargo.toml b/Cargo.toml index 28c5b8b..3538ceb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,7 +12,7 @@ members = [ ] [workspace.package] -version = "0.4.63" +version = "0.4.64" edition = "2021" rust-version = "1.95" license = "MIT OR Apache-2.0" diff --git a/crates/http-api/src/app.rs b/crates/http-api/src/app.rs index 2fd74d7..98044d3 100644 --- a/crates/http-api/src/app.rs +++ b/crates/http-api/src/app.rs @@ -1713,11 +1713,12 @@ async fn api_nfs_list(State(state): State) -> Json async fn api_nfs_add(State(state): State, Json(req): Json) -> Response { match state.nfs.add(req).await { Ok(m) => (StatusCode::CREATED, Json(m)).into_response(), - // Anything from the manager surfaces as a user-fixable validation - // error — bad host, kernel without NFS support, missing - // `mount.nfs`, dead server. We pass the message through verbatim - // so the UI can show it to the operator. - Err(e) => (StatusCode::BAD_REQUEST, format!("{e}")).into_response(), + // v0.4.64: the manager returns a structured `NfsMountError` with + // `error` + optional `hint` + the raw `stderr`, so the UI can + // show both — the raw message for completeness, the hint for + // "what to fix next". Previously this was a plain text body + // which collapsed both bits of information into one line. + Err(err) => (StatusCode::BAD_REQUEST, Json(err)).into_response(), } } diff --git a/crates/http-api/src/terminal.rs b/crates/http-api/src/terminal.rs index c0f48b8..8cd9aeb 100644 --- a/crates/http-api/src/terminal.rs +++ b/crates/http-api/src/terminal.rs @@ -321,10 +321,25 @@ async fn nfs_command(s: &AppState, args: &[String]) -> Result { export: export.to_string(), version, read_only, + // v0.4.64: terminal callers can't override the port yet + // — keep the default 2049. We could plumb a 4th arg + // later if anyone asks. + port: None, }; match s.nfs.add(req).await { Ok(m) => Ok(format!("mounted {} ({} isos)", m.id, m.iso_count)), - Err(e) => Err(format!("mount failed: {e}")), + // v0.4.64: `add` now returns a structured `NfsMountError`. + // We render the raw error plus the hint (if any) on + // separate lines so the terminal output mirrors what + // the Storage tab shows. + Err(e) => { + let mut out = format!("mount failed: {}", e.error); + if let Some(h) = e.hint { + out.push_str("\nhint: "); + out.push_str(&h); + } + Err(out) + } } } Some("unmount") => { diff --git a/crates/iso-store/src/nfs.rs b/crates/iso-store/src/nfs.rs index c1ac159..eecf004 100644 --- a/crates/iso-store/src/nfs.rs +++ b/crates/iso-store/src/nfs.rs @@ -9,14 +9,15 @@ //! 1. Operator submits a mount spec via the Storage tab: //! `{ server: "10.0.0.20", export: "/srv/isos", version: "v41" }`. //! 2. We slugify a stable id, mkdir `/nfs//`, then shell out -//! to `/bin/mount -t nfs -o vers=...,ro,nolock server:export local`. +//! to `mount.nfs -v -o vers=...,ro,nolock,proto=tcp server:export local`. //! 3. On success we walk the mount point looking for `*.iso` files and //! register each one with the `IsoStore` as an external source — same //! introspection pipeline as a web upload, but no sha256 (the bytes //! live on a remote machine; hashing them would suck them through the //! network on every restart). -//! 4. On failure we record `last_error` on the spec and persist anyway -//! so the UI can show a row in red rather than silently dropping it. +//! 4. On failure we record `last_error` + `hint` on the spec and persist +//! anyway so the UI can show a row in red with an actionable hint +//! rather than silently dropping it. //! //! ## Operational notes //! @@ -28,6 +29,28 @@ //! - Mount commands are issued sequentially under a single mutex to avoid //! `mount` racing on the same target dir. //! +//! ## v0.4.64 diagnostics rework +//! +//! Field reports showed `mount.nfs: failed to apply fstab options` (exit +//! code 32) was the dominant failure surfaced through the UI — a deeply +//! unhelpful message from nfs-utils 2.6.x that has nothing to do with +//! `/etc/fstab`. It comes from `nfs_options2string()` and lights up when +//! the kernel can't accept the assembled options, when mtab can't be +//! written (container without `CAP_SYS_ADMIN`), or when an obscure option +//! triggers a transformation edge case. In v0.4.64 we: +//! +//! 1. Probe TCP reach to `server:port` before shelling out so a wrong +//! IP / closed firewall surfaces as a clear "cannot reach NFS port" +//! instead of `failed to apply fstab options`. +//! 2. Pass `proto=tcp` explicitly on NFSv3 (UDP is widely deprecated +//! and several NAS appliances don't bind it at all). +//! 3. On `failed to apply fstab options`, retry with a stripped-down +//! option set (`vers=N,ro/rw`) — that frequently succeeds and at +//! minimum produces a real kernel error. +//! 4. Translate well-known stderr patterns into operator-friendly hints +//! and persist them on the mount so the UI can show "what to fix +//! next" instead of the raw mount.nfs message. +//! //! ## Persistence //! //! Mount specs (without runtime state) live at `/nfs.json`, @@ -42,9 +65,20 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::Arc; +use std::time::Duration; use time::OffsetDateTime; use tokio::process::Command; +/// Default port for NFS over TCP. We expose it as a constant so the +/// pre-flight probe and the option string assembly use the same value. +const DEFAULT_NFS_PORT: u16 = 2049; + +/// How long to wait for a TCP connection to the NFS server before +/// declaring it unreachable. Short enough that a wrong IP doesn't make +/// the UI hang for half a minute; long enough that a slow appliance +/// can still answer. +const PROBE_TIMEOUT: Duration = Duration::from_secs(4); + /// Wire-protocol versions we support. Keep this enum closed — silently /// accepting "auto" or letting the kernel negotiate would mean operators /// could never confirm which version is in use. @@ -65,6 +99,14 @@ impl NfsVersion { Self::V41 => "vers=4.1", } } + + /// Short label for UI surfaces and log lines. + fn label(self) -> &'static str { + match self { + Self::V3 => "NFSv3", + Self::V41 => "NFSv4.1", + } + } } /// One configured mount. The id is generated from server+export so the @@ -78,6 +120,12 @@ pub struct NfsMount { /// Read-only by default — most ISO libraries are. Operators that need /// write can flip this off but OpenPXE itself never writes. pub read_only: bool, + /// TCP port for the NFS service. Defaults to 2049; configurable for + /// the (rare) case where the appliance binds the service elsewhere. + /// v0.4.64: previously inferred at runtime; now persisted so the UI + /// can echo the value back to the operator. + #[serde(default = "default_port")] + pub port: u16, /// Local mount point under `/nfs/`. pub local_path: PathBuf, /// Whether the mount is currently active. @@ -85,6 +133,12 @@ pub struct NfsMount { /// Last error encountered on a `mount` or `umount` attempt; cleared on /// success. pub last_error: Option, + /// v0.4.64: operator-friendly translation of `last_error` — e.g. for + /// "failed to apply fstab options" we surface "CAP_SYS_ADMIN may be + /// missing on the container". `None` means we don't have a friendlier + /// rendition than the raw error. + #[serde(default)] + pub last_hint: Option, #[serde(with = "time::serde::rfc3339::option")] pub last_attempt: Option, /// Number of `.iso` files found on the share (re-counted on each scan). @@ -100,6 +154,9 @@ pub struct NfsAddRequest { pub version: NfsVersion, #[serde(default = "default_ro")] pub read_only: bool, + /// Optional TCP port — defaults to 2049 if omitted or zero. + #[serde(default)] + pub port: Option, } fn default_version() -> NfsVersion { @@ -108,6 +165,35 @@ fn default_version() -> NfsVersion { fn default_ro() -> bool { true } +fn default_port() -> u16 { + DEFAULT_NFS_PORT +} + +/// Outcome of an `add` attempt. `Ok` carries the mount; `Err` from the +/// API layer is converted to this richer shape so the UI can render the +/// raw error and the actionable hint independently. +#[derive(Debug, Clone, Serialize)] +pub struct NfsMountError { + /// The first line / summary of what went wrong. + pub error: String, + /// Verbatim stderr from `mount.nfs` (trimmed). May be empty. + pub stderr: String, + /// Operator-friendly hint or `None` if we don't have one. + pub hint: Option, +} + +impl NfsMountError { + fn from_raw(error: impl Into, stderr: impl Into) -> Self { + let stderr = stderr.into(); + let error = error.into(); + let hint = hint_for(&stderr).or_else(|| hint_for(&error)); + Self { + error, + stderr, + hint, + } + } +} #[derive(Debug, Default)] struct Inner { @@ -165,6 +251,7 @@ impl NfsManager { // when the process died. We'll try to remount each one. m.mounted = false; m.last_error = None; + m.last_hint = None; self.inner.lock().mounts.insert(m.id.clone(), m.clone()); if let Err(e) = self.try_mount(&m.id).await { tracing::warn!( @@ -178,20 +265,42 @@ impl NfsManager { } /// Add a new mount. Returns the resulting `NfsMount` (with `mounted` - /// reflecting reality) or an error if the spec was invalid. - pub async fn add(&self, req: NfsAddRequest) -> Result { - let server = req.server.trim().to_string(); + /// reflecting reality) or a structured `NfsMountError` describing + /// what went wrong. + pub async fn add( + &self, + req: NfsAddRequest, + ) -> std::result::Result { + let server = normalize_server(&req.server); let export = req.export.trim().to_string(); if server.is_empty() { - return Err(Error::Invalid("server is required".into())); + return Err(NfsMountError::from_raw( + "server is required", + "", + )); } if !export.starts_with('/') { - return Err(Error::Invalid("export path must start with '/'".into())); + return Err(NfsMountError::from_raw( + "export path must start with '/'", + "", + )); } + if export.contains('\0') || server.contains('\0') { + return Err(NfsMountError::from_raw( + "server / export must not contain NUL bytes", + "", + )); + } + let port = req.port.filter(|p| *p != 0).unwrap_or(DEFAULT_NFS_PORT); let id = mount_id(&server, &export); let local_path = self.work_root.join(&id); - tokio::fs::create_dir_all(&local_path).await?; + if let Err(e) = tokio::fs::create_dir_all(&local_path).await { + return Err(NfsMountError::from_raw( + format!("failed to create local mount point: {e}"), + "", + )); + } let mount = NfsMount { id: id.clone(), @@ -199,15 +308,28 @@ impl NfsManager { export, version: req.version, read_only: req.read_only, + port, local_path, mounted: false, last_error: None, + last_hint: None, last_attempt: None, iso_count: 0, }; self.inner.lock().mounts.insert(id.clone(), mount); self.persist_locked(); - self.try_mount(&id).await?; + self.try_mount(&id).await.map_err(|e| { + // try_mount has already persisted last_error/last_hint. We + // refetch them so the API response reflects exactly what the + // UI will see when it lists mounts. + let m = self.get(&id); + NfsMountError { + error: m.as_ref().and_then(|m| m.last_error.clone()) + .unwrap_or_else(|| e.to_string()), + stderr: String::new(), + hint: m.and_then(|m| m.last_hint), + } + })?; Ok(self.get(&id).expect("mount just inserted")) } @@ -276,63 +398,113 @@ impl NfsManager { // Already mounted? Skip — `mount` would error on a busy target // and confuse the operator's UI status. if is_mountpoint(&m.local_path).await { - self.update_status(id, true, None, now); + self.update_status(id, true, None, None, now); // Even though already mounted, we still want a fresh ISO count. let count = self.scan_and_register(&m).await.unwrap_or(0); self.update_iso_count(id, count); return Ok(()); } - let opts = mount_options(&m); - let target = format!("{}:{}", m.server, m.export); - - let output = Command::new("mount") - .arg("-t") - .arg("nfs") - .arg("-o") - .arg(&opts) - .arg(&target) - .arg(&m.local_path) - .output() - .await; - - match output { - Ok(out) if out.status.success() => { - tracing::info!( - target: "openpxe::nfs", - id = %id, server = %m.server, export = %m.export, - version = ?m.version, - "NFS mount succeeded" - ); - self.update_status(id, true, None, now); - let count = self.scan_and_register(&m).await.unwrap_or(0); - self.update_iso_count(id, count); - Ok(()) - } - Ok(out) => { - let err = format!( - "mount exit {}: {}", - out.status.code().unwrap_or(-1), - String::from_utf8_lossy(&out.stderr).trim() - ); - tracing::warn!(target: "openpxe::nfs", id = %id, "{err}"); - self.update_status(id, false, Some(err.clone()), now); - Err(Error::Invalid(err)) - } - Err(e) => { - let err = format!("could not exec /bin/mount: {e}"); - tracing::error!(target: "openpxe::nfs", id = %id, "{err}"); - self.update_status(id, false, Some(err.clone()), now); - Err(Error::Invalid(err)) - } + // v0.4.64: pre-flight TCP probe. Catches the dominant failure + // mode (wrong IP / firewall) before mount.nfs gets a chance to + // emit its unhelpful "failed to apply fstab options" message. + if let Err((err, hint)) = tcp_probe(&m.server, m.port).await { + tracing::warn!(target: "openpxe::nfs", id = %id, "{err}"); + self.update_status(id, false, Some(err.clone()), Some(hint), now); + return Err(Error::Invalid(err)); } + + // First attempt: full option set. + let full_opts = mount_options(&m, /*minimal*/ false); + let target = format!("{}:{}", m.server, m.export); + let attempt = run_mount_nfs(&full_opts, &target, &m.local_path).await; + + let (success, stderr, exit_code) = match attempt { + Ok((true, stderr, _)) => (true, stderr, 0), + Ok((false, stderr, code)) => (false, stderr, code), + Err(e) => { + let err = format!("could not exec mount(8): {e}"); + let hint = Some( + "the runtime image is missing /bin/mount or nfs-common — \ + verify the container hasn't been stripped down" + .to_string(), + ); + tracing::error!(target: "openpxe::nfs", id = %id, "{err}"); + self.update_status(id, false, Some(err.clone()), hint, now); + return Err(Error::Invalid(err)); + } + }; + + if success { + tracing::info!( + target: "openpxe::nfs", + id = %id, server = %m.server, export = %m.export, + version = %m.version.label(), port = m.port, + "NFS mount succeeded" + ); + self.update_status(id, true, None, None, now); + let count = self.scan_and_register(&m).await.unwrap_or(0); + self.update_iso_count(id, count); + return Ok(()); + } + + // Second attempt: if the first attempt failed with the + // "failed to apply fstab options" oddity, retry with a minimal + // option set. nfs-utils 2.6.x sometimes chokes on the assembled + // option string for reasons unrelated to the actual options + // being valid; the stripped form bypasses the transformation + // edge case. + let trigger_retry = looks_like_option_transform_failure(&stderr); + let (final_success, final_stderr, final_exit_code) = if trigger_retry { + tracing::info!( + target: "openpxe::nfs", id = %id, + "retrying with minimal options after option-transform failure" + ); + let minimal = mount_options(&m, /*minimal*/ true); + match run_mount_nfs(&minimal, &target, &m.local_path).await { + Ok((true, s, _)) => (true, s, 0), + Ok((false, s, c)) => (false, s, c), + Err(e) => (false, format!("could not exec mount(8): {e}"), -1), + } + } else { + (false, stderr, exit_code) + }; + + if final_success { + tracing::info!( + target: "openpxe::nfs", id = %id, + "NFS mount succeeded on minimal-options retry" + ); + self.update_status(id, true, None, None, now); + let count = self.scan_and_register(&m).await.unwrap_or(0); + self.update_iso_count(id, count); + return Ok(()); + } + + // Failure path: persist a clear error and a hint, log both. + // `mount(8)` passes mount.nfs's stderr through verbatim, so the + // user-visible text reads like "mount.nfs: ..." — we prepend the + // exit code so the operator can tell at a glance that the helper + // ran but rejected the request, vs the helper not running at all. + let err = if final_stderr.is_empty() { + format!("mount exit {final_exit_code}") + } else { + format!("mount exit {final_exit_code}: {}", final_stderr.trim()) + }; + let hint = hint_for(&final_stderr); + tracing::warn!( + target: "openpxe::nfs", id = %id, + hint = ?hint, "{err}" + ); + self.update_status(id, false, Some(err.clone()), hint, now); + Err(Error::Invalid(err)) } async fn umount_one(&self, id: &str) -> Result<()> { let _g = self.mount_lock.lock().await; let Some(m) = self.get(id) else { return Ok(()) }; if !is_mountpoint(&m.local_path).await { - self.update_status(id, false, None, OffsetDateTime::now_utc()); + self.update_status(id, false, None, None, OffsetDateTime::now_utc()); return Ok(()); } // -l = lazy: detach immediately, finish when no process has a @@ -344,7 +516,7 @@ impl NfsManager { .await; match out { Ok(o) if o.status.success() => { - self.update_status(id, false, None, OffsetDateTime::now_utc()); + self.update_status(id, false, None, None, OffsetDateTime::now_utc()); Ok(()) } Ok(o) => { @@ -353,12 +525,24 @@ impl NfsManager { o.status.code().unwrap_or(-1), String::from_utf8_lossy(&o.stderr).trim() ); - self.update_status(id, false, Some(e.clone()), OffsetDateTime::now_utc()); + self.update_status( + id, + false, + Some(e.clone()), + None, + OffsetDateTime::now_utc(), + ); Err(Error::Invalid(e)) } Err(e) => { let e = format!("could not exec /bin/umount: {e}"); - self.update_status(id, false, Some(e.clone()), OffsetDateTime::now_utc()); + self.update_status( + id, + false, + Some(e.clone()), + None, + OffsetDateTime::now_utc(), + ); Err(Error::Invalid(e)) } } @@ -410,10 +594,18 @@ impl NfsManager { Ok(count) } - fn update_status(&self, id: &str, mounted: bool, err: Option, ts: OffsetDateTime) { + fn update_status( + &self, + id: &str, + mounted: bool, + err: Option, + hint: Option, + ts: OffsetDateTime, + ) { if let Some(m) = self.inner.lock().mounts.get_mut(id) { m.mounted = mounted; m.last_error = err; + m.last_hint = hint; m.last_attempt = Some(ts); } self.persist_locked(); @@ -453,18 +645,34 @@ impl NfsManager { } } -fn mount_options(m: &NfsMount) -> String { +/// Build the `-o` option list. With `minimal=true` we strip everything +/// except the protocol version and ro/rw — used on the retry path when +/// the first attempt failed at option transformation, which historically +/// indicates one of the auxiliary options confused `nfs_options2string()`. +fn mount_options(m: &NfsMount, minimal: bool) -> String { let mut opts = vec![m.version.vers_arg().to_string()]; if m.read_only { opts.push("ro".into()); } else { opts.push("rw".into()); } + if minimal { + return opts.join(","); + } + // Explicit TCP. NFSv4.x is TCP-only by spec, but stating it + // doesn't hurt and on NFSv3 it's necessary on appliances that + // don't bind UDP (which is most modern ones). + opts.push("proto=tcp".into()); // `nolock` for v3 — many storage appliances disable lockd; we don't - // need locking for read-only ISO access anyway. + // need locking for read-only ISO access anyway. nfs-utils still + // tries to contact rpc.statd without it which is a no-op overhead. if matches!(m.version, NfsVersion::V3) { opts.push("nolock".into()); } + // Non-standard port hint to the kernel. + if m.port != DEFAULT_NFS_PORT { + opts.push(format!("port={}", m.port)); + } // Soft mount with a generous timeout — better to surface a hung share // as a user-visible error than to wedge the iPXE client forever on a // dead NFS server. @@ -474,6 +682,158 @@ fn mount_options(m: &NfsMount) -> String { opts.join(",") } +/// Invoke `mount -t nfs`. Returns `(success, stderr_trimmed, +/// exit_code)`. `stderr` is captured separately from `stdout`; +/// `mount(8)` passes mount.nfs's stderr through verbatim, so we get the +/// same diagnostics ("mount.nfs: ...") whether we invoke `mount.nfs` +/// directly or go through the generic wrapper. +/// +/// We deliberately stay on `mount` rather than `mount.nfs` directly +/// because `/bin/mount` is in every user's PATH; `mount.nfs` lives in +/// `/sbin` (or `/usr/sbin`) and is *not* in the default PATH for the +/// non-root `openpxe` user. The generic `mount` binary knows where its +/// NFS helper lives and dispatches accordingly. +async fn run_mount_nfs( + opts: &str, + target: &str, + local: &Path, +) -> std::io::Result<(bool, String, i32)> { + let output = Command::new("mount") + .arg("-t") + .arg("nfs") + .arg("-o") + .arg(opts) + .arg(target) + .arg(local) + .output() + .await?; + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + let code = output.status.code().unwrap_or(-1); + Ok((output.status.success(), stderr, code)) +} + +/// Try to open a TCP connection to `server:port` within `PROBE_TIMEOUT`. +/// On failure returns `(error_text, hint_text)` — pre-formatted so the +/// caller can persist both. +async fn tcp_probe(server: &str, port: u16) -> std::result::Result<(), (String, String)> { + use tokio::net::TcpStream; + let addr = format!("{server}:{port}"); + let connect = TcpStream::connect(&addr); + match tokio::time::timeout(PROBE_TIMEOUT, connect).await { + Ok(Ok(_stream)) => Ok(()), + Ok(Err(e)) => Err(( + format!("cannot reach NFS port: {addr}: {e}"), + format!( + "verify the NFS service is running on {server} and that port {port} is open" + ), + )), + Err(_) => Err(( + format!("cannot reach NFS port: {addr}: timed out after {}s", PROBE_TIMEOUT.as_secs()), + format!( + "no TCP answer from {server}:{port} within {}s — check the IP and any firewall in between", + PROBE_TIMEOUT.as_secs() + ), + )), + } +} + +/// Detect mount.nfs's "failed to apply fstab options" / "internal option +/// parsing error" path. These messages come from +/// `nfs_options2string()` / `nfs_validate_options()` in nfs-utils and +/// are emitted *before* the mount(2) syscall, so retrying with a +/// stripped option set often succeeds. +fn looks_like_option_transform_failure(stderr: &str) -> bool { + let s = stderr.to_ascii_lowercase(); + s.contains("failed to apply fstab options") + || s.contains("internal option parsing error") +} + +/// Translate a mount.nfs stderr blob into an operator-friendly hint. +/// Returns `None` if we don't have a translation — the caller will fall +/// back to surfacing the raw stderr. +#[allow(clippy::if_same_then_else)] // ordering matters; keep the patterns explicit +fn hint_for(stderr: &str) -> Option { + let s = stderr.to_ascii_lowercase(); + if s.contains("failed to apply fstab options") || s.contains("internal option parsing error") { + // The dominant report from the field: mount.nfs failed at the + // option-transform layer. Most common root cause is missing + // CAP_SYS_ADMIN in the container. + Some( + "mount.nfs couldn't finalize the mount. Most common cause: the container is \ + missing CAP_SYS_ADMIN (run with --cap-add=SYS_ADMIN, or use a privileged SCC on \ + OpenShift). Also check that /etc/mtab exists and the host kernel has NFS client \ + support." + .into(), + ) + } else if s.contains("operation not permitted") || s.contains("permission denied") { + Some( + "the container is missing CAP_SYS_ADMIN — mount(2) returns EPERM without it. Re-run \ + with --cap-add=SYS_ADMIN, or grant the OpenShift pod a privileged SCC." + .into(), + ) + } else if s.contains("access denied by server") { + Some( + "the server rejected this client. Check the export's allowed-hosts list includes \ + this OpenPXE host's IP (or 0.0.0.0/0 for testing)." + .into(), + ) + } else if s.contains("no route to host") || s.contains("network is unreachable") { + Some("the server is not reachable on this network. Check the IP, subnet, and routes.".into()) + } else if s.contains("connection refused") { + Some( + "the NFS service isn't listening on this address/port. Verify NFS is running and \ + that the export path is correct (e.g. UniFi UNAS Pro exports under \ + /var/nfs/shared/, not the share name on its own)." + .into(), + ) + } else if s.contains("connection timed out") { + Some( + "no answer from the server within the connect timeout. Most likely a firewall is \ + dropping the connection, or the server isn't running NFS on this port." + .into(), + ) + } else if s.contains("no such file or directory") + || s.contains("mount: bad option") + || s.contains("does not exist") + { + Some( + "the export path doesn't exist on the server, or a mount option isn't recognized. \ + Double-check the export — many NAS appliances bury it under a service root like \ + /var/nfs/shared/." + .into(), + ) + } else if s.contains("rpc: program not registered") || s.contains("mount system call failed") { + Some( + "the server didn't respond on the expected RPC programs. NFSv4.1 needs nfsd on TCP \ + 2049; NFSv3 also needs portmap (111) and mountd. If the server only speaks one \ + version, switch the dropdown to match." + .into(), + ) + } else if s.contains("protocol not supported") || s.contains("invalid argument") { + Some( + "the server doesn't speak the requested NFS version. Try the other entry in the \ + Version dropdown." + .into(), + ) + } else { + None + } +} + +/// Normalize a server input: trim, strip a `http(s)://` prefix that the +/// operator may have pasted by mistake, and drop a trailing slash. Port +/// suffixes (`host:1234`) are preserved so the kernel sees them; the +/// explicit `port=` option still wins if the operator set one. +fn normalize_server(raw: &str) -> String { + let s = raw.trim(); + let s = s + .strip_prefix("http://") + .or_else(|| s.strip_prefix("https://")) + .or_else(|| s.strip_prefix("nfs://")) + .unwrap_or(s); + s.trim_end_matches('/').to_string() +} + fn mount_id(server: &str, export: &str) -> String { let raw = format!("{server}{export}"); slugify_str(&raw) @@ -500,6 +860,23 @@ async fn is_mountpoint(path: &Path) -> bool { mod tests { use super::*; + fn make_mount(version: NfsVersion, ro: bool, port: u16) -> NfsMount { + NfsMount { + id: "x".into(), + server: "s".into(), + export: "/e".into(), + version, + read_only: ro, + port, + local_path: PathBuf::from("/tmp/x"), + mounted: false, + last_error: None, + last_hint: None, + last_attempt: None, + iso_count: 0, + } + } + #[test] fn version_arg() { assert_eq!(NfsVersion::V3.vers_arg(), "vers=3"); @@ -507,44 +884,39 @@ mod tests { } #[test] - fn mount_options_v3_includes_nolock() { - let m = NfsMount { - id: "x".into(), - server: "s".into(), - export: "/e".into(), - version: NfsVersion::V3, - read_only: true, - local_path: PathBuf::from("/tmp/x"), - mounted: false, - last_error: None, - last_attempt: None, - iso_count: 0, - }; - let opts = mount_options(&m); - assert!(opts.contains("vers=3")); - assert!(opts.contains("ro")); - assert!(opts.contains("nolock")); - assert!(opts.contains("soft")); + fn mount_options_v3_includes_nolock_and_tcp() { + let m = make_mount(NfsVersion::V3, true, DEFAULT_NFS_PORT); + let opts = mount_options(&m, false); + assert!(opts.contains("vers=3"), "got: {opts}"); + assert!(opts.contains("ro"), "got: {opts}"); + assert!(opts.contains("nolock"), "got: {opts}"); + assert!(opts.contains("proto=tcp"), "got: {opts}"); + assert!(opts.contains("soft"), "got: {opts}"); + assert!(!opts.contains("port="), "default port shouldn't appear: {opts}"); } #[test] - fn mount_options_v41_no_nolock() { - let m = NfsMount { - id: "x".into(), - server: "s".into(), - export: "/e".into(), - version: NfsVersion::V41, - read_only: false, - local_path: PathBuf::from("/tmp/x"), - mounted: false, - last_error: None, - last_attempt: None, - iso_count: 0, - }; - let opts = mount_options(&m); - assert!(opts.contains("vers=4.1")); - assert!(opts.contains("rw")); - assert!(!opts.contains("nolock")); + fn mount_options_v41_has_tcp_no_nolock() { + let m = make_mount(NfsVersion::V41, false, DEFAULT_NFS_PORT); + let opts = mount_options(&m, false); + assert!(opts.contains("vers=4.1"), "got: {opts}"); + assert!(opts.contains("rw"), "got: {opts}"); + assert!(opts.contains("proto=tcp"), "got: {opts}"); + assert!(!opts.contains("nolock"), "got: {opts}"); + } + + #[test] + fn mount_options_minimal_drops_everything_except_vers_and_mode() { + let m = make_mount(NfsVersion::V3, true, DEFAULT_NFS_PORT); + let opts = mount_options(&m, true); + assert_eq!(opts, "vers=3,ro"); + } + + #[test] + fn mount_options_non_default_port_appears() { + let m = make_mount(NfsVersion::V41, true, 2050); + let opts = mount_options(&m, false); + assert!(opts.contains("port=2050"), "got: {opts}"); } #[test] @@ -555,4 +927,58 @@ mod tests { assert!(!a.contains('/')); assert!(!a.contains('.')); } + + #[test] + fn normalize_server_strips_url_schemes_and_slashes() { + assert_eq!(normalize_server(" 10.0.0.5 "), "10.0.0.5"); + assert_eq!(normalize_server("http://10.0.0.5/"), "10.0.0.5"); + assert_eq!(normalize_server("https://nas.lan//"), "nas.lan"); + assert_eq!(normalize_server("nfs://192.168.1.51"), "192.168.1.51"); + assert_eq!(normalize_server("nas.lan:2049"), "nas.lan:2049"); + } + + #[test] + fn hint_for_fstab_options_calls_out_cap_sys_admin() { + let h = hint_for("mount.nfs: failed to apply fstab options").unwrap(); + assert!( + h.contains("CAP_SYS_ADMIN"), + "expected CAP_SYS_ADMIN guidance, got: {h}" + ); + } + + #[test] + fn hint_for_access_denied_points_at_exports_table() { + let h = hint_for("mount.nfs: access denied by server while mounting").unwrap(); + assert!( + h.to_lowercase().contains("allowed-hosts") || h.to_lowercase().contains("export"), + "expected exports hint, got: {h}" + ); + } + + #[test] + fn hint_for_connection_refused_mentions_export_path() { + let h = hint_for("mount.nfs: Connection refused").unwrap(); + assert!( + h.to_lowercase().contains("export"), + "expected export-path hint, got: {h}" + ); + } + + #[test] + fn hint_for_unknown_message_is_none() { + assert!(hint_for("some completely unrelated text").is_none()); + } + + #[test] + fn looks_like_option_transform_failure_detects_both_variants() { + assert!(looks_like_option_transform_failure( + "mount.nfs: failed to apply fstab options" + )); + assert!(looks_like_option_transform_failure( + "mount.nfs: internal option parsing error" + )); + assert!(!looks_like_option_transform_failure( + "mount.nfs: access denied" + )); + } } diff --git a/crates/webui/src/app.js b/crates/webui/src/app.js index 1d013b4..3f0feea 100644 --- a/crates/webui/src/app.js +++ b/crates/webui/src/app.js @@ -623,19 +623,41 @@ const nfsRo = el('input', {type:'checkbox'}); nfsRo.checked = true; const addNfs = el('button', {onclick: async () => { if (!nfsServer.value || !nfsExport.value) { - nfsMsg.textContent = 'Server and export are required.'; nfsMsg.className='msg err'; return; + nfsMsg.replaceChildren(document.createTextNode('Server and export are required.')); + nfsMsg.className='msg err'; return; } - nfsMsg.textContent = 'Mounting…'; nfsMsg.className = 'msg'; + nfsMsg.replaceChildren(document.createTextNode('Mounting…')); + nfsMsg.className = 'msg'; const r = await postJSON('/api/nfs', { server: nfsServer.value, export: nfsExport.value, version: nfsVer.value, read_only: nfsRo.checked, }); if (r.ok) { - nfsMsg.textContent = 'Mounted.'; nfsMsg.className = 'msg ok'; + nfsMsg.replaceChildren(document.createTextNode('Mounted.')); + nfsMsg.className = 'msg ok'; render('storage'); } else { - const t = await r.text(); - nfsMsg.textContent = 'Mount failed: ' + t; nfsMsg.className = 'msg err'; + // v0.4.64: the API now returns a structured + // {error, stderr, hint} JSON body so we can render the + // mount failure and an actionable hint as two distinct lines + // instead of one long unreadable string. The dominant field + // failure mode — "mount.nfs: failed to apply fstab options" — + // becomes useful when paired with its CAP_SYS_ADMIN hint. + let body = null; + let raw = null; + try { body = await r.clone().json(); } + catch (_) { raw = await r.text().catch(()=> 'mount failed'); } + const msg = body && body.error ? body.error : (raw || 'mount failed'); + const hint = body && body.hint; + const parts = [el('div', {}, [ + el('strong', {}, 'Mount failed: '), + document.createTextNode(msg), + ])]; + if (hint) { + parts.push(el('div', {style:'margin-top:6px;opacity:.78;font-size:12px'}, hint)); + } + nfsMsg.replaceChildren(...parts); + nfsMsg.className = 'msg err'; } }}, 'Mount share'); @@ -648,6 +670,8 @@ (m.read_only ? 'read-only' : 'read-write') + ' · ' + (m.mounted ? m.iso_count + ' isos' : 'not mounted')), m.last_error ? el('div', {class:'err'}, '⚠ ' + m.last_error) : null, + // v0.4.64: actionable hint paired with the raw error. + m.last_hint ? el('div', {style:'margin-top:4px;opacity:.78;font-size:12px'}, m.last_hint) : null, ]), el('button', {class:'ghost', onclick: async () => { const r = await postJSON('/api/nfs/' + encodeURIComponent(m.id) + '/scan', {});