From 760c9d65be2f0f50caf9fb204160e4d40f709c5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=94=90=E5=B0=8F=E9=B8=AD?= Date: Sun, 6 Sep 2026 14:12:25 +0800 Subject: [PATCH] fix(replication): close IAM snapshot, marker purge and broadcast gaps (#7195) --- crates/ecstore/src/api/mod.rs | 12 +- crates/ecstore/src/bucket/metadata.rs | 48 +- crates/ecstore/src/bucket/metadata_sys.rs | 242 +++- .../replication_object_decision_boundary.rs | 10 +- .../bucket/replication/replication_pool.rs | 97 ++ .../replication/replication_resyncer.rs | 42 +- crates/iam/src/manager.rs | 229 +++- crates/iam/src/sys.rs | 297 ++++- crates/policy/src/policy/doc.rs | 21 +- crates/replication/src/delete.rs | 166 ++- crates/replication/src/filemeta.rs | 20 + crates/replication/src/lib.rs | 6 +- crates/replication/src/mrf.rs | 169 ++- rustfs/src/admin/handlers/site_replication.rs | 1166 +++++++++++++++-- rustfs/src/admin/handlers/user.rs | 1 + rustfs/src/admin/storage_api.rs | 50 + rustfs/src/app/storage_api.rs | 2 + rustfs/src/site_replication/hooks.rs | 247 +++- rustfs/src/site_replication/mod.rs | 29 +- rustfs/src/site_replication/repair.rs | 6 +- rustfs/src/site_replication/retry.rs | 114 +- rustfs/src/site_replication/state.rs | 125 ++ rustfs/src/site_replication/tests.rs | 494 ++++++- rustfs/src/site_replication/transport.rs | 30 +- 24 files changed, 3379 insertions(+), 244 deletions(-) diff --git a/crates/ecstore/src/api/mod.rs b/crates/ecstore/src/api/mod.rs index fb7af79ae..e0cc06d26 100644 --- a/crates/ecstore/src/api/mod.rs +++ b/crates/ecstore/src/api/mod.rs @@ -159,15 +159,17 @@ pub mod bucket { BUCKET_CONFIG_PUBLISH_HOOK, BucketConfigPublishHook, BucketMetadataMutationGuard, BucketMetadataSys, ObjectLockConfigState, acquire_bucket_metadata_transaction_lock, acquire_bucket_metadata_transaction_lock_for_incarnation, acquire_scanner_bucket_incarnation_fence, - capture_bucket_metadata_incarnation, delete, delete_if_incarnation, delete_under_transaction_lock, get, - get_accelerate_config, get_bucket_policy, get_bucket_policy_raw, get_bucket_targets_config, get_config_from_disk, - get_cors_config, get_durability_config, get_global_bucket_metadata_sys, get_lifecycle_config, get_logging_config, - get_notification_config, get_object_lock_config, get_object_lock_config_state, get_on_demand_migration_config, + capture_bucket_metadata_incarnation, delete, delete_if_incarnation, delete_if_incarnation_at, + delete_under_transaction_lock, get, get_accelerate_config, get_bucket_policy, get_bucket_policy_raw, + get_bucket_targets_config, get_config_from_disk, get_cors_config, get_durability_config, + get_global_bucket_metadata_sys, get_lifecycle_config, get_logging_config, get_notification_config, + get_object_lock_config, get_object_lock_config_state, get_on_demand_migration_config, get_on_demand_migration_config_in, get_public_access_block_config, get_quota_config, get_replication_config, get_request_payment_config, get_sse_config, get_tagging_config, get_versioning_config, get_website_config, init_bucket_metadata_sys, list_bucket_targets, reload_bucket_metadata, remove_bucket_metadata, set_bucket_metadata, update, update_bucket_targets_under_transaction_lock, update_config_with, update_if_incarnation, - update_quota_if_incarnation, update_under_transaction_lock, + update_if_incarnation_at, update_quota_if_incarnation, update_quota_if_incarnation_at, update_under_transaction_lock, + update_under_transaction_lock_at, }; #[cfg(feature = "test-util")] pub use crate::bucket::metadata_sys::{ConfigWriteLockProbe, test_support}; diff --git a/crates/ecstore/src/bucket/metadata.rs b/crates/ecstore/src/bucket/metadata.rs index 04b97f10f..b959dee60 100644 --- a/crates/ecstore/src/bucket/metadata.rs +++ b/crates/ecstore/src/bucket/metadata.rs @@ -791,9 +791,22 @@ impl BucketMetadata { } } + /// Replace one config payload and stamp its `*_config_updated_at` with the + /// local clock. This is the entry for edits that originate here: the + /// local write time is the edit's source time. pub fn update_config(&mut self, config_file: &str, data: Vec) -> Result { - let updated = OffsetDateTime::now_utc(); + self.update_config_at(config_file, data, OffsetDateTime::now_utc()) + } + /// [`Self::update_config`] with an explicit `updated_at` stamp. + /// + /// For a config replicated from another site the edit's source time is + /// the peer's `updated_at`, not the moment it lands here: staleness of + /// the next incoming item is judged against the stored stamp, so stamping + /// the local apply time would reject a newer source edit that was merely + /// delivered late (backlog#2292). Only replication receivers should pass + /// a foreign time; local edits keep [`Self::update_config`]. + pub fn update_config_at(&mut self, config_file: &str, data: Vec, updated: OffsetDateTime) -> Result { match config_file { BUCKET_POLICY_CONFIG => { self.policy_config_json = data; @@ -1525,6 +1538,39 @@ mod test { assert_eq!(metadata.bucket_incarnation_id, incarnation); } + /// backlog#2292: a replicated config is stamped with the source + /// `updated_at` it was given, not the local clock, while the plain + /// `update_config` entry keeps stamping the local clock. + #[test] + fn update_config_at_stamps_the_given_time_and_update_config_stamps_now() { + let source_time = OffsetDateTime::now_utc() - time::Duration::hours(3); + let mut metadata = BucketMetadata::new("source-stamped"); + + let stamped = metadata + .update_config_at(BUCKET_POLICY_CONFIG, br#"{"Version":"2012-10-17","Statement":[]}"#.to_vec(), source_time) + .unwrap(); + assert_eq!(stamped, source_time); + assert_eq!(metadata.policy_config_updated_at, source_time); + + let tagging = b"kv".to_vec(); + let stamped = metadata + .update_config_at(BUCKET_TAGGING_CONFIG, tagging, source_time) + .unwrap(); + assert_eq!(stamped, source_time); + assert_eq!(metadata.tagging_config_updated_at, source_time); + + let before = OffsetDateTime::now_utc(); + let stamped = metadata + .update_config(BUCKET_POLICY_CONFIG, br#"{"Version":"2012-10-17","Statement":[]}"#.to_vec()) + .unwrap(); + assert!(stamped >= before, "a local edit is stamped with the local clock"); + assert_eq!(metadata.policy_config_updated_at, stamped); + assert_eq!( + metadata.tagging_config_updated_at, source_time, + "restamping one config must not move another config's stamp" + ); + } + #[test] fn object_locking_requires_lock_metadata_not_plain_versioning() { use s3s::dto::ObjectLockEnabled; diff --git a/crates/ecstore/src/bucket/metadata_sys.rs b/crates/ecstore/src/bucket/metadata_sys.rs index 17f185b63..4d0aab0ee 100644 --- a/crates/ecstore/src/bucket/metadata_sys.rs +++ b/crates/ecstore/src/bucket/metadata_sys.rs @@ -567,6 +567,32 @@ pub async fn update_if_incarnation( config_file, data, Some(expected_incarnation_id), + None, + )) + .await +} + +/// [`update_if_incarnation`] stamping the config with `updated_at` instead of +/// the local clock. +/// +/// For a site-replication receiver the edit's source time is the peer's +/// `updated_at`; persisting it keeps the stored `*_config_updated_at` on the +/// source clock so the next item's staleness is judged source-time against +/// source-time (backlog#2292). See [`BucketMetadata::update_config_at`]. +pub async fn update_if_incarnation_at( + bucket: &str, + config_file: &str, + data: Vec, + expected_incarnation_id: Uuid, + updated_at: OffsetDateTime, +) -> Result { + Box::pin(update_with_sys_expected( + get_bucket_metadata_sys()?, + bucket, + config_file, + data, + Some(expected_incarnation_id), + Some(updated_at), )) .await } @@ -577,6 +603,30 @@ pub async fn delete_if_incarnation(bucket: &str, config_file: &str, expected_inc bucket, config_file, Some(expected_incarnation_id), + None, + )) + .await +} + +/// [`delete_if_incarnation`] stamping the cleared config with `updated_at` +/// (a replicated deletion's source time) instead of the local clock. +/// +/// The stamp survives the deletion as the config's `*_config_updated_at`, and +/// that is what the next incoming item is judged against: a local stamp on +/// the delete would reject a newer source re-create that was merely delivered +/// later (backlog#2292). See [`update_if_incarnation_at`]. +pub async fn delete_if_incarnation_at( + bucket: &str, + config_file: &str, + expected_incarnation_id: Uuid, + updated_at: OffsetDateTime, +) -> Result { + Box::pin(delete_with_sys_expected( + get_bucket_metadata_sys()?, + bucket, + config_file, + Some(expected_incarnation_id), + Some(updated_at), )) .await } @@ -598,34 +648,41 @@ async fn update_with_sys( config_file: &str, data: Vec, ) -> Result { - update_with_sys_expected(sys, bucket, config_file, data, None).await + update_with_sys_expected(sys, bucket, config_file, data, None, None).await } +/// `updated_at` is the stamp persisted on the config; `None` uses the local +/// clock (the edit originates here), `Some` carries a replicated edit's +/// source time (backlog#2292). async fn update_with_sys_expected( sys: Arc>, bucket: &str, config_file: &str, data: Vec, expected_incarnation_id: Option, + updated_at: Option, ) -> Result { let guard = acquire_config_write_guard_for_incarnation(sys.clone(), bucket, expected_incarnation_id).await?; - update_under_config_write_guard(sys, &guard, config_file, data).await + update_under_config_write_guard(sys, &guard, config_file, data, updated_at).await } /// [`delete`] against an explicitly supplied metadata system. See /// [`update_with_sys`]. async fn delete_with_sys(sys: Arc>, bucket: &str, config_file: &str) -> Result { - delete_with_sys_expected(sys, bucket, config_file, None).await + delete_with_sys_expected(sys, bucket, config_file, None, None).await } +/// `updated_at`: `None` stamps the local clock; `Some` persists a replicated +/// deletion's source time (backlog#2292). async fn delete_with_sys_expected( sys: Arc>, bucket: &str, config_file: &str, expected_incarnation_id: Option, + updated_at: Option, ) -> Result { let guard = acquire_config_write_guard_for_incarnation(sys.clone(), bucket, expected_incarnation_id).await?; - delete_under_config_write_guard(sys, &guard, config_file).await + delete_under_config_write_guard(sys, &guard, config_file, updated_at).await } /// Owns the complete bucket-config mutation fence. @@ -772,7 +829,21 @@ pub async fn update_under_transaction_lock( data: Vec, ) -> Result { guard.ensure_valid(bucket)?; - update_under_config_write_guard(get_bucket_metadata_sys()?, guard, config_file, data).await + update_under_config_write_guard(get_bucket_metadata_sys()?, guard, config_file, data, None).await +} + +/// [`update_under_transaction_lock`] stamping the config with `updated_at` +/// (a replicated edit's source time) instead of the local clock; see +/// [`update_if_incarnation_at`] (backlog#2292). +pub async fn update_under_transaction_lock_at( + guard: &BucketMetadataMutationGuard, + bucket: &str, + config_file: &str, + data: Vec, + updated_at: OffsetDateTime, +) -> Result { + guard.ensure_valid(bucket)?; + update_under_config_write_guard(get_bucket_metadata_sys()?, guard, config_file, data, Some(updated_at)).await } /// Clear one config file while the caller holds this bucket's transaction lock. @@ -782,7 +853,7 @@ pub async fn delete_under_transaction_lock( config_file: &str, ) -> Result { guard.ensure_valid(bucket)?; - delete_under_config_write_guard(get_bucket_metadata_sys()?, guard, config_file).await + delete_under_config_write_guard(get_bucket_metadata_sys()?, guard, config_file, None).await } pub async fn update_quota_if_incarnation( @@ -790,6 +861,29 @@ pub async fn update_quota_if_incarnation( data: Vec, expected_incarnation_id: Uuid, proof: &crate::services::notification_sys::CrossPoolFenceFleetProofToken, +) -> Result { + update_quota_if_incarnation_stamped(bucket, data, expected_incarnation_id, proof, None).await +} + +/// [`update_quota_if_incarnation`] stamping the quota config with +/// `updated_at` (a replicated edit's source time) instead of the local +/// clock; see [`update_if_incarnation_at`] (backlog#2292). +pub async fn update_quota_if_incarnation_at( + bucket: &str, + data: Vec, + expected_incarnation_id: Uuid, + proof: &crate::services::notification_sys::CrossPoolFenceFleetProofToken, + updated_at: OffsetDateTime, +) -> Result { + update_quota_if_incarnation_stamped(bucket, data, expected_incarnation_id, proof, Some(updated_at)).await +} + +async fn update_quota_if_incarnation_stamped( + bucket: &str, + data: Vec, + expected_incarnation_id: Uuid, + proof: &crate::services::notification_sys::CrossPoolFenceFleetProofToken, + updated_at: Option, ) -> Result { let sys = get_bucket_metadata_sys()?; let guard = Box::pin(acquire_config_write_guard_for_incarnation( @@ -807,7 +901,7 @@ pub async fn update_quota_if_incarnation( achieved: 0, }); } - update_under_config_write_guard(sys, &guard, rustfs_config::QUOTA_CONFIG_FILE, data).await + update_under_config_write_guard(sys, &guard, rustfs_config::QUOTA_CONFIG_FILE, data, updated_at).await } pub async fn update_bucket_targets_under_transaction_lock( @@ -823,6 +917,7 @@ async fn update_under_config_write_guard( guard: &BucketMetadataMutationGuard, config_file: &str, data: Vec, + updated_at: Option, ) -> Result { guard.ensure_valid(&guard.bucket)?; let metadata_sys = sys.read().await.clone(); @@ -834,7 +929,7 @@ async fn update_under_config_write_guard( Some(&guard.transaction_guard), &guard.bucket, "bucket config transaction", - metadata_sys.update_checked(&guard.bucket, config_file, data, true, guard.incarnation_id), + metadata_sys.update_checked(&guard.bucket, config_file, data, true, guard.incarnation_id, updated_at), ), ) .await?; @@ -846,6 +941,7 @@ async fn delete_under_config_write_guard( sys: Arc>, guard: &BucketMetadataMutationGuard, config_file: &str, + updated_at: Option, ) -> Result { guard.ensure_valid(&guard.bucket)?; let metadata_sys = sys.read().await.clone(); @@ -857,7 +953,7 @@ async fn delete_under_config_write_guard( Some(&guard.transaction_guard), &guard.bucket, "bucket config deletion transaction", - metadata_sys.update_checked(&guard.bucket, config_file, Vec::new(), false, guard.incarnation_id), + metadata_sys.update_checked(&guard.bucket, config_file, Vec::new(), false, guard.incarnation_id, updated_at), ), ) .await?; @@ -1762,15 +1858,17 @@ impl BucketMetadataSys { /// `update` and the config read alone). Keep these boxed. pub async fn update(&self, bucket: &str, config_file: &str, data: Vec) -> Result { let incarnation_id = Box::pin(self.get_bucket_incarnation_id(bucket)).await?; - Box::pin(self.update_checked(bucket, config_file, data, true, incarnation_id)).await + Box::pin(self.update_checked(bucket, config_file, data, true, incarnation_id, None)).await } pub async fn delete(&self, bucket: &str, config_file: &str) -> Result { let incarnation_id = self.get_bucket_incarnation_id(bucket).await?; - self.update_checked(bucket, config_file, Vec::new(), false, incarnation_id) + self.update_checked(bucket, config_file, Vec::new(), false, incarnation_id, None) .await } + /// `updated_at`: `None` stamps the local clock; `Some` persists a + /// replicated edit's source time (backlog#2292). async fn update_checked( &self, bucket: &str, @@ -1778,6 +1876,7 @@ impl BucketMetadataSys { data: Vec, parse: bool, expected_incarnation_id: Uuid, + updated_at: Option, ) -> Result { // Load through this system's own store, the one `save` persists to // (backlog#1052 S7). Reading from the ambient handle instead made the @@ -1788,7 +1887,10 @@ impl BucketMetadataSys { return Err(Error::BucketNotFound(bucket.to_string())); } - let updated = bm.update_config(config_file, data)?; + let updated = match updated_at { + Some(updated_at) => bm.update_config_at(config_file, data, updated_at)?, + None => bm.update_config(config_file, data)?, + }; Box::pin(self.save(bm)).await?; @@ -3755,6 +3857,106 @@ mod tests { ); } + /// backlog#2292: the explicit-stamp write path persists the given source + /// time as the config's `*_config_updated_at` — through the incarnation + /// path and through an already-held transaction guard — and survives a + /// reload from disk, while the plain path keeps stamping the local clock. + #[tokio::test] + async fn explicit_updated_at_is_persisted_as_the_config_stamp() { + let (dirs, ecstore) = isolated_store_over_temp_disks().await; + let bucket = "source-stamped-config"; + for dir in &dirs { + std::fs::create_dir_all(dir.path().join(bucket)).expect("bucket volume should be created"); + } + let sys = Arc::new(RwLock::new(BucketMetadataSys::new(ecstore))); + let source_time = OffsetDateTime::now_utc() - Duration::from_secs(3 * 3600); + let policy = br#"{"Version":"2012-10-17","Statement":[]}"#.to_vec(); + let tagging = b"kv".to_vec(); + + // Incarnation path (`update_if_incarnation_at` minus the ambient lookup). + let stamped = + update_with_sys_expected(sys.clone(), bucket, BUCKET_POLICY_CONFIG, policy.clone(), None, Some(source_time)) + .await + .expect("source-stamped policy write should persist"); + assert_eq!(stamped, source_time); + + // Held-guard path (`update_under_transaction_lock_at` minus the ambient lookup). + let guard = acquire_config_write_guard(sys.clone(), bucket).await.expect("write guard"); + let stamped = update_under_config_write_guard(sys.clone(), &guard, BUCKET_TAGGING_CONFIG, tagging, Some(source_time)) + .await + .expect("source-stamped tagging write should persist"); + drop(guard); + assert_eq!(stamped, source_time); + + let metadata_sys = sys.read().await.clone(); + metadata_sys.metadata_map.write().await.clear(); + let reloaded = metadata_sys.get_config_from_disk(bucket).await.expect("reload from disk"); + assert_eq!(reloaded.policy_config_updated_at, source_time); + assert_eq!(reloaded.tagging_config_updated_at, source_time); + + // The plain path is unchanged: a local edit is stamped with the local clock. + let before = OffsetDateTime::now_utc(); + let stamped = update_with_sys(sys.clone(), bucket, BUCKET_POLICY_CONFIG, policy) + .await + .expect("locally stamped policy write should persist"); + assert!(stamped >= before, "the plain write path must keep stamping the local clock"); + let reloaded = metadata_sys.get_config_from_disk(bucket).await.expect("reload from disk"); + assert_eq!(reloaded.policy_config_updated_at, stamped); + assert_eq!( + reloaded.tagging_config_updated_at, source_time, + "an unrelated config keeps its source stamp" + ); + } + + /// backlog#2292: a replicated delete persists the source time as the + /// cleared config's `*_config_updated_at`, so the receive-side gate + /// (source time against stored stamp) lets a newer source re-create land + /// even when the delete was applied later than the re-create's source + /// time; the plain delete keeps stamping the local clock. + #[tokio::test] + async fn explicit_updated_at_is_persisted_by_a_delete() { + let (dirs, ecstore) = isolated_store_over_temp_disks().await; + let bucket = "source-stamped-delete"; + for dir in &dirs { + std::fs::create_dir_all(dir.path().join(bucket)).expect("bucket volume should be created"); + } + let sys = Arc::new(RwLock::new(BucketMetadataSys::new(ecstore))); + let policy = br#"{"Version":"2012-10-17","Statement":[]}"#.to_vec(); + let created_at = OffsetDateTime::now_utc() - Duration::from_secs(3 * 3600); + let deleted_at = created_at + Duration::from_secs(60); + let recreated_at = deleted_at + Duration::from_secs(60); + + update_with_sys_expected(sys.clone(), bucket, BUCKET_POLICY_CONFIG, policy.clone(), None, Some(created_at)) + .await + .expect("source-stamped policy write should persist"); + let stamped = delete_with_sys_expected(sys.clone(), bucket, BUCKET_POLICY_CONFIG, None, Some(deleted_at)) + .await + .expect("source-stamped policy delete should persist"); + assert_eq!(stamped, deleted_at); + + let metadata_sys = sys.read().await.clone(); + metadata_sys.metadata_map.write().await.clear(); + let reloaded = metadata_sys.get_config_from_disk(bucket).await.expect("reload from disk"); + assert!(reloaded.policy_config_json.is_empty(), "the delete cleared the payload"); + assert_eq!(reloaded.policy_config_updated_at, deleted_at, "the delete kept the source stamp"); + assert!( + recreated_at >= reloaded.policy_config_updated_at, + "a re-create newer than the delete's source time is not stale against the stored stamp" + ); + + // The plain delete path is unchanged: stamped with the local clock. + update_with_sys_expected(sys.clone(), bucket, BUCKET_POLICY_CONFIG, policy, None, Some(recreated_at)) + .await + .expect("re-create should persist"); + let before = OffsetDateTime::now_utc(); + let stamped = delete_with_sys_expected(sys.clone(), bucket, BUCKET_POLICY_CONFIG, None, None) + .await + .expect("locally stamped delete should persist"); + assert!(stamped >= before, "the plain delete path must keep stamping the local clock"); + let reloaded = metadata_sys.get_config_from_disk(bucket).await.expect("reload from disk"); + assert_eq!(reloaded.policy_config_updated_at, stamped); + } + /// The load and the persisted write share one write guard, so concurrent /// rewrites of the same config compose instead of clobbering each other. /// Moving the load outside that guard loses all but the last tag. @@ -3971,10 +4173,16 @@ mod tests { let new_incarnation = store.bucket_incarnation_id_from_disk(bucket).await.unwrap(); assert_ne!(old_incarnation, new_incarnation); - let err = - update_with_sys_expected(sys.clone(), bucket, BUCKET_TAGGING_CONFIG, b"".to_vec(), Some(old_incarnation)) - .await - .expect_err("a request authorized for the deleted incarnation must fail closed"); + let err = update_with_sys_expected( + sys.clone(), + bucket, + BUCKET_TAGGING_CONFIG, + b"".to_vec(), + Some(old_incarnation), + None, + ) + .await + .expect_err("a request authorized for the deleted incarnation must fail closed"); assert!(matches!(err, Error::BucketNotFound(name) if name == bucket)); let persisted = sys.read().await.get_config_from_disk(bucket).await.unwrap(); @@ -4009,7 +4217,7 @@ mod tests { }], }) .unwrap(); - update_under_config_write_guard(sys, &guard, BUCKET_TAGGING_CONFIG, tagging) + update_under_config_write_guard(sys, &guard, BUCKET_TAGGING_CONFIG, tagging, None) .await .unwrap(); assert!(!delete.is_finished()); diff --git a/crates/ecstore/src/bucket/replication/replication_object_decision_boundary.rs b/crates/ecstore/src/bucket/replication/replication_object_decision_boundary.rs index 3544ac27b..553a57e1b 100644 --- a/crates/ecstore/src/bucket/replication/replication_object_decision_boundary.rs +++ b/crates/ecstore/src/bucket/replication/replication_object_decision_boundary.rs @@ -20,9 +20,9 @@ pub use rustfs_replication::{ pub(crate) use rustfs_replication::{ ReplicationDeleteSource, ReplicationMultipartPartInput, ReplicationResyncTargetObject, delete_marker_purge_mrf_entry, delete_marker_purge_version_id, delete_replication_creates_marker, delete_replication_missing_source_decision, - delete_replication_object_opts, heal_uses_delete_replication_path, is_object_lock_denied_delete, - is_retryable_delete_replication_head_error, is_version_delete_replication, replicate_delete_outcome, replication_etags_match, - replication_multipart_complete_actual_size, replication_multipart_part_plan, replication_single_put_size_error, - resync_existing_delete_replication_info, resync_target_for_object, should_retry_delete_marker_purge, - single_part_replica_etag_mismatch, target_delete_version_id, + delete_replication_object_opts, delete_replication_target_version_id, heal_uses_delete_replication_path, + is_object_lock_denied_delete, is_retryable_delete_replication_head_error, is_version_delete_replication, + replicate_delete_outcome, replication_etags_match, replication_multipart_complete_actual_size, + replication_multipart_part_plan, replication_single_put_size_error, resync_existing_delete_replication_info, + resync_target_for_object, should_retry_delete_marker_purge, single_part_replica_etag_mismatch, }; diff --git a/crates/ecstore/src/bucket/replication/replication_pool.rs b/crates/ecstore/src/bucket/replication/replication_pool.rs index 9efa9864e..e20ad9dbe 100644 --- a/crates/ecstore/src/bucket/replication/replication_pool.rs +++ b/crates/ecstore/src/bucket/replication/replication_pool.rs @@ -882,6 +882,20 @@ fn reconstructed_heal_delete_info( ) -> DeletedObjectReplicationInfo { let mut rstate = oi.replication_state(); rstate.replicate_decision_str = dsc.to_string(); + // The caller hands us a blank ObjectInfo (the source marker may already be + // gone), so the state above carries no target-assigned marker version ids. + // Restore them from the journal: `delete_marker_purge_version_id` must hit + // the id the target reported, not fall back to the source marker id, which + // a target that mints its own ids answers with an idempotent 204 that would + // acknowledge the intent while the real marker stays behind (backlog#2290). + // The corrupt flag rides along so a refusal stays a refusal after restart. + for (arn, version_id) in &entry.target_delete_marker_version_ids { + rstate + .target_delete_marker_version_ids + .entry(arn.clone()) + .or_insert_with(|| version_id.clone()); + } + rstate.target_delete_marker_version_ids_corrupt |= entry.target_delete_marker_version_ids_corrupt; let delete_marker_mtime = entry .delete_marker_mtime @@ -6601,4 +6615,87 @@ mod tests { replacement_data ); } + + /// backlog#2290: a delete-marker purge intent that survives a restart + /// through the MRF journal addresses the marker version the TARGET + /// assigned, exactly as the live watcher does (see the + /// `requires_delayed_purge` spawn). The journal carries the per-ARN ids + /// (`targetDeleteMarkerVersionIDs`) and replay restores them into the + /// reconstructed replication state; without that the replay would fall + /// back to the source marker id, which a target that mints its own ids + /// answers with an idempotent 204 — the entry would be acknowledged while + /// the real marker stayed behind. + #[test] + fn mrf_delete_marker_purge_replay_preserves_target_assigned_marker_version() { + use super::super::replication_object_decision_boundary::{delete_marker_purge_mrf_entry, delete_marker_purge_version_id}; + + let arn = "arn:minio:replication::generic-target:photos".to_string(); + let source_marker = uuid::Uuid::new_v4(); + let remote_marker = "remote-assigned-marker-version".to_string(); + + let live_oi = ObjectInfo { + bucket: "photos".to_string(), + name: "obj".to_string(), + version_id: Some(source_marker), + delete_marker: true, + ..Default::default() + }; + let mut live_state = live_oi.replication_state(); + live_state.replicate_decision_str = replicate_decision_for_admitted_targets(std::slice::from_ref(&arn)).to_string(); + live_state + .target_delete_marker_version_ids + .insert(arn.clone(), remote_marker.clone()); + let live = DeletedObjectReplicationInfo { + delete_object: ReplicationDeletedObject { + object_name: "obj".to_string(), + delete_marker: true, + delete_marker_version_id: Some(source_marker), + replication_state: Some(live_state), + ..Default::default() + }, + bucket: "photos".to_string(), + ..Default::default() + }; + assert_eq!( + delete_marker_purge_version_id(live.delete_object.replication_state.as_ref(), &arn, source_marker), + Some(Some(remote_marker.clone())), + "the live purge addresses the recorded target version" + ); + + // Watch window exhausted: persist the intent, restart, replay it. + let entry = delete_marker_purge_mrf_entry(&live, vec![arn.clone()]); + let replay_oi = ObjectInfo { + bucket: entry.bucket.clone(), + name: entry.object.clone(), + version_id: entry.version_id, + delete_marker: entry.delete_marker, + ..Default::default() + }; + let dsc = replicate_decision_for_admitted_targets(&entry.target_arns); + let replayed = reconstructed_heal_delete_info(&entry, &replay_oi, &dsc); + + assert_eq!( + delete_marker_purge_version_id(replayed.delete_object.replication_state.as_ref(), &arn, source_marker), + Some(Some(remote_marker)), + "the MRF replay must address the target-assigned marker version, not source marker {source_marker}" + ); + + // A refusal (inconsistent recorded ids) must stay a refusal across the + // journal round trip instead of degrading into the source-id fallback. + let mut refused = live; + refused + .delete_object + .replication_state + .as_mut() + .expect("state was set above") + .target_delete_marker_version_ids_corrupt = true; + let entry = delete_marker_purge_mrf_entry(&refused, vec![arn.clone()]); + assert!(entry.target_delete_marker_version_ids_corrupt); + let replayed = reconstructed_heal_delete_info(&entry, &replay_oi, &dsc); + assert_eq!( + delete_marker_purge_version_id(replayed.delete_object.replication_state.as_ref(), &arn, source_marker), + None, + "the MRF replay must keep refusing to guess when the recorded ids were inconsistent" + ); + } } diff --git a/crates/ecstore/src/bucket/replication/replication_resyncer.rs b/crates/ecstore/src/bucket/replication/replication_resyncer.rs index cf78dee70..64cbebeaa 100644 --- a/crates/ecstore/src/bucket/replication/replication_resyncer.rs +++ b/crates/ecstore/src/bucket/replication/replication_resyncer.rs @@ -32,11 +32,11 @@ use super::replication_msgp_boundary::ReplicationMsgpCodec; use super::replication_object_config::{ReplicationConfig, get_replication_config, must_replicate}; use super::replication_object_decision_boundary::{ MustReplicateOptions, ReplicationMultipartPartInput, delete_marker_purge_mrf_entry, delete_marker_purge_version_id, - delete_replication_creates_marker, heal_uses_delete_replication_path, is_object_lock_denied_delete, - is_retryable_delete_replication_head_error, is_version_delete_replication, replicate_delete_outcome, replication_etags_match, - replication_multipart_complete_actual_size, replication_multipart_part_plan, replication_single_put_size_error, - resync_existing_delete_replication_info, should_retry_delete_marker_purge, single_part_replica_etag_mismatch, - target_delete_version_id, + delete_replication_creates_marker, delete_replication_target_version_id, heal_uses_delete_replication_path, + is_object_lock_denied_delete, is_retryable_delete_replication_head_error, is_version_delete_replication, + replicate_delete_outcome, replication_etags_match, replication_multipart_complete_actual_size, + replication_multipart_part_plan, replication_single_put_size_error, resync_existing_delete_replication_info, + should_retry_delete_marker_purge, single_part_replica_etag_mismatch, }; use super::replication_queue_boundary::{DeletedObjectReplicationInfo, ReplicationQueueAdmission}; use super::replication_resync_boundary::ResyncStatusType; @@ -2051,7 +2051,11 @@ pub(crate) async fn replicate_delete_with_outcome( let is_version_purge = is_version_delete_replication(&dobj.delete_object); - let requires_delayed_purge = should_retry_delete_marker_purge(&dobj.delete_object); + // The watcher exists to purge a replicated marker once the SOURCE marker + // vanishes. A version purge is that purge already (its failures reach the + // journal as a purge entry), so it must not spawn a second watcher that + // journals a duplicate intent (backlog#2290). + let requires_delayed_purge = should_retry_delete_marker_purge(&dobj.delete_object) && !is_version_purge; let (replication_status, prev_status) = if !is_version_purge { ( @@ -2761,12 +2765,6 @@ fn unavailable_delete_target_info(dobj: &DeletedObjectReplicationInfo, arn: &str } async fn replicate_delete_to_target(dobj: &DeletedObjectReplicationInfo, tgt_client: Arc) -> ReplicatedTargetInfo { - let version_id = if let Some(version_id) = &dobj.delete_object.delete_marker_version_id { - version_id.to_owned() - } else { - dobj.delete_object.version_id.unwrap_or_default() - }; - let mut rinfo = dobj .delete_object .replication_state @@ -2799,7 +2797,25 @@ async fn replicate_delete_to_target(dobj: &DeletedObjectReplicationInfo, tgt_cli return rinfo; } - let version_id = target_delete_version_id(version_id, is_version_purge); + // Purging a replicated delete marker addresses the version the target + // assigned (recorded when the marker was created there); see + // `delete_replication_target_version_id`. A corrupt record is a failure, + // not a guess: the entry stays visible until the metadata is repaired. + let Some(version_id) = delete_replication_target_version_id(&dobj.delete_object, &tgt_client.arn) else { + warn!( + event = EVENT_DELETE_MARKER_PURGE_FAILED, + component = LOG_COMPONENT_ECSTORE, + subsystem = LOG_SUBSYSTEM_REPLICATION_RESYNC, + bucket = tgt_client.bucket, + object = dobj.delete_object.object_name, + arn = %tgt_client.arn, + reason = "recorded_target_version_inconsistent", + "Replicated version purge refused: recorded target delete-marker version metadata is inconsistent" + ); + rinfo.version_purge_status = VersionPurgeStatusType::Failed; + rinfo.error = Some("recorded target delete-marker version metadata is inconsistent".to_string()); + return rinfo; + }; if dobj.delete_object.delete_marker && dobj.delete_object.delete_marker_version_id.is_some() { match head_object_for_worker( diff --git a/crates/iam/src/manager.rs b/crates/iam/src/manager.rs index 384ed9a0a..a859fe59a 100644 --- a/crates/iam/src/manager.rs +++ b/crates/iam/src/manager.rs @@ -429,6 +429,27 @@ where } } + /// The cached mapping record for one user or group, looked up in the same + /// cache partition `policy_db_set` writes it to (group / STS / regular+service + /// user). `None` when no mapping is stored. + pub async fn get_mapped_policy_record(&self, name: &str, user_type: UserType, is_group: bool) -> Option { + let cache = self.cache.snapshot(); + if is_group { + cache.group_policies.get(name).cloned() + } else if user_type == UserType::Sts { + cache.sts_policies.get(name).cloned() + } else { + cache.user_policies.get(name).cloned() + } + } + + /// The cached group record (members, status, own timestamp) without the + /// mapped-policy overlay `get_group_description` applies. `None` when the + /// group does not exist. + pub async fn get_group_info(&self, name: &str) -> Option { + self.cache.snapshot().groups.get(name).cloned() + } + pub async fn get_policy(&self, name: &str) -> Result { if name.is_empty() { return Err(Error::InvalidArgument); @@ -534,6 +555,17 @@ where } pub async fn set_policy(&self, name: &str, policy: Policy) -> Result { + self.set_policy_at(name, policy, OffsetDateTime::now_utc()).await + } + + /// [`Self::set_policy`] stamping the document with `updated_at` instead + /// of the local clock. + /// + /// A site-replication receiver passes the edit's source time: the next + /// incoming revision is judged against the stored `UpdateDate`, so a + /// local stamp would reject a newer source edit that was merely delivered + /// later (backlog#2291). The returned stamp is the one persisted. + pub async fn set_policy_at(&self, name: &str, policy: Policy, updated_at: OffsetDateTime) -> Result { if name.is_empty() || policy.is_empty() { return Err(Error::InvalidArgument); } @@ -544,18 +576,17 @@ where .get(name) .map(|v| { let mut p = v.clone(); - p.update(policy.clone()); + p.update_at(policy.clone(), updated_at); p }) - .unwrap_or_else(|| PolicyDoc::new(policy)); + .unwrap_or_else(|| PolicyDoc::new_at(policy, updated_at)); self.api.save_policy_doc(name, policy_doc.clone()).await?; - let now = OffsetDateTime::now_utc(); + self.cache + .add_or_update_policy_doc(name, &policy_doc, OffsetDateTime::now_utc()); - self.cache.add_or_update_policy_doc(name, &policy_doc, now); - - Ok(now) + Ok(updated_at) } pub async fn list_policies(&self, bucket_name: &str) -> Result> { @@ -789,6 +820,12 @@ where /// create a service account and update cache pub async fn add_service_account(&self, cred: Credentials) -> Result { + self.add_service_account_at(cred, OffsetDateTime::now_utc()).await + } + + /// [`Self::add_service_account`] stamping the identity with `updated_at` + /// instead of the local clock; see [`Self::set_policy_at`] (backlog#2291). + pub async fn add_service_account_at(&self, cred: Credentials, updated_at: OffsetDateTime) -> Result { if cred.access_key.is_empty() || cred.parent_user.is_empty() { return Err(Error::InvalidArgument); } @@ -800,7 +837,8 @@ where } drop(cache); - let u = UserIdentity::new(cred); + let mut u = UserIdentity::new(cred); + u.update_at = Some(updated_at); self.api .save_user_identity(&u.credentials.access_key, UserType::Svc, u.clone(), None) @@ -808,10 +846,22 @@ where self.update_user_with_claims(&u.credentials.access_key, u.clone())?; - Ok(OffsetDateTime::now_utc()) + Ok(updated_at) } pub async fn update_service_account(&self, name: &str, opts: UpdateServiceAccountOpts) -> Result { + self.update_service_account_at(name, opts, OffsetDateTime::now_utc()).await + } + + /// [`Self::update_service_account`] stamping the identity with + /// `updated_at` instead of the local clock; see [`Self::set_policy_at`] + /// (backlog#2291). + pub async fn update_service_account_at( + &self, + name: &str, + opts: UpdateServiceAccountOpts, + updated_at: OffsetDateTime, + ) -> Result { let _mutation_guard = self.cache.service_account_mutation_lock().lock().await; let cache = self.cache.snapshot(); let Some(ui) = cache.users.get(name).cloned() else { @@ -858,13 +908,7 @@ where } if let Some(status) = opts.status { - match status.as_str() { - val if val == AccountStatus::Enabled.as_ref() => cr.status = auth::ACCOUNT_ON.to_owned(), - val if val == AccountStatus::Disabled.as_ref() => cr.status = auth::ACCOUNT_OFF.to_owned(), - auth::ACCOUNT_ON => cr.status = auth::ACCOUNT_ON.to_owned(), - auth::ACCOUNT_OFF => cr.status = auth::ACCOUNT_OFF.to_owned(), - _ => cr.status = auth::ACCOUNT_OFF.to_owned(), - } + cr.status = account_status_flag(&status).to_owned(); } let mut m: HashMap = if token_without_expiration { @@ -916,8 +960,8 @@ where cr.session_token = jwt_sign(&m, &cr.secret_key)?; - let u = UserIdentity::new(cr); - let updated_at = u.update_at.unwrap_or_else(OffsetDateTime::now_utc); + let mut u = UserIdentity::new(cr); + u.update_at = Some(updated_at); self.api .save_user_identity(&u.credentials.access_key, UserType::Svc, u.clone(), None) .await?; @@ -1149,6 +1193,20 @@ where Ok((policies.into_iter().collect(), update_at)) } pub async fn policy_db_set(&self, name: &str, user_type: UserType, is_group: bool, policy: &str) -> Result { + self.policy_db_set_at(name, user_type, is_group, policy, OffsetDateTime::now_utc()) + .await + } + + /// [`Self::policy_db_set`] stamping the mapping with `updated_at` instead + /// of the local clock; see [`Self::set_policy_at`] (backlog#2291). + pub async fn policy_db_set_at( + &self, + name: &str, + user_type: UserType, + is_group: bool, + policy: &str, + updated_at: OffsetDateTime, + ) -> Result { if name.is_empty() { return Err(Error::InvalidArgument); } @@ -1168,10 +1226,11 @@ where self.cache.delete_user_policy(name, OffsetDateTime::now_utc()); } - return Ok(OffsetDateTime::now_utc()); + return Ok(updated_at); } - let mp = MappedPolicy::new(policy); + let mut mp = MappedPolicy::new(policy); + mp.update_at = updated_at; let cache = self.cache.snapshot(); let policy_docs_cache = Arc::clone(&cache.policy_docs); @@ -1194,7 +1253,7 @@ where self.cache.add_or_update_user_policy(name, &mp, OffsetDateTime::now_utc()); } - Ok(OffsetDateTime::now_utc()) + Ok(updated_at) } pub async fn set_temp_user(&self, access_key: &str, cred: &Credentials, policy_name: Option<&str>) -> Result { @@ -1391,6 +1450,17 @@ where } pub async fn add_user(&self, access_key: &str, args: &AddOrUpdateUserReq) -> Result { + self.add_user_at(access_key, args, OffsetDateTime::now_utc()).await + } + + /// [`Self::add_user`] stamping the identity with `updated_at` instead of + /// the local clock; see [`Self::set_policy_at`] (backlog#2291). + pub async fn add_user_at( + &self, + access_key: &str, + args: &AddOrUpdateUserReq, + updated_at: OffsetDateTime, + ) -> Result { let cache = self.cache.snapshot(); let users = Arc::clone(&cache.users); if let Some(x) = users.get(access_key) { @@ -1408,12 +1478,13 @@ where _ => auth::ACCOUNT_OFF, } }; - let user_entry = UserIdentity::from(Credentials { + let mut user_entry = UserIdentity::from(Credentials { access_key: access_key.to_string(), secret_key: args.secret_key.to_string(), status: status.to_owned(), ..Default::default() }); + user_entry.update_at = Some(updated_at); self.api .save_user_identity(access_key, UserType::Reg, user_entry.clone(), None) @@ -1421,7 +1492,7 @@ where self.update_user_with_claims(access_key, user_entry)?; - Ok(OffsetDateTime::now_utc()) + Ok(updated_at) } pub async fn delete_user(&self, access_key: &str, utype: UserType) -> Result<()> { @@ -1599,6 +1670,17 @@ where } pub async fn set_user_status(&self, access_key: &str, status: AccountStatus) -> Result { + self.set_user_status_at(access_key, status, OffsetDateTime::now_utc()).await + } + + /// [`Self::set_user_status`] stamping the identity with `updated_at` + /// instead of the local clock; see [`Self::set_policy_at`] (backlog#2291). + pub async fn set_user_status_at( + &self, + access_key: &str, + status: AccountStatus, + updated_at: OffsetDateTime, + ) -> Result { if access_key.is_empty() { return Err(Error::InvalidArgument); } @@ -1625,12 +1707,13 @@ where } }; - let user_entry = UserIdentity::from(Credentials { + let mut user_entry = UserIdentity::from(Credentials { access_key: access_key.to_string(), secret_key: u.credentials.secret_key.clone(), status: status.to_owned(), ..Default::default() }); + user_entry.update_at = Some(updated_at); drop(cache); drop(users); @@ -1640,7 +1723,7 @@ where self.update_user_with_claims(access_key, user_entry)?; - Ok(OffsetDateTime::now_utc()) + Ok(updated_at) } fn update_user_with_claims(&self, k: &str, u: UserIdentity) -> Result<()> { @@ -1676,6 +1759,17 @@ where } pub async fn add_users_to_group(&self, group: &str, members: Vec) -> Result { + self.add_users_to_group_at(group, members, OffsetDateTime::now_utc()).await + } + + /// [`Self::add_users_to_group`] stamping the group with `updated_at` + /// instead of the local clock; see [`Self::set_policy_at`] (backlog#2291). + pub async fn add_users_to_group_at( + &self, + group: &str, + members: Vec, + updated_at: OffsetDateTime, + ) -> Result { if group.is_empty() { return Err(Error::InvalidArgument); } @@ -1693,6 +1787,14 @@ where } } + // The group's own timestamp moves with every membership or status + // change: site replication judges an incoming group item against it + // (backlog#2291), so it must reflect the last change, not creation. + // `updated_at` is the record's stamp only; the cache is published + // with the local clock, because `LockedCache::exec` drops a write + // whose time predates the entity's load time — a replicated edit + // whose source time is older than this node's startup would + // otherwise never reach the cache. let gi = match cache.groups.get(group) { Some(res) => { let mut gi = res.clone(); @@ -1701,15 +1803,20 @@ where uniq_set.extend(members.iter().cloned()); gi.members = uniq_set.into_iter().collect(); + gi.update_at = Some(updated_at); + gi + } + None => { + let mut gi = GroupInfo::new(members.clone()); + gi.update_at = Some(updated_at); gi } - None => GroupInfo::new(members.clone()), }; drop(cache); self.api.save_group_info(group, gi.clone()).await?; - let now = self.cache.with_write_lock(|cache| { + self.cache.with_write_lock(|cache| { let now = OffsetDateTime::now_utc(); cache.add_or_update_group(group, &gi, now); @@ -1719,13 +1826,18 @@ where m.insert(group.to_string()); cache.add_or_update_user_group_membership(member, &m, now); }); - now }); - Ok(now) + Ok(updated_at) } pub async fn set_group_status(&self, name: &str, enable: bool) -> Result { + self.set_group_status_at(name, enable, OffsetDateTime::now_utc()).await + } + + /// [`Self::set_group_status`] stamping the group with `updated_at` instead + /// of the local clock; see [`Self::set_policy_at`] (backlog#2291). + pub async fn set_group_status_at(&self, name: &str, enable: bool, updated_at: OffsetDateTime) -> Result { if name.is_empty() { return Err(Error::InvalidArgument); } @@ -1743,12 +1855,15 @@ where } else { gi.status = STATUS_DISABLED.to_owned(); } + gi.update_at = Some(updated_at); self.api.save_group_info(name, gi.clone()).await?; + // Cache publication time is the local clock, not the record stamp + // (see `add_users_to_group_at`). self.cache.add_or_update_group(name, &gi, OffsetDateTime::now_utc()); - Ok(OffsetDateTime::now_utc()) + Ok(updated_at) } pub async fn get_group_description(&self, name: &str) -> Result { @@ -1818,6 +1933,20 @@ where name: &str, members: Vec, update_cache_only: bool, + ) -> Result { + self.remove_members_from_group_at(name, members, update_cache_only, OffsetDateTime::now_utc()) + .await + } + + /// [`Self::remove_members_from_group`] stamping the group with + /// `updated_at` instead of the local clock; see [`Self::set_policy_at`] + /// (backlog#2291). + pub async fn remove_members_from_group_at( + &self, + name: &str, + members: Vec, + update_cache_only: bool, + updated_at: OffsetDateTime, ) -> Result { let cache = self.cache.snapshot(); let mut gi = cache @@ -1830,12 +1959,14 @@ where let s: HashSet<&String> = HashSet::from_iter(gi.members.iter()); let d: HashSet<&String> = HashSet::from_iter(members.iter()); gi.members = s.difference(&d).map(|v| v.to_string()).collect::>(); - + gi.update_at = Some(updated_at); if !update_cache_only { self.api.save_group_info(name, gi.clone()).await?; } - let now = self.cache.with_write_lock(|cache| { + self.cache.with_write_lock(|cache| { + // Sample after storage completes so a concurrent reload cannot + // make this publication older than the cache it must update. let now = OffsetDateTime::now_utc(); cache.add_or_update_group(name, &gi, now); @@ -1847,13 +1978,25 @@ where cache.add_or_update_user_group_membership(member, &m, now); } }); - now }); - Ok(now) + Ok(updated_at) } pub async fn remove_users_from_group(&self, group: &str, members: Vec) -> Result { + self.remove_users_from_group_at(group, members, OffsetDateTime::now_utc()) + .await + } + + /// [`Self::remove_users_from_group`] stamping the group with `updated_at` + /// instead of the local clock; a group delete (no members) leaves no + /// record and returns the stamp unchanged (backlog#2291). + pub async fn remove_users_from_group_at( + &self, + group: &str, + members: Vec, + updated_at: OffsetDateTime, + ) -> Result { if group.is_empty() { return Err(Error::InvalidArgument); } @@ -1902,18 +2045,17 @@ where return Err(err); } - let now = self.cache.with_write_lock(|cache| { + self.cache.with_write_lock(|cache| { let now = OffsetDateTime::now_utc(); self.remove_group_from_memberships_map_unlocked(cache, group, now); cache.delete_group(group, now); cache.delete_group_policy(group, now); - now }); - return Ok(now); + return Ok(updated_at); } - self.remove_members_from_group(group, members, false).await + self.remove_members_from_group_at(group, members, false, updated_at).await } fn remove_group_from_memberships_map_unlocked(&self, cache: &mut LockedCache, group: &str, now: OffsetDateTime) { @@ -2235,6 +2377,19 @@ where } } +/// The stored `status` flag for a service-account status given on the admin +/// or replication wire: the madmin `enabled` / `disabled` words and the stored +/// `on` / `off` flags are both accepted; anything else disables the account. +pub(crate) fn account_status_flag(status: &str) -> &'static str { + match status { + val if val == AccountStatus::Enabled.as_ref() => auth::ACCOUNT_ON, + val if val == AccountStatus::Disabled.as_ref() => auth::ACCOUNT_OFF, + auth::ACCOUNT_ON => auth::ACCOUNT_ON, + auth::ACCOUNT_OFF => auth::ACCOUNT_OFF, + _ => auth::ACCOUNT_OFF, + } +} + pub fn get_default_policies() -> HashMap { let default_policies = &DEFAULT_POLICIES; default_policies diff --git a/crates/iam/src/sys.rs b/crates/iam/src/sys.rs index cd38d5f4e..5b0a58c92 100644 --- a/crates/iam/src/sys.rs +++ b/crates/iam/src/sys.rs @@ -385,7 +385,14 @@ impl IamSys { } pub async fn set_policy(&self, name: &str, policy: Policy) -> Result { - let updated_at = self.store.set_policy(name, policy).await?; + self.set_policy_at(name, policy, OffsetDateTime::now_utc()).await + } + + /// [`Self::set_policy`] stamping the document with `updated_at` (a + /// replicated edit's source time) instead of the local clock; see + /// `IamCache::set_policy_at` (backlog#2291). + pub async fn set_policy_at(&self, name: &str, policy: Policy, updated_at: OffsetDateTime) -> Result { + let updated_at = self.store.set_policy_at(name, policy, updated_at).await?; if !self.has_watcher() { for r in notify_iam_load_policy(name).await { @@ -643,7 +650,18 @@ impl IamSys { } pub async fn set_user_status(&self, name: &str, status: rustfs_madmin::AccountStatus) -> Result { - let updated_at = self.store.set_user_status(name, status).await?; + self.set_user_status_at(name, status, OffsetDateTime::now_utc()).await + } + + /// [`Self::set_user_status`] stamping the identity with `updated_at` (a + /// replicated edit's source time) instead of the local clock (backlog#2291). + pub async fn set_user_status_at( + &self, + name: &str, + status: rustfs_madmin::AccountStatus, + updated_at: OffsetDateTime, + ) -> Result { + let updated_at = self.store.set_user_status_at(name, status, updated_at).await?; self.notify_for_user(name, false).await; @@ -655,6 +673,20 @@ impl IamSys { parent_user: &str, groups: Option>, opts: NewServiceAccountOpts, + ) -> Result<(Credentials, OffsetDateTime)> { + self.new_service_account_at(parent_user, groups, opts, OffsetDateTime::now_utc()) + .await + } + + /// [`Self::new_service_account`] stamping the identity with `updated_at` + /// (a replicated edit's source time) instead of the local clock + /// (backlog#2291). + pub async fn new_service_account_at( + &self, + parent_user: &str, + groups: Option>, + opts: NewServiceAccountOpts, + updated_at: OffsetDateTime, ) -> Result<(Credentials, OffsetDateTime)> { if parent_user.is_empty() { return Err(IamError::InvalidArgument); @@ -724,11 +756,18 @@ impl IamSys { let mut cred = create_new_credentials_with_metadata(&access_key, &secret_key, &m, &secret_key)?; cred.parent_user = parent_user.to_owned(); cred.groups = groups; - cred.status = ACCOUNT_ON.to_owned(); + // The status is part of the created identity: a replicated disabled + // account must never exist enabled, not even between a create and a + // follow-up status write (backlog#2289). + cred.status = opts + .status + .as_deref() + .map_or(ACCOUNT_ON, crate::manager::account_status_flag) + .to_owned(); cred.name = opts.name; cred.description = opts.description; - let create_at = self.store.add_service_account(cred.clone()).await?; + let create_at = self.store.add_service_account_at(cred.clone(), updated_at).await?; self.notify_for_service_account(&cred.access_key).await; @@ -736,11 +775,23 @@ impl IamSys { } pub async fn update_service_account(&self, name: &str, opts: UpdateServiceAccountOpts) -> Result { + self.update_service_account_at(name, opts, OffsetDateTime::now_utc()).await + } + + /// [`Self::update_service_account`] stamping the identity with + /// `updated_at` (a replicated edit's source time) instead of the local + /// clock (backlog#2291). + pub async fn update_service_account_at( + &self, + name: &str, + opts: UpdateServiceAccountOpts, + updated_at: OffsetDateTime, + ) -> Result { if name == SITE_REPLICATOR_SERVICE_ACCOUNT && !opts.allow_site_replicator_account { return Err(IamError::IAMActionNotAllowed); } - let updated_at = self.store.update_service_account(name, opts).await?; + let updated_at = self.store.update_service_account_at(name, opts, updated_at).await?; self.notify_for_service_account(name).await; @@ -940,6 +991,17 @@ impl IamSys { } pub async fn create_user(&self, access_key: &str, args: &AddOrUpdateUserReq) -> Result { + self.create_user_at(access_key, args, OffsetDateTime::now_utc()).await + } + + /// [`Self::create_user`] stamping the identity with `updated_at` (a + /// replicated edit's source time) instead of the local clock (backlog#2291). + pub async fn create_user_at( + &self, + access_key: &str, + args: &AddOrUpdateUserReq, + updated_at: OffsetDateTime, + ) -> Result { if !is_access_key_valid(access_key) { return Err(IamError::InvalidAccessKeyLength); } @@ -952,7 +1014,7 @@ impl IamSys { return Err(IamError::InvalidSecretKeyLength); } - let updated_at = self.store.add_user(access_key, args).await?; + let updated_at = self.store.add_user_at(access_key, args, updated_at).await?; self.load_user(access_key, UserType::Reg).await?; self.notify_for_user(access_key, false).await; @@ -1026,10 +1088,21 @@ impl IamSys { } pub async fn add_users_to_group(&self, group: &str, users: Vec) -> Result { + self.add_users_to_group_at(group, users, OffsetDateTime::now_utc()).await + } + + /// [`Self::add_users_to_group`] stamping the group with `updated_at` (a + /// replicated edit's source time) instead of the local clock (backlog#2291). + pub async fn add_users_to_group_at( + &self, + group: &str, + users: Vec, + updated_at: OffsetDateTime, + ) -> Result { if contains_reserved_chars(group) { return Err(IamError::GroupNameContainsReservedChars); } - let updated_at = self.store.add_users_to_group(group, users).await?; + let updated_at = self.store.add_users_to_group_at(group, users, updated_at).await?; self.notify_for_group(group).await; @@ -1037,7 +1110,19 @@ impl IamSys { } pub async fn remove_users_from_group(&self, group: &str, users: Vec) -> Result { - let updated_at = self.store.remove_users_from_group(group, users).await?; + self.remove_users_from_group_at(group, users, OffsetDateTime::now_utc()).await + } + + /// [`Self::remove_users_from_group`] stamping the group with `updated_at` + /// (a replicated edit's source time) instead of the local clock + /// (backlog#2291). + pub async fn remove_users_from_group_at( + &self, + group: &str, + users: Vec, + updated_at: OffsetDateTime, + ) -> Result { + let updated_at = self.store.remove_users_from_group_at(group, users, updated_at).await?; self.notify_for_group(group).await; @@ -1045,7 +1130,13 @@ impl IamSys { } pub async fn set_group_status(&self, group: &str, enable: bool) -> Result { - let updated_at = self.store.set_group_status(group, enable).await?; + self.set_group_status_at(group, enable, OffsetDateTime::now_utc()).await + } + + /// [`Self::set_group_status`] stamping the group with `updated_at` (a + /// replicated edit's source time) instead of the local clock (backlog#2291). + pub async fn set_group_status_at(&self, group: &str, enable: bool, updated_at: OffsetDateTime) -> Result { + let updated_at = self.store.set_group_status_at(group, enable, updated_at).await?; self.notify_for_group(group).await; @@ -1055,6 +1146,22 @@ impl IamSys { self.store.get_group_description(group).await } + /// The stored group record itself (see `IamCache::get_group_info`). + pub async fn get_group_info(&self, group: &str) -> Option { + self.store.get_group_info(group).await + } + + /// The stored policy document, `Error::NoSuchPolicy` when absent. + pub async fn get_policy_doc(&self, name: &str) -> Result { + self.store.get_policy_doc(name).await + } + + /// The stored mapping record for one user or group (see + /// `IamCache::get_mapped_policy_record`). + pub async fn get_mapped_policy_record(&self, name: &str, user_type: UserType, is_group: bool) -> Option { + self.store.get_mapped_policy_record(name, user_type, is_group).await + } + pub async fn list_groups_load(&self) -> Result> { self.store.update_groups().await } @@ -1064,7 +1171,24 @@ impl IamSys { } pub async fn policy_db_set(&self, name: &str, user_type: UserType, is_group: bool, policy: &str) -> Result { - let updated_at = self.store.policy_db_set(name, user_type, is_group, policy).await?; + self.policy_db_set_at(name, user_type, is_group, policy, OffsetDateTime::now_utc()) + .await + } + + /// [`Self::policy_db_set`] stamping the mapping with `updated_at` (a + /// replicated edit's source time) instead of the local clock (backlog#2291). + pub async fn policy_db_set_at( + &self, + name: &str, + user_type: UserType, + is_group: bool, + policy: &str, + updated_at: OffsetDateTime, + ) -> Result { + let updated_at = self + .store + .policy_db_set_at(name, user_type, is_group, policy, updated_at) + .await?; if !self.has_watcher() { for r in notify_iam_load_policy_mapping(name, user_type.to_u64(), is_group).await { @@ -1846,6 +1970,11 @@ pub struct NewServiceAccountOpts { pub expiration: Option, pub allow_site_replicator_account: bool, pub claims: Option>, + /// Status the account is created with (`enabled` / `disabled` or the + /// stored `on` / `off` flags); `None` creates it enabled. Site + /// replication passes the source account's status so a disabled account + /// is never enabled on the peer, not even transiently (backlog#2289). + pub status: Option, } pub struct UpdateServiceAccountOpts { @@ -2081,6 +2210,9 @@ mod tests { block_delete: Arc, delete_started: Arc, release_delete: Arc, + block_group_save: Arc, + group_save_started: Arc, + group_save_release: Arc, } impl StsTestMockStore { @@ -2094,6 +2226,9 @@ mod tests { block_delete: Arc::new(std::sync::atomic::AtomicBool::new(false)), delete_started: Arc::new(tokio::sync::Notify::new()), release_delete: Arc::new(tokio::sync::Notify::new()), + block_group_save: Arc::new(std::sync::atomic::AtomicBool::new(false)), + group_save_started: Arc::new(tokio::sync::Notify::new()), + group_save_release: Arc::new(tokio::sync::Notify::new()), } } @@ -2197,11 +2332,15 @@ mod tests { } async fn save_group_info(&self, _name: &str, _item: GroupInfo) -> Result<()> { - Err(Error::InvalidArgument) + if self.block_group_save.load(std::sync::atomic::Ordering::SeqCst) { + self.group_save_started.notify_one(); + self.group_save_release.notified().await; + } + Ok(()) } async fn delete_group_info(&self, _name: &str) -> Result<()> { - Err(Error::InvalidArgument) + Ok(()) } async fn load_group(&self, name: &str, m: &mut HashMap) -> Result<()> { @@ -2378,6 +2517,140 @@ mod tests { IamSys::new(cache) } + async fn assert_group_write_during_reload_is_published(remove: bool) { + let iam_sys = Arc::new(temp_env::async_with_vars([("RUSTFS_SKIP_BACKGROUND_TASK", Some("1"))], test_iam_sys()).await); + let member = "sts-fallback-test-parent"; + let group = if remove { "testgroup" } else { "new-published-group" }; + let source_time = OffsetDateTime::now_utc() - time::Duration::hours(1); + iam_sys + .store + .api + .block_group_save + .store(true, std::sync::atomic::Ordering::SeqCst); + let before = iam_sys.store.cache.snapshot(); + let writer_iam = iam_sys.clone(); + let writer = tokio::spawn(async move { + if remove { + writer_iam + .remove_users_from_group_at(group, vec![member.to_string()], source_time) + .await + } else { + writer_iam + .add_users_to_group_at(group, vec![member.to_string()], source_time) + .await + } + }); + tokio::time::timeout(std::time::Duration::from_secs(5), iam_sys.store.api.group_save_started.notified()) + .await + .expect("group save should reach the barrier"); + // The pending store write has not changed the cache, so the production + // full-reload snapshot guard permits this replacement. + assert!(iam_sys.store.cache.with_write_lock(|cache| cache.matches_snapshot(&before))); + iam_sys + .store + .api + .load_all(&iam_sys.store.cache) + .await + .expect("reload while group save is pending"); + iam_sys.store.api.group_save_release.notify_one(); + assert_eq!(writer.await.expect("join group writer").expect("group write should succeed"), source_time); + let info = iam_sys + .get_group_info(group) + .await + .expect("successful group write must remain readable after reload"); + assert_eq!(info.update_at, Some(source_time), "source timestamp must remain on the record"); + assert_eq!(info.members, if remove { Vec::new() } else { vec![member.to_string()] }); + let groups = iam_sys.store.cache.snapshot().user_group_memberships.get(member).cloned(); + assert_eq!( + groups.is_some_and(|groups| groups.contains(group)), + !remove, + "membership index must reflect the write" + ); + } + + #[tokio::test] + #[serial] + async fn add_group_write_during_reload_publishes_after_store_save() { + assert_group_write_during_reload_is_published(false).await; + } + + #[tokio::test] + #[serial] + async fn remove_group_write_during_reload_publishes_after_store_save() { + assert_group_write_during_reload_is_published(true).await; + } + + /// Review finding on rustfs#7195: a replicated group edit carries a source + /// stamp that may predate this node's cache load time. The stamp belongs on + /// the record only; publishing the cache with it makes `LockedCache::exec` + /// drop the write, so the group is written to the store but unreadable + /// here and the receiver's next `set_group_status_at` fails with + /// `NoSuchGroup`. Add, status and removal must all publish with the local + /// clock while keeping the source stamp on `GroupInfo::update_at`. + #[tokio::test] + async fn group_writes_stamped_before_the_cache_load_time_still_publish() { + let iam_sys = test_iam_sys().await; + let member = "group-stamp-member"; + let identity = UserIdentity { + version: 1, + credentials: Credentials { + access_key: member.to_string(), + secret_key: "longenoughsecret".to_string(), + status: "on".to_string(), + ..Default::default() + }, + update_at: Some(OffsetDateTime::now_utc()), + }; + iam_sys.store.cache.with_write_lock(|cache| { + cache.add_or_update_user(member, &identity, OffsetDateTime::now_utc()); + // The startup load publishes every entity with the load time. + cache.replace_groups(CacheEntity::new(HashMap::new())); + cache.replace_user_group_memberships(CacheEntity::new(HashMap::new())); + }); + + let group = "group-stamp"; + let source_time = OffsetDateTime::now_utc() - time::Duration::hours(1); + let stamped = iam_sys + .add_users_to_group_at(group, vec![member.to_string()], source_time) + .await + .expect("add members with a source stamp older than the cache load"); + assert_eq!(stamped, source_time, "the returned stamp is the source time"); + let info = iam_sys + .get_group_info(group) + .await + .expect("the group must be readable right after the add"); + assert_eq!(info.members, vec![member.to_string()]); + assert_eq!(info.update_at, Some(source_time), "the record keeps the source stamp"); + let memberships = iam_sys.store.cache.snapshot().user_group_memberships.get(member).cloned(); + assert!( + memberships.is_some_and(|groups| groups.contains(group)), + "the membership index is published too" + ); + + let disabled_at = source_time + time::Duration::seconds(1); + iam_sys + .set_group_status_at(group, false, disabled_at) + .await + .expect("status change with a source stamp older than the cache load"); + let info = iam_sys.get_group_info(group).await.expect("group after status change"); + assert_eq!(info.status, "disabled"); + assert_eq!(info.update_at, Some(disabled_at)); + + let removed_at = source_time + time::Duration::seconds(2); + iam_sys + .remove_users_from_group_at(group, vec![member.to_string()], removed_at) + .await + .expect("removal with a source stamp older than the cache load"); + let info = iam_sys.get_group_info(group).await.expect("group after removal"); + assert!(info.members.is_empty(), "the removal must be visible in the cache"); + assert_eq!(info.update_at, Some(removed_at)); + let memberships = iam_sys.store.cache.snapshot().user_group_memberships.get(member).cloned(); + assert!( + !memberships.is_some_and(|groups| groups.contains(group)), + "the membership index follows the removal" + ); + } + fn service_account_opts(access_key: &str, secret_key: &str) -> NewServiceAccountOpts { NewServiceAccountOpts { access_key: access_key.to_string(), diff --git a/crates/policy/src/policy/doc.rs b/crates/policy/src/policy/doc.rs index dc2b83fa9..30ef4ca03 100644 --- a/crates/policy/src/policy/doc.rs +++ b/crates/policy/src/policy/doc.rs @@ -45,18 +45,33 @@ pub struct PolicyDoc { impl PolicyDoc { pub fn new(policy: Policy) -> Self { + Self::new_at(policy, OffsetDateTime::now_utc()) + } + + /// [`Self::new`] with an explicit `UpdateDate` (and `CreateDate`). + /// + /// A replicated document keeps the edit's source time: the receiver + /// judges the next incoming revision against the stored stamp, so a + /// local stamp would reject a newer source edit that was merely + /// delivered later. + pub fn new_at(policy: Policy, at: OffsetDateTime) -> Self { Self { version: 1, policy, - create_date: Some(OffsetDateTime::now_utc()), - update_date: Some(OffsetDateTime::now_utc()), + create_date: Some(at), + update_date: Some(at), } } pub fn update(&mut self, policy: Policy) { + self.update_at(policy, OffsetDateTime::now_utc()); + } + + /// [`Self::update`] with an explicit `UpdateDate`; see [`Self::new_at`]. + pub fn update_at(&mut self, policy: Policy, at: OffsetDateTime) { self.version += 1; self.policy = policy; - self.update_date = Some(OffsetDateTime::now_utc()); + self.update_date = Some(at); if self.create_date.is_none() { self.create_date = self.update_date; diff --git a/crates/replication/src/delete.rs b/crates/replication/src/delete.rs index ffd014796..88a492533 100644 --- a/crates/replication/src/delete.rs +++ b/crates/replication/src/delete.rs @@ -76,6 +76,21 @@ impl ReplicationWorkerOperation for DeletedObjectReplicationInfo { .delete_object .delete_marker_mtime .and_then(|t| i64::try_from(t.unix_timestamp_nanos()).ok()), + // Carry the target-assigned marker version ids (and the fail-closed corrupt + // flag) into the journal so a purge intent replayed after a restart addresses + // the same version the live path did (backlog#2290). Only delete-marker state + // ever records these; other deletes serialize an empty map. + target_delete_marker_version_ids: self + .delete_object + .replication_state + .as_ref() + .map(|state| state.target_delete_marker_version_ids.clone()) + .unwrap_or_default(), + target_delete_marker_version_ids_corrupt: self + .delete_object + .replication_state + .as_ref() + .is_some_and(|state| state.target_delete_marker_version_ids_corrupt), target_arns: self.admitted_target_arns(), force_delete_id: self.delete_object.force_delete_id, force_delete_generation: self.delete_object.force_delete_generation, @@ -238,6 +253,28 @@ pub fn delete_marker_purge_version_id( }) } +/// The version a delete replication addresses on `arn`, or `None` to refuse. +/// +/// A version purge whose purged version is a delete marker must address the +/// marker version the TARGET assigned — the recorded mapping, exactly as the +/// delayed-purge watcher does. The source-side `DELETE ?versionId=` +/// replicates as such a purge, and a generic S3 target answers a DELETE of an +/// unknown versionId with 204 while keeping its marker, so addressing it by +/// the source id reported success and left the marker behind (backlog#2290, +/// R6.1 on the VMs). Nothing recorded falls back to the source-derived id +/// (id-mirroring peers); a corrupt record refuses, as the watcher does. +pub fn delete_replication_target_version_id(dobj: &DeletedObject, arn: &str) -> Option> { + let is_version_purge = is_version_delete_replication(dobj); + if is_version_purge + && !dobj.delete_marker + && let Some(marker) = dobj.delete_marker_version_id + { + return delete_marker_purge_version_id(dobj.replication_state.as_ref(), arn, marker); + } + let source_version = dobj.delete_marker_version_id.or(dobj.version_id).unwrap_or_default(); + Some(target_delete_version_id(source_version, is_version_purge)) +} + /// Shape an exhausted purge intent as a marker-creation delete entry. Replay /// reconstructs it with `delete_marker: true`, finds the source marker gone, /// and funnels into the stale-marker branch of `replicate_delete_with_outcome` @@ -258,9 +295,9 @@ mod tests { use super::{ DeletedObjectReplicationInfo, delete_marker_purge_mrf_entry, delete_marker_purge_version_id, - delete_replication_creates_marker, is_object_lock_denied_delete, is_retryable_delete_replication_head_error, - is_version_delete_replication, replicate_delete_outcome, resync_existing_delete_replication_info, - should_retry_delete_marker_purge, target_delete_version_id, + delete_replication_creates_marker, delete_replication_target_version_id, is_object_lock_denied_delete, + is_retryable_delete_replication_head_error, is_version_delete_replication, replicate_delete_outcome, + resync_existing_delete_replication_info, should_retry_delete_marker_purge, target_delete_version_id, }; use crate::storage_api::DeletedObject; use crate::{ @@ -595,6 +632,76 @@ mod tests { assert_eq!(entry.retry_count, 0); assert_eq!(entry.bucket, "bucket-a"); assert_eq!(entry.object, "doc.txt"); + assert!( + entry.target_delete_marker_version_ids.is_empty(), + "no recorded target marker ids means the journal carries none" + ); + assert!(!entry.target_delete_marker_version_ids_corrupt); + } + + /// backlog#2290: a purge intent journaled to MRF must carry the marker + /// version ids the targets assigned, plus the fail-closed corrupt flag, + /// so a replay after restart addresses the same version the live path did. + #[test] + fn delete_marker_purge_mrf_entry_carries_target_assigned_marker_versions() { + let delete_marker_version_id = Uuid::new_v4(); + let mut state = ReplicationState::default(); + state + .target_delete_marker_version_ids + .insert("arn:a".to_string(), "remote-marker-a".to_string()); + state + .target_delete_marker_version_ids + .insert("arn:b".to_string(), "remote-marker-b".to_string()); + let mut dobj = DeletedObjectReplicationInfo { + delete_object: DeletedObject { + object_name: "doc.txt".to_string(), + delete_marker: false, + version_id: Some(Uuid::new_v4()), + delete_marker_version_id: Some(delete_marker_version_id), + replication_state: Some(state), + ..Default::default() + }, + bucket: "bucket-a".to_string(), + ..Default::default() + }; + + let entry = delete_marker_purge_mrf_entry(&dobj, vec!["arn:a".to_string()]); + assert_eq!( + entry.target_delete_marker_version_ids, + HashMap::from([ + ("arn:a".to_string(), "remote-marker-a".to_string()), + ("arn:b".to_string(), "remote-marker-b".to_string()), + ]), + "every recorded target marker id survives the journal, regardless of the retried ARN subset" + ); + assert!(!entry.target_delete_marker_version_ids_corrupt); + assert_eq!( + delete_marker_purge_version_id( + Some(&ReplicationState { + target_delete_marker_version_ids: entry.target_delete_marker_version_ids, + ..Default::default() + }), + "arn:a", + delete_marker_version_id + ), + Some(Some("remote-marker-a".to_string())) + ); + + // The live path refuses to purge on inconsistent metadata and reports the target + // as failed; the journaled intent must keep refusing after a restart. + dobj.delete_object + .replication_state + .as_mut() + .expect("state was set above") + .target_delete_marker_version_ids_corrupt = true; + let entry = delete_marker_purge_mrf_entry(&dobj, vec!["arn:a".to_string()]); + assert!(entry.target_delete_marker_version_ids_corrupt); + + // A delete without replication state journals an empty map. + dobj.delete_object.replication_state = None; + let entry = dobj.to_mrf_entry(); + assert!(entry.target_delete_marker_version_ids.is_empty()); + assert!(!entry.target_delete_marker_version_ids_corrupt); } #[test] @@ -656,4 +763,57 @@ mod tests { assert!(!is_object_lock_denied_delete(Some("InternalError"), Some("retention lookup failed"))); assert!(!is_object_lock_denied_delete(None, Some("legal hold"))); } + + fn purge_of_marker(marker: Uuid, state: Option) -> DeletedObject { + DeletedObject { + object_name: "obj".to_string(), + delete_marker: false, + delete_marker_version_id: Some(marker), + version_id: None, + replication_state: state, + ..Default::default() + } + } + + #[test] + fn delete_replication_target_version_id_addresses_recorded_marker_for_purges() { + let arn = "arn:minio:replication::generic:photos"; + let marker = Uuid::new_v4(); + let mut state = ReplicationState::default(); + state + .target_delete_marker_version_ids + .insert(arn.to_string(), "remote-marker".to_string()); + + // purge of a replicated marker: the target's own version + assert_eq!( + delete_replication_target_version_id(&purge_of_marker(marker, Some(state.clone())), arn), + Some(Some("remote-marker".to_string())) + ); + // nothing recorded for this arn: the source-derived id (id-mirroring peers) + assert_eq!( + delete_replication_target_version_id(&purge_of_marker(marker, None), arn), + Some(Some(marker.to_string())) + ); + // corrupt record: refuse instead of guessing + state.target_delete_marker_version_ids_corrupt = true; + assert_eq!(delete_replication_target_version_id(&purge_of_marker(marker, Some(state)), arn), None); + + // marker creation keeps the source id (the target mints its own on a + // versionless DELETE; the id only travels in the source header) + let creation = DeletedObject { + object_name: "obj".to_string(), + delete_marker: true, + delete_marker_version_id: Some(marker), + ..Default::default() + }; + assert_eq!(delete_replication_target_version_id(&creation, arn), Some(Some(marker.to_string()))); + // plain version purge: the source version id + let version = Uuid::new_v4(); + let purge = DeletedObject { + object_name: "obj".to_string(), + version_id: Some(version), + ..Default::default() + }; + assert_eq!(delete_replication_target_version_id(&purge, arn), Some(Some(version.to_string()))); + } } diff --git a/crates/replication/src/filemeta.rs b/crates/replication/src/filemeta.rs index 7af1b3141..555b58c57 100644 --- a/crates/replication/src/filemeta.rs +++ b/crates/replication/src/filemeta.rs @@ -641,6 +641,26 @@ pub struct MrfReplicateEntry { #[serde(rename = "deleteMarkerMtime", skip_serializing_if = "Option::is_none", default)] pub delete_marker_mtime: Option, + // For delete-marker purge intents: the exact version id each target assigned to the + // replicated marker, keyed by target ARN. A generic S3 target mints its own version ids + // and answers a DELETE of an unknown id with 204, so a replay that fell back to the source + // marker id would be acknowledged while the real marker stayed behind (backlog#2290). + // Old files lack this key; default=empty means "unknown" and replay keeps the source-id + // fallback it always had. + #[serde(rename = "targetDeleteMarkerVersionIDs", skip_serializing_if = "HashMap::is_empty", default)] + pub target_delete_marker_version_ids: HashMap, + + // Companion to the map above: the source metadata disagreed about the recorded ids when + // the intent was journaled, so the live path refused to guess and reported the target as + // failed. Replay must keep refusing instead of falling back to the source id. Old files + // lack this key; default=false. + #[serde( + rename = "targetDeleteMarkerVersionIDsCorrupt", + skip_serializing_if = "std::ops::Not::not", + default + )] + pub target_delete_marker_version_ids_corrupt: bool, + #[serde(rename = "targetARNs", skip_serializing_if = "Vec::is_empty", default)] pub target_arns: Vec, diff --git a/crates/replication/src/lib.rs b/crates/replication/src/lib.rs index 1aa20daca..295340026 100644 --- a/crates/replication/src/lib.rs +++ b/crates/replication/src/lib.rs @@ -41,9 +41,9 @@ pub use config::{ }; pub use delete::{ DeletedObjectReplicationInfo, delete_marker_purge_mrf_entry, delete_marker_purge_version_id, - delete_replication_creates_marker, is_object_lock_denied_delete, is_retryable_delete_replication_head_error, - is_version_delete_replication, replicate_delete_outcome, resync_existing_delete_replication_info, - should_retry_delete_marker_purge, target_delete_version_id, + delete_replication_creates_marker, delete_replication_target_version_id, is_object_lock_denied_delete, + is_retryable_delete_replication_head_error, is_version_delete_replication, replicate_delete_outcome, + resync_existing_delete_replication_info, should_retry_delete_marker_purge, target_delete_version_id, }; pub use filemeta::{ NULL_VERSION_ID, REPLICATE_EXISTING, REPLICATE_EXISTING_DELETE, REPLICATE_HEAL, REPLICATE_HEAL_DELETE, REPLICATE_INCOMING, diff --git a/crates/replication/src/mrf.rs b/crates/replication/src/mrf.rs index 8698e7c2a..285c8bf82 100644 --- a/crates/replication/src/mrf.rs +++ b/crates/replication/src/mrf.rs @@ -31,8 +31,13 @@ const CAPABILITY_OPERATION_KIND: u64 = 1 << 0; const CAPABILITY_TARGET_ARNS: u64 = 1 << 1; const CAPABILITY_FORCE_DELETE: u64 = 1 << 2; const CAPABILITY_DELETE_MARKER_MTIME: u64 = 1 << 3; -const MRF_KNOWN_CAPABILITIES: u64 = - CAPABILITY_OPERATION_KIND | CAPABILITY_TARGET_ARNS | CAPABILITY_FORCE_DELETE | CAPABILITY_DELETE_MARKER_MTIME; +// Per-ARN target-assigned delete-marker version ids on purge intents (backlog#2290). +const CAPABILITY_TARGET_DELETE_MARKER_VERSION_IDS: u64 = 1 << 4; +const MRF_KNOWN_CAPABILITIES: u64 = CAPABILITY_OPERATION_KIND + | CAPABILITY_TARGET_ARNS + | CAPABILITY_FORCE_DELETE + | CAPABILITY_DELETE_MARKER_MTIME + | CAPABILITY_TARGET_DELETE_MARKER_VERSION_IDS; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum MrfCapability { @@ -40,6 +45,7 @@ pub enum MrfCapability { TargetArns, ForceDelete, DeleteMarkerMtime, + TargetDeleteMarkerVersionIds, } impl MrfCapability { @@ -49,6 +55,7 @@ impl MrfCapability { Self::TargetArns => CAPABILITY_TARGET_ARNS, Self::ForceDelete => CAPABILITY_FORCE_DELETE, Self::DeleteMarkerMtime => CAPABILITY_DELETE_MARKER_MTIME, + Self::TargetDeleteMarkerVersionIds => CAPABILITY_TARGET_DELETE_MARKER_VERSION_IDS, } } } @@ -601,9 +608,17 @@ pub fn decode_mrf_file(data: &[u8]) -> Result> { #[cfg(test)] mod tests { use super::*; + use std::collections::HashMap; use uuid::Uuid; + // Capability word 31 = OperationKind | TargetArns | ForceDelete | DeleteMarkerMtime | + // TargetDeleteMarkerVersionIds (backlog#2290). const ENVELOPE_FIXTURE: &[u8] = &[ + b'M', b'R', b'F', b'E', 1, 0, 1, 0, 1, 0, 0, 0, 31, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 1, 2, 3, + ]; + + // The envelope a binary from before backlog#2290 writes: same header, capability word 15. + const PRE_TARGET_MARKER_IDS_ENVELOPE_FIXTURE: &[u8] = &[ b'M', b'R', b'F', b'E', 1, 0, 1, 0, 1, 0, 0, 0, 15, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 1, 2, 3, ]; @@ -626,6 +641,8 @@ mod tests { delete_marker_version_id: None, delete_marker: false, delete_marker_mtime: None, + target_delete_marker_version_ids: HashMap::new(), + target_delete_marker_version_ids_corrupt: false, target_arns: vec!["arn:target-a".to_string()], }, MrfReplicateEntry { @@ -642,6 +659,8 @@ mod tests { delete_marker_version_id: None, delete_marker: false, delete_marker_mtime: None, + target_delete_marker_version_ids: HashMap::new(), + target_delete_marker_version_ids_corrupt: false, target_arns: vec!["arn:target-a".to_string(), "arn:target-b".to_string()], }, MrfReplicateEntry { @@ -658,6 +677,11 @@ mod tests { delete_marker_version_id: Some(del_vid), delete_marker: true, delete_marker_mtime: Some(1_705_312_200_123_456_789), + target_delete_marker_version_ids: HashMap::from([ + ("arn:target-a".to_string(), "remote-marker-a".to_string()), + ("arn:target-b".to_string(), "remote-marker-b".to_string()), + ]), + target_delete_marker_version_ids_corrupt: false, target_arns: vec!["arn:target-a".to_string()], }, ]; @@ -685,6 +709,54 @@ mod tests { Some(1_705_312_200_123_456_789), "delete-marker mtime must survive the MRF disk round-trip" ); + assert!(decoded[0].target_delete_marker_version_ids.is_empty()); + assert!(decoded[1].target_delete_marker_version_ids.is_empty()); + assert_eq!( + decoded[2].target_delete_marker_version_ids, + HashMap::from([ + ("arn:target-a".to_string(), "remote-marker-a".to_string()), + ("arn:target-b".to_string(), "remote-marker-b".to_string()), + ]), + "target-assigned marker version ids must survive the MRF disk round-trip (backlog#2290)" + ); + assert!(!decoded[2].target_delete_marker_version_ids_corrupt); + } + + /// backlog#2290: the corrupt flag rides the same journal round trip, and an + /// entry that carries neither field encodes exactly as it did before the + /// field existed (both keys are skipped when empty/false). + #[test] + fn mrf_file_round_trips_target_marker_ids_corrupt_flag_and_skips_empty_keys() { + let corrupt = MrfReplicateEntry { + bucket: "bucket-a".to_string(), + object: "delete-a".to_string(), + op: MrfOpKind::Delete, + delete_marker: true, + delete_marker_version_id: Some(Uuid::new_v4()), + target_delete_marker_version_ids_corrupt: true, + target_arns: vec!["arn:target-a".to_string()], + ..Default::default() + }; + let decoded = decode_mrf_file(&encode_mrf_file(std::slice::from_ref(&corrupt)).expect("mrf file should encode")) + .expect("mrf file should decode"); + assert_eq!(decoded, vec![corrupt]); + assert!(decoded[0].target_delete_marker_version_ids_corrupt); + + let plain = MrfReplicateEntry { + bucket: "bucket-a".to_string(), + object: "delete-a".to_string(), + op: MrfOpKind::Delete, + delete_marker: true, + target_arns: vec!["arn:target-a".to_string()], + ..Default::default() + }; + let encoded = encode_mrf_file(std::slice::from_ref(&plain)).expect("mrf file should encode"); + let payload = String::from_utf8_lossy(&encoded); + assert!( + !payload.contains("targetDeleteMarkerVersionIDs"), + "an entry without recorded ids must not grow the new keys: {payload}" + ); + assert_eq!(decode_mrf_file(&encoded).expect("mrf file should decode"), vec![plain]); } #[test] @@ -719,6 +791,99 @@ mod tests { // Old files lack the deleteMarkerMtime key; it must default to None so replay keeps the // pre-#867 fallback to the current time. assert_eq!(decoded[0].delete_marker_mtime, None); + // Old files also lack the target marker id keys; they must default to an empty map + // and a clear corrupt flag so replay keeps the pre-#2290 source-id fallback. + assert!(decoded[0].target_delete_marker_version_ids.is_empty()); + assert!(!decoded[0].target_delete_marker_version_ids_corrupt); + } + + /// backlog#2290: a delete-marker entry written by a binary that predates the + /// `targetDeleteMarkerVersionIDs` key decodes with an empty map and a clear + /// corrupt flag — the exact shape replay handled before the field existed. + #[test] + fn mrf_pre_target_marker_ids_delete_entry_decodes_with_empty_map() { + let marker_version_id = Uuid::new_v4(); + let mut payload = Vec::new(); + rmp::encode::write_array_len(&mut payload, 1).expect("array len should encode"); + rmp::encode::write_map_len(&mut payload, 9).expect("map len should encode"); + rmp::encode::write_str(&mut payload, "bucket").expect("bucket key should encode"); + rmp::encode::write_str(&mut payload, "old-bucket").expect("bucket value should encode"); + rmp::encode::write_str(&mut payload, "object").expect("object key should encode"); + rmp::encode::write_str(&mut payload, "old-key").expect("object value should encode"); + rmp::encode::write_str(&mut payload, "retryCount").expect("retry key should encode"); + rmp::encode::write_i32(&mut payload, 0).expect("retry value should encode"); + rmp::encode::write_str(&mut payload, "size").expect("size key should encode"); + rmp::encode::write_i64(&mut payload, 0).expect("size value should encode"); + rmp::encode::write_str(&mut payload, "op").expect("op key should encode"); + rmp::encode::write_str(&mut payload, "delete").expect("op value should encode"); + rmp::encode::write_str(&mut payload, "forceDelete").expect("forceDelete key should encode"); + rmp::encode::write_bool(&mut payload, false).expect("forceDelete value should encode"); + rmp::encode::write_str(&mut payload, "deleteMarkerVersionID").expect("marker id key should encode"); + // Uuid serializes as a 16-byte bin in the MessagePack journal. + rmp::encode::write_bin(&mut payload, marker_version_id.as_bytes()).expect("marker id value should encode"); + rmp::encode::write_str(&mut payload, "deleteMarker").expect("deleteMarker key should encode"); + rmp::encode::write_bool(&mut payload, true).expect("deleteMarker value should encode"); + rmp::encode::write_str(&mut payload, "targetARNs").expect("targetARNs key should encode"); + rmp::encode::write_array_len(&mut payload, 1).expect("targetARNs len should encode"); + rmp::encode::write_str(&mut payload, "arn:target-a").expect("targetARNs value should encode"); + + let mut data = Vec::with_capacity(4 + payload.len()); + data.extend_from_slice(&MRF_META_FORMAT.to_le_bytes()); + data.extend_from_slice(&MRF_META_VERSION.to_le_bytes()); + data.extend_from_slice(&payload); + + let decoded = decode_mrf_file(&data).expect("pre-#2290 delete-marker entry should decode"); + + assert_eq!(decoded.len(), 1); + assert_eq!(decoded[0].op, MrfOpKind::Delete); + assert!(decoded[0].delete_marker); + assert_eq!(decoded[0].delete_marker_version_id, Some(marker_version_id)); + assert_eq!(decoded[0].target_arns, vec!["arn:target-a".to_string()]); + assert!(decoded[0].target_delete_marker_version_ids.is_empty()); + assert!(!decoded[0].target_delete_marker_version_ids_corrupt); + } + + /// backlog#2290: the new field is fenced by its own capability bit exactly + /// like the earlier optional fields — a reader without the bit refuses an + /// envelope that advertises it, while the current reader still accepts the + /// pre-#2290 envelope. + #[test] + fn envelope_target_marker_ids_capability_is_fenced_and_backward_compatible() { + assert!(MrfCapabilities::current().contains(MrfCapability::TargetDeleteMarkerVersionIds)); + assert_eq!(MrfCapabilities::with(MrfCapability::TargetDeleteMarkerVersionIds).bits(), 1 << 4); + + // Old envelope, current reader: accepted, and the negotiated set lacks the new bit. + let legacy = MrfEnvelope::decode(PRE_TARGET_MARKER_IDS_ENVELOPE_FIXTURE, MrfProtocolCapabilities::current()) + .expect("pre-#2290 envelope should decode"); + assert_eq!(legacy.protocol().capabilities().bits(), 15); + assert!( + !legacy + .protocol() + .capabilities() + .contains(MrfCapability::TargetDeleteMarkerVersionIds) + ); + assert_eq!(legacy.payload(), &[1, 2, 3]); + + // Current envelope, reader that only knows the pre-#2290 bits: refused. + let pre_2290_reader = MrfProtocolCapabilities::new(1, 1, MrfCapabilities::from_bits(15).expect("known bits")); + assert_eq!( + MrfEnvelope::decode(ENVELOPE_FIXTURE, pre_2290_reader), + Err(MrfEnvelopeError::MissingCapabilities { + required: 31, + available: 15, + }) + ); + + // Negotiation with such a peer drops the bit instead of failing. + let negotiated = MrfProtocolCapabilities::current() + .negotiate(pre_2290_reader) + .expect("negotiation with a pre-#2290 peer should succeed"); + assert!( + !negotiated + .capabilities() + .contains(MrfCapability::TargetDeleteMarkerVersionIds) + ); + assert!(negotiated.capabilities().contains(MrfCapability::DeleteMarkerMtime)); } #[test] diff --git a/rustfs/src/admin/handlers/site_replication.rs b/rustfs/src/admin/handlers/site_replication.rs index 51c7dda99..318c8a093 100644 --- a/rustfs/src/admin/handlers/site_replication.rs +++ b/rustfs/src/admin/handlers/site_replication.rs @@ -66,8 +66,8 @@ use rustfs_madmin::{ ReplicateRemoveStatus, ResyncBucketStatus, SITE_REPL_API_VERSION, SR_IAM_ITEM_STS_ACC, SR_IAM_ITEM_STS_ACC_LEGACY, SRBucketInfo, SRBucketMeta, SRBucketStatsSummary, SRGroupInfo, SRGroupStatsSummary, SRIAMItem, SRIAMUser, SRILMExpiryStatsSummary, SRInfo, SRMetric, SRMetricsSummary, SRPeerError, SRPeerJoinReq, SRPendingOperation, SRPolicyMapping, - SRPolicyStatsSummary, SRRemoveReq, SRResyncOpStatus, SRSTSCredential, SRSessionPolicy, SRSiteSummary, SRStateEditReq, - SRStateInfo, SRStatusInfo, SRSvcAccChange, SRSvcAccCreate, SRUserStatsSummary, SiteReplicationInfo, SyncStatus, WorkerStat, + SRPolicyStatsSummary, SRRemoveReq, SRResyncOpStatus, SRSTSCredential, SRSiteSummary, SRStateEditReq, SRStateInfo, + SRStatusInfo, SRSvcAccChange, SRSvcAccCreate, SRUserStatsSummary, SiteReplicationInfo, SyncStatus, WorkerStat, }; use rustfs_policy::policy::{ Policy, @@ -98,7 +98,6 @@ use uuid::Uuid; // paths keep resolving while this file keeps only the HTTP handlers. pub(crate) use crate::site_replication::*; -const SERVICE_ACCOUNT_ENVELOPE_VERSION: u64 = 2; // Serializes peer-join admission (staleness check -> IAM upsert -> state // commit) across every node of this site; see admit_peer_join. Never an // actual object — only a namespace-lock key, like the repair execution lock. @@ -1479,6 +1478,7 @@ async fn set_site_replicator_service_account_secret(parent_user: &str, secret_ke expiration: None, allow_site_replicator_account: true, claims: None, + status: None, }, ) .await @@ -1721,6 +1721,7 @@ async fn reconcile_site_replicator_service_account() -> S3Result<()> { expiration: None, allow_site_replicator_account: true, claims: None, + status: None, }, ) .await @@ -1980,7 +1981,7 @@ async fn bootstrap_existing_metadata_after_add( return errors; } }; - let plan = match site_replication_bootstrap_plan(&info) { + let plan = match build_site_replication_bootstrap_plan(&info).await { Ok(plan) => plan, Err(err) => { let mut errors = SiteReplicationErrorSummary::default(); @@ -3498,6 +3499,7 @@ fn remove_sites(mut state: SiteReplicationState, req: SRRemoveReq) -> SiteReplic state.resync_status.clear(); state.retry_queue.clear(); state.iam_deletion_replays.clear(); + state.iam_deletion_marks.clear(); state.pending_endpoint_refresh = None; state.updated_at = Some(OffsetDateTime::now_utc()); return state; @@ -3509,6 +3511,7 @@ fn remove_sites(mut state: SiteReplicationState, req: SRRemoveReq) -> SiteReplic state.resync_status.clear(); state.retry_queue.clear(); state.iam_deletion_replays.clear(); + state.iam_deletion_marks.clear(); state.pending_endpoint_refresh = None; state.updated_at = Some(OffsetDateTime::now_utc()); return state; @@ -5386,6 +5389,38 @@ fn is_stale_update(local_updated_at: OffsetDateTime, incoming_updated_at: Option incoming_updated_at.is_some_and(|incoming_updated_at| incoming_updated_at < local_updated_at) } +/// Verdict for an incoming IAM item judged against the local record it would +/// overwrite or delete (backlog#2291). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum IamItemVerdict { + /// Apply the item: there is no local record, the item carries no source + /// timestamp (older peer), or it is at least as new as the local record. + Apply, + /// The local record was written from a newer source change; acknowledge the + /// item without touching the record. Covers both directions: a delayed + /// grant must not undo a newer revoke, and a delayed revoke must not undo a + /// newer grant. + SkipStale, +} + +/// Ordering rule shared by the `policy`, `policy-mapping` and `group-info` +/// item paths (and matching `iam-user` / `service-account`). +/// +/// `local_record_updated_at` is `None` when the targeted record does not +/// exist locally: nothing can be stale relative to an absent record, so a +/// create is applied and a delete falls through to the idempotent no-op paths +/// (backlog#2071). A record that exists but predates timestamps passes +/// `Some(UNIX_EPOCH)` and therefore never rejects an item. +fn judge_iam_item_staleness( + local_record_updated_at: Option, + incoming_updated_at: Option, +) -> IamItemVerdict { + match local_record_updated_at { + Some(local_updated_at) if is_stale_update(local_updated_at, incoming_updated_at) => IamItemVerdict::SkipStale, + _ => IamItemVerdict::Apply, + } +} + fn bucket_meta_local_updated_at( bucket_meta: &crate::admin::storage_api::bucket::metadata::BucketMetadata, config_file: &str, @@ -5586,6 +5621,18 @@ async fn apply_bucket_meta_item(item: SRBucketMeta) -> S3Result<()> { _ => unreachable!(), }; + // Persist the SOURCE `updated_at` as the stored `*_config_updated_at` + // stamp, on writes and on deletes alike (backlog#2292). The staleness + // gate above compares the next item's source time against that stamp, so + // stamping the local apply time would reject a newer source edit that was + // merely delivered after this write or delete (two quick edits under + // delivery delay, or a peer clock ahead of ours). + // Items without a source time keep the local stamp; lc-config keeps it + // too: its staleness axis is the in-document `expiry_updated_at` the merge + // above records, and the whole-config time is only its deletion / legacy + // lower bound. + let source_updated_at = if item.r#type == "lc-config" { None } else { item.updated_at }; + if !skip_config_write { if let Some(data) = data { if item.r#type == "quota-config" { @@ -5604,13 +5651,25 @@ async fn apply_bucket_meta_item(item: SRBucketMeta) -> S3Result<()> { "durable quota capability is not confirmed across the cluster".to_string(), ) })?; - metadata_sys::update_quota_if_incarnation(&item.bucket, data, expected_incarnation_id, &proof) - .await - .map_err(ApiError::from)?; + match source_updated_at { + Some(source_updated_at) => { + metadata_sys::update_quota_if_incarnation_at( + &item.bucket, + data, + expected_incarnation_id, + &proof, + source_updated_at, + ) + .await + } + None => { + metadata_sys::update_quota_if_incarnation(&item.bucket, data, expected_incarnation_id, &proof).await + } + } + .map_err(ApiError::from)?; } else { - metadata_sys::update_if_incarnation(&item.bucket, config_file, data, expected_incarnation_id) - .await - .map_err(ApiError::from)?; + write_replicated_bucket_config(&item.bucket, config_file, data, expected_incarnation_id, source_updated_at) + .await?; } } else { if let Some(guard) = lifecycle_guard.as_ref() { @@ -5618,9 +5677,8 @@ async fn apply_bucket_meta_item(item: SRBucketMeta) -> S3Result<()> { .await .map_err(ApiError::from)?; } else { - metadata_sys::update_if_incarnation(&item.bucket, config_file, data, expected_incarnation_id) - .await - .map_err(ApiError::from)?; + write_replicated_bucket_config(&item.bucket, config_file, data, expected_incarnation_id, source_updated_at) + .await?; } } } else { @@ -5629,9 +5687,23 @@ async fn apply_bucket_meta_item(item: SRBucketMeta) -> S3Result<()> { .await .map_err(ApiError::from)?; } else { - metadata_sys::delete_if_incarnation(&item.bucket, config_file, expected_incarnation_id) - .await - .map_err(ApiError::from)?; + // A delete is stamped like a write: the source time survives + // as the config's `*_config_updated_at`, so a newer source + // re-create delivered later is not judged stale against the + // local time this delete landed (backlog#2292). + match source_updated_at { + Some(source_updated_at) => { + metadata_sys::delete_if_incarnation_at( + &item.bucket, + config_file, + expected_incarnation_id, + source_updated_at, + ) + .await + } + None => metadata_sys::delete_if_incarnation(&item.bucket, config_file, expected_incarnation_id).await, + } + .map_err(ApiError::from)?; } } } @@ -5656,43 +5728,28 @@ async fn apply_bucket_meta_item(item: SRBucketMeta) -> S3Result<()> { Ok(()) } -fn group_info_requires_upsert(update: &rustfs_madmin::GroupAddRemove) -> bool { - !update.is_remove +/// Write one replicated bucket config, stamped with the item's source +/// `updated_at` when it carries one and with the local clock otherwise +/// (backlog#2292; see [`apply_bucket_meta_item`]). +async fn write_replicated_bucket_config( + bucket: &str, + config_file: &str, + data: Vec, + expected_incarnation_id: Uuid, + source_updated_at: Option, +) -> S3Result<()> { + match source_updated_at { + Some(source_updated_at) => { + metadata_sys::update_if_incarnation_at(bucket, config_file, data, expected_incarnation_id, source_updated_at).await + } + None => metadata_sys::update_if_incarnation(bucket, config_file, data, expected_incarnation_id).await, + } + .map_err(ApiError::from)?; + Ok(()) } -pub(crate) fn encode_service_account_replication_policy( - claims: &HashMap, - session_policy: Option<&str>, -) -> S3Result<(SRSessionPolicy, Option)> { - if !claims.contains_key(OIDC_VIRTUAL_PARENT_CLAIM) { - return session_policy - .map(SRSessionPolicy::from_json) - .transpose() - .map(|policy| policy.unwrap_or_default()) - .map(|policy| (policy, None)) - .map_err(|err| s3_error!(InvalidArgument, "marshal policy failed: {:?}", err)); - } - - let policy = match session_policy { - Some(policy) => serde_json::from_str::(policy) - .map_err(|err| s3_error!(InvalidArgument, "invalid service account replication policy: {:?}", err))?, - None => Policy::default(), - }; - if policy.statements.is_empty() && (!policy.id.is_empty() || !policy.version.is_empty()) - || policy.version.is_empty() && !policy.statements.is_empty() - { - return Err(s3_error!(InvalidArgument, "service account replication policy is not normalized")); - } - let policy = serde_json::to_string(&policy) - .map_err(|err| s3_error!(InternalError, "marshal service account replication policy failed: {:?}", err))?; - let policy = SRSessionPolicy::from_json(&policy) - .map_err(|err| s3_error!(InternalError, "marshal service account replication policy failed: {:?}", err))?; - Ok(( - policy, - Some(rustfs_madmin::SRSvcAccReplicationEnvelope { - version: SERVICE_ACCOUNT_ENVELOPE_VERSION, - }), - )) +fn group_info_requires_upsert(update: &rustfs_madmin::GroupAddRemove) -> bool { + !update.is_remove } #[derive(Debug)] @@ -5761,32 +5818,96 @@ async fn apply_iam_item(item: SRIAMItem) -> S3Result<()> { let Some(iam_sys) = current_iam_handle() else { return Err(s3_error!(InvalidRequest, "iam not init")); }; - let incoming_updated_at = item.updated_at; - match item.r#type.as_str() { - "policy" => apply_iam_policy_item(&iam_sys, &item.name, item.policy).await, - "policy-mapping" => apply_iam_policy_mapping_item(&iam_sys, item.policy_mapping).await, - "group-info" => apply_iam_group_info_item(&iam_sys, item.group_info).await, // MinIO madmin-go sends `SRIAMItemSTSAcc = "sts-account"`. The legacy alias // `sts-credential` (emitted by older RustFS releases) stays accepted permanently // so mixed-version RustFS sites keep replicating STS credentials during rolling // upgrades; it is a compatibility layer, not temporary code. - SR_IAM_ITEM_STS_ACC | SR_IAM_ITEM_STS_ACC_LEGACY => apply_iam_sts_account_item(&iam_sys, item.sts_credential).await, - "iam-user" => apply_iam_user_item(&iam_sys, item.iam_user, incoming_updated_at).await, - "service-account" => apply_iam_service_account_item(&iam_sys, item.svc_acc_change, incoming_updated_at).await, - _ => Err(s3_error!( - NotImplemented, - "site replication IAM item type `{}` is not supported", - item.r#type - )), + // + // STS credentials carry no source revision and leave no deletion mark, + // so they stay outside the ordered transaction below. + SR_IAM_ITEM_STS_ACC | SR_IAM_ITEM_STS_ACC_LEGACY => { + return apply_iam_sts_account_item(&iam_sys, item.sts_credential).await; + } + "policy" | "policy-mapping" | "group-info" | "iam-user" | "service-account" => {} + _ => { + return Err(s3_error!( + NotImplemented, + "site replication IAM item type `{}` is not supported", + item.r#type + )); + } } + + // One transaction per item (backlog#2291). The staleness verdict, the IAM + // write and the deletion-mark commit run under the distributed + // state-object lock, so an older grant and a newer revoke delivered + // concurrently — to this node or to a sibling node of this site — are + // applied one after the other, each judged against what the other left + // behind. The write stamps the record with the item's source + // `updated_at`, which is what the next item is judged against: stamping + // the local apply time would reject a newer source edit that was merely + // delivered later. A committed deletion leaves no record, so its source + // timestamp is kept as a mark in the same commit; failing to persist the + // mark fails the item, and the sender retries the (idempotent) deletion + // rather than leaving a revoke that a stale grant could still undo. + with_site_replication_state_transaction(move |mut state| async move { + let incoming_updated_at = item.updated_at; + let deletion_mark_entities = iam_item_deletion_mark_entities(&item); + let verdict = match item.r#type.as_str() { + "policy" => apply_iam_policy_item(&iam_sys, &state, &item.name, item.policy, incoming_updated_at).await?, + "policy-mapping" => apply_iam_policy_mapping_item(&iam_sys, &state, item.policy_mapping, incoming_updated_at).await?, + "group-info" => apply_iam_group_info_item(&iam_sys, &state, item.group_info, incoming_updated_at).await?, + "iam-user" => apply_iam_user_item(&iam_sys, &state, item.iam_user, incoming_updated_at).await?, + "service-account" => { + apply_iam_service_account_item(&iam_sys, &state, item.svc_acc_change, incoming_updated_at).await? + } + _ => unreachable!("unsupported IAM item types are rejected before the transaction"), + }; + let changed = verdict == IamItemVerdict::Apply + && incoming_updated_at + .filter(|_| !deletion_mark_entities.is_empty()) + .is_some_and(|deleted_at| record_iam_deletion_marks(&mut state, &deletion_mark_entities, deleted_at)); + Ok(((), changed.then_some(state))) + }) + .await } -async fn apply_iam_policy_item(iam_sys: &IamSys, name: &str, policy: Option) -> S3Result<()> { +/// The stamp a replicated write persists on the record: the item's source +/// `updated_at`, or the local clock for an item from a peer that predates +/// timestamps (those keep last-writer-wins, see [`judge_iam_item_staleness`]). +fn replicated_write_stamp(incoming_updated_at: Option) -> OffsetDateTime { + incoming_updated_at.unwrap_or_else(OffsetDateTime::now_utc) +} + +async fn apply_iam_policy_item( + iam_sys: &IamSys, + marks: &SiteReplicationState, + name: &str, + policy: Option, + incoming_updated_at: Option, +) -> S3Result { + // Judge the item against the local document's own timestamp — the source + // time of the edit that wrote it — so a delayed older body (or delete) + // cannot overwrite a newer edit; once the document is deleted, its + // deletion mark stands in for it (backlog#2291). + let local_updated_at = match iam_sys.get_policy_doc(name).await { + Ok(doc) => Some(doc.update_date.unwrap_or(OffsetDateTime::UNIX_EPOCH)), + Err(err) if rustfs_iam::error::is_err_no_such_policy(&err) => { + iam_deletion_mark(marks, &[iam_policy_deletion_mark_entity(name)]) + } + Err(err) => return Err(ApiError::from(err).into()), + }; + if judge_iam_item_staleness(local_updated_at, incoming_updated_at) == IamItemVerdict::SkipStale { + return Ok(IamItemVerdict::SkipStale); + } if let Some(policy) = policy { let policy: Policy = serde_json::from_value(policy).map_err(|e| s3_error!(InvalidRequest, "invalid policy body: {}", e))?; - iam_sys.set_policy(name, policy).await.map_err(ApiError::from)?; + iam_sys + .set_policy_at(name, policy, replicated_write_stamp(incoming_updated_at)) + .await + .map_err(ApiError::from)?; } else { // Idempotent delete: the retry drain replays recorded deletions, and // an entity already absent here IS the converged outcome — erroring @@ -5797,26 +5918,87 @@ async fn apply_iam_policy_item(iam_sys: &IamSys, name: &str, policy Err(err) => return Err(ApiError::from(err).into()), } } - Ok(()) + Ok(IamItemVerdict::Apply) } -async fn apply_iam_policy_mapping_item(iam_sys: &IamSys, policy_mapping: Option) -> S3Result<()> { +async fn apply_iam_policy_mapping_item( + iam_sys: &IamSys, + marks: &SiteReplicationState, + policy_mapping: Option, + incoming_updated_at: Option, +) -> S3Result { let Some(mapping) = policy_mapping else { return Err(s3_error!(InvalidRequest, "policyMapping is required")); }; let user_type = user_type_from_sr_wire(mapping.user_type).ok_or_else(|| s3_error!(InvalidRequest, "invalid userType"))?; + // Judge the item against the stored mapping's timestamp so a delayed older + // attach (or an older detach, `policy == ""`) cannot overwrite a newer one + // (backlog#2291). A detach removes the mapping outright, so once it is + // gone the detach's deletion mark stands in for the record. + let local_updated_at = match iam_sys + .get_mapped_policy_record(&mapping.user_or_group, user_type, mapping.is_group) + .await + { + Some(record) => Some(record.update_at), + None => iam_deletion_mark( + marks, + &[iam_policy_mapping_deletion_mark_entity( + &mapping.user_or_group, + mapping.user_type, + mapping.is_group, + )], + ), + }; + if judge_iam_item_staleness(local_updated_at, incoming_updated_at) == IamItemVerdict::SkipStale { + return Ok(IamItemVerdict::SkipStale); + } iam_sys - .policy_db_set(&mapping.user_or_group, user_type, mapping.is_group, &mapping.policy) + .policy_db_set_at( + &mapping.user_or_group, + user_type, + mapping.is_group, + &mapping.policy, + replicated_write_stamp(incoming_updated_at), + ) .await .map_err(ApiError::from)?; - Ok(()) + Ok(IamItemVerdict::Apply) } -async fn apply_iam_group_info_item(iam_sys: &IamSys, group_info: Option) -> S3Result<()> { +async fn apply_iam_group_info_item( + iam_sys: &IamSys, + marks: &SiteReplicationState, + group_info: Option, + incoming_updated_at: Option, +) -> S3Result { let Some(group_info) = group_info else { return Err(s3_error!(InvalidRequest, "groupInfo is required")); }; let update = group_info.update_req; + // The record is the group itself: its own timestamp moves on every + // membership or status change, so a delayed older add cannot re-add a + // member a newer removal took out, and a delayed older removal (or group + // delete) cannot undo a newer add (backlog#2291). Once the group is gone + // the marks of its deletion and of its members' removals stand in for it, + // so a stale add cannot re-create it or re-add a removed member. + let local_updated_at = match iam_sys.get_group_info(&update.group).await { + Some(group) => Some(group.update_at.unwrap_or(OffsetDateTime::UNIX_EPOCH)), + None => { + let entities: Vec = std::iter::once(iam_group_deletion_mark_entity(&update.group)) + .chain( + update + .members + .iter() + .map(|member| iam_group_member_deletion_mark_entity(&update.group, member)), + ) + .collect(); + iam_deletion_mark(marks, &entities) + } + }; + if judge_iam_item_staleness(local_updated_at, incoming_updated_at) == IamItemVerdict::SkipStale { + return Ok(IamItemVerdict::SkipStale); + } + let stamp = replicated_write_stamp(incoming_updated_at); if !group_info_requires_upsert(&update) { // Idempotent removal: a replayed deletion may find the group or a // member already gone (deleted here earlier, or the user tombstone @@ -5831,25 +6013,25 @@ async fn apply_iam_group_info_item(iam_sys: &IamSys, group_info: Op } } if members.is_empty() && !update.members.is_empty() { - return Ok(()); + return Ok(IamItemVerdict::Apply); } - match iam_sys.remove_users_from_group(&update.group, members).await { + match iam_sys.remove_users_from_group_at(&update.group, members, stamp).await { Ok(_) => {} Err(err) if rustfs_iam::error::is_err_no_such_group(&err) => {} Err(err) => return Err(ApiError::from(err).into()), } - return Ok(()); + return Ok(IamItemVerdict::Apply); } iam_sys - .add_users_to_group(&update.group, update.members) + .add_users_to_group_at(&update.group, update.members, stamp) .await .map_err(ApiError::from)?; iam_sys - .set_group_status(&update.group, matches!(update.status, GroupStatus::Enabled)) + .set_group_status_at(&update.group, matches!(update.status, GroupStatus::Enabled), stamp) .await .map_err(ApiError::from)?; - Ok(()) + Ok(IamItemVerdict::Apply) } async fn apply_iam_sts_account_item(iam_sys: &IamSys, sts_credential: Option) -> S3Result<()> { @@ -5889,17 +6071,23 @@ async fn apply_iam_sts_account_item(iam_sys: &IamSys, sts_credentia async fn apply_iam_user_item( iam_sys: &IamSys, + marks: &SiteReplicationState, iam_user: Option, incoming_updated_at: Option, -) -> S3Result<()> { +) -> S3Result { let Some(user) = iam_user else { return Err(s3_error!(InvalidRequest, "iamUser is required")); }; - if let Some(local) = iam_sys.get_user(&user.access_key).await - && is_stale_update(local.update_at.unwrap_or(OffsetDateTime::UNIX_EPOCH), incoming_updated_at) - { - return Ok(()); + // Once the identity is deleted, its deletion mark stands in for the + // record so a stale re-create cannot resurrect it (backlog#2291). + let local_updated_at = match iam_sys.get_user(&user.access_key).await { + Some(local) => Some(local.update_at.unwrap_or(OffsetDateTime::UNIX_EPOCH)), + None => iam_deletion_mark(marks, &[iam_user_deletion_mark_entity(&user.access_key)]), + }; + if judge_iam_item_staleness(local_updated_at, incoming_updated_at) == IamItemVerdict::SkipStale { + return Ok(IamItemVerdict::SkipStale); } + let stamp = replicated_write_stamp(incoming_updated_at); if user.is_delete_req { iam_sys.delete_user(&user.access_key, true).await.map_err(ApiError::from)?; } else { @@ -5909,36 +6097,42 @@ async fn apply_iam_user_item( let is_status_only_update = user_req.secret_key.is_empty() && user_req.policy.is_none(); if is_status_only_update { iam_sys - .set_user_status(&user.access_key, user_req.status) + .set_user_status_at(&user.access_key, user_req.status, stamp) .await .map_err(ApiError::from)?; } else { iam_sys - .create_user(&user.access_key, &user_req) + .create_user_at(&user.access_key, &user_req, stamp) .await .map_err(ApiError::from)?; } } - Ok(()) + Ok(IamItemVerdict::Apply) } async fn apply_iam_service_account_item( iam_sys: &IamSys, + marks: &SiteReplicationState, svc_acc_change: Option, incoming_updated_at: Option, -) -> S3Result<()> { +) -> S3Result { let Some(change) = svc_acc_change else { return Err(s3_error!(InvalidRequest, "serviceAccountChange is required")); }; let envelope = change.oidc_service_account_envelope; + let stamp = replicated_write_stamp(incoming_updated_at); if let Some(create) = change.create { - let local_updated_at = iam_sys - .get_user(&create.access_key) - .await - .map(|local| local.update_at.unwrap_or(OffsetDateTime::UNIX_EPOCH)); + // Like the user path: with the account already deleted here, the + // recorded deletion mark is the timestamp a stale create/update + // (a snapshot or a delayed delivery) has to beat (backlog#2291). + let local_updated_at = match iam_sys.get_user(&create.access_key).await { + Some(local) => Some(local.update_at.unwrap_or(OffsetDateTime::UNIX_EPOCH)), + None if create.access_key == SITE_REPLICATOR_SERVICE_ACCOUNT => None, + None => iam_deletion_mark(marks, &[format!("svc-acc:{}", create.access_key)]), + }; let replicated_policy = if create.access_key == SITE_REPLICATOR_SERVICE_ACCOUNT { if local_updated_at.is_some_and(|local_updated_at| is_stale_update(local_updated_at, incoming_updated_at)) { - return Ok(()); + return Ok(IamItemVerdict::SkipStale); } ReplicatedServiceAccountPolicy { policy: Some(site_replicator_service_account_policy()?), @@ -5948,7 +6142,7 @@ async fn apply_iam_service_account_item( let Some(replicated_policy) = decode_service_account_replication_policy(&create, envelope.as_ref(), incoming_updated_at, local_updated_at)? else { - return Ok(()); + return Ok(IamItemVerdict::SkipStale); }; replicated_policy }; @@ -5962,7 +6156,7 @@ async fn apply_iam_service_account_item( )); } iam_sys - .update_service_account( + .update_service_account_at( &create.access_key, UpdateServiceAccountOpts { name: replicated_policy.metadata_for_existing_account(create.name), @@ -5974,13 +6168,19 @@ async fn apply_iam_service_account_item( parent_user: None, allow_site_replicator_account: create.access_key == SITE_REPLICATOR_SERVICE_ACCOUNT, }, + stamp, ) .await .map_err(ApiError::from)?; } Err(err) if is_err_no_such_service_account(&err) => { + // A snapshot (bootstrap / repair / retry resend) carries the + // account's current status, and the account is created with + // it in the same write: a disabled account must never exist + // enabled here, not even between a create and a follow-up + // status write that might fail (backlog#2289). iam_sys - .new_service_account( + .new_service_account_at( &create.parent, Some(create.groups), NewServiceAccountOpts { @@ -5992,21 +6192,23 @@ async fn apply_iam_service_account_item( expiration: create.expiration, allow_site_replicator_account: true, claims: Some(create.claims), + status: (!create.status.is_empty()).then_some(create.status), }, + stamp, ) .await .map_err(ApiError::from)?; } Err(err) => return Err(ApiError::from(err).into()), } - return Ok(()); + return Ok(IamItemVerdict::Apply); } if let Some(update) = change.update { if let Some(local) = iam_sys.get_user(&update.access_key).await && is_stale_update(local.update_at.unwrap_or(OffsetDateTime::UNIX_EPOCH), incoming_updated_at) { - return Ok(()); + return Ok(IamItemVerdict::SkipStale); } let allow_site_replicator_account = update.access_key == SITE_REPLICATOR_SERVICE_ACCOUNT; let session_policy = if allow_site_replicator_account { @@ -6015,7 +6217,7 @@ async fn apply_iam_service_account_item( update.session_policy.as_str().and_then(|raw| serde_json::from_str(raw).ok()) }; iam_sys - .update_service_account( + .update_service_account_at( &update.access_key, UpdateServiceAccountOpts { session_policy, @@ -6029,23 +6231,24 @@ async fn apply_iam_service_account_item( parent_user: None, allow_site_replicator_account, }, + stamp, ) .await .map_err(ApiError::from)?; - return Ok(()); + return Ok(IamItemVerdict::Apply); } if let Some(delete) = change.delete { if let Some(local) = iam_sys.get_user(&delete.access_key).await && is_stale_update(local.update_at.unwrap_or(OffsetDateTime::UNIX_EPOCH), incoming_updated_at) { - return Ok(()); + return Ok(IamItemVerdict::SkipStale); } iam_sys .delete_service_account(&delete.access_key, true) .await .map_err(ApiError::from)?; - return Ok(()); + return Ok(IamItemVerdict::Apply); } Err(s3_error!(InvalidRequest, "serviceAccountChange is empty")) @@ -6111,6 +6314,7 @@ fn adopt_add_commit_state(state: &mut SiteReplicationState, next_state: SiteRepl sync_state_initialized, edit_generation: _, applied_edit_generations: _, + iam_deletion_marks: _, } = next_state; state.name = name; state.service_account_access_key = service_account_access_key; @@ -6707,6 +6911,7 @@ async fn apply_peer_join_service_account(join_req: SRPeerJoinReq) -> S3Result<() expiration: None, allow_site_replicator_account: join_req.svc_acct_access_key == SITE_REPLICATOR_SERVICE_ACCOUNT, claims: None, + status: None, }, ) .await @@ -7692,7 +7897,7 @@ impl Operation for SiteReplicationRepairHandler { let local_peer = current_local_peer(&req, &state); let body: SiteReplicationRepairRequest = read_site_replication_json(req, "", false).await?; let info = build_sr_info(&state, &local_peer).await?; - let plan = site_replication_bootstrap_plan(&info)?; + let plan = build_site_replication_bootstrap_plan(&info).await?; let signing_key = current_token_signing_key().ok_or_else(|| { S3Error::with_message(S3ErrorCode::InternalError, "token signing key is not initialized".to_string()) })?; @@ -7885,6 +8090,7 @@ impl Operation for SRRotateServiceAccountHandler { mod tests { use super::*; use crate::site_replication::identity::deployment_id_for_endpoint; + use rustfs_madmin::SRSessionPolicy; /// A peer the status probe could not reach must render as offline. /// @@ -8039,6 +8245,343 @@ mod tests { } } + // --- Review regressions on rustfs#7195 (backlog#2289 / #2291 / #2292): + // delivery order and concurrency through the real receiver. + + /// Two-peer state so the apply transaction's persist keeps the state + /// object (a single-peer state is cleared on write) and the deletion + /// marks it records survive between items. + async fn seed_two_peer_state_for_iam_apply() { + let seed = SiteReplicationState { + peers: BTreeMap::from([ + ( + "site-a".to_string(), + PeerInfo { + deployment_id: "site-a".to_string(), + ..peer("site-a", "https://a.example:9000") + }, + ), + ( + "site-b".to_string(), + PeerInfo { + deployment_id: "site-b".to_string(), + ..peer("site-b", "https://b.example:9000") + }, + ), + ]), + ..Default::default() + }; + save_site_replication_state(&seed).await.expect("seed state"); + } + + async fn clear_seeded_state() { + save_site_replication_state(&SiteReplicationState::default()) + .await + .expect("clear state"); + } + + fn sr_item(item_type: &str, updated_at: OffsetDateTime) -> SRIAMItem { + SRIAMItem { + r#type: item_type.to_string(), + updated_at: Some(updated_at), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + ..Default::default() + } + } + + fn allow_actions_policy(actions: &[&str]) -> serde_json::Value { + serde_json::json!({ + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": actions, "Resource": ["arn:aws:s3:::*"]}] + }) + } + + fn sr_policy_item(name: &str, body: serde_json::Value, updated_at: OffsetDateTime) -> SRIAMItem { + let mut item = sr_item("policy", updated_at); + item.name = name.to_string(); + item.policy = Some(body); + item + } + + fn sr_mapping_item(user: &str, policy: &str, updated_at: OffsetDateTime) -> SRIAMItem { + let mut item = sr_item("policy-mapping", updated_at); + item.policy_mapping = Some(SRPolicyMapping { + user_or_group: user.to_string(), + user_type: 0, + is_group: false, + policy: policy.to_string(), + ..Default::default() + }); + item + } + + fn sr_group_item(group: &str, members: &[&str], is_remove: bool, updated_at: OffsetDateTime) -> SRIAMItem { + let mut item = sr_item("group-info", updated_at); + item.group_info = Some(SRGroupInfo { + update_req: rustfs_madmin::GroupAddRemove { + group: group.to_string(), + members: members.iter().map(|member| member.to_string()).collect(), + status: rustfs_madmin::GroupStatus::Enabled, + is_remove, + }, + api_version: Some(SITE_REPL_API_VERSION.to_string()), + }); + item + } + + fn sr_user_item( + access_key: &str, + user_req: Option, + updated_at: OffsetDateTime, + ) -> SRIAMItem { + let mut item = sr_item("iam-user", updated_at); + item.iam_user = Some(SRIAMUser { + access_key: access_key.to_string(), + is_delete_req: user_req.is_none(), + user_req, + api_version: Some(SITE_REPL_API_VERSION.to_string()), + }); + item + } + + fn user_req(secret_key: &str, status: rustfs_madmin::AccountStatus) -> rustfs_madmin::AddOrUpdateUserReq { + rustfs_madmin::AddOrUpdateUserReq { + secret_key: secret_key.to_string(), + policy: None, + status, + } + } + + fn sr_service_account_create_item(parent: &str, access_key: &str, status: &str, updated_at: OffsetDateTime) -> SRIAMItem { + let mut item = sr_item("service-account", updated_at); + item.svc_acc_change = Some(SRSvcAccChange { + create: Some(SRSvcAccCreate { + parent: parent.to_string(), + access_key: access_key.to_string(), + secret_key: "replicated-svc-secret-123".to_string(), + groups: Vec::new(), + claims: HashMap::new(), + session_policy: rustfs_madmin::SRSessionPolicy::default(), + status: status.to_string(), + name: String::new(), + description: String::new(), + expiration: None, + api_version: Some(SITE_REPL_API_VERSION.to_string()), + }), + ..Default::default() + }); + item + } + + async fn stored_policy_json(name: &str) -> (String, Option) { + let iam = current_iam_handle().expect("test IAM"); + let doc = iam.get_policy_doc(name).await.expect("policy doc"); + (serde_json::to_string(&doc.policy).expect("serialize policy"), doc.update_date) + } + + /// Review finding on rustfs#7195 (P1): two source edits T1 < T2 that both + /// predate their delivery. The record T1 writes must carry T1 — not the + /// later receive time — or T2 is judged stale against it and the newer + /// revoke is silently dropped. Exercised through the real receiver for + /// every gated item type. + #[tokio::test] + #[serial] + async fn apply_iam_item_applies_delayed_in_order_updates_through_the_receiver() { + publish_ready_iam_context().await; + seed_two_peer_state_for_iam_apply().await; + let iam = current_iam_handle().expect("test IAM"); + let t1 = OffsetDateTime::now_utc() - time::Duration::hours(2); + let t2 = t1 + time::Duration::minutes(5); + + // policy: the grant, then the narrower revision. + let policy = "sr-delayed-order-policy"; + apply_iam_item(sr_policy_item(policy, allow_actions_policy(&["s3:GetObject", "s3:PutObject"]), t1)) + .await + .expect("T1 grant"); + apply_iam_item(sr_policy_item(policy, allow_actions_policy(&["s3:GetObject"]), t2)) + .await + .expect("T2 narrowed body"); + let (stored, stamp) = stored_policy_json(policy).await; + assert_eq!(stamp, Some(t2), "the stored stamp is the source time of the last applied edit"); + assert!( + !stored.contains("s3:PutObject"), + "the narrower T2 body must replace the T1 grant: {stored}" + ); + + // policy-mapping: attach the wide policy, then the narrow one. + for (name, actions) in [ + ("sr-delayed-order-wide", &["s3:*"][..]), + ("sr-delayed-order-narrow", &["s3:GetObject"][..]), + ] { + let body: rustfs_policy::policy::Policy = serde_json::from_value(allow_actions_policy(actions)).expect("policy body"); + iam.set_policy(name, body).await.expect("local policy"); + } + let user = "sr-delayed-order-user"; + apply_iam_item(sr_mapping_item(user, "sr-delayed-order-wide", t1)) + .await + .expect("T1 attach"); + apply_iam_item(sr_mapping_item(user, "sr-delayed-order-narrow", t2)) + .await + .expect("T2 attach"); + let mapping = iam + .get_mapped_policy_record(user, rustfs_iam::store::UserType::Reg, false) + .await + .expect("mapping"); + assert_eq!(mapping.policies, "sr-delayed-order-narrow"); + assert_eq!(mapping.update_at, t2); + + // group: add the member, then remove it. + let member = "sr-delayed-order-member"; + iam.create_user(member, &user_req("member-secret-key-123", rustfs_madmin::AccountStatus::Enabled)) + .await + .expect("member"); + let group = "sr-delayed-order-group"; + apply_iam_item(sr_group_item(group, &[member], false, t1)) + .await + .expect("T1 add"); + apply_iam_item(sr_group_item(group, &[member], true, t2)) + .await + .expect("T2 remove"); + let info = iam.get_group_info(group).await.expect("group"); + assert!(info.members.is_empty(), "the T2 removal must land after the delayed T1 add"); + assert_eq!(info.update_at, Some(t2)); + + // iam-user: create enabled, then the status-only disable. + let access_key = "sr-delayed-order-account"; + apply_iam_item(sr_user_item( + access_key, + Some(user_req("account-secret-key-123", rustfs_madmin::AccountStatus::Enabled)), + t1, + )) + .await + .expect("T1 create"); + apply_iam_item(sr_user_item(access_key, Some(user_req("", rustfs_madmin::AccountStatus::Disabled)), t2)) + .await + .expect("T2 disable"); + let identity = iam.get_user(access_key).await.expect("user"); + assert_eq!(identity.credentials.status, "off", "the T2 disable must land after the delayed T1 create"); + assert_eq!(identity.update_at, Some(t2)); + + clear_seeded_state().await; + } + + /// Review finding on rustfs#7195: an older grant and a newer revoke for + /// the same record delivered concurrently must always leave the revoke, + /// whichever request reaches the transaction first — the verdict and + /// the write of one cannot interleave with the other's. + #[tokio::test] + #[serial] + async fn apply_iam_item_serializes_a_concurrent_older_grant_and_newer_revoke() { + publish_ready_iam_context().await; + seed_two_peer_state_for_iam_apply().await; + let t1 = OffsetDateTime::now_utc() - time::Duration::hours(2); + let t2 = t1 + time::Duration::minutes(5); + + for round in 0..6u32 { + let policy = format!("sr-race-policy-{round}"); + let grant = tokio::spawn(apply_iam_item(sr_policy_item( + &policy, + allow_actions_policy(&["s3:GetObject", "s3:PutObject"]), + t1, + ))); + let revoke = tokio::spawn(apply_iam_item(sr_policy_item(&policy, allow_actions_policy(&["s3:GetObject"]), t2))); + let (grant, revoke) = if round % 2 == 0 { + tokio::join!(grant, revoke) + } else { + let (revoke, grant) = tokio::join!(revoke, grant); + (grant, revoke) + }; + grant.expect("join grant").expect("grant delivery is acknowledged"); + revoke.expect("join revoke").expect("revoke delivery is acknowledged"); + let (stored, stamp) = stored_policy_json(&policy).await; + assert!( + !stored.contains("s3:PutObject"), + "round {round}: the grant won over the newer revoke: {stored}" + ); + assert_eq!(stamp, Some(t2), "round {round}"); + } + + clear_seeded_state().await; + } + + /// Review finding on rustfs#7195: a replicated delete followed by the + /// delayed delivery of the older create must not resurrect the entity, + /// and the mark that fences it is committed by the same transaction that + /// applied the delete; a genuinely newer create still lands. + #[tokio::test] + #[serial] + async fn apply_iam_item_rejects_a_stale_recreate_after_a_replicated_delete() { + publish_ready_iam_context().await; + seed_two_peer_state_for_iam_apply().await; + let iam = current_iam_handle().expect("test IAM"); + let t1 = OffsetDateTime::now_utc() - time::Duration::hours(2); + let t3 = t1 + time::Duration::minutes(10); + let t4 = t3 + time::Duration::minutes(10); + let access_key = "sr-recreate-account"; + let create = |at| { + sr_user_item( + access_key, + Some(user_req("recreate-secret-key-123", rustfs_madmin::AccountStatus::Enabled)), + at, + ) + }; + + apply_iam_item(create(t1)).await.expect("T1 create"); + assert!(iam.get_user(access_key).await.is_some()); + apply_iam_item(sr_user_item(access_key, None, t3)).await.expect("T3 delete"); + assert!(iam.get_user(access_key).await.is_none()); + let state = load_site_replication_state().await.expect("state"); + assert_eq!( + state.iam_deletion_marks.get(&iam_user_deletion_mark_entity(access_key)), + Some(&t3), + "the delete's mark is committed with the delete" + ); + + apply_iam_item(create(t1)).await.expect("the stale replay is acknowledged"); + assert!( + iam.get_user(access_key).await.is_none(), + "a create older than the recorded deletion must not re-create the user" + ); + + apply_iam_item(create(t4)).await.expect("T4 create"); + let identity = iam.get_user(access_key).await.expect("a newer create lands"); + assert_eq!(identity.update_at, Some(t4)); + + clear_seeded_state().await; + } + + /// Review finding on rustfs#7195 (backlog#2289): a replicated disabled + /// service account is created disabled in one write, never enabled and + /// then switched off, and carries the source stamp. + #[tokio::test] + #[serial] + async fn apply_iam_item_creates_a_replicated_service_account_with_its_status() { + publish_ready_iam_context().await; + seed_two_peer_state_for_iam_apply().await; + let iam = current_iam_handle().expect("test IAM"); + let t1 = OffsetDateTime::now_utc() - time::Duration::hours(2); + let parent = "sr-svc-parent"; + iam.create_user(parent, &user_req("parent-secret-key-123", rustfs_madmin::AccountStatus::Enabled)) + .await + .expect("parent"); + + for (access_key, status, expected) in [ + ("sr-svc-disabled", "off", "off"), + ("sr-svc-enabled", "on", "on"), + ("sr-svc-default", "", "on"), + ] { + apply_iam_item(sr_service_account_create_item(parent, access_key, status, t1)) + .await + .expect("service account create"); + let (credentials, _) = iam.get_service_account(access_key).await.expect("service account"); + assert_eq!(credentials.status, expected, "{access_key}"); + let identity = iam.get_user(access_key).await.expect("identity"); + assert_eq!(identity.update_at, Some(t1), "{access_key} carries the source stamp"); + } + + clear_seeded_state().await; + } + #[tokio::test] #[serial] async fn apply_iam_item_accepts_minio_sts_account_item_type() { @@ -12186,6 +12729,355 @@ mod tests { assert!(!is_stale_update(local, None)); } + /// Minimal model of one replicated IAM record (a policy document body, a + /// user/group mapping, or a group's member set) as the apply paths treat + /// it: `None` is "absent", `Some((content, stamp))` is the local record + /// with the timestamp of the change that last wrote it. Applying an item + /// goes through `judge_iam_item_staleness` exactly like the three apply + /// functions do; a delete (`incoming == None`) on an absent record is the + /// idempotent no-op of backlog#2071. + fn apply_iam_item_to_model( + record: &mut Option<(&'static str, OffsetDateTime)>, + incoming: Option<&'static str>, + incoming_updated_at: Option, + ) -> IamItemVerdict { + let verdict = judge_iam_item_staleness(record.map(|(_, stamp)| stamp), incoming_updated_at); + if verdict == IamItemVerdict::Apply { + *record = incoming.map(|content| (content, incoming_updated_at.unwrap_or(OffsetDateTime::UNIX_EPOCH))); + } + verdict + } + + fn at(seconds: i64) -> OffsetDateTime { + OffsetDateTime::UNIX_EPOCH + time::Duration::seconds(seconds) + } + + /// backlog#2291: a revoke (narrowed policy body, detached mapping, member + /// removed from the group) followed by the delayed delivery of the older + /// grant must leave the revoke in place. + #[test] + fn test_iam_item_stale_grant_after_revoke_is_not_applied() { + let mut record = None; + assert_eq!(apply_iam_item_to_model(&mut record, Some("grant"), Some(at(10))), IamItemVerdict::Apply); + assert_eq!(apply_iam_item_to_model(&mut record, Some("revoke"), Some(at(20))), IamItemVerdict::Apply); + + // The older grant is redelivered (retry drain, slow peer) after the revoke. + assert_eq!( + apply_iam_item_to_model(&mut record, Some("grant"), Some(at(10))), + IamItemVerdict::SkipStale, + "a grant older than the local revoke must be acknowledged without being applied" + ); + assert_eq!(record, Some(("revoke", at(20))), "the revoke must survive the stale grant"); + } + + /// backlog#2291: the mirror image — a grant followed by the delayed delivery + /// of an older revoke (older body, older detach, older member removal, or + /// an older delete) must leave the grant in place. + #[test] + fn test_iam_item_stale_revoke_after_grant_is_not_applied() { + let mut record = None; + assert_eq!(apply_iam_item_to_model(&mut record, Some("revoke"), Some(at(10))), IamItemVerdict::Apply); + assert_eq!(apply_iam_item_to_model(&mut record, Some("grant"), Some(at(20))), IamItemVerdict::Apply); + + assert_eq!( + apply_iam_item_to_model(&mut record, Some("revoke"), Some(at(10))), + IamItemVerdict::SkipStale, + "a revoke older than the local grant must not be applied" + ); + assert_eq!( + apply_iam_item_to_model(&mut record, None, Some(at(15))), + IamItemVerdict::SkipStale, + "a delete older than the local record must not remove it" + ); + assert_eq!(record, Some(("grant", at(20)))); + } + + /// backlog#2291: an item at least as new as the local record is applied, + /// including a newer delete; equal timestamps are not stale (same rule as + /// `iam-user`). + #[test] + fn test_iam_item_newer_than_local_record_is_applied() { + let mut record = Some(("grant", at(20))); + assert_eq!(apply_iam_item_to_model(&mut record, Some("revoke"), Some(at(20))), IamItemVerdict::Apply); + assert_eq!(record, Some(("revoke", at(20)))); + + assert_eq!(apply_iam_item_to_model(&mut record, Some("grant"), Some(at(30))), IamItemVerdict::Apply); + assert_eq!(record, Some(("grant", at(30)))); + + assert_eq!(apply_iam_item_to_model(&mut record, None, Some(at(40))), IamItemVerdict::Apply); + assert_eq!(record, None, "a newer delete removes the record"); + } + + /// backlog#2291: peers that predate item timestamps keep today's + /// last-writer-wins behaviour — an item without `updatedAt` is applied even + /// over a newer local record. + #[test] + fn test_iam_item_without_source_timestamp_is_applied() { + let mut record = Some(("grant", at(20))); + assert_eq!(apply_iam_item_to_model(&mut record, Some("revoke"), None), IamItemVerdict::Apply); + assert_eq!(record.map(|(content, _)| content), Some("revoke")); + + assert_eq!(judge_iam_item_staleness(Some(at(20)), None), IamItemVerdict::Apply); + assert_eq!(judge_iam_item_staleness(None, None), IamItemVerdict::Apply); + } + + /// backlog#2291: nothing is stale relative to an absent record. A create + /// with any timestamp is applied, and a delete falls through to the + /// idempotent no-op paths (backlog#2071) instead of being judged. + #[test] + fn test_iam_item_targeting_absent_record_is_applied() { + assert_eq!(judge_iam_item_staleness(None, Some(at(1))), IamItemVerdict::Apply); + + let mut record = None; + assert_eq!(apply_iam_item_to_model(&mut record, None, Some(at(1))), IamItemVerdict::Apply); + assert_eq!(record, None); + assert_eq!(apply_iam_item_to_model(&mut record, Some("grant"), Some(at(1))), IamItemVerdict::Apply); + assert_eq!(record, Some(("grant", at(1)))); + + // A record that predates timestamps is reported as UNIX_EPOCH by the + // apply paths and therefore never rejects an item. + assert_eq!( + judge_iam_item_staleness(Some(OffsetDateTime::UNIX_EPOCH), Some(at(1))), + IamItemVerdict::Apply + ); + } + + /// The apply paths once the record is gone: the local timestamp the gate + /// sees is the deletion mark (or `None` when no deletion was recorded), + /// and a committed deletion records its source timestamp as the mark — + /// the same sequence `apply_iam_item` runs. + fn apply_iam_item_to_deleted_record_model( + marks: &mut SiteReplicationState, + entity: &str, + incoming_is_delete: bool, + incoming_updated_at: Option, + ) -> IamItemVerdict { + let entities = vec![entity.to_string()]; + let verdict = judge_iam_item_staleness(iam_deletion_mark(marks, &entities), incoming_updated_at); + if verdict == IamItemVerdict::Apply + && incoming_is_delete + && let Some(deleted_at) = incoming_updated_at + { + // Pruning is judged from the deletion's own clock in the model. + record_iam_deletion_marks_at(marks, &entities, deleted_at, deleted_at); + } + verdict + } + + /// backlog#2291 (real-VM case R6.3a of backlog#2080): a detach deletes the + /// mapping outright, so the older grant that arrives afterwards finds no + /// record — the deletion mark must stand in for it and reject the grant. + /// The same holds for a deleted policy document, user or group. + #[test] + fn test_iam_item_stale_grant_after_record_deletion_is_not_applied() { + let mut marks = SiteReplicationState::default(); + let entity = "policy-mapping:alice:0:false"; + + // The revoke (detach) is applied first: the record is gone, the mark stays. + assert_eq!( + apply_iam_item_to_deleted_record_model(&mut marks, entity, true, Some(at(20))), + IamItemVerdict::Apply + ); + assert_eq!(marks.iam_deletion_marks.get(entity), Some(&at(20))); + + // The older grant is delivered after the revoke. + assert_eq!( + apply_iam_item_to_deleted_record_model(&mut marks, entity, false, Some(at(10))), + IamItemVerdict::SkipStale, + "a grant older than the recorded deletion must not re-create the record" + ); + // A replayed copy of the same revoke stays a no-op and keeps the mark. + assert_eq!( + apply_iam_item_to_deleted_record_model(&mut marks, entity, true, Some(at(20))), + IamItemVerdict::Apply + ); + assert_eq!(marks.iam_deletion_marks.get(entity), Some(&at(20))); + // An older replayed revoke is stale against the newer one. + assert_eq!( + apply_iam_item_to_deleted_record_model(&mut marks, entity, true, Some(at(5))), + IamItemVerdict::SkipStale + ); + assert_eq!( + marks.iam_deletion_marks.get(entity), + Some(&at(20)), + "an older deletion never lowers the mark" + ); + } + + /// backlog#2291: a mark only fences items older than the deletion. A grant + /// newer than (or as new as) the recorded deletion re-creates the record, + /// an unmarked entity and an item without a source timestamp keep today's + /// behaviour. + #[test] + fn test_iam_item_newer_than_deletion_mark_is_applied() { + let mut marks = SiteReplicationState::default(); + let entity = "policy:readonly"; + assert_eq!( + apply_iam_item_to_deleted_record_model(&mut marks, entity, true, Some(at(20))), + IamItemVerdict::Apply + ); + + assert_eq!( + apply_iam_item_to_deleted_record_model(&mut marks, entity, false, Some(at(20))), + IamItemVerdict::Apply, + "a grant as new as the deletion is not stale" + ); + assert_eq!( + apply_iam_item_to_deleted_record_model(&mut marks, entity, false, Some(at(30))), + IamItemVerdict::Apply + ); + assert_eq!( + apply_iam_item_to_deleted_record_model(&mut marks, entity, false, None), + IamItemVerdict::Apply, + "an item from a peer without timestamps keeps last-writer-wins" + ); + assert_eq!( + apply_iam_item_to_deleted_record_model(&mut marks, "policy:other", false, Some(at(1))), + IamItemVerdict::Apply, + "no mark, no record: nothing to be stale against" + ); + assert_eq!( + judge_iam_item_staleness(iam_deletion_mark(&marks, &[]), Some(at(1))), + IamItemVerdict::Apply + ); + } + + /// backlog#2291: a group's removal marks are per member (plus the group + /// itself for a group delete), so with the group gone a stale add is + /// judged against the newest mark among the group and the members it + /// would add. + #[test] + fn test_iam_group_item_after_deletion_is_judged_against_member_marks() { + let mut marks = SiteReplicationState::default(); + let bob = iam_group_member_deletion_mark_entity("devs", "bob"); + let group = iam_group_deletion_mark_entity("devs"); + record_iam_deletion_marks_at(&mut marks, std::slice::from_ref(&bob), at(20), at(20)); + record_iam_deletion_marks_at(&mut marks, std::slice::from_ref(&group), at(30), at(30)); + + // The gate for an add of `bob` to the (deleted) group. + let add_bob = [group.clone(), bob.clone()]; + assert_eq!(iam_deletion_mark(&marks, &add_bob), Some(at(30))); + assert_eq!( + judge_iam_item_staleness(iam_deletion_mark(&marks, &add_bob), Some(at(25))), + IamItemVerdict::SkipStale + ); + assert_eq!( + judge_iam_item_staleness(iam_deletion_mark(&marks, &add_bob), Some(at(30))), + IamItemVerdict::Apply + ); + + // An add of `carol` to a group that was only ever partially emptied + // (no group delete) is judged against carol's own mark only. + marks.iam_deletion_marks.remove(&group); + let add_carol = [group.clone(), iam_group_member_deletion_mark_entity("devs", "carol")]; + assert_eq!(iam_deletion_mark(&marks, &add_carol), None); + assert_eq!( + judge_iam_item_staleness(iam_deletion_mark(&marks, &add_carol), Some(at(1))), + IamItemVerdict::Apply + ); + let add_bob = [group, bob]; + assert_eq!( + judge_iam_item_staleness(iam_deletion_mark(&marks, &add_bob), Some(at(10))), + IamItemVerdict::SkipStale + ); + } + + /// Cheap wiring guard for backlog#2291: every one of the `policy`, + /// `policy-mapping`, `group-info`, `iam-user` and `service-account` apply + /// paths must judge the item against the local record (falling back to + /// the deletion marks of the transaction's state when the record is + /// absent) before it writes or deletes anything, must stamp every write + /// with the item's source time, and `apply_iam_item` must run verdict, + /// write and mark commit inside one state transaction. The ordering rule + /// itself is covered by the `test_iam_item_*` model tests above and the + /// `apply_iam_item_*` receiver tests; this only pins that no path bypasses + /// it again. + #[test] + fn test_iam_policy_mapping_and_group_items_gate_on_incoming_updated_at() { + let source = include_str!("site_replication.rs"); + let locally_stamped_writes = [ + ".set_policy(", + ".policy_db_set(", + ".add_users_to_group(", + ".remove_users_from_group(", + ".set_group_status(", + ".create_user(", + ".set_user_status(", + ".new_service_account(", + ".update_service_account(", + ]; + for (start, end, judged_by_shared_verdict) in [ + ("async fn apply_iam_policy_item(", "async fn apply_iam_policy_mapping_item(", true), + ("async fn apply_iam_policy_mapping_item(", "async fn apply_iam_group_info_item(", true), + ("async fn apply_iam_group_info_item(", "async fn apply_iam_sts_account_item(", true), + ("async fn apply_iam_user_item(", "async fn apply_iam_service_account_item(", true), + ("async fn apply_iam_service_account_item(", "fn claims_unix_timestamp(", false), + ] { + let body = source + .split(start) + .nth(1) + .and_then(|rest| rest.split(end).next()) + .expect(start); + if judged_by_shared_verdict { + assert!( + body.contains("judge_iam_item_staleness(local_updated_at, incoming_updated_at)"), + "{start} must judge the item against the local record before applying it" + ); + } else { + assert!( + body.contains("is_stale_update(local_updated_at, incoming_updated_at)"), + "{start} must judge the item against the local record before applying it" + ); + } + assert!( + body.contains("iam_deletion_mark("), + "{start} must fall back to the deletion marks of the transaction's state when the record is absent" + ); + assert!( + body.contains("replicated_write_stamp(incoming_updated_at)"), + "{start} must stamp its writes with the item's source time" + ); + for write in locally_stamped_writes { + assert!( + !body.contains(write), + "{start} must not stamp a replicated write with the local clock ({write})" + ); + } + } + let dispatch = source + .split("async fn apply_iam_item(") + .nth(1) + .and_then(|rest| rest.split("fn replicated_write_stamp(").next()) + .expect("apply_iam_item"); + assert!( + dispatch.contains("with_site_replication_state_transaction(move |mut state| async move {"), + "apply_iam_item must run verdict, write and mark commit in one state transaction" + ); + assert!( + dispatch.contains("record_iam_deletion_marks(&mut state, &deletion_mark_entities, deleted_at)"), + "apply_iam_item must record the mark of a deletion it committed in the same transaction" + ); + assert!( + !dispatch.contains("commit_iam_deletion_marks("), + "the mark must not be committed in a second transaction" + ); + // backlog#2289: a replicated service account is created with its + // status; a second status write could fail and leave it enabled. + let create_branch = source + .split("Err(err) if is_err_no_such_service_account(&err) => {") + .nth(1) + .and_then(|rest| rest.split("Err(err) => return Err(ApiError::from(err).into()),").next()) + .expect("service account create branch"); + assert!( + create_branch.contains("status: (!create.status.is_empty()).then_some(create.status),"), + "the service account must be created with the source status" + ); + assert!( + !create_branch.contains("update_service_account"), + "the created service account's status must not depend on a second write" + ); + } + #[test] fn test_apply_state_edit_req_only_updates_ilm_expiry_flags() { let mut state = SiteReplicationState::default(); @@ -13717,4 +14609,76 @@ mod tests { ); } } + + /// backlog#2292: the receiver persists the SOURCE `updated_at` of an + /// applied bucket config and judges the next item's source time against + /// it. Stamping the local apply time instead rejected a source edit that + /// was newer than the applied one but delivered after the local stamp + /// (two quick source edits under delivery delay; a peer clock ahead of + /// ours) and acknowledged it with 200. + #[test] + fn test_bucket_meta_staleness_is_judged_against_the_applied_source_timestamp() { + let apply_wall_clock = OffsetDateTime::now_utc(); + let source_edit_t1 = apply_wall_clock - time::Duration::seconds(30); + let source_edit_t2 = source_edit_t1 + time::Duration::seconds(2); + let source_edit_t0 = source_edit_t1 - time::Duration::seconds(2); + assert!( + source_edit_t2 < apply_wall_clock, + "T2 is newer at the source yet older than the local apply clock" + ); + + // Edit T1 arrives first and is applied the way apply_bucket_meta_item + // persists a replicated config: stamped with its source time. + let mut meta = crate::admin::storage_api::bucket::metadata::BucketMetadata::new("photos"); + meta.update_config_at( + BUCKET_POLICY_CONFIG, + br#"{"Version":"2012-10-17","Statement":[]}"#.to_vec(), + source_edit_t1, + ) + .expect("apply edit T1"); + let local_updated_at = bucket_meta_local_updated_at(&meta, BUCKET_POLICY_CONFIG); + assert_eq!( + local_updated_at, source_edit_t1, + "the stored stamp is the source time, not the apply clock" + ); + + // Edit T2 is newer at the source but delivered late: it must apply. + assert!( + !is_stale_update(local_updated_at, Some(source_edit_t2)), + "edit T2 ({source_edit_t2}) is newer than applied edit T1 ({source_edit_t1}) but is rejected against local stamp {local_updated_at}" + ); + // Edit T0 predates the applied edit: it stays rejected. + assert!( + is_stale_update(local_updated_at, Some(source_edit_t0)), + "edit T0 ({source_edit_t0}) is older than applied edit T1 ({source_edit_t1}) and must be rejected" + ); + // An item without a source time is never judged stale (unchanged). + assert!(!is_stale_update(local_updated_at, None)); + } + + /// backlog#2292: the replicated-config write in `apply_bucket_meta_item` + /// must go through the source-stamped entries; a plain + /// `update_if_incarnation` there would reintroduce local stamping. + #[test] + fn test_apply_bucket_meta_item_writes_through_the_source_stamped_entries() { + let source = include_str!("site_replication.rs"); + let apply = source + .split("async fn apply_bucket_meta_item") + .nth(1) + .and_then(|rest| rest.split("fn group_info_requires_upsert").next()) + .expect("apply_bucket_meta_item source"); + assert!( + apply.contains("update_quota_if_incarnation_at("), + "durable quota must carry the source stamp" + ); + assert!(apply.contains("update_if_incarnation_at("), "bucket configs must carry the source stamp"); + assert!( + apply.contains("delete_if_incarnation_at("), + "bucket config deletes must carry the source stamp too, or a newer re-create is judged stale" + ); + assert!( + !apply.contains("metadata_sys::update_if_incarnation(&item.bucket"), + "no replicated config write may bypass the source stamp" + ); + } } diff --git a/rustfs/src/admin/handlers/user.rs b/rustfs/src/admin/handlers/user.rs index a9d4ee4f0..04d28290f 100644 --- a/rustfs/src/admin/handlers/user.rs +++ b/rustfs/src/admin/handlers/user.rs @@ -1131,6 +1131,7 @@ impl Operation for ImportIam { expiration: req.expiration, allow_site_replicator_account: false, claims: Some(req.claims), + status: None, }; let groups = if req.groups.is_empty() { None } else { Some(req.groups) }; diff --git a/rustfs/src/admin/storage_api.rs b/rustfs/src/admin/storage_api.rs index 024fd2f59..97e87c68c 100644 --- a/rustfs/src/admin/storage_api.rs +++ b/rustfs/src/admin/storage_api.rs @@ -335,6 +335,25 @@ pub(crate) mod metadata_sys { super::ecstore_bucket::metadata_sys::update_if_incarnation(bucket, config_file, data, expected_incarnation_id).await } + /// [`update_if_incarnation`] stamping the config with a replicated edit's + /// source `updated_at` instead of the local clock (backlog#2292). + pub(crate) async fn update_if_incarnation_at( + bucket: &str, + config_file: &str, + data: Vec, + expected_incarnation_id: uuid::Uuid, + updated_at: OffsetDateTime, + ) -> Result { + super::ecstore_bucket::metadata_sys::update_if_incarnation_at( + bucket, + config_file, + data, + expected_incarnation_id, + updated_at, + ) + .await + } + pub(crate) async fn update_quota_if_incarnation( bucket: &str, data: Vec, @@ -344,6 +363,25 @@ pub(crate) mod metadata_sys { super::ecstore_bucket::metadata_sys::update_quota_if_incarnation(bucket, data, expected_incarnation_id, proof).await } + /// [`update_quota_if_incarnation`] stamping the quota with a replicated + /// edit's source `updated_at` instead of the local clock (backlog#2292). + pub(crate) async fn update_quota_if_incarnation_at( + bucket: &str, + data: Vec, + expected_incarnation_id: uuid::Uuid, + proof: &super::ecstore_notification::CrossPoolFenceFleetProofToken, + updated_at: OffsetDateTime, + ) -> Result { + super::ecstore_bucket::metadata_sys::update_quota_if_incarnation_at( + bucket, + data, + expected_incarnation_id, + proof, + updated_at, + ) + .await + } + pub(crate) async fn capture_bucket_metadata_incarnation(bucket: &str) -> Result { super::ecstore_bucket::metadata_sys::capture_bucket_metadata_incarnation(bucket).await } @@ -398,6 +436,18 @@ pub(crate) mod metadata_sys { super::ecstore_bucket::metadata_sys::delete_if_incarnation(bucket, config_file, expected_incarnation_id).await } + /// [`delete_if_incarnation`] stamping the cleared config with a replicated + /// deletion's source `updated_at` instead of the local clock (backlog#2292). + pub(crate) async fn delete_if_incarnation_at( + bucket: &str, + config_file: &str, + expected_incarnation_id: uuid::Uuid, + updated_at: OffsetDateTime, + ) -> Result { + super::ecstore_bucket::metadata_sys::delete_if_incarnation_at(bucket, config_file, expected_incarnation_id, updated_at) + .await + } + pub(crate) async fn get_bucket_policy(bucket: &str) -> Result<(BucketPolicy, OffsetDateTime)> { super::ecstore_bucket::metadata_sys::get_bucket_policy(bucket).await } diff --git a/rustfs/src/app/storage_api.rs b/rustfs/src/app/storage_api.rs index 2719ccb72..7a48d5ff1 100644 --- a/rustfs/src/app/storage_api.rs +++ b/rustfs/src/app/storage_api.rs @@ -718,6 +718,8 @@ pub(crate) mod bucket { delete_marker_version_id: None, delete_marker: false, delete_marker_mtime: None, + target_delete_marker_version_ids: Default::default(), + target_delete_marker_version_ids_corrupt: false, target_arns, force_delete_id: Some(operation_id), force_delete_generation: Some(i64::try_from(generation.unix_timestamp_nanos()).unwrap_or(i64::MAX)), diff --git a/rustfs/src/site_replication/hooks.rs b/rustfs/src/site_replication/hooks.rs index 95a830500..2af6aa000 100644 --- a/rustfs/src/site_replication/hooks.rs +++ b/rustfs/src/site_replication/hooks.rs @@ -302,7 +302,164 @@ pub(crate) fn site_replication_state_replicates_ilm_expiry(state: &SiteReplicati state.peers.values().any(|peer| peer.replicate_ilm_expiry) } -pub(crate) fn site_replication_bootstrap_plan(info: &SRInfo) -> S3Result { +/// Secret-bearing half of the IAM snapshot. `SRInfo` is served to admin +/// callers (`site-replication/info`, status, add preflight) and must stay +/// secret-free, so the bootstrap plan receives credentials through this +/// separate value, built only on the paths that deliver to peers (site add +/// bootstrap, repair, retry snapshot resend). Never persisted, never served. +#[derive(Debug, Clone, Default)] +pub(crate) struct SiteReplicationIamCredentials { + /// Built-in users (access key -> credential); temp and service accounts + /// are excluded, external/IdP users never appear here. + pub(crate) users: BTreeMap, + /// Every service account except the site replicator's own, already + /// shaped as the `service-account` create item the live hook emits. + pub(crate) service_accounts: Vec, +} + +#[derive(Debug, Clone)] +pub(crate) struct SiteReplicationUserCredential { + pub(crate) secret_key: String, + pub(crate) status: AccountStatus, + /// The user record's own update time (the axis the receiver's staleness + /// check compares against), unlike `UserInfo::updated_at` which + /// `list_users` overwrites with the policy mapping's time. + pub(crate) updated_at: Option, +} + +#[derive(Debug, Clone)] +pub(crate) struct SiteReplicationServiceAccountSnapshot { + pub(crate) create: SRSvcAccCreate, + pub(crate) envelope: Option, + pub(crate) updated_at: Option, +} + +pub(crate) const SERVICE_ACCOUNT_ENVELOPE_VERSION: u64 = 2; + +pub(crate) fn encode_service_account_replication_policy( + claims: &HashMap, + session_policy: Option<&str>, +) -> S3Result<(SRSessionPolicy, Option)> { + if !claims.contains_key(OIDC_VIRTUAL_PARENT_CLAIM) { + return session_policy + .map(SRSessionPolicy::from_json) + .transpose() + .map(|policy| policy.unwrap_or_default()) + .map(|policy| (policy, None)) + .map_err(|err| s3_error!(InvalidArgument, "marshal policy failed: {:?}", err)); + } + + let policy = match session_policy { + Some(policy) => serde_json::from_str::(policy) + .map_err(|err| s3_error!(InvalidArgument, "invalid service account replication policy: {:?}", err))?, + None => Policy::default(), + }; + if policy.statements.is_empty() && (!policy.id.is_empty() || !policy.version.is_empty()) + || policy.version.is_empty() && !policy.statements.is_empty() + { + return Err(s3_error!(InvalidArgument, "service account replication policy is not normalized")); + } + let policy = serde_json::to_string(&policy) + .map_err(|err| s3_error!(InternalError, "marshal service account replication policy failed: {:?}", err))?; + let policy = SRSessionPolicy::from_json(&policy) + .map_err(|err| s3_error!(InternalError, "marshal service account replication policy failed: {:?}", err))?; + Ok(( + policy, + Some(SRSvcAccReplicationEnvelope { + version: SERVICE_ACCOUNT_ENVELOPE_VERSION, + }), + )) +} + +/// Read the credentials the IAM snapshot needs straight from the IAM store: +/// `list_users` deliberately strips secret keys and skips service accounts, +/// which is right for an admin listing and wrong for a peer snapshot (the +/// plan builder used to drop every user for lack of a secret, so a status +/// change or secret rotation committed while a peer was unreachable never +/// reached it — backlog#2289). +pub(crate) async fn build_sr_iam_credentials() -> S3Result { + let mut credentials = SiteReplicationIamCredentials::default(); + let Some(iam_sys) = current_iam_handle() else { + return Ok(credentials); + }; + + let mut users = HashMap::new(); + iam_sys.load_users(UserType::Reg, &mut users).await.map_err(ApiError::from)?; + for (access_key, identity) in users { + if identity.credentials.is_temp() || identity.credentials.is_service_account() { + continue; + } + credentials.users.insert( + access_key, + SiteReplicationUserCredential { + secret_key: identity.credentials.secret_key, + status: if identity.credentials.status == "off" { + AccountStatus::Disabled + } else { + AccountStatus::Enabled + }, + updated_at: identity.update_at, + }, + ); + } + + let mut service_accounts = HashMap::new(); + iam_sys + .load_users(UserType::Svc, &mut service_accounts) + .await + .map_err(ApiError::from)?; + let mut service_accounts: Vec<_> = service_accounts.into_iter().collect(); + service_accounts.sort_by(|(a, _), (b, _)| a.cmp(b)); + for (access_key, identity) in service_accounts { + // The replicator account is installed by join / rotate, never by a snapshot. + if access_key == SITE_REPLICATOR_SERVICE_ACCOUNT || !identity.credentials.is_service_account() { + continue; + } + let claims = iam_sys.get_claims_for_svc_acc(&access_key).await.map_err(ApiError::from)?; + let (account, session_policy) = iam_sys.get_service_account(&access_key).await.map_err(ApiError::from)?; + let session_policy = session_policy + .map(|policy| serde_json::to_string(&policy)) + .transpose() + .map_err(|err| { + S3Error::with_message( + S3ErrorCode::InternalError, + format!("marshal service account session policy failed: {err:?}"), + ) + })?; + let (session_policy, envelope) = encode_service_account_replication_policy(&claims, session_policy.as_deref())?; + credentials.service_accounts.push(SiteReplicationServiceAccountSnapshot { + create: SRSvcAccCreate { + parent: identity.credentials.parent_user, + access_key, + secret_key: identity.credentials.secret_key, + groups: identity.credentials.groups.unwrap_or_default(), + claims, + session_policy, + status: identity.credentials.status, + name: account.name.unwrap_or_default(), + description: account.description.unwrap_or_default(), + expiration: account.expiration, + api_version: Some(SITE_REPL_API_VERSION.to_string()), + }, + envelope, + updated_at: identity.update_at, + }); + } + + Ok(credentials) +} + +/// The bootstrap plan for peer delivery: `info` (secret-free) plus the IAM +/// credentials read at this moment. +pub(crate) async fn build_site_replication_bootstrap_plan(info: &SRInfo) -> S3Result { + let credentials = build_sr_iam_credentials().await?; + site_replication_bootstrap_plan(info, &credentials) +} + +pub(crate) fn site_replication_bootstrap_plan( + info: &SRInfo, + credentials: &SiteReplicationIamCredentials, +) -> S3Result { let mut plan = SiteReplicationBootstrapPlan::default(); let replicate_ilm_expiry = site_replication_info_replicates_ilm_expiry(info); @@ -318,24 +475,57 @@ pub(crate) fn site_replication_bootstrap_plan(info: &SRInfo) -> S3Result S3Result<()> { let Some(runtime) = runtime_site_replication_targets().await? else { return Ok(()); }; + // A local revoke must out-rank a stale grant a peer delivers later, so its + // mark is committed before the broadcast (backlog#2291). The broadcast + // still goes out when the mark cannot be persisted: the peers' own records + // remain the primary gate, the mark only covers the deleted case. + if let Err(err) = record_iam_deletion_marks_for_item(&item).await { + warn!( + component = LOG_COMPONENT_ADMIN, + subsystem = LOG_SUBSYSTEM_SITE_REPLICATION, + event = EVENT_ADMIN_SITE_REPLICATION_STATE, + item_type = %item.r#type, + result = "iam_deletion_mark_not_recorded", + error = ?err, + "failed to record local IAM deletion mark before broadcast" + ); + } let mut first_error: Option = None; for peer in runtime.state.peers.values() { if peer.deployment_id == runtime.local_peer.deployment_id diff --git a/rustfs/src/site_replication/mod.rs b/rustfs/src/site_replication/mod.rs index 630c50576..675aa5b5d 100644 --- a/rustfs/src/site_replication/mod.rs +++ b/rustfs/src/site_replication/mod.rs @@ -79,13 +79,16 @@ use http::header::{CONTENT_TYPE, HOST}; use http::{HeaderMap, HeaderValue, Uri}; use hyper::{Method, StatusCode}; use rustfs_config::{DEFAULT_CONSOLE_ADDRESS, DEFAULT_RUSTFS_TLS_PATH, ENV_RUSTFS_CONSOLE_ADDRESS, ENV_RUSTFS_TLS_PATH}; +use rustfs_iam::federation::OIDC_VIRTUAL_PARENT_CLAIM; use rustfs_iam::store::{MappedPolicy, UserType, sr_wire_user_type}; use rustfs_iam::sys::SITE_REPLICATOR_SERVICE_ACCOUNT; use rustfs_madmin::{ - AddOrUpdateUserReq, GroupAddRemove, GroupStatus, PeerInfo, PeerSite, ReplicateEditStatus, SITE_REPL_API_VERSION, - SRBucketInfo, SRBucketMeta, SRGroupInfo, SRIAMItem, SRIAMPolicy, SRInfo, SRPolicyMapping, SRRemoveReq, SRResyncOpStatus, - SRRetryStats, SRStateInfo, SyncStatus, + AccountStatus, AddOrUpdateUserReq, GroupAddRemove, GroupStatus, PeerInfo, PeerSite, ReplicateEditStatus, + SITE_REPL_API_VERSION, SRBucketInfo, SRBucketMeta, SRGroupInfo, SRIAMItem, SRIAMPolicy, SRInfo, SRPolicyMapping, SRRemoveReq, + SRResyncOpStatus, SRRetryStats, SRSessionPolicy, SRStateInfo, SRSvcAccChange, SRSvcAccCreate, SRSvcAccDelete, + SRSvcAccReplicationEnvelope, SyncStatus, }; +use rustfs_policy::policy::Policy; use rustfs_signer::constants::UNSIGNED_PAYLOAD; use rustfs_signer::sign_v4; use rustfs_tls_runtime::{GlobalPublishedOutboundTlsState, TlsGeneration}; @@ -107,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 b2785c0fd..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(); @@ -726,7 +726,7 @@ pub(crate) async fn execute_site_replication_repair_locked( return Err(s3_error!(InvalidRequest, "site replication is not configured")); } let info = build_sr_info(&state, &request.local_peer).await?; - let plan = site_replication_bootstrap_plan(&info)?; + let plan = build_site_replication_bootstrap_plan(&info).await?; let plan_token = site_replication_repair_plan_token(&state, &plan)?; let preflight_token = site_replication_repair_preflight_token(&state, &plan, request.signing_key.as_bytes())?; let sites = site_replication_repair_sites(&state, &request.local_peer, &plan, request.signing_key.as_bytes())?; diff --git a/rustfs/src/site_replication/retry.rs b/rustfs/src/site_replication/retry.rs index 4b818c752..c9b9e73a0 100644 --- a/rustfs/src/site_replication/retry.rs +++ b/rustfs/src/site_replication/retry.rs @@ -397,12 +397,12 @@ pub(crate) fn iam_deletion_replay_matches(record: &SiteReplicationIamDeletionRep /// newer revision of one another. pub(crate) fn iam_item_deletion_entity(item: &SRIAMItem) -> Option { match item.r#type.as_str() { - "policy" if item.policy.is_none() => Some(format!("policy:{}", item.name)), + "policy" if item.policy.is_none() => Some(iam_policy_deletion_mark_entity(&item.name)), "iam-user" => item .iam_user .as_ref() .filter(|user| user.is_delete_req) - .map(|user| format!("iam-user:{}", user.access_key)), + .map(|user| iam_user_deletion_mark_entity(&user.access_key)), "group-info" => item .group_info .as_ref() @@ -416,7 +416,7 @@ pub(crate) fn iam_item_deletion_entity(item: &SRIAMItem) -> Option { .policy_mapping .as_ref() .filter(|mapping| mapping.policy.is_empty()) - .map(|mapping| format!("policy-mapping:{}:{}:{}", mapping.user_or_group, mapping.user_type, mapping.is_group)), + .map(|mapping| iam_policy_mapping_deletion_mark_entity(&mapping.user_or_group, mapping.user_type, mapping.is_group)), "service-account" => item .svc_acc_change .as_ref() @@ -426,6 +426,82 @@ pub(crate) fn iam_item_deletion_entity(item: &SRIAMItem) -> Option { } } +/// The entities whose deletion a deletion-shaped IAM item commits, keyed the +/// way the receive-side staleness gate looks them up once the local record is +/// gone (backlog#2291); empty for creates and updates. Group member removal +/// yields one entity per removed member so a stale re-add of that member can +/// be judged, and a group delete (no members) yields the group itself. +pub(crate) fn iam_item_deletion_mark_entities(item: &SRIAMItem) -> Vec { + if item.r#type == "group-info" { + let Some(update) = item + .group_info + .as_ref() + .map(|group| &group.update_req) + .filter(|update| update.is_remove) + else { + return Vec::new(); + }; + if update.members.is_empty() { + return vec![iam_group_deletion_mark_entity(&update.group)]; + } + return update + .members + .iter() + .map(|member| iam_group_member_deletion_mark_entity(&update.group, member)) + .collect(); + } + iam_item_deletion_entity(item).into_iter().collect() +} + +pub(crate) fn iam_policy_deletion_mark_entity(name: &str) -> String { + format!("policy:{name}") +} + +pub(crate) fn iam_user_deletion_mark_entity(access_key: &str) -> String { + format!("iam-user:{access_key}") +} + +/// `user_type` is the SR wire integer, as carried by the item on both sides. +pub(crate) fn iam_policy_mapping_deletion_mark_entity(user_or_group: &str, user_type: i64, is_group: bool) -> String { + format!("policy-mapping:{user_or_group}:{user_type}:{is_group}") +} + +pub(crate) fn iam_group_deletion_mark_entity(group: &str) -> String { + format!("group:{group}") +} + +pub(crate) fn iam_group_member_deletion_mark_entity(group: &str, member: &str) -> String { + format!("group-member:{group}:{member}") +} + +/// Persist the deletion marks of `item` (its source `updated_at` per entity +/// of [`iam_item_deletion_mark_entities`]) through the state transaction. +/// No-op for creates/updates and for items without a source timestamp +/// (older peers): a mark without a source clock could not be ordered against +/// later items. Called before a local deletion is broadcast and after a +/// replicated deletion is applied, so both sides out-rank a stale grant that +/// arrives later. +pub(crate) async fn record_iam_deletion_marks_for_item(item: &SRIAMItem) -> S3Result<()> { + let entities = iam_item_deletion_mark_entities(item); + let Some(deleted_at) = item.updated_at.filter(|_| !entities.is_empty()) else { + return Ok(()); + }; + commit_iam_deletion_marks(entities, deleted_at).await +} + +/// [`record_iam_deletion_marks`] under the state transaction; the write is +/// skipped when no mark moves. +pub(crate) async fn commit_iam_deletion_marks(entities: Vec, deleted_at: OffsetDateTime) -> S3Result<()> { + update_site_replication_state_when_changed(move |state| { + Ok(if record_iam_deletion_marks(state, &entities, deleted_at) { + StateCommit::Changed(()) + } else { + StateCommit::Unchanged(()) + }) + }) + .await +} + /// Failure bookkeeping for one IAM item delivery: upsert the collapsed retry /// event and, when the item is a deletion, record its body for replay. Both /// live in the same state so the caller commits them in one transaction — a @@ -791,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(); @@ -954,6 +1030,7 @@ pub(crate) enum IamSnapshotKey { User(String), Group(String), PolicyMapping { target: String, user_type: i64, is_group: bool }, + ServiceAccount(String), } pub(crate) fn iam_snapshot_key(item: &SRIAMItem) -> Option { @@ -972,6 +1049,11 @@ pub(crate) fn iam_snapshot_key(item: &SRIAMItem) -> Option { user_type: mapping.user_type, is_group: mapping.is_group, }), + "service-account" => item + .svc_acc_change + .as_ref() + .and_then(|change| change.create.as_ref()) + .map(|create| IamSnapshotKey::ServiceAccount(create.access_key.clone())), _ => None, } } @@ -1006,6 +1088,24 @@ pub(crate) fn iam_snapshot_tombstones(item: &SRIAMItem, observed_at: OffsetDateT mapping.policy.clear(); } } + "service-account" => { + let Some(access_key) = item + .svc_acc_change + .as_ref() + .and_then(|change| change.create.as_ref()) + .map(|create| create.access_key.clone()) + else { + return Vec::new(); + }; + tombstone.svc_acc_change = Some(SRSvcAccChange { + delete: Some(SRSvcAccDelete { + access_key, + api_version: Some(SITE_REPL_API_VERSION.to_string()), + }), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + ..Default::default() + }); + } _ => return Vec::new(), } vec![tombstone] @@ -1701,7 +1801,7 @@ pub(crate) async fn drain_site_replication_retry_queue_locked( // tick and only when a snapshot resend is actually due. let plan = if needs_plan { let info = build_sr_info(&runtime.state, &runtime.local_peer).await?; - Some(site_replication_bootstrap_plan(&info)?) + Some(build_site_replication_bootstrap_plan(&info).await?) } else { None }; @@ -1841,7 +1941,7 @@ pub(crate) async fn drain_one_site_replication_retry_event( } } let fresh_info = build_sr_info(&runtime.state, &runtime.local_peer).await?; - let fresh_plan = site_replication_bootstrap_plan(&fresh_info)?; + let fresh_plan = build_site_replication_bootstrap_plan(&fresh_info).await?; let fresh_snapshot = RetrySnapshot::from_plan(&action, &fresh_plan).expect("snapshot action has a snapshot"); if fresh_snapshot.fingerprint()? == current_fingerprint { if is_iam { diff --git a/rustfs/src/site_replication/state.rs b/rustfs/src/site_replication/state.rs index 04619a9ac..efc8b16e5 100644 --- a/rustfs/src/site_replication/state.rs +++ b/rustfs/src/site_replication/state.rs @@ -64,6 +64,104 @@ pub(crate) struct SiteReplicationState { /// newer edit that already landed. #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] pub(crate) applied_edit_generations: BTreeMap, + /// Source timestamp of the newest IAM deletion committed on this site, + /// keyed by the deleted entity (`iam_item_deletion_mark_entities`). A + /// deletion leaves no local record to judge a later item against, so this + /// is what lets the receive-side staleness gate reject a grant that is + /// older than the revoke it would otherwise undo (backlog#2291). Marks + /// are kept for [`SITE_REPLICATION_IAM_DELETION_MARK_RETENTION`] and never + /// evicted by count: see that constant for why a count bound would open + /// exactly the window the marks exist to close. + #[serde(default, with = "rfc3339_map", skip_serializing_if = "BTreeMap::is_empty")] + pub(crate) iam_deletion_marks: BTreeMap, +} + +/// How long an IAM deletion mark outlives the deletion it records. +/// +/// A mark fences the delivery paths that can still carry an older grant for +/// the deleted entity: a live delivery delayed in transit, the same grant +/// arriving on a sibling node while the revoke is being applied, and a +/// snapshot (bootstrap / repair / resend) built by a peer that has not yet +/// received the deletion — which is bounded by this site's own retry queue +/// towards that peer, whose backoff tops out at one day +/// (`SITE_REPLICATION_RETRY_DRAIN_MAX_BACKOFF_SECS`). The retry drain itself +/// never replays a stale grant: it resends snapshots of the current records +/// and the recorded deletion bodies. Thirty days is an order of magnitude +/// beyond every one of those windows. Marks are pruned by age only — a count +/// bound would drop a mark that is still inside the delivery window as soon +/// as enough newer deletions happen, letting the delayed grant re-create the +/// entity, which is the very hole the marks close. +pub(crate) const SITE_REPLICATION_IAM_DELETION_MARK_RETENTION: time::Duration = time::Duration::days(30); + +/// Record that deletions of `entities` with source timestamp `deleted_at` +/// were committed here. Newest wins per entity: an older deletion never +/// lowers a mark. Marks older than the retention are pruned in the same +/// pass. Returns whether the state changed. +pub(crate) fn record_iam_deletion_marks( + state: &mut SiteReplicationState, + entities: &[String], + deleted_at: OffsetDateTime, +) -> bool { + record_iam_deletion_marks_at(state, entities, deleted_at, OffsetDateTime::now_utc()) +} + +/// [`record_iam_deletion_marks`] pruning against an explicit `now`. +pub(crate) fn record_iam_deletion_marks_at( + state: &mut SiteReplicationState, + entities: &[String], + deleted_at: OffsetDateTime, + now: OffsetDateTime, +) -> bool { + let mut changed = false; + for entity in entities { + if state + .iam_deletion_marks + .get(entity) + .is_some_and(|existing| *existing >= deleted_at) + { + continue; + } + state.iam_deletion_marks.insert(entity.clone(), deleted_at); + changed = true; + } + let expired_before = now - SITE_REPLICATION_IAM_DELETION_MARK_RETENTION; + let before = state.iam_deletion_marks.len(); + state.iam_deletion_marks.retain(|_, deleted_at| *deleted_at >= expired_before); + changed || state.iam_deletion_marks.len() != before +} + +/// Newest deletion mark among `entities`, or `None` when no deletion of any +/// of them was recorded here. The receive-side staleness gate feeds this in +/// as the local timestamp when the targeted record is absent. +pub(crate) fn iam_deletion_mark(state: &SiteReplicationState, entities: &[String]) -> Option { + entities + .iter() + .filter_map(|entity| state.iam_deletion_marks.get(entity).copied()) + .max() +} + +/// RFC 3339 map values, matching the other timestamps in the state object +/// (`time::serde::rfc3339` only applies to a single field). +mod rfc3339_map { + use serde::{Deserialize, Deserializer, Serialize, Serializer}; + use std::collections::BTreeMap; + use time::OffsetDateTime; + + #[derive(Serialize, Deserialize)] + #[serde(transparent)] + struct Stamp(#[serde(with = "time::serde::rfc3339")] OffsetDateTime); + + pub(super) fn serialize(map: &BTreeMap, serializer: S) -> Result { + serializer.collect_map(map.iter().map(|(entity, deleted_at)| (entity, Stamp(*deleted_at)))) + } + + pub(super) fn deserialize<'de, D: Deserializer<'de>>(deserializer: D) -> Result, D::Error> { + let map = BTreeMap::::deserialize(deserializer)?; + Ok(map + .into_iter() + .map(|(entity, Stamp(deleted_at))| (entity, deleted_at)) + .collect()) + } } #[derive(Debug, Clone, Serialize, Deserialize, Default)] @@ -323,6 +421,33 @@ where update_site_replication_state_when_changed(move |state| update(state).map(StateCommit::Changed)).await } +/// The state transaction for work that has to await inside it: an IAM write +/// that must be ordered with the staleness verdict taken before it and the +/// deletion mark committed after it (backlog#2291). Same boundary as +/// [`update_site_replication_state`] — load and persist under the +/// distributed state-object write lock, so two nodes of this site cannot +/// interleave their verdicts and writes — and the same rules inside: no peer +/// network calls and no other config locks. The closure hands the state back +/// as `Some` when it changed it; `None` skips the write. +pub(crate) async fn with_site_replication_state_transaction(transaction: F) -> S3Result +where + T: Send + 'static, + F: FnOnce(SiteReplicationState) -> Fut + Send + 'static, + Fut: std::future::Future)>> + Send + 'static, +{ + with_site_replication_state_lock(move || async move { + let store = current_object_store_handle() + .ok_or_else(|| S3Error::with_message(S3ErrorCode::InternalError, "Not init".to_string()))?; + let state = load_site_replication_state_no_lock(store.clone()).await?; + let (result, changed) = transaction(state).await?; + if let Some(state) = changed { + persist_site_replication_state_no_lock(store, state).await?; + } + Ok(result) + }) + .await +} + /// [`update_site_replication_state`] for closures that may find nothing to /// do — see [`StateCommit`]. pub(crate) async fn update_site_replication_state_when_changed(update: F) -> S3Result diff --git a/rustfs/src/site_replication/tests.rs b/rustfs/src/site_replication/tests.rs index 3b2c48666..f1c06131f 100644 --- a/rustfs/src/site_replication/tests.rs +++ b/rustfs/src/site_replication/tests.rs @@ -554,6 +554,145 @@ fn test_iam_item_deletion_entity_shapes() { assert!(iam_item_deletion_entity(&policy_set).is_none()); } +/// Deletion marks (backlog#2291) key on the same entities as the replay +/// records, except that a group member removal is marked per member (so a +/// stale re-add of one member can be judged) and a group delete marks the +/// group itself. Creates and updates leave no mark. +#[test] +fn test_iam_item_deletion_mark_entities_shapes() { + assert_eq!( + iam_item_deletion_mark_entities(&user_delete_item("alice")), + vec!["iam-user:alice".to_string()] + ); + assert_eq!( + iam_item_deletion_mark_entities(&policy_delete_item("readonly")), + vec!["policy:readonly".to_string()] + ); + + let mut group_remove = SRIAMItem { + r#type: "group-info".to_string(), + group_info: Some(SRGroupInfo { + update_req: GroupAddRemove { + group: "devs".to_string(), + members: vec!["bob".to_string(), "alice".to_string()], + status: GroupStatus::Enabled, + is_remove: true, + }, + api_version: None, + }), + ..Default::default() + }; + assert_eq!( + iam_item_deletion_mark_entities(&group_remove), + vec!["group-member:devs:bob".to_string(), "group-member:devs:alice".to_string()] + ); + group_remove + .group_info + .as_mut() + .expect("group info") + .update_req + .members + .clear(); + assert_eq!( + iam_item_deletion_mark_entities(&group_remove), + vec!["group:devs".to_string()], + "a removal without members deletes the group" + ); + group_remove.group_info.as_mut().expect("group info").update_req.is_remove = false; + assert!(iam_item_deletion_mark_entities(&group_remove).is_empty()); + + let mapping_clear = SRIAMItem { + r#type: "policy-mapping".to_string(), + policy_mapping: Some(SRPolicyMapping { + user_or_group: "alice".to_string(), + user_type: 0, + is_group: false, + policy: String::new(), + ..Default::default() + }), + ..Default::default() + }; + assert_eq!( + iam_item_deletion_mark_entities(&mapping_clear), + vec!["policy-mapping:alice:0:false".to_string()] + ); + + let mut user_create = user_delete_item("alice"); + user_create.iam_user.as_mut().expect("iam user").is_delete_req = false; + assert!(iam_item_deletion_mark_entities(&user_create).is_empty()); +} + +/// Newest wins per entity, marks are pruned by age only (never by count: a +/// count bound would drop a mark still inside the delivery window as soon as +/// enough newer deletions happen), and the timestamps survive the state +/// object as RFC 3339. +#[test] +fn test_record_iam_deletion_marks_newest_wins_and_expires_by_age_only() { + let at = |seconds: i64| OffsetDateTime::UNIX_EPOCH + time::Duration::seconds(seconds); + let now = at(1_000_000); + let mut state = SiteReplicationState::default(); + let alice = vec!["iam-user:alice".to_string()]; + + assert!(record_iam_deletion_marks_at(&mut state, &alice, at(20), now)); + assert!( + !record_iam_deletion_marks_at(&mut state, &alice, at(10), now), + "an older deletion does not move the mark" + ); + assert!( + !record_iam_deletion_marks_at(&mut state, &alice, at(20), now), + "a replayed deletion is not a change" + ); + assert_eq!(iam_deletion_mark(&state, &alice), Some(at(20))); + assert!(record_iam_deletion_marks_at(&mut state, &alice, at(30), now)); + assert_eq!(iam_deletion_mark(&state, &alice), Some(at(30))); + assert_eq!(iam_deletion_mark(&state, &["iam-user:bob".to_string()]), None); + assert!(!record_iam_deletion_marks_at(&mut state, &[], at(40), now)); + + // Many newer deletions never evict an older mark that is still within the retention. + let members: Vec = (0..4096).map(|index| format!("group-member:devs:user-{index:04}")).collect(); + for (index, member) in members.iter().enumerate() { + record_iam_deletion_marks_at(&mut state, std::slice::from_ref(member), at(100 + index as i64), now); + } + assert_eq!(state.iam_deletion_marks.len(), members.len() + 1); + assert_eq!(iam_deletion_mark(&state, &alice), Some(at(30)), "no count-based eviction"); + + // Marks older than the retention are pruned, on the pass that records a + // newer one and on a pass that changes nothing else; younger ones stay. + let later = at(100) + SITE_REPLICATION_IAM_DELETION_MARK_RETENTION; + assert!( + record_iam_deletion_marks_at(&mut state, &["iam-user:carol".to_string()], at(200_000), later), + "pruning alone is a change" + ); + assert_eq!(iam_deletion_mark(&state, &alice), None, "alice's mark aged out"); + assert_eq!( + iam_deletion_mark(&state, &members[..1]), + Some(at(100)), + "a mark exactly at the retention edge stays, and so do the younger ones" + ); + assert_eq!(state.iam_deletion_marks.len(), members.len() + 1); + assert_eq!(iam_deletion_mark(&state, &["iam-user:carol".to_string()]), Some(at(200_000))); + let mut state = SiteReplicationState::default(); + record_iam_deletion_marks_at(&mut state, &alice, at(30), now); + let past_edge = at(30) + SITE_REPLICATION_IAM_DELETION_MARK_RETENTION + time::Duration::seconds(1); + assert!( + record_iam_deletion_marks_at(&mut state, &[], at(0), past_edge), + "a pass that only prunes reports the change" + ); + assert_eq!(iam_deletion_mark(&state, &alice), None); + record_iam_deletion_marks_at(&mut state, &alice, at(30), now); + + let json = serde_json::to_value(&state).expect("serialize state"); + assert_eq!(json["iam_deletion_marks"]["iam-user:alice"], serde_json::json!("1970-01-01T00:00:30Z")); + let reloaded = parse_site_replication_state(&serde_json::to_vec(&state).expect("serialize state")).expect("parse state"); + assert_eq!(reloaded.iam_deletion_marks, state.iam_deletion_marks); + assert!( + parse_site_replication_state(br#"{"name":"a","service_account_access_key":"","service_account_parent":"","peers":{},"updated_at":null,"resync_status":{}}"#) + .expect("state without marks") + .iam_deletion_marks + .is_empty() + ); +} + /// A failed deletion delivery persists a replay record next to the collapsed /// retry entry; a fresh entry is stamped `deletions_recorded` so a later /// replay can settle it, and a repeated deletion of the same entity keeps the @@ -1679,7 +1818,8 @@ fn test_site_replication_bootstrap_plan_includes_replayable_snapshot_items() { }, ); - let plan = site_replication_bootstrap_plan(&info).expect("bootstrap plan should build"); + let plan = + site_replication_bootstrap_plan(&info, &SiteReplicationIamCredentials::default()).expect("bootstrap plan should build"); assert_eq!(plan.iam_items.iter().map(|item| item.r#type.as_str()).collect::>(), { vec!["policy", "iam-user", "group-info", "policy-mapping"] @@ -1717,7 +1857,8 @@ fn test_site_replication_bootstrap_plan_skips_lifecycle_by_default() { }, ); - let plan = site_replication_bootstrap_plan(&info).expect("bootstrap plan should build"); + let plan = + site_replication_bootstrap_plan(&info, &SiteReplicationIamCredentials::default()).expect("bootstrap plan should build"); assert!(!plan.bucket_items.iter().any(|item| item.r#type == "lc-config")); } @@ -1748,7 +1889,8 @@ fn test_site_replication_bootstrap_plan_emits_timestamped_lifecycle_delete() { }, ); - let plan = site_replication_bootstrap_plan(&info).expect("bootstrap plan should build"); + let plan = + site_replication_bootstrap_plan(&info, &SiteReplicationIamCredentials::default()).expect("bootstrap plan should build"); let item = plan .bucket_items @@ -1935,8 +2077,8 @@ fn test_site_replication_repair_preflight_token_is_deterministic_for_equal_state }, ); - let plan_a = site_replication_bootstrap_plan(&info).expect("first plan"); - let plan_b = site_replication_bootstrap_plan(&info).expect("second plan"); + let plan_a = site_replication_bootstrap_plan(&info, &SiteReplicationIamCredentials::default()).expect("first plan"); + let plan_b = site_replication_bootstrap_plan(&info, &SiteReplicationIamCredentials::default()).expect("second plan"); let token_a = site_replication_repair_preflight_token(&state, &plan_a, b"test-signing-key").expect("first token"); let token_b = site_replication_repair_preflight_token(&state, &plan_b, b"test-signing-key").expect("second token"); @@ -3219,3 +3361,345 @@ fn test_reconcile_adds_missing_peer_rules_to_existing_config() { assert!(rule_ids.contains(&"site-repl-dep-b")); assert!(rule_ids.contains(&"site-repl-dep-c")); } + +/// backlog#2289: the IAM snapshot (retry resend, repair, site-add bootstrap) +/// used to be built from `list_users`, whose `UserInfo` never carries a +/// secret key, so the plan dropped every user and a status change or secret +/// rotation committed while a peer was unreachable never reached it. The +/// credentials now come from a separate store read; SRInfo stays secret-free. +#[test] +fn test_bootstrap_plan_carries_users_from_the_credential_snapshot() { + let mut info = SRInfo::default(); + // Exactly what `list_users` builds: status, policy, updated_at — never secret_key. + info.user_info_map.insert( + "alice".to_string(), + rustfs_madmin::UserInfo { + status: rustfs_madmin::AccountStatus::Disabled, + policy_name: Some("readwrite".to_string()), + updated_at: Some(OffsetDateTime::from_unix_timestamp(1_700_000_000).expect("timestamp")), + ..Default::default() + }, + ); + info.user_info_map.insert( + "external-idp-user".to_string(), + rustfs_madmin::UserInfo { + status: rustfs_madmin::AccountStatus::Enabled, + ..Default::default() + }, + ); + let user_updated_at = OffsetDateTime::from_unix_timestamp(1_700_000_500).expect("timestamp"); + let mut credentials = SiteReplicationIamCredentials::default(); + credentials.users.insert( + "alice".to_string(), + SiteReplicationUserCredential { + secret_key: "alice-secret".to_string(), + status: rustfs_madmin::AccountStatus::Disabled, + updated_at: Some(user_updated_at), + }, + ); + + let plan = site_replication_bootstrap_plan(&info, &credentials).expect("bootstrap plan should build"); + + let users: Vec<_> = plan.iam_items.iter().filter(|item| item.r#type == "iam-user").collect(); + assert_eq!(users.len(), 1, "only the user with a credential travels: {:?}", plan.iam_items); + let alice = users[0].iam_user.as_ref().expect("iam user body"); + assert_eq!(alice.access_key, "alice"); + let req = alice.user_req.as_ref().expect("user request"); + assert_eq!(req.secret_key, "alice-secret"); + assert_eq!(req.status, rustfs_madmin::AccountStatus::Disabled); + assert_eq!(req.policy.as_deref(), Some("readwrite")); + // the user record's own axis, not the policy-mapping time list_users reports + assert_eq!(users[0].updated_at, Some(user_updated_at)); +} + +fn service_account_snapshot(access_key: &str, parent: &str, status: &str) -> SiteReplicationServiceAccountSnapshot { + SiteReplicationServiceAccountSnapshot { + create: rustfs_madmin::SRSvcAccCreate { + parent: parent.to_string(), + access_key: access_key.to_string(), + secret_key: format!("{access_key}-secret"), + groups: Vec::new(), + claims: HashMap::new(), + session_policy: SRSessionPolicy::default(), + status: status.to_string(), + name: String::new(), + description: String::new(), + expiration: None, + api_version: Some(SITE_REPL_API_VERSION.to_string()), + }, + envelope: None, + updated_at: Some(OffsetDateTime::from_unix_timestamp(1_700_000_600).expect("timestamp")), + } +} + +/// backlog#2289: service accounts were absent from every snapshot (the +/// listing filters them). They now travel as the create item the live hook +/// emits — after their parents — carrying secret and status. +#[test] +fn test_bootstrap_plan_emits_service_accounts_after_their_parents() { + let mut info = SRInfo::default(); + info.user_info_map + .insert("alice".to_string(), rustfs_madmin::UserInfo::default()); + let mut credentials = SiteReplicationIamCredentials::default(); + credentials.users.insert( + "alice".to_string(), + SiteReplicationUserCredential { + secret_key: "alice-secret".to_string(), + status: rustfs_madmin::AccountStatus::Enabled, + updated_at: None, + }, + ); + credentials + .service_accounts + .push(service_account_snapshot("alice-svc", "alice", "off")); + + let plan = site_replication_bootstrap_plan(&info, &credentials).expect("bootstrap plan should build"); + + let types: Vec<_> = plan.iam_items.iter().map(|item| item.r#type.as_str()).collect(); + assert_eq!(types, vec!["iam-user", "service-account"]); + let change = plan.iam_items[1].svc_acc_change.as_ref().expect("service account change"); + let create = change.create.as_ref().expect("create body"); + assert_eq!((create.access_key.as_str(), create.parent.as_str()), ("alice-svc", "alice")); + assert_eq!(create.secret_key, "alice-svc-secret"); + assert_eq!(create.status, "off", "a disabled account must arrive disabled"); + assert!(change.delete.is_none() && change.update.is_none()); +} + +/// A service account present in the previous snapshot but gone from the +/// fresh one is replayed as an explicit delete, like the other IAM kinds. +#[test] +fn test_retry_snapshot_tombstones_removed_service_accounts() { + let observed_at = OffsetDateTime::from_unix_timestamp(1_700_001_000).expect("timestamp"); + let mut info = SRInfo::default(); + info.user_info_map + .insert("alice".to_string(), rustfs_madmin::UserInfo::default()); + let mut credentials = SiteReplicationIamCredentials::default(); + credentials.users.insert( + "alice".to_string(), + SiteReplicationUserCredential { + secret_key: "alice-secret".to_string(), + status: rustfs_madmin::AccountStatus::Enabled, + updated_at: None, + }, + ); + let mut with_account = credentials.clone(); + with_account + .service_accounts + .push(service_account_snapshot("alice-svc", "alice", "on")); + let previous = site_replication_bootstrap_plan(&info, &with_account).expect("previous plan"); + let fresh = site_replication_bootstrap_plan(&info, &credentials).expect("fresh plan"); + + let replay = RetrySnapshot::replay_after_change( + &RetrySnapshot::Iam(previous.iam_items), + &RetrySnapshot::Iam(fresh.iam_items), + observed_at, + ); + let RetrySnapshot::Iam(items) = replay else { + panic!("IAM snapshot expected"); + }; + let tombstone = items + .iter() + .find(|item| item.r#type == "service-account") + .expect("service account tombstone"); + let change = tombstone.svc_acc_change.as_ref().expect("change"); + assert_eq!(change.delete.as_ref().map(|delete| delete.access_key.as_str()), Some("alice-svc")); + assert!(change.create.is_none()); + assert_eq!(tombstone.updated_at, Some(observed_at)); +} + +/// Spawns a one-shot HTTP peer that answers 200 and flips the returned flag +/// once a request head has arrived. +async fn spawn_reached_probe_peer() -> (String, Arc, tokio::task::JoinHandle<()>) { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind healthy peer"); + let endpoint = format!("http://{}", listener.local_addr().expect("healthy peer address")); + let reached = Arc::new(AtomicBool::new(false)); + let reached_by_server = reached.clone(); + let server = tokio::spawn(async move { + let Ok((mut stream, _)) = listener.accept().await else { + return; + }; + let mut request = Vec::new(); + let mut buffer = [0_u8; 1024]; + loop { + let Ok(read) = stream.read(&mut buffer).await else { + return; + }; + if read == 0 { + return; + } + request.extend_from_slice(&buffer[..read]); + if request.windows(4).any(|window| window == b"\r\n\r\n") { + break; + } + } + reached_by_server.store(true, Ordering::SeqCst); + let _ = stream + .write_all(b"HTTP/1.1 200 OK\r\ncontent-length: 2\r\nconnection: close\r\n\r\nok") + .await; + }); + (endpoint, reached, server) +} + +/// Three-peer runtime whose local peer is `local`; BTreeMap order visits the +/// failing peer `b` before the healthy peer `c`. +fn broadcast_runtime_with_failing_peer_before_healthy(failing_endpoint: &str, healthy_endpoint: &str) -> SiteReplicationRuntime { + let local_peer = PeerInfo { + deployment_id: "local".to_string(), + ..peer("local", "http://127.0.0.1:9") + }; + let mut state = SiteReplicationState { + name: "local".to_string(), + service_account_access_key: "site-replicator-0".to_string(), + ..Default::default() + }; + state.peers.insert("local".to_string(), local_peer.clone()); + state.peers.insert( + "b".to_string(), + PeerInfo { + deployment_id: "b".to_string(), + ..peer("b", failing_endpoint) + }, + ); + state.peers.insert( + "c".to_string(), + PeerInfo { + deployment_id: "c".to_string(), + ..peer("c", healthy_endpoint) + }, + ); + SiteReplicationRuntime { + state, + local_peer, + service_account_secret_key: "site-replicator-secret".to_string(), + } +} + +const BROADCAST_PROBE_DELETE_BUCKET_PATH: &str = + "/rustfs/admin/v3/site-replication/peer/bucket-ops?bucket=photos&operation=delete-bucket"; + +/// The generic JSON broadcast (bucket make/delete, bucket-meta hook, bucket +/// ops) attempts every remote peer: a peer whose request fails must not stop +/// delivery to the peers that follow it in deployment-id order, and the +/// failure is still reported to the caller (backlog#2293). +#[tokio::test] +#[serial] +async fn test_broadcast_json_reaches_healthy_peers_after_a_failed_peer() { + // Peer "b": nothing listens on the port, so the connect is refused. + let refused = TcpListener::bind("127.0.0.1:0").await.expect("bind refused-peer probe"); + let refused_endpoint = format!("http://{}", refused.local_addr().expect("refused-peer address")); + drop(refused); + + let (healthy_endpoint, reached, server) = spawn_reached_probe_peer().await; + let runtime = broadcast_runtime_with_failing_peer_before_healthy(&refused_endpoint, &healthy_endpoint); + + let result = temp_env::async_with_vars([(ALLOW_LOOPBACK_REPLICATION_TARGET_ENV, Some("true"))], async { + broadcast_site_replication_json_with_runtime(&runtime, BROADCAST_PROBE_DELETE_BUCKET_PATH, &serde_json::json!({})).await + }) + .await; + + let err = result.expect_err("peer b refuses connections, the broadcast must report it"); + assert!( + reached.load(Ordering::SeqCst), + "peer c never received the broadcast once peer b failed: {err}" + ); + server.abort(); +} + +/// Same guarantee when the failing peer never gets a transport: an endpoint +/// that `PeerTransport::for_runtime_peer` rejects must be skipped past (and +/// reported), not abort the broadcast before the healthy peers (backlog#2293). +#[tokio::test] +#[serial] +async fn test_broadcast_json_reaches_healthy_peers_after_a_peer_without_transport() { + // Peer "b": a scheme the peer connection validator refuses outright. + let forbidden_endpoint = "ftp://peer-b.example.com"; + + let (healthy_endpoint, reached, server) = spawn_reached_probe_peer().await; + let runtime = broadcast_runtime_with_failing_peer_before_healthy(forbidden_endpoint, &healthy_endpoint); + + let result = temp_env::async_with_vars([(ALLOW_LOOPBACK_REPLICATION_TARGET_ENV, Some("true"))], async { + broadcast_site_replication_json_with_runtime(&runtime, BROADCAST_PROBE_DELETE_BUCKET_PATH, &serde_json::json!({})).await + }) + .await; + + let err = result.expect_err("peer b has no usable transport, the broadcast must report it"); + assert!( + err.to_string().contains("invalid persisted site replication peer"), + "the reported error must be peer b's transport failure: {err}" + ); + assert!( + reached.load(Ordering::SeqCst), + "peer c never received the broadcast once peer b failed to get a transport: {err}" + ); + 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" + ); +} diff --git a/rustfs/src/site_replication/transport.rs b/rustfs/src/site_replication/transport.rs index bc941c9f0..18d4850b9 100644 --- a/rustfs/src/site_replication/transport.rs +++ b/rustfs/src/site_replication/transport.rs @@ -876,6 +876,14 @@ pub(crate) async fn broadcast_site_replication_json(path: &str, bo broadcast_site_replication_json_with_runtime(&runtime, path, body).await } +/// PUT `body` to `path` on every remote peer of the runtime. +/// +/// Every peer is attempted: one peer's failure — transport construction +/// included — must not skip the peers that follow it in deployment-id order, +/// or they silently miss the change with no retry record (backlog#2293). A +/// success settles the peer/path's queued retry event, a failure enqueues one +/// under the request `path` (so the drain classifies it as today), and the +/// first error is returned once all peers were attempted. pub(crate) async fn broadcast_site_replication_json_with_runtime( runtime: &SiteReplicationRuntime, path: &str, @@ -883,20 +891,30 @@ pub(crate) async fn broadcast_site_replication_json_with_runtime( ) -> S3Result<()> { let state = &runtime.state; let local_peer = &runtime.local_peer; + let mut first_error: Option = None; for peer in state.peers.values() { if peer.deployment_id == local_peer.deployment_id || same_identity_endpoint(&peer.endpoint, &local_peer.endpoint) { continue; } - let transport = PeerTransport::for_runtime_peer(peer).await?; - PeerAdminRequest::put(&transport.connection, path, &state.service_account_access_key) - .with_client(&transport.client) - .send_with_retry_event(peer, &runtime.service_account_secret_key, body) - .await?; + let sent = match PeerTransport::for_runtime_peer(peer).await { + Ok(transport) => PeerAdminRequest::put(&transport.connection, path, &state.service_account_access_key) + .with_client(&transport.client) + .send_with_retry_event(peer, &runtime.service_account_secret_key, body) + .await + .map(|_| ()), + Err(err) => { + enqueue_site_replication_retry_event(peer, path, &err).await; + Err(err) + } + }; + if let Err(err) = sent { + first_error.get_or_insert(err); + } } - Ok(()) + first_error.map_or(Ok(()), Err) } pub(crate) fn parse_endpoint_refresh_status(peer: &PeerInfo, body: &[u8]) -> S3Result<()> {