diff --git a/.github/workflows/windows-filesystem.yml b/.github/workflows/windows-filesystem.yml index a1707b331..3e64ffabf 100644 --- a/.github/workflows/windows-filesystem.yml +++ b/.github/workflows/windows-filesystem.yml @@ -19,6 +19,7 @@ on: branches: [ main ] paths: - "crates/ecstore/src/disk/**" + - "crates/ecstore/src/layout/**" - "crates/ecstore/src/store/init_format.rs" - "crates/ecstore/src/bucket/utils.rs" - "crates/ecstore/Cargo.toml" @@ -36,6 +37,7 @@ on: branches: [ main ] paths: - "crates/ecstore/src/disk/**" + - "crates/ecstore/src/layout/**" - "crates/ecstore/src/store/init_format.rs" - "crates/ecstore/src/bucket/utils.rs" - "crates/ecstore/Cargo.toml" @@ -99,6 +101,10 @@ jobs: shell: pwsh run: cargo test -p rustfs-ecstore --lib windows_ -- --nocapture + - name: Test endpoint path parsing + shell: pwsh + run: "cargo test -p rustfs-ecstore --lib layout::endpoint:: -- --nocapture" + - name: Test startup temporary-directory cleanup shell: pwsh run: cargo test -p rustfs-ecstore --lib cleanup_tmp_on_startup_ -- --nocapture diff --git a/crates/ecstore/src/disk/local.rs b/crates/ecstore/src/disk/local.rs index f50b52769..300d969cb 100644 --- a/crates/ecstore/src/disk/local.rs +++ b/crates/ecstore/src/disk/local.rs @@ -25300,6 +25300,22 @@ mod test { assert!(debug.contains("endpoint")); } + #[cfg(windows)] + #[tokio::test] + async fn windows_local_disk_opens_url_style_drive_endpoint() { + use tempfile::tempdir; + + let dir = tempdir().expect("temp dir should be created"); + let drive_path = dir.path().to_string_lossy().replace('\\', "/"); + let endpoint = + Endpoint::try_from(format!("http://127.0.0.1:9000/{drive_path}").as_str()).expect("URL endpoint should parse"); + assert_eq!(Path::new(&endpoint.get_file_path()), dir.path()); + + LocalDisk::new(&endpoint, false) + .await + .expect("a URL-style drive endpoint must open as a local disk"); + } + #[tokio::test] async fn std_backend_truncate_append_stream_and_full_read_restore_bytes() { use tempfile::tempdir; diff --git a/crates/ecstore/src/layout/endpoint.rs b/crates/ecstore/src/layout/endpoint.rs index 5e9ca0c58..f93e68808 100644 --- a/crates/ecstore/src/layout/endpoint.rs +++ b/crates/ecstore/src/layout/endpoint.rs @@ -101,23 +101,30 @@ impl TryFrom<&str> for Endpoint { let path = Path::new(&path).absolutize()?; #[cfg(windows)] - let path = if has_leading_slash_windows_drive(&path) { + let path = match strip_windows_drive_url_path(&path) { // Url::path() exposes file-like Windows paths as `/C:/...`. // Strip only that synthetic leading slash; plain URL paths // such as `/export1` must stay URL paths, not become // relative paths under the current drive. - Path::new(&path[1..]).absolutize()?.to_string_lossy().into_owned() - } else { - path + Some(drive_path) => Path::new(drive_path).absolutize()?.to_string_lossy().into_owned(), + None => path, }; #[cfg(windows)] let path = Path::new(&path); debug!("endpoint try_from: path={}", path.display()); - if path.parent().is_none() || path.as_os_str().is_empty() { + // A Windows drive root such as `C:\` has no parent, but it is + // accepted for parity with the path-style `C:\` endpoint. + #[cfg(windows)] + let is_root = path.parent().is_none() && !is_windows_drive_root(path); + #[cfg(not(windows))] + let is_root = path.parent().is_none(); + if is_root || path.as_os_str().is_empty() { return Err(Error::other("empty or root path is not supported in URL endpoint")); } + #[cfg(windows)] + reject_windows_unc_path(path)?; match path.to_str() { Some(v) => url.set_path(v), @@ -230,20 +237,63 @@ impl Endpoint { std::borrow::Cow::Borrowed(path) } }; + // Url::path() always starts with `/`, so every scheme stores a Windows + // drive path as `/C:/...`. Restore the drive path for `file` and + // `http(s)` endpoints alike; Windows would otherwise resolve `/C:/data` + // as an invalid `C:` component under the current drive. #[cfg(windows)] - if self.url.scheme() == "file" { - let stripped: &str = decoded.strip_prefix('/').unwrap_or(&decoded); - debug!("get_file_path windows: path={}", stripped); - return stripped.to_string(); + if let Some(drive_path) = strip_windows_drive_url_path(&decoded) { + debug!("get_file_path windows: path={}", drive_path); + return drive_path.to_string(); } decoded.into_owned() } } -#[cfg(windows)] -fn has_leading_slash_windows_drive(path: &str) -> bool { +/// Returns `C:/...` for a URL path shaped like `/C:/...`, otherwise `None`. +/// +/// The separator after the colon is required: `/C:foo` is a plain URL path, +/// and `/export1` must never be reinterpreted as a drive path. +#[cfg_attr(not(windows), allow(dead_code))] +fn strip_windows_drive_url_path(path: &str) -> Option<&str> { let bytes = path.as_bytes(); - bytes.len() >= 4 && bytes[0] == b'/' && bytes[1].is_ascii_alphabetic() && bytes[2] == b':' && bytes[3] == b'/' + if bytes.len() >= 4 && bytes[0] == b'/' && bytes[1].is_ascii_alphabetic() && bytes[2] == b':' && bytes[3] == b'/' { + Some(&path[1..]) + } else { + None + } +} + +#[cfg(windows)] +fn is_windows_drive_root(path: &Path) -> bool { + use std::path::{Component, Prefix}; + + let mut components = path.components(); + matches!( + components.next(), + Some(Component::Prefix(prefix)) if matches!(prefix.kind(), Prefix::Disk(_) | Prefix::VerbatimDisk(_)) + ) && matches!(components.next(), Some(Component::RootDir)) + && components.next().is_none() +} + +/// UNC shares are rejected instead of supported: `Url::from_file_path` turns +/// `\\nas\share\d1` into `file://nas/share/d1`, so the share server would be +/// read as the endpoint host and the share name dropped from the drive path, +/// and a network share does not provide the local-volume semantics (atomic +/// rename, fsync, volume identity) that erasure-coded drives rely on. +#[cfg(windows)] +fn reject_windows_unc_path(path: &Path) -> Result<()> { + use std::path::{Component, Prefix}; + + if let Some(Component::Prefix(prefix)) = path.components().next() + && matches!(prefix.kind(), Prefix::UNC(..) | Prefix::VerbatimUNC(..)) + { + return Err(Error::other( + "UNC network share paths are not supported as drive endpoints; use a local volume path such as \ +D:\\rustfs\\data1, or http://host:9000/D:/rustfs/data1 for URL endpoints", + )); + } + Ok(()) } /// parse a file path into a URL. @@ -261,6 +311,9 @@ fn url_parse_from_file_path(value: &str) -> Result { Err(err) => return Err(Error::other(format!("absolute path failed: {err}"))), }; + #[cfg(windows)] + reject_windows_unc_path(&file_path)?; + match Url::from_file_path(file_path) { Ok(url) => Ok(url), Err(_) => Err(Error::other("Convert a file path into an URL failed")), @@ -538,17 +591,110 @@ mod test { assert_eq!(url_endpoint.get_file_path(), "/path/to/data"); } + #[test] + fn strip_windows_drive_url_path_requires_drive_separator() { + assert_eq!(strip_windows_drive_url_path("/C:/data1"), Some("C:/data1")); + assert_eq!(strip_windows_drive_url_path("/d:/rustfs/data1"), Some("d:/rustfs/data1")); + assert_eq!(strip_windows_drive_url_path("/C:/"), Some("C:/")); + assert_eq!(strip_windows_drive_url_path("/C:foo"), None); + assert_eq!(strip_windows_drive_url_path("/C:"), None); + assert_eq!(strip_windows_drive_url_path("/export1"), None); + assert_eq!(strip_windows_drive_url_path("/1:/data"), None); + assert_eq!(strip_windows_drive_url_path("C:/data1"), None); + } + + #[cfg(not(windows))] + #[test] + fn url_endpoint_drive_shaped_path_stays_a_url_path_off_windows() { + let endpoint = Endpoint::try_from("http://host:9000/C:/data1").unwrap(); + assert_eq!(endpoint.get_file_path(), "/C:/data1"); + } + #[cfg(windows)] #[test] - fn test_windows_url_drive_path_requires_separator_after_colon() { - let drive_path_endpoint = Endpoint::try_from("http://host/C:/data").unwrap(); - assert_eq!(drive_path_endpoint.get_type(), EndpointType::Url); - assert!(has_leading_slash_windows_drive(Url::parse("http://host/C:/data").unwrap().path())); + fn windows_url_endpoint_file_path_restores_drive_path() { + for (arg, expected) in [ + ("http://host:9000/C:/data1", "C:/data1"), + ("https://host:9000/d:/rustfs/data4", "d:/rustfs/data4"), + ("http://host:9000/C:\\data1", "C:/data1"), + ("http://host:9000/C:/rustfs/../data1", "C:/data1"), + ("http://host:9000/C:/Program%20Files/rustfs", "C:/Program Files/rustfs"), + ] { + let endpoint = Endpoint::try_from(arg).unwrap_or_else(|err| panic!("{arg}: {err}")); + assert_eq!(endpoint.get_type(), EndpointType::Url, "{arg}"); + assert_eq!(endpoint.get_file_path(), expected, "{arg}"); + assert!(Path::new(&endpoint.get_file_path()).is_absolute(), "{arg}"); + } + } - let url_path_endpoint = Endpoint::try_from("http://host/C:foo").unwrap(); - assert_eq!(url_path_endpoint.get_type(), EndpointType::Url); - assert!(!has_leading_slash_windows_drive(Url::parse("http://host/C:foo").unwrap().path())); - assert_eq!(url_path_endpoint.get_file_path(), "/C:foo"); + #[cfg(windows)] + #[test] + fn windows_url_endpoint_keeps_url_identity_after_drive_path_parse() { + let endpoint = Endpoint::try_from("http://host:9000/C:/data1").unwrap(); + assert_eq!(endpoint.url.as_str(), "http://host:9000/C:/data1"); + assert_eq!(endpoint.to_string(), "http://host:9000/C:/data1"); + assert_eq!(endpoint.host_port(), "host:9000"); + } + + #[cfg(windows)] + #[test] + fn windows_url_endpoint_keeps_plain_url_paths() { + for (arg, expected) in [("http://host:9000/export1", "/export1"), ("http://host:9000/C:foo", "/C:foo")] { + let endpoint = Endpoint::try_from(arg).unwrap(); + assert_eq!(endpoint.get_type(), EndpointType::Url, "{arg}"); + assert_eq!(endpoint.get_file_path(), expected, "{arg}"); + } + } + + #[cfg(windows)] + #[test] + fn windows_url_and_path_endpoints_share_drive_file_path() { + let path_endpoint = Endpoint::try_from("C:\\rustfs\\data1").unwrap(); + let url_endpoint = Endpoint::try_from("http://host:9000/C:/rustfs/data1").unwrap(); + let verbatim_endpoint = Endpoint::try_from("\\\\?\\C:\\rustfs\\data1").unwrap(); + + assert_eq!(path_endpoint.get_type(), EndpointType::Path); + assert_eq!(path_endpoint.get_file_path(), "C:/rustfs/data1"); + assert_eq!(url_endpoint.get_file_path(), path_endpoint.get_file_path()); + assert_eq!(verbatim_endpoint.get_file_path(), path_endpoint.get_file_path()); + } + + #[cfg(windows)] + #[test] + fn windows_url_endpoint_resolves_existing_drive_directory() { + let dir = tempfile::tempdir().unwrap(); + let drive_path = dir.path().to_string_lossy().replace('\\', "/"); + let endpoint = Endpoint::try_from(format!("http://127.0.0.1:9000/{drive_path}").as_str()).unwrap(); + + let file_path = endpoint.get_file_path(); + assert_eq!(Path::new(&file_path), dir.path()); + rustfs_utils::canonicalize(&file_path) + .unwrap_or_else(|err| panic!("URL endpoint path '{file_path}' must resolve to the temp directory: {err}")); + } + + #[cfg(windows)] + #[test] + fn windows_drive_root_endpoints_are_accepted_in_both_forms() { + let path_endpoint = Endpoint::try_from("D:\\").unwrap(); + assert_eq!(path_endpoint.get_file_path(), "D:/"); + + let url_endpoint = Endpoint::try_from("http://host:9000/D:/").unwrap(); + assert_eq!(url_endpoint.get_type(), EndpointType::Url); + assert_eq!(url_endpoint.get_file_path(), "D:/"); + } + + #[cfg(windows)] + #[test] + fn windows_unc_endpoints_are_rejected() { + for arg in [ + "\\\\nas\\share\\d1", + "//nas/share/d1", + "\\\\?\\UNC\\nas\\share\\d1", + "http://host:9000//nas/share/d1", + ] { + let err = Endpoint::try_from(arg).expect_err(arg); + assert!(err.to_string().contains("UNC network share paths are not supported"), "{arg}: {err}"); + } } #[test] diff --git a/crates/ecstore/src/layout/endpoints.rs b/crates/ecstore/src/layout/endpoints.rs index d6f17d9b3..95abdc22a 100644 --- a/crates/ecstore/src/layout/endpoints.rs +++ b/crates/ecstore/src/layout/endpoints.rs @@ -1729,11 +1729,21 @@ fn validate_local_physical_disk_independence(pools: &[Endpoints]) -> Result<()> Err(Error::other(format!( "local erasure endpoints must use distinct physical disks; detected shared devices [{details}]. \ -validation diagnostics: [{diagnostics_summary}]. \ +validation diagnostics: [{diagnostics_summary}]. {SHARED_DEVICE_PLATFORM_HINT}\ Set {ENV_UNSAFE_BYPASS_DISK_CHECK}=true only for local testing or CI to bypass this safety check" ))) } +/// Windows reports the volume mount point (for example `C:\`) as the device +/// identity, so every directory on one drive letter or mounted volume counts +/// as the same physical disk. +#[cfg(windows)] +const SHARED_DEVICE_PLATFORM_HINT: &str = "On Windows the device identity is the volume mount point, so directories on the same \ +drive letter or mounted volume (for example C:\\data1 and C:\\data2) count as one disk; place each endpoint on its own \ +volume. "; +#[cfg(not(windows))] +const SHARED_DEVICE_PLATFORM_HINT: &str = ""; + fn validate_local_cross_device_mounts(local_paths: &[String]) -> Result<()> { rustfs_utils::os::check_cross_device_mounts(local_paths) .map_err(|err| Error::other(format!("local endpoint cross-device mount validation failed: {err}"))) @@ -3581,6 +3591,38 @@ mod test { } } + #[cfg(windows)] + #[serial] + #[tokio::test] + async fn windows_url_endpoints_on_one_volume_resolve_and_report_shared_volume() { + async_with_vars([(ENV_UNSAFE_BYPASS_DISK_CHECK, None::<&str>), (ENV_MINIO_CI, None::<&str>)], async { + let dir = tempfile::tempdir().unwrap(); + let mut endpoints = Vec::new(); + for name in ["disk1", "disk2"] { + let disk = dir.path().join(name); + std::fs::create_dir_all(&disk).unwrap(); + let drive_path = disk.to_string_lossy().replace('\\', "/"); + let mut endpoint = Endpoint::try_from(format!("http://127.0.0.1:9000/{drive_path}").as_str()).unwrap(); + endpoint.is_local = true; + endpoints.push(endpoint); + } + + let err = validate_local_physical_disk_independence(&[Endpoints::from(endpoints)]).unwrap_err(); + + // Before the drive-path fix, `/C:/...` failed canonicalization here + // with "failed to resolve local endpoint path". + let err_text = err.to_string(); + assert!( + !err_text.contains("failed to resolve local endpoint path"), + "unexpected error: {err_text}" + ); + assert!(err_text.contains("distinct physical disks"), "unexpected error: {err_text}"); + assert!(err_text.contains("same drive letter or mounted volume"), "unexpected error: {err_text}"); + assert!(err_text.contains(ENV_UNSAFE_BYPASS_DISK_CHECK), "unexpected error: {err_text}"); + }) + .await; + } + #[cfg(target_os = "linux")] #[serial] #[tokio::test] diff --git a/docs/operations/cluster-lifecycle-operations.md b/docs/operations/cluster-lifecycle-operations.md index f07056026..87f7c9ab9 100644 --- a/docs/operations/cluster-lifecycle-operations.md +++ b/docs/operations/cluster-lifecycle-operations.md @@ -44,6 +44,14 @@ Rules enforced by `DisksLayout::from_volumes`: with more than one argument every Startup order (`init_startup_storage_foundation` in `rustfs/src/startup_storage.rs`): parse endpoints, validate storage classes against every pool (`ECStore::validate_startup_storage_class`), enforce the unsupported-filesystem policy, initialize local disks, initialize lock clients, then load or create the format quorum and build the store. A storage-class error stops startup before any disk is touched. +### 2.1 Windows endpoints + +Multi-node and multi-pool layouts need URL endpoints, so on Windows the drive path goes after the port with forward slashes: `http://node{1...4}:9000/D:/rustfs/data{1...4}`. `Endpoint::get_file_path` (`crates/ecstore/src/layout/endpoint.rs`) turns the URL path `/D:/rustfs/data1` back into `D:/rustfs/data1` for every scheme; a URL path without a drive, such as `/export1`, stays a URL path. A drive root (`D:\` or `http://node1:9000/D:/`) is accepted in both forms. UNC shares (`\\nas\share\d1`, `//nas/share/d1`, `\\?\UNC\...`) are rejected at parse time: they are not local volumes, and parsing would read the share server as the endpoint host. + +Endpoint parsing then requires distinct physical disks (`validate_local_physical_disk_independence` in `crates/ecstore/src/layout/endpoints.rs`). On Windows the device identity is the volume mount point from `GetVolumePathNameW` (`crates/utils/src/os/windows.rs`), so `D:/rustfs/data1` through `D:/rustfs/data4` are one disk and startup fails with `must use distinct physical disks`. Give each endpoint its own volume (a drive letter or a volume mounted into an NTFS folder). For a single-machine test only, `RUSTFS_UNSAFE_BYPASS_DISK_CHECK=true` skips the check; it gives no protection against losing that volume. + +A single local path (`rustfs D:\rustfs\data`) is a single-node single-drive deployment and can never gain a pool (ยง7). If the machine may expand later, start with URL endpoints and at least two drives per pool. + ## 3. Startup configuration | Variable | Effect | Boundary |