From dff214240889604faff2618d9790fb79249f40d1 Mon Sep 17 00:00:00 2001 From: overtrue Date: Sun, 6 Sep 2026 02:20:59 +0800 Subject: [PATCH] feat(admin): opt-in replacement of unreadable bucket targets --- crates/ecstore/src/api/mod.rs | 2 +- .../ecstore/src/bucket/bucket_target_sys.rs | 126 +++++++++++++++++- rustfs/src/admin/handlers/replication.rs | 46 ++++++- rustfs/src/admin/storage_api.rs | 1 + 4 files changed, 167 insertions(+), 8 deletions(-) diff --git a/crates/ecstore/src/api/mod.rs b/crates/ecstore/src/api/mod.rs index e752713ff..be6346443 100644 --- a/crates/ecstore/src/api/mod.rs +++ b/crates/ecstore/src/api/mod.rs @@ -32,7 +32,7 @@ pub mod bucket { pub mod bucket_target_sys { pub use crate::bucket::bucket_target_sys::{ AdvancedPutOptions, BucketTargetError, BucketTargetSys, PutObjectOptions, RemoveObjectOptions, S3ClientError, - SsecPassthroughCapability, TargetClient, append_version_id_query, + SsecPassthroughCapability, TargetClient, UnreadableTargetsPolicy, append_version_id_query, }; } diff --git a/crates/ecstore/src/bucket/bucket_target_sys.rs b/crates/ecstore/src/bucket/bucket_target_sys.rs index db4e80f29..ce8ec5a67 100644 --- a/crates/ecstore/src/bucket/bucket_target_sys.rs +++ b/crates/ecstore/src/bucket/bucket_target_sys.rs @@ -369,6 +369,26 @@ struct SsecPassthroughRecord { recorded_at: Instant, } +/// What a target write does when the bucket's persisted target set exists but +/// cannot be decoded. +/// +/// `docs/architecture/remote-credential-sealing-adr.md` forbids rewriting a +/// configuration that could not be fully read, because re-serializing a +/// partial in-memory view is the one mechanism by which a configured target +/// really disappears. That rule guards against an *unintentional* overwrite, +/// so an operator who names the hazard keeps a repair path +/// (rustfs/backlog#2309); everything that does not name it stays refused. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum UnreadableTargetsPolicy { + /// Refuse the write with [`BucketTargetError::BucketRemoteTargetsUnreadable`]. + #[default] + FailClosed, + /// Discard the unreadable set; the target being written becomes the whole + /// configuration. Reachable only from an admin request that asked for it + /// explicitly, and audited by the caller. + Replace, +} + #[derive(Debug, Default)] pub struct BucketTargetSys { pub arn_remotes_map: Arc>>, @@ -791,20 +811,45 @@ impl BucketTargetSys { bucket: &str, target: &BucketTarget, update: bool, + unreadable_policy: UnreadableTargetsPolicy, ) -> Result { self.validate_target(bucket, target).await?; - let mut bucket_targets = match self.list_bucket_targets(bucket).await { - Ok(targets) => targets, - Err(BucketTargetError::BucketRemoteTargetNotFound { .. }) => BucketTargets::default(), - Err(err) => return Err(err), - }; + let mut bucket_targets = self.targets_base_for_write(bucket, unreadable_policy).await?; Self::upsert_target_entry(&mut bucket_targets.targets, target, update)?; Ok(bucket_targets) } + /// The persisted target set a write merges into. + /// + /// An absent configuration starts from the empty set. An unreadable one is + /// refused, because re-serializing a partial view of a set this node could + /// not decode is how a configured target disappears for good — unless the + /// caller carries the operator's explicit + /// [`UnreadableTargetsPolicy::Replace`] opt-in, which discards it + /// deliberately (rustfs/backlog#2309). + async fn targets_base_for_write( + &self, + bucket: &str, + unreadable_policy: UnreadableTargetsPolicy, + ) -> Result { + match self.list_bucket_targets(bucket).await { + Ok(targets) => Ok(targets), + Err(BucketTargetError::BucketRemoteTargetNotFound { .. }) => Ok(BucketTargets::default()), + // The opt-in discards only a set this node genuinely cannot read. + // A readable set still merges through the arm above, so the policy + // can never drop a target that was visible here. + Err(BucketTargetError::BucketRemoteTargetsUnreadable { .. }) + if unreadable_policy == UnreadableTargetsPolicy::Replace => + { + Ok(BucketTargets::default()) + } + Err(err) => Err(err), + } + } + pub async fn validate_target(&self, bucket: &str, target: &BucketTarget) -> Result<(), BucketTargetError> { if !target.target_type.is_valid() { return Err(BucketTargetError::BucketRemoteArnTypeInvalid { @@ -4313,4 +4358,75 @@ mod tests { let window = LastMinuteLatency::new(); assert_eq!(window.get_total().avg, Duration::from_secs(0)); } + + fn repair_target(bucket: &str, id: &str) -> BucketTarget { + BucketTarget { + source_bucket: bucket.to_string(), + endpoint: "remote.example.com".to_string(), + target_bucket: "remote".to_string(), + arn: format!("arn:rustfs:replication:us-east-1:{bucket}:{id}"), + target_type: BucketTargetType::ReplicationService, + region: "us-east-1".to_string(), + ..Default::default() + } + } + + /// rustfs/backlog#2309: after rustfs/rustfs#7172 an undecodable + /// `bucket-targets.json` left the bucket with no API repair path at all. + /// The refusal is the default and stays the default; the operator's + /// explicit opt-in is the only thing that discards the set, and it starts + /// the replacement from empty rather than from a partial view of bytes + /// this node never decoded. + #[tokio::test] + async fn an_unreadable_target_set_is_replaced_only_with_the_explicit_opt_in() { + let sys = BucketTargetSys::default(); + let bucket = "targets-repair-opt-in"; + sys.mark_targets_unreadable(bucket).await; + + assert!( + matches!( + sys.targets_base_for_write(bucket, UnreadableTargetsPolicy::FailClosed).await, + Err(BucketTargetError::BucketRemoteTargetsUnreadable { .. }) + ), + "without the opt-in an unreadable target set must still refuse the write" + ); + assert_eq!( + UnreadableTargetsPolicy::default(), + UnreadableTargetsPolicy::FailClosed, + "a caller that says nothing must get the refusal" + ); + + let base = sys + .targets_base_for_write(bucket, UnreadableTargetsPolicy::Replace) + .await + .expect("the explicit opt-in must let an operator replace an unreadable set"); + assert!( + base.is_empty(), + "the replacement must start from an empty set, never from a partial decode" + ); + } + + /// The opt-in is not a wipe switch. On a set this node can read, both + /// policies take the same merge path, so a stray `replace-unreadable=true` + /// cannot drop a visible target — which is what makes the flag safe to + /// repeat in an operator's repair script. + #[tokio::test] + async fn the_opt_in_never_discards_a_readable_target_set() { + let sys = BucketTargetSys::default(); + let bucket = "targets-repair-readable"; + let existing = repair_target(bucket, "keep"); + sys.targets_map + .write() + .await + .insert(bucket.to_string(), vec![existing.clone()]); + + for policy in [UnreadableTargetsPolicy::FailClosed, UnreadableTargetsPolicy::Replace] { + let base = sys + .targets_base_for_write(bucket, policy) + .await + .expect("a readable target set must be readable under either policy"); + assert_eq!(base.targets.len(), 1, "{policy:?} must keep the persisted target"); + assert_eq!(base.targets[0].arn, existing.arn, "{policy:?} must not rewrite the persisted target"); + } + } } diff --git a/rustfs/src/admin/handlers/replication.rs b/rustfs/src/admin/handlers/replication.rs index b7bf5ef78..00a7ca6df 100644 --- a/rustfs/src/admin/handlers/replication.rs +++ b/rustfs/src/admin/handlers/replication.rs @@ -29,7 +29,7 @@ use crate::admin::storage_api::bucket::replication::{REMOTE_TARGET_READ_ONLY_HIS use crate::admin::storage_api::bucket::target::{ BucketTarget, BucketTargetType, Credentials as TargetCredentials, LatencyStat, duration_from_secs_or_nanos, }; -use crate::admin::storage_api::bucket::target_sys::{BucketTargetError, BucketTargetSys}; +use crate::admin::storage_api::bucket::target_sys::{BucketTargetError, BucketTargetSys, UnreadableTargetsPolicy}; use crate::admin::storage_api::contract::bucket::{BucketOperations, BucketOptions}; use crate::admin::storage_api::contract::list::ListOperations as _; use crate::admin::storage_api::error::StorageError; @@ -57,6 +57,20 @@ use url::Host; const SUPPORTED_REMOTE_TARGET_API: &str = "s3v4"; +const LOG_COMPONENT_ADMIN: &str = "admin"; +const LOG_SUBSYSTEM_REPLICATION: &str = "replication"; +const EVENT_ADMIN_REMOTE_TARGET_STATE: &str = "admin_remote_target_state"; + +/// `set-remote-target?replace-unreadable=true`: the operator's explicit +/// acknowledgement that this bucket's persisted target set cannot be decoded +/// and is to be discarded (rustfs/backlog#2309). +/// +/// Without it the refusal from rustfs/rustfs#7172 stands, which is what keeps +/// an unreadable set from being silently rewritten from a partial view. The +/// flag is deliberately absent from `PutBucketReplication`: targets are +/// repaired first, then the rule is set. +const REPLACE_UNREADABLE_TARGETS_PARAM: &str = "replace-unreadable"; + /// Field groups a `set-remote-target?update=true` request may modify, mirroring /// MinIO's `TargetUpdateType` / `GetTargetUpdateOps` query contract: the update /// overlays only the requested groups onto the stored target, so a client can @@ -541,6 +555,7 @@ impl Operation for SetRemoteTargetHandler { }; let update = queries.get("update").is_some_and(|v| v == "true"); + let replace_unreadable = queries.get(REPLACE_UNREADABLE_TARGETS_PARAM).is_some_and(|v| v == "true"); warn!("set remote target, bucket: {}, update: {}", bucket, update); @@ -708,10 +723,37 @@ impl Operation for SetRemoteTargetHandler { let arn = remote_target.arn.clone(); + let unreadable_policy = if replace_unreadable { + UnreadableTargetsPolicy::Replace + } else { + UnreadableTargetsPolicy::FailClosed + }; + let discarding_unreadable_targets = replace_unreadable + && matches!( + bucket_target_sys.list_bucket_targets(bucket).await, + Err(BucketTargetError::BucketRemoteTargetsUnreadable { .. }) + ); + let targets = bucket_target_sys - .set_target(bucket, &remote_target, update) + .set_target(bucket, &remote_target, update, unreadable_policy) .await .map_err(map_bucket_target_error)?; + + // Audited only where the discard actually happened: the flag alone is + // not an event, on a readable set it changes nothing, and a refused + // write must not leave a record claiming the set was replaced. + if discarding_unreadable_targets { + warn!( + event = EVENT_ADMIN_REMOTE_TARGET_STATE, + component = LOG_COMPONENT_ADMIN, + subsystem = LOG_SUBSYSTEM_REPLICATION, + action = "set_remote_target", + result = "unreadable_targets_replaced", + bucket = %bucket, + arn = %remote_target.arn, + "admin remote target state" + ); + } let json_targets = serde_json::to_vec(&targets).map_err(|e| { error!("Serialization error: {}", e); S3Error::with_message(S3ErrorCode::InternalError, "Failed to serialize targets".to_string()) diff --git a/rustfs/src/admin/storage_api.rs b/rustfs/src/admin/storage_api.rs index ea7ae56b5..4618678b3 100644 --- a/rustfs/src/admin/storage_api.rs +++ b/rustfs/src/admin/storage_api.rs @@ -212,6 +212,7 @@ pub(crate) mod bucket_target_sys { pub(crate) type S3ClientError = super::ecstore_bucket::bucket_target_sys::S3ClientError; pub(crate) type SsecPassthroughCapability = super::ecstore_bucket::bucket_target_sys::SsecPassthroughCapability; pub(crate) type TargetClient = super::ecstore_bucket::bucket_target_sys::TargetClient; + pub(crate) type UnreadableTargetsPolicy = super::ecstore_bucket::bucket_target_sys::UnreadableTargetsPolicy; } pub(crate) mod lifecycle {