From 850639c67fe79f3c0e3f79fb079ef4c99cf1c75a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=94=90=E5=B0=8F=E9=B8=AD?= Date: Sat, 5 Sep 2026 19:31:25 +0800 Subject: [PATCH] fix(site-replication): hash repair tasks and retry snapshots with sorted JSON keys (backlog#2289) Service-account items carry their claims in a HashMap, and serde_json is built with preserve_order, so two serializations of the same plan could differ in key order. The repair preflight token then went stale between dry-run and execute (412 on the real VMs once snapshots carried service accounts) and a retry snapshot resend could never look stable. Serialize through a key-sorted JSON value for the task id and the fingerprint. --- rustfs/src/site_replication/mod.rs | 20 ++++++++ rustfs/src/site_replication/repair.rs | 4 +- rustfs/src/site_replication/retry.rs | 4 +- rustfs/src/site_replication/tests.rs | 70 +++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 4 deletions(-) diff --git a/rustfs/src/site_replication/mod.rs b/rustfs/src/site_replication/mod.rs index 05c523067..675aa5b5d 100644 --- a/rustfs/src/site_replication/mod.rs +++ b/rustfs/src/site_replication/mod.rs @@ -110,6 +110,26 @@ use tracing::{info, warn}; use url::{Url, form_urlencoded}; use uuid::Uuid; +/// Serialize `value` with every JSON object's keys sorted, for hashing and +/// equality checks. `HashMap` fields (service-account claims) iterate in a +/// per-instance random order and `serde_json` is built with `preserve_order`, +/// so two identical plans would otherwise hash differently: the repair +/// preflight token went stale between dry-run and execute, and a retry +/// snapshot resend never looked "stable" (backlog#2289 follow-up). +pub(crate) fn canonical_json_vec(value: &T) -> serde_json::Result> { + fn sort_keys(value: Value) -> Value { + match value { + Value::Object(map) => { + let sorted: BTreeMap = map.into_iter().map(|(key, value)| (key, sort_keys(value))).collect(); + Value::Object(sorted.into_iter().collect()) + } + Value::Array(items) => Value::Array(items.into_iter().map(sort_keys).collect()), + other => other, + } + } + serde_json::to_vec(&sort_keys(serde_json::to_value(value)?)) +} + pub(crate) const LOG_COMPONENT_ADMIN: &str = "admin"; pub(crate) const LOG_SUBSYSTEM_SITE_REPLICATION: &str = "site_replication"; diff --git a/rustfs/src/site_replication/repair.rs b/rustfs/src/site_replication/repair.rs index fdfe69440..ec341d70f 100644 --- a/rustfs/src/site_replication/repair.rs +++ b/rustfs/src/site_replication/repair.rs @@ -234,9 +234,9 @@ impl SiteReplicationRepairTask<'_> { pub(crate) fn id(&self) -> S3Result { let payload = match self { - Self::Iam(item) => serde_json::to_vec(item), + Self::Iam(item) => canonical_json_vec(item), Self::BucketMake(_) | Self::Replication(_) => serde_json::to_vec(&serde_json::json!({})), - Self::BucketMetadata(item) => serde_json::to_vec(item), + Self::BucketMetadata(item) => canonical_json_vec(item), } .map_err(|err| S3Error::with_message(S3ErrorCode::InternalError, format!("serialize repair task failed: {err}")))?; let mut digest = Sha256::new(); diff --git a/rustfs/src/site_replication/retry.rs b/rustfs/src/site_replication/retry.rs index a627e0b48..c9b9e73a0 100644 --- a/rustfs/src/site_replication/retry.rs +++ b/rustfs/src/site_replication/retry.rs @@ -867,8 +867,8 @@ impl RetrySnapshot { pub(crate) fn fingerprint(&self) -> S3Result>> { let mut payloads = match self { - Self::Iam(items) => items.iter().map(serde_json::to_vec).collect::, _>>(), - Self::BucketMetadata(items) => items.iter().map(serde_json::to_vec).collect::, _>>(), + Self::Iam(items) => items.iter().map(canonical_json_vec).collect::, _>>(), + Self::BucketMetadata(items) => items.iter().map(canonical_json_vec).collect::, _>>(), } .map_err(|err| S3Error::with_message(S3ErrorCode::InternalError, format!("serialize retry snapshot failed: {err}")))?; payloads.sort_unstable(); diff --git a/rustfs/src/site_replication/tests.rs b/rustfs/src/site_replication/tests.rs index 2ac08ed2e..a91d95d35 100644 --- a/rustfs/src/site_replication/tests.rs +++ b/rustfs/src/site_replication/tests.rs @@ -3609,3 +3609,73 @@ async fn test_broadcast_json_reaches_healthy_peers_after_a_peer_without_transpor ); server.abort(); } + +fn service_account_item_with_claims(order: &[&str]) -> SRIAMItem { + let mut claims = HashMap::new(); + for key in order { + claims.insert((*key).to_string(), serde_json::json!(format!("value-of-{key}"))); + } + SRIAMItem { + r#type: "service-account".to_string(), + svc_acc_change: Some(SRSvcAccChange { + create: Some(rustfs_madmin::SRSvcAccCreate { + parent: "alice".to_string(), + access_key: "alice-svc".to_string(), + secret_key: "alice-svc-secret".to_string(), + groups: Vec::new(), + claims, + session_policy: SRSessionPolicy::default(), + status: "on".to_string(), + name: String::new(), + description: String::new(), + expiration: None, + api_version: Some(SITE_REPL_API_VERSION.to_string()), + }), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + ..Default::default() + }), + updated_at: Some(OffsetDateTime::from_unix_timestamp(1_700_000_000).expect("timestamp")), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + ..Default::default() + } +} + +/// The repair preflight token and the retry-snapshot fingerprint hash the +/// serialized items. Service-account claims live in a `HashMap`, whose +/// iteration order differs between instances, so the hash must not depend on +/// it (the real-VM repair returned 412 "preflight is stale" between dry-run +/// and execute once snapshots carried service accounts). +#[test] +fn test_repair_task_id_and_retry_fingerprint_ignore_claim_map_order() { + let forward = service_account_item_with_claims(&["accessKey", "exp", "parent", "sa-policy", "sub", "tenant"]); + let backward = service_account_item_with_claims(&["tenant", "sub", "sa-policy", "parent", "exp", "accessKey"]); + + let canonical = canonical_json_vec(&forward).expect("canonical json"); + let text = String::from_utf8(canonical).expect("utf8"); + let positions: Vec = [ + "\"accessKey\"", + "\"exp\"", + "\"parent\"", + "\"sa-policy\"", + "\"sub\"", + "\"tenant\"", + ] + .iter() + .map(|key| text.find(key).expect("claim key present")) + .collect(); + assert!( + positions.windows(2).all(|pair| pair[0] < pair[1]), + "claim keys must serialize sorted: {text}" + ); + + assert_eq!( + SiteReplicationRepairTask::Iam(&forward).id().expect("id"), + SiteReplicationRepairTask::Iam(&backward).id().expect("id"), + "identical items must yield the same repair task id regardless of claim map order" + ); + assert_eq!( + RetrySnapshot::Iam(vec![forward]).fingerprint().expect("fingerprint"), + RetrySnapshot::Iam(vec![backward]).fingerprint().expect("fingerprint"), + "identical snapshots must fingerprint equal regardless of claim map order" + ); +}