From 3e2252e4bb504b84e6c91a34707e60f252a1a8d9 Mon Sep 17 00:00:00 2001 From: 0xdx2 Date: Sun, 21 Dec 2025 17:54:23 +0800 Subject: [PATCH] =?UTF-8?q?fix(config):Update=20argument=20parsing=20for?= =?UTF-8?q?=20volumes=20and=20server=5Fdomains=20to=20support=20del?= =?UTF-8?q?=E2=80=A6=20(#1209)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: houseme Co-authored-by: houseme Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- Cargo.lock | 1 + rustfs/Cargo.toml | 1 + rustfs/src/config/config_test.rs | 457 +++++++++++++++++++++++++++++++ rustfs/src/config/mod.rs | 15 +- 4 files changed, 472 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4f7d153b..3b2d43a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7081,6 +7081,7 @@ dependencies = [ "serde", "serde_json", "serde_urlencoded", + "serial_test", "shadow-rs", "socket2 0.6.1", "subtle", diff --git a/rustfs/Cargo.toml b/rustfs/Cargo.toml index e4c685eb..e54a52fd 100644 --- a/rustfs/Cargo.toml +++ b/rustfs/Cargo.toml @@ -144,6 +144,7 @@ pprof = { workspace = true } [dev-dependencies] uuid = { workspace = true, features = ["v4"] } +serial_test = { workspace = true } [build-dependencies] http.workspace = true diff --git a/rustfs/src/config/config_test.rs b/rustfs/src/config/config_test.rs index 1f875fae..4e449b04 100644 --- a/rustfs/src/config/config_test.rs +++ b/rustfs/src/config/config_test.rs @@ -13,9 +13,48 @@ // limitations under the License. #[cfg(test)] +#[allow(unsafe_op_in_unsafe_fn)] mod tests { use crate::config::Opt; use clap::Parser; + use rustfs_ecstore::disks_layout::DisksLayout; + use serial_test::serial; + use std::env; + + /// Helper function to run test with environment variable set. + /// Automatically cleans up the environment variable after the test. + /// + /// # Safety + /// This function uses unsafe env::set_var and env::remove_var. + /// Tests using this helper must be marked with #[serial] to avoid race conditions. + #[allow(unsafe_code)] + fn with_env_var(key: &str, value: &str, test_fn: F) + where + F: FnOnce(), + { + unsafe { + env::set_var(key, value); + } + // Ensure cleanup happens even if test panics + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(test_fn)); + unsafe { + env::remove_var(key); + } + // Re-panic if the test failed + if let Err(e) = result { + std::panic::resume_unwind(e); + } + } + + /// Helper to parse volumes and verify the layout. + fn verify_layout(volumes: &[T], verify_fn: F) + where + T: AsRef, + F: FnOnce(&DisksLayout), + { + let layout = DisksLayout::from_volumes(volumes).expect("Failed to parse volumes"); + verify_fn(&layout); + } #[test] fn test_default_console_configuration() { @@ -66,4 +105,422 @@ mod tests { assert_eq!(endpoint_port, 9000); assert_eq!(console_port, 9001); } + + #[test] + fn test_volumes_and_disk_layout_parsing() { + use rustfs_ecstore::disks_layout::DisksLayout; + + // Test case 1: Single volume path + let args = vec!["rustfs", "/data/vol1"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 1); + assert_eq!(opt.volumes[0], "/data/vol1"); + + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse single volume"); + assert!(!layout.is_empty_layout()); + assert!(layout.is_single_drive_layout()); + assert_eq!(layout.get_single_drive_layout(), "/data/vol1"); + + // Test case 2: Multiple volume paths (space-separated via env) + let args = vec!["rustfs", "/data/vol1", "/data/vol2", "/data/vol3", "/data/vol4"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 4); + + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse multiple volumes"); + assert!(!layout.is_empty_layout()); + assert!(!layout.is_single_drive_layout()); + assert_eq!(layout.get_set_count(0), 1); + assert_eq!(layout.get_drives_per_set(0), 4); + + // Test case 3: Ellipses pattern - simple range + let args = vec!["rustfs", "/data/vol{1...4}"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 1); + assert_eq!(opt.volumes[0], "/data/vol{1...4}"); + + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse ellipses pattern"); + assert!(!layout.is_empty_layout()); + assert_eq!(layout.get_set_count(0), 1); + assert_eq!(layout.get_drives_per_set(0), 4); + + // Test case 4: Ellipses pattern - larger range that creates multiple sets + let args = vec!["rustfs", "/data/vol{1...16}"]; + let opt = Opt::parse_from(args); + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse ellipses with multiple sets"); + assert!(!layout.is_empty_layout()); + assert_eq!(layout.get_drives_per_set(0), 16); + + // Test case 5: Distributed setup pattern + let args = vec!["rustfs", "http://server{1...4}/data/vol{1...4}"]; + let opt = Opt::parse_from(args); + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse distributed pattern"); + assert!(!layout.is_empty_layout()); + assert_eq!(layout.get_drives_per_set(0), 16); + + // Test case 6: Multiple pools (legacy: false) + let args = vec!["rustfs", "http://server1/data{1...4}", "http://server2/data{1...4}"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 2); + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse multiple pools"); + assert!(!layout.legacy); + assert_eq!(layout.pools.len(), 2); + + // Test case 7: Minimum valid drives for erasure coding (2 drives minimum) + let args = vec!["rustfs", "/data/vol1", "/data/vol2"]; + let opt = Opt::parse_from(args); + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Should succeed with 2 drives"); + assert_eq!(layout.get_drives_per_set(0), 2); + + // Test case 8: Invalid - single drive not enough for erasure coding + let args = vec!["rustfs", "/data/vol1"]; + let opt = Opt::parse_from(args); + // Single drive is special case and should succeed for single drive layout + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Single drive should work"); + assert!(layout.is_single_drive_layout()); + + // Test case 9: Command line with both address and volumes + let args = vec![ + "rustfs", + "/data/vol{1...8}", + "--address", + ":9000", + "--console-address", + ":9001", + ]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 1); + assert_eq!(opt.address, ":9000"); + assert_eq!(opt.console_address, ":9001"); + + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse with address args"); + assert!(!layout.is_empty_layout()); + assert_eq!(layout.get_drives_per_set(0), 8); + + // Test case 10: Multiple ellipses in single argument - nested pattern + let args = vec!["rustfs", "/data{0...3}/vol{0...4}"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 1); + assert_eq!(opt.volumes[0], "/data{0...3}/vol{0...4}"); + + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse nested ellipses pattern"); + assert!(!layout.is_empty_layout()); + // 4 data dirs * 5 vols = 20 drives + let total_drives = layout.get_set_count(0) * layout.get_drives_per_set(0); + assert_eq!(total_drives, 20, "Expected 20 drives from /data{{0...3}}/vol{{0...4}}"); + + // Test case 11: Multiple pools with nested ellipses patterns + let args = vec!["rustfs", "/data{0...3}/vol{0...4}", "/data{4...7}/vol{0...4}"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 2); + + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse multiple pools with nested patterns"); + assert!(!layout.legacy); + assert_eq!(layout.pools.len(), 2); + + // Each pool should have 20 drives (4 * 5) + let pool0_drives = layout.get_set_count(0) * layout.get_drives_per_set(0); + let pool1_drives = layout.get_set_count(1) * layout.get_drives_per_set(1); + assert_eq!(pool0_drives, 20, "Pool 0 should have 20 drives"); + assert_eq!(pool1_drives, 20, "Pool 1 should have 20 drives"); + + // Test case 11: Complex distributed pattern with multiple ellipses + let args = vec!["rustfs", "http://server{1...2}.local/disk{1...8}"]; + let opt = Opt::parse_from(args); + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse distributed nested pattern"); + assert!(!layout.is_empty_layout()); + // 2 servers * 8 disks = 16 drives + let total_drives = layout.get_set_count(0) * layout.get_drives_per_set(0); + assert_eq!(total_drives, 16, "Expected 16 drives from server{{1...2}}/disk{{1...8}}"); + + // Test case 12: Zero-padded patterns + let args = vec!["rustfs", "/data/vol{01...16}"]; + let opt = Opt::parse_from(args); + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse zero-padded pattern"); + assert!(!layout.is_empty_layout()); + assert_eq!(layout.get_drives_per_set(0), 16); + } + + /// Test environment variable parsing for volumes. + /// Uses #[serial] to avoid concurrent env var modifications. + #[test] + #[serial] + #[allow(unsafe_code)] + fn test_rustfs_volumes_env_variable() { + // Test case 1: Single volume via environment variable + with_env_var("RUSTFS_VOLUMES", "/data/vol1", || { + let args = vec!["rustfs"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 1); + assert_eq!(opt.volumes[0], "/data/vol1"); + + let layout = DisksLayout::from_volumes(&opt.volumes).expect("Failed to parse single volume from env"); + assert!(layout.is_single_drive_layout()); + }); + + // Test case 2: Multiple volumes via environment variable (space-separated) + with_env_var("RUSTFS_VOLUMES", "/data/vol1 /data/vol2 /data/vol3 /data/vol4", || { + let args = vec!["rustfs"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 4); + assert_eq!(opt.volumes[0], "/data/vol1"); + assert_eq!(opt.volumes[1], "/data/vol2"); + assert_eq!(opt.volumes[2], "/data/vol3"); + assert_eq!(opt.volumes[3], "/data/vol4"); + + verify_layout(&opt.volumes, |layout| { + assert!(!layout.is_single_drive_layout()); + assert_eq!(layout.get_drives_per_set(0), 4); + }); + }); + + // Test case 3: Ellipses pattern via environment variable + with_env_var("RUSTFS_VOLUMES", "/data/vol{1...4}", || { + let args = vec!["rustfs"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 1); + assert_eq!(opt.volumes[0], "/data/vol{1...4}"); + + verify_layout(&opt.volumes, |layout| { + assert_eq!(layout.get_drives_per_set(0), 4); + }); + }); + + // Test case 4: Larger range with ellipses + with_env_var("RUSTFS_VOLUMES", "/data/vol{1...16}", || { + let args = vec!["rustfs"]; + let opt = Opt::parse_from(args); + verify_layout(&opt.volumes, |layout| { + assert_eq!(layout.get_drives_per_set(0), 16); + }); + }); + + // Test case 5: Distributed setup pattern + with_env_var("RUSTFS_VOLUMES", "http://server{1...4}/data/vol{1...4}", || { + let args = vec!["rustfs"]; + let opt = Opt::parse_from(args); + verify_layout(&opt.volumes, |layout| { + assert_eq!(layout.get_drives_per_set(0), 16); + }); + }); + + // Test case 6: Multiple pools via environment variable (space-separated) + with_env_var("RUSTFS_VOLUMES", "http://server1/data{1...4} http://server2/data{1...4}", || { + let args = vec!["rustfs"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 2); + verify_layout(&opt.volumes, |layout| { + assert!(!layout.legacy); + assert_eq!(layout.pools.len(), 2); + }); + }); + + // Test case 7: Nested ellipses pattern + with_env_var("RUSTFS_VOLUMES", "/data{0...3}/vol{0...4}", || { + let args = vec!["rustfs"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 1); + assert_eq!(opt.volumes[0], "/data{0...3}/vol{0...4}"); + + verify_layout(&opt.volumes, |layout| { + let total_drives = layout.get_set_count(0) * layout.get_drives_per_set(0); + assert_eq!(total_drives, 20, "Expected 20 drives from /data{{0...3}}/vol{{0...4}}"); + }); + }); + + // Test case 8: Multiple pools with nested ellipses + with_env_var("RUSTFS_VOLUMES", "/data{0...3}/vol{0...4} /data{4...7}/vol{0...4}", || { + let args = vec!["rustfs"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 2); + + verify_layout(&opt.volumes, |layout| { + assert_eq!(layout.pools.len(), 2); + let pool0_drives = layout.get_set_count(0) * layout.get_drives_per_set(0); + let pool1_drives = layout.get_set_count(1) * layout.get_drives_per_set(1); + assert_eq!(pool0_drives, 20, "Pool 0 should have 20 drives"); + assert_eq!(pool1_drives, 20, "Pool 1 should have 20 drives"); + }); + }); + + // Test case 9: Complex distributed pattern with multiple ellipses + with_env_var("RUSTFS_VOLUMES", "http://server{1...2}.local/disk{1...8}", || { + let args = vec!["rustfs"]; + let opt = Opt::parse_from(args); + verify_layout(&opt.volumes, |layout| { + let total_drives = layout.get_set_count(0) * layout.get_drives_per_set(0); + assert_eq!(total_drives, 16, "Expected 16 drives from server{{1...2}}/disk{{1...8}}"); + }); + }); + + // Test case 10: Zero-padded patterns + with_env_var("RUSTFS_VOLUMES", "/data/vol{01...16}", || { + let args = vec!["rustfs"]; + let opt = Opt::parse_from(args); + verify_layout(&opt.volumes, |layout| { + assert_eq!(layout.get_drives_per_set(0), 16); + }); + }); + + // Test case 11: Environment variable with additional CLI options + with_env_var("RUSTFS_VOLUMES", "/data/vol{1...8}", || { + let args = vec!["rustfs", "--address", ":9000", "--console-address", ":9001"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 1); + assert_eq!(opt.address, ":9000"); + assert_eq!(opt.console_address, ":9001"); + + verify_layout(&opt.volumes, |layout| { + assert_eq!(layout.get_drives_per_set(0), 8); + }); + }); + + // Test case 12: Command line argument overrides environment variable + with_env_var("RUSTFS_VOLUMES", "/data/vol1", || { + let args = vec!["rustfs", "/override/vol1"]; + let opt = Opt::parse_from(args); + assert_eq!(opt.volumes.len(), 1); + // CLI argument should override environment variable + assert_eq!(opt.volumes[0], "/override/vol1"); + }); + } + + /// Test boundary cases for path parsing. + /// NOTE: Current implementation uses space as delimiter, + /// which means paths with spaces are NOT supported. + #[test] + #[serial] + #[allow(unsafe_code)] + fn test_volumes_boundary_cases() { + // Test case 1: Paths with spaces are not properly supported (known limitation) + // This test documents the current behavior - space-separated paths will be split + with_env_var("RUSTFS_VOLUMES", "/data/my disk/vol1", || { + let args = vec!["rustfs"]; + let opt = Opt::try_parse_from(args).expect("Failed to parse with spaces in path"); + // Current behavior: space causes split into 2 volumes + assert_eq!(opt.volumes.len(), 2, "Paths with spaces are split (known limitation)"); + assert_eq!(opt.volumes[0], "/data/my"); + assert_eq!(opt.volumes[1], "disk/vol1"); + }); + + // Test case 2: Empty environment variable causes parsing failure + // because volumes is required and NonEmptyStringValueParser filters empty strings + with_env_var("RUSTFS_VOLUMES", "", || { + let args = vec!["rustfs"]; + let result = Opt::try_parse_from(args); + // Should fail because no volumes provided (empty string filtered out) + assert!(result.is_err(), "Empty RUSTFS_VOLUMES should fail parsing (required field)"); + }); + + // Test case 2b: Multiple consecutive spaces create empty strings during splitting + // This causes parsing to fail because volumes is required and empty strings are invalid + with_env_var("RUSTFS_VOLUMES", "/data/vol1 /data/vol2", || { + let args = vec!["rustfs"]; + let result = Opt::try_parse_from(args); + // Should fail because double space creates an empty element + assert!(result.is_err(), "Multiple consecutive spaces should cause parsing failure"); + }); + + // Test case 3: Very long path with ellipses (stress test) + // Note: Large drive counts may be automatically split into multiple sets + let long_path = format!("/very/long/path/structure/with/many/directories/vol{{1...{}}}", 100); + with_env_var("RUSTFS_VOLUMES", &long_path, || { + let args = vec!["rustfs"]; + let opt = Opt::try_parse_from(args).expect("Failed to parse with long ellipses path"); + verify_layout(&opt.volumes, |layout| { + // Total drives should be 100, but may be distributed across sets + let total_drives = layout.get_set_count(0) * layout.get_drives_per_set(0); + assert_eq!(total_drives, 100, "Total drives should be 100"); + }); + }); + } + + /// Test error handling for invalid ellipses patterns. + #[test] + fn test_invalid_ellipses_patterns() { + // Test case 1: Invalid ellipses format (letters instead of numbers) + let args = vec!["rustfs", "/data/vol{a...z}"]; + let opt = Opt::parse_from(args); + let result = DisksLayout::from_volumes(&opt.volumes); + assert!(result.is_err(), "Invalid ellipses pattern with letters should fail"); + + // Test case 2: Reversed range (larger to smaller) + let args = vec!["rustfs", "/data/vol{10...1}"]; + let opt = Opt::parse_from(args); + let result = DisksLayout::from_volumes(&opt.volumes); + // Depending on implementation, this may succeed with 0 drives or fail + // Document actual behavior + if let Ok(layout) = result { + assert!( + layout.is_empty_layout() || layout.get_drives_per_set(0) == 0, + "Reversed range should result in empty layout" + ); + } + } + + #[test] + fn test_server_domains_parsing() { + // Test case 1: server domains without ports + let args = vec![ + "rustfs", + "/data/vol1", + "--server-domains", + "example.com,api.example.com,cdn.example.com", + ]; + let opt = Opt::parse_from(args); + + assert_eq!(opt.server_domains.len(), 3); + assert_eq!(opt.server_domains[0], "example.com"); + assert_eq!(opt.server_domains[1], "api.example.com"); + assert_eq!(opt.server_domains[2], "cdn.example.com"); + + // Test case 2: server domains with ports + let args = vec![ + "rustfs", + "/data/vol1", + "--server-domains", + "example.com:9000,api.example.com:8080,cdn.example.com:443", + ]; + let opt = Opt::parse_from(args); + + assert_eq!(opt.server_domains.len(), 3); + assert_eq!(opt.server_domains[0], "example.com:9000"); + assert_eq!(opt.server_domains[1], "api.example.com:8080"); + assert_eq!(opt.server_domains[2], "cdn.example.com:443"); + + // Test case 3: mixed server domains (with and without ports) + let args = vec![ + "rustfs", + "/data/vol1", + "--server-domains", + "example.com,api.example.com:9000,cdn.example.com,storage.example.com:8443", + ]; + let opt = Opt::parse_from(args); + + assert_eq!(opt.server_domains.len(), 4); + assert_eq!(opt.server_domains[0], "example.com"); + assert_eq!(opt.server_domains[1], "api.example.com:9000"); + assert_eq!(opt.server_domains[2], "cdn.example.com"); + assert_eq!(opt.server_domains[3], "storage.example.com:8443"); + + // Test case 4: single domain with port + let args = vec!["rustfs", "/data/vol1", "--server-domains", "example.com:9000"]; + let opt = Opt::parse_from(args); + + assert_eq!(opt.server_domains.len(), 1); + assert_eq!(opt.server_domains[0], "example.com:9000"); + + // Test case 5: localhost with different ports + let args = vec![ + "rustfs", + "/data/vol1", + "--server-domains", + "localhost:9000,127.0.0.1:9000,localhost", + ]; + let opt = Opt::parse_from(args); + + assert_eq!(opt.server_domains.len(), 3); + assert_eq!(opt.server_domains[0], "localhost:9000"); + assert_eq!(opt.server_domains[1], "127.0.0.1:9000"); + assert_eq!(opt.server_domains[2], "localhost"); + } } diff --git a/rustfs/src/config/mod.rs b/rustfs/src/config/mod.rs index 1e553d89..14923522 100644 --- a/rustfs/src/config/mod.rs +++ b/rustfs/src/config/mod.rs @@ -13,6 +13,7 @@ // limitations under the License. use clap::Parser; +use clap::builder::NonEmptyStringValueParser; use const_str::concat; use std::string::ToString; shadow_rs::shadow!(build); @@ -50,7 +51,12 @@ const LONG_VERSION: &str = concat!( #[command(version = SHORT_VERSION, long_version = LONG_VERSION)] pub struct Opt { /// DIR points to a directory on a filesystem. - #[arg(required = true, env = "RUSTFS_VOLUMES")] + #[arg( + required = true, + env = "RUSTFS_VOLUMES", + value_delimiter = ' ', + value_parser = NonEmptyStringValueParser::new() + )] pub volumes: Vec, /// bind to a specific ADDRESS:PORT, ADDRESS can be an IP or hostname @@ -58,7 +64,12 @@ pub struct Opt { pub address: String, /// Domain name used for virtual-hosted-style requests. - #[arg(long, env = "RUSTFS_SERVER_DOMAINS")] + #[arg( + long, + env = "RUSTFS_SERVER_DOMAINS", + value_delimiter = ',', + value_parser = NonEmptyStringValueParser::new() + )] pub server_domains: Vec, /// Access key used for authentication.