From 61b310026000175861251a6c40b289bfeb02723f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Thu, 1 Jan 2026 23:26:32 +0800 Subject: [PATCH] fix: s3 list object versions next marker (#1328) --- Cargo.lock | 10 +- crates/ecstore/src/store_list_objects.rs | 87 ++++++++++++++- rustfs/src/auth.rs | 19 +++- rustfs/src/storage/ecfs.rs | 86 ++++++++++++++- scripts/s3-tests/cleanup.sh | 128 +++++++++++++++++++++++ scripts/s3-tests/run.sh | 7 +- 6 files changed, 324 insertions(+), 13 deletions(-) create mode 100755 scripts/s3-tests/cleanup.sh diff --git a/Cargo.lock b/Cargo.lock index f5bd2365..3ce8d55d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5820,7 +5820,7 @@ dependencies = [ "libc", "log", "openssl", - "openssl-probe", + "openssl-probe 0.1.6", "openssl-sys", "schannel", "security-framework 2.11.1", @@ -6251,6 +6251,12 @@ dependencies = [ "syn 2.0.111", ] +[[package]] +name = "openssl-probe" +version = "0.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d05e27ee213611ffe7d6348b942e8f942b37114c00cc03cec254295a4a17852e" + [[package]] name = "openssl-probe" version = "0.2.0" @@ -8835,7 +8841,7 @@ version = "0.8.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "612460d5f7bea540c490b2b6395d8e34a953e52b491accd6c86c8164c5932a63" dependencies = [ - "openssl-probe", + "openssl-probe 0.2.0", "rustls-pki-types", "schannel", "security-framework 3.5.1", diff --git a/crates/ecstore/src/store_list_objects.rs b/crates/ecstore/src/store_list_objects.rs index 676d43cf..214718ab 100644 --- a/crates/ecstore/src/store_list_objects.rs +++ b/crates/ecstore/src/store_list_objects.rs @@ -387,7 +387,12 @@ impl ECStore { } let version_marker = if let Some(marker) = version_marker { - Some(Uuid::parse_str(&marker)?) + // "null" is used for non-versioned objects in AWS S3 API + if marker == "null" { + None + } else { + Some(Uuid::parse_str(&marker)?) + } } else { None }; @@ -445,7 +450,13 @@ impl ECStore { if is_truncated { get_objects .last() - .map(|last| (Some(last.name.clone()), last.version_id.map(|v| v.to_string()))) + .map(|last| { + ( + Some(last.name.clone()), + // AWS S3 API returns "null" for non-versioned objects + Some(last.version_id.map(|v| v.to_string()).unwrap_or_else(|| "null".to_string())), + ) + }) .unwrap_or_default() } else { (None, None) @@ -1374,6 +1385,78 @@ fn calc_common_counter(infos: &[DiskInfo], read_quorum: usize) -> u64 { #[cfg(test)] mod test { + use uuid::Uuid; + + /// Test that "null" version marker is handled correctly + /// AWS S3 API uses "null" string to represent non-versioned objects + #[test] + fn test_null_version_marker_handling() { + // "null" should be treated as None (non-versioned) + let version_marker = "null"; + let parsed: Option = if version_marker == "null" { + None + } else { + Uuid::parse_str(version_marker).ok() + }; + assert!(parsed.is_none(), "\"null\" should be parsed as None"); + + // Valid UUID should be parsed correctly + let valid_uuid = "550e8400-e29b-41d4-a716-446655440000"; + let parsed: Option = if valid_uuid == "null" { + None + } else { + Uuid::parse_str(valid_uuid).ok() + }; + assert!(parsed.is_some(), "Valid UUID should be parsed correctly"); + assert_eq!(parsed.unwrap().to_string(), "550e8400-e29b-41d4-a716-446655440000"); + } + + /// Test that next_version_idmarker returns "null" for non-versioned objects + #[test] + fn test_next_version_idmarker_null_string() { + // When version_id is None, next_version_idmarker should be "null" + let version_id: Option = None; + let next_version_idmarker = version_id.map(|v| v.to_string()).unwrap_or_else(|| "null".to_string()); + assert_eq!(next_version_idmarker, "null"); + + // When version_id is Some, next_version_idmarker should be the UUID string + let version_id: Option = Some(Uuid::parse_str("550e8400-e29b-41d4-a716-446655440000").unwrap()); + let next_version_idmarker = version_id.map(|v| v.to_string()).unwrap_or_else(|| "null".to_string()); + assert_eq!(next_version_idmarker, "550e8400-e29b-41d4-a716-446655440000"); + } + + /// Test the round-trip: next_version_idmarker -> VersionIdMarker parameter -> parsing + #[test] + fn test_version_marker_round_trip() { + // Scenario 1: Non-versioned object + // Server returns "null" as NextVersionIdMarker + // Client sends "null" as VersionIdMarker + // Server parses "null" as None + let server_response = "null"; + let client_request = server_response; + let parsed: Option = if client_request == "null" { + None + } else { + Uuid::parse_str(client_request).ok() + }; + assert!(parsed.is_none()); + + // Scenario 2: Versioned object + // Server returns UUID as NextVersionIdMarker + // Client sends UUID as VersionIdMarker + // Server parses UUID correctly + let uuid_str = "550e8400-e29b-41d4-a716-446655440000"; + let server_response = uuid_str; + let client_request = server_response; + let parsed: Option = if client_request == "null" { + None + } else { + Uuid::parse_str(client_request).ok() + }; + assert!(parsed.is_some()); + assert_eq!(parsed.unwrap().to_string(), uuid_str); + } + // use std::sync::Arc; // use crate::cache_value::metacache_set::list_path_raw; diff --git a/rustfs/src/auth.rs b/rustfs/src/auth.rs index 01fc0570..c673f686 100644 --- a/rustfs/src/auth.rs +++ b/rustfs/src/auth.rs @@ -126,10 +126,21 @@ impl S3Auth for IAMAuth { } if let Ok(iam_store) = rustfs_iam::get() { - if let Some(id) = iam_store.get_user(access_key).await { - return Ok(SecretKey::from(id.credentials.secret_key.clone())); - } else { - tracing::warn!("get_user failed: no such user, access_key: {access_key}"); + // Use check_key instead of get_user to ensure user is loaded from disk if not in cache + // This is important for newly created users that may not be in cache yet. + // check_key will automatically attempt to load the user from disk if not found in cache. + match iam_store.check_key(access_key).await { + Ok((Some(id), _valid)) => { + // Return secret key for signature verification regardless of user status. + // Authorization will be checked separately in the authorization phase. + return Ok(SecretKey::from(id.credentials.secret_key.clone())); + } + Ok((None, _)) => { + tracing::warn!("get_secret_key failed: no such user, access_key: {access_key}"); + } + Err(e) => { + tracing::warn!("get_secret_key failed: check_key error, access_key: {access_key}, error: {e:?}"); + } } } else { tracing::warn!("get_secret_key failed: iam not initialized, access_key: {access_key}"); diff --git a/rustfs/src/storage/ecfs.rs b/rustfs/src/storage/ecfs.rs index a5e9dc3e..9f5d8640 100644 --- a/rustfs/src/storage/ecfs.rs +++ b/rustfs/src/storage/ecfs.rs @@ -3044,8 +3044,9 @@ impl S3 for FS { }) .collect::>(); - // Only set next_version_id_marker if it has a value, per AWS S3 API spec - // boto3 expects it to be a string or omitted, not None + // Only set next_key_marker and next_version_id_marker if they have values, per AWS S3 API spec + // boto3 expects them to be strings or omitted, not None or empty strings + let next_key_marker = object_infos.next_marker.filter(|v| !v.is_empty()); let next_version_id_marker = object_infos.next_version_idmarker.filter(|v| !v.is_empty()); let output = ListObjectVersionsOutput { @@ -3057,7 +3058,7 @@ impl S3 for FS { common_prefixes: Some(common_prefixes), versions: Some(objects), delete_markers: Some(delete_markers), - next_key_marker: object_infos.next_marker, + next_key_marker, next_version_id_marker, ..Default::default() }; @@ -6191,4 +6192,83 @@ mod tests { // // These are better suited for integration tests rather than unit tests. // The current tests focus on the testable parts without external dependencies. + + /// Test that next_key_marker and next_version_id_marker are filtered correctly + /// AWS S3 API requires these fields to be omitted when empty, not set to None or "" + #[test] + fn test_next_marker_filtering() { + // Test filter behavior for empty strings + let empty_string = Some(String::new()); + let filtered = empty_string.filter(|v| !v.is_empty()); + assert!(filtered.is_none(), "Empty string should be filtered to None"); + + // Test filter behavior for non-empty strings + let non_empty = Some("some-marker".to_string()); + let filtered = non_empty.filter(|v| !v.is_empty()); + assert!(filtered.is_some(), "Non-empty string should not be filtered"); + assert_eq!(filtered.unwrap(), "some-marker"); + + // Test filter behavior for None + let none_value: Option = None; + let filtered = none_value.filter(|v| !v.is_empty()); + assert!(filtered.is_none(), "None should remain None"); + } + + /// Test version_id handling for ListObjectVersions response + /// Per AWS S3 API spec: + /// - Versioned objects: version_id is a UUID string + /// - Non-versioned objects: version_id should be "null" string + #[test] + fn test_version_id_formatting() { + use uuid::Uuid; + + // Non-versioned object: version_id is None, should format as "null" + let version_id: Option = None; + let formatted = version_id.map(|v| v.to_string()).unwrap_or_else(|| "null".to_string()); + assert_eq!(formatted, "null"); + + // Versioned object: version_id is Some(UUID), should format as UUID string + let uuid = Uuid::parse_str("550e8400-e29b-41d4-a716-446655440000").unwrap(); + let version_id: Option = Some(uuid); + let formatted = version_id.map(|v| v.to_string()).unwrap_or_else(|| "null".to_string()); + assert_eq!(formatted, "550e8400-e29b-41d4-a716-446655440000"); + } + + /// Test that ListObjectVersionsOutput markers are correctly set + /// This verifies the fix for boto3 ParamValidationError + #[test] + fn test_list_object_versions_markers_handling() { + // Simulate the marker filtering logic from list_object_versions + + // Case 1: Both markers have values (truncated result with versioned object) + let next_marker = Some("object-key".to_string()); + let next_version_idmarker = Some("550e8400-e29b-41d4-a716-446655440000".to_string()); + + let filtered_key_marker = next_marker.filter(|v| !v.is_empty()); + let filtered_version_marker = next_version_idmarker.filter(|v| !v.is_empty()); + + assert!(filtered_key_marker.is_some()); + assert!(filtered_version_marker.is_some()); + + // Case 2: Markers are empty strings (non-truncated result) + let next_marker = Some(String::new()); + let next_version_idmarker = Some(String::new()); + + let filtered_key_marker = next_marker.filter(|v| !v.is_empty()); + let filtered_version_marker = next_version_idmarker.filter(|v| !v.is_empty()); + + assert!(filtered_key_marker.is_none(), "Empty key marker should be filtered to None"); + assert!(filtered_version_marker.is_none(), "Empty version marker should be filtered to None"); + + // Case 3: Truncated result with non-versioned object (version_id is "null") + let next_marker = Some("object-key".to_string()); + let next_version_idmarker = Some("null".to_string()); + + let filtered_key_marker = next_marker.filter(|v| !v.is_empty()); + let filtered_version_marker = next_version_idmarker.filter(|v| !v.is_empty()); + + assert!(filtered_key_marker.is_some()); + assert!(filtered_version_marker.is_some()); + assert_eq!(filtered_version_marker.unwrap(), "null"); + } } diff --git a/scripts/s3-tests/cleanup.sh b/scripts/s3-tests/cleanup.sh new file mode 100755 index 00000000..91a0c027 --- /dev/null +++ b/scripts/s3-tests/cleanup.sh @@ -0,0 +1,128 @@ +#!/bin/bash +# Copyright 2024 RustFS Team +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Cleanup script for port 9000 and test data directory +# This script kills any process using port 9000 and cleans the test data directory + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "${SCRIPT_DIR}/../.." && pwd)" +DATA_DIR="${PROJECT_ROOT}/target/test-data/rustfs-single" +PORT="${S3_PORT:-9000}" +HOST="${S3_HOST:-127.0.0.1}" + +log_info() { + echo "[INFO] $*" +} + +log_warn() { + echo "[WARN] $*" +} + +cleanup_port_9000() { + log_info "Checking for processes using port ${PORT}..." + + # Try to find process using port 9000 + PID="" + + # Try lsof first (works on macOS and Linux) + if command -v lsof >/dev/null 2>&1; then + PID=$(lsof -ti:${PORT} 2>/dev/null || true) + fi + + # Fallback: try using netstat or ss + if [ -z "$PID" ]; then + if command -v netstat >/dev/null 2>&1; then + PID=$(netstat -tuln 2>/dev/null | grep ":${PORT} " | awk '{print $7}' | cut -d'/' -f1 | head -1 || true) + elif command -v ss >/dev/null 2>&1; then + PID=$(ss -tuln 2>/dev/null | grep ":${PORT} " | awk '{print $6}' | cut -d',' -f2 | cut -d'=' -f2 | head -1 || true) + fi + fi + + # If we found a PID, kill it + if [ -n "$PID" ] && [ "$PID" != "-" ]; then + log_info "Found process ${PID} using port ${PORT}, killing it..." + kill -9 "$PID" 2>/dev/null || true + sleep 2 + + # Verify the process is gone + if kill -0 "$PID" 2>/dev/null; then + log_warn "Process ${PID} is still running, trying force kill..." + kill -9 "$PID" 2>/dev/null || true + sleep 1 + fi + else + log_info "No process found using port ${PORT}" + fi + + # Also try to kill any rustfs processes (more aggressive cleanup) + if pgrep -f "rustfs.*${PORT}" >/dev/null 2>&1; then + log_info "Killing any remaining rustfs processes using port ${PORT}..." + pkill -f "rustfs.*${PORT}" 2>/dev/null || true + sleep 1 + fi + + # Verify port is released + log_info "Verifying port ${PORT} is released..." + for i in {1..10}; do + if command -v nc >/dev/null 2>&1; then + if ! nc -z "${HOST}" "${PORT}" 2>/dev/null; then + log_info "Port ${PORT} is now available" + break + fi + elif timeout 1 bash -c "cat < /dev/null > /dev/tcp/${HOST}/${PORT}" 2>/dev/null; then + # Port is still in use + if [ $i -eq 10 ]; then + log_warn "Port ${PORT} may still be in use after cleanup attempts" + fi + else + log_info "Port ${PORT} is now available" + break + fi + sleep 1 + done +} + +cleanup_data_directory() { + log_info "Cleaning test data directory: ${DATA_DIR}" + + if [ -d "${DATA_DIR}" ]; then + # Remove all contents but keep the directory structure + find "${DATA_DIR}" -mindepth 1 -delete 2>/dev/null || true + + # Recreate subdirectories if they were deleted + for i in {0..3}; do + mkdir -p "${DATA_DIR}/rustfs${i}" + done + + log_info "Test data directory cleaned" + else + log_info "Test data directory does not exist, creating it..." + mkdir -p "${DATA_DIR}" + for i in {0..3}; do + mkdir -p "${DATA_DIR}/rustfs${i}" + done + fi +} + +main() { + log_info "Starting cleanup..." + cleanup_port_9000 + cleanup_data_directory + log_info "Cleanup completed" +} + +main "$@" diff --git a/scripts/s3-tests/run.sh b/scripts/s3-tests/run.sh index 5681ab2c..3ff81f7f 100755 --- a/scripts/s3-tests/run.sh +++ b/scripts/s3-tests/run.sh @@ -504,13 +504,15 @@ fi log_info "Using template: ${TEMPLATE_PATH}" log_info "Generating config: ${CONF_OUTPUT_PATH}" -export S3_HOST +# Export all required variables for envsubst +export S3_HOST S3_ACCESS_KEY S3_SECRET_KEY S3_ALT_ACCESS_KEY S3_ALT_SECRET_KEY envsubst < "${TEMPLATE_PATH}" > "${CONF_OUTPUT_PATH}" || { log_error "Failed to generate s3tests config" exit 1 } # Step 7: Provision s3-tests alt user +# Note: Main user (rustfsadmin) is a system user and doesn't need to be created via API log_info "Provisioning s3-tests alt user..." if ! command -v awscurl >/dev/null 2>&1; then python3 -m pip install --user --upgrade pip awscurl || { @@ -520,7 +522,8 @@ if ! command -v awscurl >/dev/null 2>&1; then export PATH="$HOME/.local/bin:$PATH" fi -# Admin API requires AWS SigV4 signing +# Provision alt user (required by suite) +log_info "Provisioning alt user (${S3_ALT_ACCESS_KEY})..." awscurl \ --service s3 \ --region "${S3_REGION}" \