mirror of
https://github.com/rustfs/rustfs.git
synced 2026-09-01 17:58:22 +00:00
fix(s3): round-trip null-version delete-marker identity (#6765)
* fix(s3): round-trip null-version delete-marker identity through listing and delete responses On a versioning-suspended bucket, a null delete marker's identity was lost on the way back to the client at three points (issue #6745): ListObjectVersions advertised the marker's VersionId as the literal nil UUID instead of null; deleting by that id succeeded but the DeleteObjects/DeleteObject response reported the identity as null with no way to correlate it to the request; and the response lacked DeleteMarker/DeleteMarkerVersionId because the marker-ness comparison mixed the client-facing identity (Some(nil)) with the storage identity (None), so the removal also mis-recorded accounting and fired DeleteMarkerCreated semantics on later paths. - Listing (bucket_usecase, s3_api/bucket, build_list_versions_next_marker) now maps the synthesized nil UUID to the literal null everywhere it reaches the wire, and VersionMarker::parse folds a nil-UUID marker from older listings into VersionMarker::Null so pagination resumes correctly. - delete_objects normalizes both sides of the marker-ness comparison via delete_file_info_version_id (matching the adjacent explicit_delete_marker admission check) and reports DeleteMarkerVersionId as null for an explicit null-marker removal. - resolve_delete_version_state reports delete_marker for an explicit-version delete whose target is a delete marker even when the bucket is versioning-suspended, fixing x-amz-delete-marker on the single-object path. - The DeleteObjects response entry echoes the version identity the request addressed for marker removals, marker-removal accounting no longer records a marker creation, and notification events fire DeleteMarkerCreated only for actual marker creation. Fixes #6745 * fix(s3): keep null-marker removal write shape undeleted and report marker semantics response-side The first cut marked the storage delete request deleted for a null-marker removal, which FileMeta::delete_version interprets as the suspended-bucket delete-mints-a-marker write and re-creates the marker just removed. Carry marker-ness to responses via explicit_delete_removed_marker (single path) and a response-only branch flag (batch path) instead, keeping every storage write shape byte-identical to the pre-fix behavior. Adds an embedded end-to-end regression test covering the full issue #6745 round trip.
This commit is contained in:
Zhengchao An
@@ -936,8 +936,13 @@ fn build_list_object_versions_metadata_output(
|
||||
.filter(|object| !object.name.is_empty())
|
||||
.map(|object| {
|
||||
let object_name = encode_list_versions_value(&object.name, encoding_type);
|
||||
// A null version surfaces as `Some(Uuid::nil())` on the
|
||||
// client-facing `ObjectInfo`; AWS advertises it as the literal
|
||||
// string `null`, and a nil UUID must never reach the wire
|
||||
// (issue #6745).
|
||||
let version_id = object
|
||||
.version_id
|
||||
.filter(|version| !version.is_nil())
|
||||
.map(|version| version.to_string())
|
||||
.unwrap_or_else(|| "null".to_string());
|
||||
let permission = permissions.get(&object.name).copied().unwrap_or_default();
|
||||
@@ -4159,7 +4164,9 @@ mod tests {
|
||||
match &output.entries[0] {
|
||||
ListObjectVersionMetadataEntry::Version(version, extension) => {
|
||||
assert_eq!(version.key.as_deref(), Some("obj-a"));
|
||||
assert_eq!(version.version_id.as_deref(), Some(Uuid::nil().to_string().as_str()));
|
||||
// A null version's synthesized nil UUID must surface as the
|
||||
// literal `null`, never as `00000000-…` (issue #6745).
|
||||
assert_eq!(version.version_id.as_deref(), Some("null"));
|
||||
assert_eq!(extension.user_tags.as_deref(), Some("env=prod"));
|
||||
assert_eq!(extension.internal, Some(ObjectInternalInfo { k: 4, m: 2 }));
|
||||
assert_eq!(
|
||||
|
||||
@@ -327,6 +327,27 @@ fn delete_response_version_id(version_id: Option<Uuid>, synthetic_version_id: bo
|
||||
}
|
||||
}
|
||||
|
||||
/// Version identity for a `DeleteObjects` `<Deleted>` entry (and its
|
||||
/// notification). A delete marker removed by version id carries no storage
|
||||
/// `version_id` on the committed result, so fall back to the identity the
|
||||
/// request addressed — clients correlate response entries against what they
|
||||
/// sent (issue #6745). Marker creation keeps `None`: its request carried no
|
||||
/// version id.
|
||||
fn delete_entry_response_version_id(
|
||||
committed_version_id: Option<Uuid>,
|
||||
committed_delete_marker: bool,
|
||||
requested_version_id: Option<Uuid>,
|
||||
synthetic_version_id: bool,
|
||||
) -> Option<String> {
|
||||
delete_response_version_id(committed_version_id, synthetic_version_id).or_else(|| {
|
||||
if committed_delete_marker {
|
||||
delete_response_version_id(requested_version_id, synthetic_version_id)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
fn reduce_delete_objects_result<'a>(
|
||||
object: &ObjectToDelete,
|
||||
deleted: &'a StorageDeletedObject,
|
||||
@@ -395,6 +416,11 @@ impl DefaultObjectUsecase {
|
||||
delete_object: Option<StorageDeletedObject>,
|
||||
error: Option<s3s::dto::Error>,
|
||||
synthetic_version_id: bool,
|
||||
// The version identity the request addressed, before any
|
||||
// synthetic-directory synthesis. `None` means the request carried
|
||||
// no version id, i.e. a committed delete marker was created, not
|
||||
// removed (issue #6745).
|
||||
requested_version_id: Option<Uuid>,
|
||||
}
|
||||
|
||||
let mut delete_results = vec![DeleteResult::default(); delete.objects.len()];
|
||||
@@ -481,6 +507,7 @@ impl DefaultObjectUsecase {
|
||||
..Default::default()
|
||||
};
|
||||
delete_results[idx].synthetic_version_id = synthetic_version_id;
|
||||
delete_results[idx].requested_version_id = version_uuid;
|
||||
|
||||
authorized_deletes.push(AuthorizedDelete { idx, object });
|
||||
}
|
||||
@@ -648,10 +675,16 @@ impl DefaultObjectUsecase {
|
||||
let creates_delete_marker = object_to_delete[i].version_id.is_none() && versioned && !version_suspended;
|
||||
let committed_delete_marker = dobjs[i].delete_marker;
|
||||
let delete_accounting = accounting.get(i).and_then(Option::as_ref);
|
||||
// `requested_version_id` distinguishes marker creation
|
||||
// (no version in the request) from marker removal by
|
||||
// version id; an explicit null-version request maps to
|
||||
// `Some(Uuid::nil())`, which `delete_request_targets_current`
|
||||
// would treat as versionless and mis-record a marker
|
||||
// removal as a marker creation (issue #6745).
|
||||
let update = delete_memory_update(
|
||||
creates_delete_marker,
|
||||
committed_delete_marker,
|
||||
delete_request_targets_current(object_to_delete[i].version_id),
|
||||
delete_results[didx].requested_version_id.is_none(),
|
||||
delete_accounting.and_then(|value| value.size),
|
||||
delete_accounting.is_some_and(|value| value.removed_current_object),
|
||||
);
|
||||
@@ -673,7 +706,12 @@ impl DefaultObjectUsecase {
|
||||
result.synthetic_version_id,
|
||||
),
|
||||
key: Some(object.object_name.clone()),
|
||||
version_id: delete_response_version_id(object.version_id, result.synthetic_version_id),
|
||||
version_id: delete_entry_response_version_id(
|
||||
object.version_id,
|
||||
object.delete_marker,
|
||||
result.requested_version_id,
|
||||
result.synthetic_version_id,
|
||||
),
|
||||
})
|
||||
.collect();
|
||||
let deleted_cache_keys = delete_results
|
||||
@@ -732,7 +770,10 @@ impl DefaultObjectUsecase {
|
||||
let _activity_guard = DeleteTailActivityGuard::new(DeleteTailStage::Notify);
|
||||
for res in delete_results {
|
||||
if let Some(dobj) = res.delete_object {
|
||||
let event_name = delete_event_name_for_marker(dobj.delete_marker);
|
||||
// `DeleteMarkerCreated` is a creation event; a delete
|
||||
// marker removed by an explicit version id is a plain
|
||||
// versioned delete (issue #6745).
|
||||
let event_name = delete_event_name_for_marker(dobj.delete_marker && res.requested_version_id.is_none());
|
||||
let event_args = EventArgsBuilder::new(
|
||||
event_name,
|
||||
notify_bucket.clone(),
|
||||
@@ -742,7 +783,15 @@ impl DefaultObjectUsecase {
|
||||
..Default::default()
|
||||
}),
|
||||
)
|
||||
.version_id(delete_response_version_id(dobj.version_id, res.synthetic_version_id).unwrap_or_default())
|
||||
.version_id(
|
||||
delete_entry_response_version_id(
|
||||
dobj.version_id,
|
||||
dobj.delete_marker,
|
||||
res.requested_version_id,
|
||||
res.synthetic_version_id,
|
||||
)
|
||||
.unwrap_or_default(),
|
||||
)
|
||||
.req_params(req_params.clone())
|
||||
.resp_elements(resp_elements.clone())
|
||||
.host(get_request_host(&req_headers))
|
||||
@@ -1082,7 +1131,9 @@ impl DefaultObjectUsecase {
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let event_name = delete_event_name_for_marker(delete_marker);
|
||||
// `DeleteMarkerCreated` is a creation event; a delete marker removed
|
||||
// by an explicit version id is a plain versioned delete (issue #6745).
|
||||
let event_name = delete_event_name_for_marker(delete_marker && version_id_clone.is_none());
|
||||
|
||||
helper = helper.event_name(event_name);
|
||||
helper = helper.object(obj_info).version_id(response_version_id.unwrap_or_default());
|
||||
@@ -1118,6 +1169,32 @@ mod tests {
|
||||
assert_eq!(delete_response_version_id(None, false), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn delete_entry_response_version_id_echoes_requested_identity_for_marker_removal() {
|
||||
let version_id = Uuid::new_v4();
|
||||
|
||||
// Marker removed by explicit null version id: echo `null`.
|
||||
assert_eq!(
|
||||
delete_entry_response_version_id(None, true, Some(Uuid::nil()), false),
|
||||
Some("null".to_string())
|
||||
);
|
||||
// Marker removed by a real version id: echo that id.
|
||||
assert_eq!(
|
||||
delete_entry_response_version_id(None, true, Some(version_id), false),
|
||||
Some(version_id.to_string())
|
||||
);
|
||||
// Marker creation (no version in the request): no version identity.
|
||||
assert_eq!(delete_entry_response_version_id(None, true, None, false), None);
|
||||
// Non-marker deletes keep the committed identity and never fall back.
|
||||
assert_eq!(
|
||||
delete_entry_response_version_id(Some(version_id), false, Some(Uuid::nil()), false),
|
||||
Some(version_id.to_string())
|
||||
);
|
||||
assert_eq!(delete_entry_response_version_id(None, false, Some(version_id), false), None);
|
||||
// Synthetic directory deletes stay without a version identity.
|
||||
assert_eq!(delete_entry_response_version_id(None, true, Some(Uuid::nil()), true), None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn execute_delete_object_rejects_invalid_object_key() {
|
||||
let input = DeleteObjectInput::builder()
|
||||
|
||||
@@ -26,6 +26,7 @@ use s3s::dto::{
|
||||
use s3s::{S3Error, S3ErrorCode};
|
||||
use tracing::debug;
|
||||
use urlencoding::encode;
|
||||
use uuid::Uuid;
|
||||
|
||||
use crate::storage::storage_api::s3_api_consumer::bucket::StorageObjectInfo as ObjectInfo;
|
||||
|
||||
@@ -198,6 +199,16 @@ pub(crate) fn parse_list_objects_v2_params(
|
||||
})
|
||||
}
|
||||
|
||||
/// A null version surfaces as `Some(Uuid::nil())` on the client-facing
|
||||
/// `ObjectInfo`; AWS advertises it as the literal string `null`, and a nil
|
||||
/// UUID must never reach the wire (issue #6745).
|
||||
fn list_versions_response_version_id(version_id: Option<Uuid>) -> String {
|
||||
version_id
|
||||
.filter(|id| !id.is_nil())
|
||||
.map(|id| id.to_string())
|
||||
.unwrap_or_else(|| "null".to_string())
|
||||
}
|
||||
|
||||
pub(crate) fn build_list_object_versions_output(
|
||||
object_infos: ListObjectVersionsInfo,
|
||||
bucket: String,
|
||||
@@ -213,7 +224,7 @@ pub(crate) fn build_list_object_versions_output(
|
||||
key: Some(encode_output_value(&v.name)),
|
||||
last_modified: v.mod_time.map(Timestamp::from),
|
||||
size: Some(v.size),
|
||||
version_id: Some(v.version_id.map(|id| id.to_string()).unwrap_or_else(|| "null".to_string())),
|
||||
version_id: Some(list_versions_response_version_id(v.version_id)),
|
||||
is_latest: Some(v.is_latest),
|
||||
e_tag: v.etag.clone().map(|etag| to_s3s_etag(&etag)),
|
||||
storage_class: v.storage_class.clone().map(ObjectVersionStorageClass::from),
|
||||
@@ -227,7 +238,7 @@ pub(crate) fn build_list_object_versions_output(
|
||||
.filter(|o| o.delete_marker)
|
||||
.map(|o| DeleteMarkerEntry {
|
||||
key: Some(encode_output_value(&o.name)),
|
||||
version_id: Some(o.version_id.map(|id| id.to_string()).unwrap_or_else(|| "null".to_string())),
|
||||
version_id: Some(list_versions_response_version_id(o.version_id)),
|
||||
is_latest: Some(o.is_latest),
|
||||
last_modified: o.mod_time.map(Timestamp::from),
|
||||
..Default::default()
|
||||
@@ -940,6 +951,13 @@ mod tests {
|
||||
is_latest: false,
|
||||
..Default::default()
|
||||
},
|
||||
ObjectInfo {
|
||||
name: "obj-null-delete-marker".to_string(),
|
||||
delete_marker: true,
|
||||
version_id: Some(Uuid::nil()),
|
||||
is_latest: true,
|
||||
..Default::default()
|
||||
},
|
||||
ObjectInfo {
|
||||
name: String::new(),
|
||||
delete_marker: false,
|
||||
@@ -975,12 +993,16 @@ mod tests {
|
||||
let versions = output.versions.unwrap_or_default();
|
||||
assert_eq!(versions.len(), 1);
|
||||
assert_eq!(versions[0].key, Some("obj-a".to_string()));
|
||||
assert_eq!(versions[0].version_id, Some(Uuid::nil().to_string()));
|
||||
// A null version's synthesized nil UUID must surface as the literal
|
||||
// `null`, never as `00000000-…` (issue #6745).
|
||||
assert_eq!(versions[0].version_id, Some("null".to_string()));
|
||||
|
||||
let delete_markers = output.delete_markers.unwrap_or_default();
|
||||
assert_eq!(delete_markers.len(), 1);
|
||||
assert_eq!(delete_markers.len(), 2);
|
||||
assert_eq!(delete_markers[0].key, Some("obj-delete-marker".to_string()));
|
||||
assert_eq!(delete_markers[0].version_id, Some("null".to_string()));
|
||||
assert_eq!(delete_markers[1].key, Some("obj-null-delete-marker".to_string()));
|
||||
assert_eq!(delete_markers[1].version_id, Some("null".to_string()));
|
||||
|
||||
let prefixes = output.common_prefixes.unwrap_or_default();
|
||||
assert_eq!(
|
||||
|
||||
@@ -21,6 +21,7 @@
|
||||
|
||||
use aws_sdk_s3::config::{Credentials, Region};
|
||||
use aws_sdk_s3::primitives::ByteStream;
|
||||
use aws_sdk_s3::types::{BucketVersioningStatus, Delete, ObjectIdentifier, VersioningConfiguration};
|
||||
use aws_sdk_s3::{Client, Config};
|
||||
use rustfs::embedded::{RustFSServerBuilder, find_available_port};
|
||||
|
||||
@@ -118,3 +119,133 @@ async fn test_embedded_server_basic_s3_operations_body() {
|
||||
|
||||
server.shutdown().await;
|
||||
}
|
||||
|
||||
// Regression test for issue #6745: on a versioning-suspended bucket, a null
|
||||
// delete marker's version identity must round-trip as the literal `null`
|
||||
// through ListObjectVersions, DeleteObject, and DeleteObjects, and removing
|
||||
// the marker by version id must carry the delete-marker flags.
|
||||
#[test]
|
||||
fn test_null_version_delete_marker_round_trip() {
|
||||
common::run_embedded_test(test_null_version_delete_marker_round_trip_body);
|
||||
}
|
||||
|
||||
async fn test_null_version_delete_marker_round_trip_body() {
|
||||
let port = match find_available_port() {
|
||||
Ok(port) => port,
|
||||
Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return,
|
||||
Err(err) => panic!("find free port: {err}"),
|
||||
};
|
||||
let server = RustFSServerBuilder::new()
|
||||
.address(format!("127.0.0.1:{port}"))
|
||||
.access_key("testaccesskey")
|
||||
.secret_key("testsecretkey")
|
||||
.build()
|
||||
.await
|
||||
.expect("start embedded server");
|
||||
let client = s3_client(&server.endpoint(), server.access_key(), server.secret_key());
|
||||
|
||||
let bucket = "null-marker-bucket";
|
||||
client.create_bucket().bucket(bucket).send().await.expect("create bucket");
|
||||
for status in [BucketVersioningStatus::Enabled, BucketVersioningStatus::Suspended] {
|
||||
client
|
||||
.put_bucket_versioning()
|
||||
.bucket(bucket)
|
||||
.versioning_configuration(VersioningConfiguration::builder().status(status).build())
|
||||
.send()
|
||||
.await
|
||||
.expect("set bucket versioning state");
|
||||
}
|
||||
|
||||
// A versionless DELETE on the suspended bucket mints a null delete marker
|
||||
// and must report it as version `null`.
|
||||
for key in ["doc/f1.txt", "doc/f2.txt"] {
|
||||
client
|
||||
.put_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.body(ByteStream::from_static(b"null-version payload"))
|
||||
.send()
|
||||
.await
|
||||
.expect("put object");
|
||||
let deleted = client
|
||||
.delete_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.send()
|
||||
.await
|
||||
.expect("delete object without version id");
|
||||
assert_eq!(deleted.delete_marker(), Some(true), "suspended-bucket delete should mint a marker");
|
||||
assert_eq!(deleted.version_id(), Some("null"), "the minted marker is the null version");
|
||||
}
|
||||
|
||||
// The markers must be listed under the literal `null`, never a nil UUID.
|
||||
let listed = client
|
||||
.list_object_versions()
|
||||
.bucket(bucket)
|
||||
.send()
|
||||
.await
|
||||
.expect("list object versions");
|
||||
assert!(listed.versions().is_empty(), "the null versions were replaced by markers");
|
||||
let markers = listed.delete_markers();
|
||||
assert_eq!(markers.len(), 2);
|
||||
for marker in markers {
|
||||
assert_eq!(marker.version_id(), Some("null"), "listing must advertise the null version as `null`");
|
||||
}
|
||||
|
||||
// Removing one marker by its listed version id over DeleteObject must
|
||||
// acknowledge the marker identity on the wire.
|
||||
let removed = client
|
||||
.delete_object()
|
||||
.bucket(bucket)
|
||||
.key("doc/f1.txt")
|
||||
.version_id("null")
|
||||
.send()
|
||||
.await
|
||||
.expect("delete marker by null version id");
|
||||
assert_eq!(
|
||||
removed.delete_marker(),
|
||||
Some(true),
|
||||
"x-amz-delete-marker must be true for a marker removal"
|
||||
);
|
||||
assert_eq!(removed.version_id(), Some("null"));
|
||||
|
||||
// Removing the other via DeleteObjects must produce an entry a client can
|
||||
// correlate with its request: same key, version `null`, marker flags set.
|
||||
let delete = Delete::builder()
|
||||
.objects(
|
||||
ObjectIdentifier::builder()
|
||||
.key("doc/f2.txt")
|
||||
.version_id("null")
|
||||
.build()
|
||||
.expect("object identifier"),
|
||||
)
|
||||
.build()
|
||||
.expect("delete payload");
|
||||
let batch = client
|
||||
.delete_objects()
|
||||
.bucket(bucket)
|
||||
.delete(delete)
|
||||
.send()
|
||||
.await
|
||||
.expect("delete objects by null version id");
|
||||
assert!(batch.errors().is_empty(), "batch delete reported errors: {:?}", batch.errors());
|
||||
let entries = batch.deleted();
|
||||
assert_eq!(entries.len(), 1);
|
||||
assert_eq!(entries[0].key(), Some("doc/f2.txt"));
|
||||
assert_eq!(entries[0].version_id(), Some("null"), "the entry must echo the requested identity");
|
||||
assert_eq!(entries[0].delete_marker(), Some(true));
|
||||
assert_eq!(entries[0].delete_marker_version_id(), Some("null"));
|
||||
|
||||
// With identity round-tripping, one pass leaves the bucket truly empty
|
||||
// and deletable.
|
||||
let after = client
|
||||
.list_object_versions()
|
||||
.bucket(bucket)
|
||||
.send()
|
||||
.await
|
||||
.expect("list versions after cleanup");
|
||||
assert!(after.versions().is_empty() && after.delete_markers().is_empty());
|
||||
client.delete_bucket().bucket(bucket).send().await.expect("delete bucket");
|
||||
|
||||
server.shutdown().await;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user