fix(replication): scope site-owned rule detection to reconciler-derived rules

The `site-repl-*` prefix alone classified any rule as site-owned, so on a
bucket outside site replication an owner's `site-repl-user` rule survived
DeleteBucketReplication (rule and target kept, success returned). Rule ids
do not reserve that namespace.

A rule is reconciler-owned only when it matches what the reconciler
derives: id `site-repl-<deployment id>` for a current remote site
replication peer and a destination ARN naming that same deployment id.
The S3 put/delete path reads the remote peer set (empty when site
replication is disabled) and keeps exactly those rules; everything else
is operator state the request replaces or deletes. An incoming rule that
claims a current peer's id is dropped so the reconciler rule's id stays
unique. The peer ingestion path and the reconciler keep their prefix
predicate unchanged.
This commit is contained in:
唐小鸭
2026-08-23 00:53:38 +08:00
parent ce9b69d811
commit a3733c1a1c
8 changed files with 227 additions and 54 deletions
+4 -3
View File
@@ -200,9 +200,10 @@ pub mod bucket {
delete_replication_state_from_config, delete_replication_version_id, get_global_replication_pool,
get_global_replication_stats, get_proxy_targets, init_background_replication,
invalid_replication_config_status_field, is_site_replication_rule, merge_incoming_replication_config,
persist_force_delete_intent, read_durable_mrf_backlog, replication_state_to_filemeta, replication_status_to_filemeta,
replication_statuses_map, replication_target_arn_deployment_id, replication_target_arns, resync_start_conflict_id,
should_remove_replication_target, should_schedule_delete_replication, should_use_existing_delete_replication_info,
merge_user_replication_config, persist_force_delete_intent, read_durable_mrf_backlog, replication_state_to_filemeta,
replication_status_to_filemeta, replication_statuses_map, replication_target_arn_deployment_id,
replication_target_arns, resync_start_conflict_id, should_remove_replication_target,
should_schedule_delete_replication, should_use_existing_delete_replication_info,
should_use_existing_delete_replication_source, unsupported_replication_config_field,
validate_replication_config_structure, validate_replication_config_target_arns, version_purge_status_to_filemeta,
};
+3 -2
View File
@@ -48,8 +48,9 @@ pub use replication_config_boundary::{
REPLICATION_CAPABILITY_CONTRACT_VERSION, REPLICATION_READ_ONLY_HISTORICAL_FIELDS, REPLICATION_WRITABLE_FIELDS,
ReplicationConfigStructureError, ReplicationConfigurationExt, ReplicationTargetValidationError,
invalid_replication_config_status_field, is_site_replication_rule, merge_incoming_replication_config,
replication_target_arn_deployment_id, replication_target_arns, should_remove_replication_target,
unsupported_replication_config_field, validate_replication_config_structure, validate_replication_config_target_arns,
merge_user_replication_config, replication_target_arn_deployment_id, replication_target_arns,
should_remove_replication_target, unsupported_replication_config_field, validate_replication_config_structure,
validate_replication_config_target_arns,
};
pub(crate) use replication_filemeta_boundary::version_purge_statuses_map;
pub use replication_filemeta_boundary::{
@@ -17,6 +17,7 @@ pub use rustfs_replication::{
REPLICATION_CAPABILITY_CONTRACT_VERSION, REPLICATION_READ_ONLY_HISTORICAL_FIELDS, REPLICATION_WRITABLE_FIELDS,
ReplicationConfigStructureError, ReplicationConfigurationExt, ReplicationRuleExt, ReplicationTargetValidationError,
invalid_replication_config_status_field, is_site_replication_rule, merge_incoming_replication_config,
replication_target_arn_deployment_id, replication_target_arns, should_remove_replication_target,
unsupported_replication_config_field, validate_replication_config_structure, validate_replication_config_target_arns,
merge_user_replication_config, replication_target_arn_deployment_id, replication_target_arns,
should_remove_replication_target, unsupported_replication_config_field, validate_replication_config_structure,
validate_replication_config_target_arns,
};
+93 -10
View File
@@ -282,10 +282,40 @@ pub fn replication_target_arn_deployment_id(arn: &str) -> Option<String> {
None
}
/// Whether `rule` is a site-replication rule (`site-repl-*` id) owned by the
/// local site's reconciler rather than authored by an operator.
/// Rule id prefix the site-replication reconciler stamps on the rules it
/// derives (`site-repl-<peer deployment id>`).
pub const SITE_REPLICATION_RULE_ID_PREFIX: &str = "site-repl-";
/// Whether `rule` carries a site-replication rule id (`site-repl-*`). The
/// reconciler and the peer ingestion path treat the whole namespace as theirs
/// on a site-replication bucket; the S3 edit path must not — rule ids are not
/// reserved, so see [`site_replication_rule_deployment_id`].
pub fn is_site_replication_rule(rule: &ReplicationRule) -> bool {
rule.id.as_deref().is_some_and(|id| id.starts_with("site-repl-"))
rule.id
.as_deref()
.is_some_and(|id| id.starts_with(SITE_REPLICATION_RULE_ID_PREFIX))
}
/// Deployment id of the peer a reconciler-derived rule replicates to, or
/// `None` for any other rule. The reconciler builds each rule from one peer:
/// the id is `site-repl-<deployment id>` and the destination ARN names that
/// same deployment id — an operator-authored `site-repl-user` rule, or a
/// `site-repl-<peer>` id pasted onto a foreign ARN, fails the agreement check.
/// Callers that know the current peer set must also confirm the id is one of
/// those peers before treating the rule as reconciler-owned.
pub fn site_replication_rule_deployment_id(rule: &ReplicationRule) -> Option<&str> {
let deployment_id = rule.id.as_deref()?.strip_prefix(SITE_REPLICATION_RULE_ID_PREFIX)?;
(!deployment_id.is_empty()
&& replication_target_arn_deployment_id(&rule.destination.bucket).as_deref() == Some(deployment_id))
.then_some(deployment_id)
}
/// Whether `rule` is one the local reconciler derived for a current remote
/// site-replication peer in `peer_deployment_ids`. With an empty peer set
/// (site replication disabled) nothing qualifies, so a bucket outside site
/// replication keeps the verbatim S3 put/delete semantics.
pub fn is_reconciler_owned_site_replication_rule(rule: &ReplicationRule, peer_deployment_ids: &HashSet<String>) -> bool {
site_replication_rule_deployment_id(rule).is_some_and(|deployment_id| peer_deployment_ids.contains(deployment_id))
}
/// Merge an incoming replication config into the local one.
@@ -301,6 +331,41 @@ pub fn is_site_replication_rule(rule: &ReplicationRule) -> bool {
pub fn merge_incoming_replication_config(
incoming: Option<ReplicationConfiguration>,
local: Option<ReplicationConfiguration>,
) -> Option<ReplicationConfiguration> {
merge_replication_config_keeping_site_rules(incoming, local, is_site_replication_rule)
}
/// [`merge_incoming_replication_config`] for the S3 put/delete-bucket-replication
/// path (issue #1948): only rules the local reconciler derived for a current
/// peer in `peer_deployment_ids` survive as site rules; every other stored
/// rule — including an operator-authored `site-repl-*` id — is operator state
/// that the request replaces or deletes. An incoming rule whose id is a
/// current peer's `site-repl-<id>` is dropped whatever its ARN: accepting it
/// would duplicate the reconciler rule's id.
pub fn merge_user_replication_config(
incoming: Option<ReplicationConfiguration>,
local: Option<ReplicationConfiguration>,
peer_deployment_ids: &HashSet<String>,
) -> Option<ReplicationConfiguration> {
let incoming = incoming.map(|mut config| {
config.rules.retain(|rule| {
!rule
.id
.as_deref()
.and_then(|id| id.strip_prefix(SITE_REPLICATION_RULE_ID_PREFIX))
.is_some_and(|deployment_id| peer_deployment_ids.contains(deployment_id))
});
config
});
merge_replication_config_keeping_site_rules(incoming, local, |rule| {
is_reconciler_owned_site_replication_rule(rule, peer_deployment_ids)
})
}
fn merge_replication_config_keeping_site_rules(
incoming: Option<ReplicationConfiguration>,
local: Option<ReplicationConfiguration>,
is_site_rule: impl Fn(&ReplicationRule) -> bool,
) -> Option<ReplicationConfiguration> {
let incoming_role = incoming.as_ref().map(|config| config.role.clone()).unwrap_or_default();
// Operator rules first, then the local site rules — the same order the
@@ -309,14 +374,9 @@ pub fn merge_incoming_replication_config(
let mut rules: Vec<ReplicationRule> = incoming
.into_iter()
.flat_map(|config| config.rules)
.filter(|rule| !is_site_replication_rule(rule))
.filter(|rule| !is_site_rule(rule))
.collect();
rules.extend(
local
.into_iter()
.flat_map(|config| config.rules)
.filter(is_site_replication_rule),
);
rules.extend(local.into_iter().flat_map(|config| config.rules).filter(&is_site_rule));
if rules.is_empty() {
return None;
@@ -1611,4 +1671,27 @@ mod tests {
"the child rule must win for target A while the overlapping child target B remains eligible"
);
}
#[test]
fn site_replication_rule_deployment_id_requires_id_and_arn_agreement() {
let reconciler_rule = replication_rule("site-repl-peer-dep", "arn:rustfs:replication::peer-dep:bucket");
assert_eq!(site_replication_rule_deployment_id(&reconciler_rule), Some("peer-dep"));
// A remote-target ARN carries the remote's deployment id (or a random
// uuid), never the operator's rule id.
let operator_named_rule = replication_rule("site-repl-user", "arn:minio:replication:us-east-1:2f1c-remote:bucket");
assert_eq!(site_replication_rule_deployment_id(&operator_named_rule), None);
let foreign_arn = replication_rule("site-repl-peer-dep", "arn:rustfs:replication::other-dep:bucket");
assert_eq!(site_replication_rule_deployment_id(&foreign_arn), None);
let empty_id = replication_rule("site-repl-", "arn:rustfs:replication::peer-dep:bucket");
assert_eq!(site_replication_rule_deployment_id(&empty_id), None);
let peers = HashSet::from(["peer-dep".to_string()]);
assert!(is_reconciler_owned_site_replication_rule(&reconciler_rule, &peers));
assert!(!is_reconciler_owned_site_replication_rule(&reconciler_rule, &HashSet::new()));
let removed_peer = replication_rule("site-repl-gone-dep", "arn:rustfs:replication::gone-dep:bucket");
assert!(!is_reconciler_owned_site_replication_rule(&removed_peer, &peers));
}
}
+4 -3
View File
@@ -32,9 +32,10 @@ pub use config::{
ObjectOpts, REMOTE_TARGET_CAPABILITY_CONTRACT_VERSION, REMOTE_TARGET_UNSUPPORTED_FIELDS, REMOTE_TARGET_WRITABLE_FIELDS,
REPLICATION_CAPABILITY_CONTRACT_VERSION, REPLICATION_READ_ONLY_HISTORICAL_FIELDS, REPLICATION_WRITABLE_FIELDS,
ReplicationConfigStructureError, ReplicationConfigurationExt, ReplicationTargetValidationError,
active_replication_rule_destination_arns, invalid_replication_config_status_field, is_site_replication_rule,
merge_incoming_replication_config, replication_target_arn_deployment_id, replication_target_arns,
should_remove_replication_target, unsupported_replication_config_field, validate_replication_config_structure,
active_replication_rule_destination_arns, invalid_replication_config_status_field, is_reconciler_owned_site_replication_rule,
is_site_replication_rule, merge_incoming_replication_config, merge_user_replication_config,
replication_target_arn_deployment_id, replication_target_arns, should_remove_replication_target,
site_replication_rule_deployment_id, unsupported_replication_config_field, validate_replication_config_structure,
validate_replication_config_target_arns,
};
pub use delete::{
@@ -1128,6 +1128,28 @@ pub(crate) async fn site_replication_enabled() -> S3Result<bool> {
Ok(load_site_replication_state().await?.enabled())
}
/// Deployment ids of the remote peers the reconciler derives a
/// `site-repl-<id>` rule for on every bucket (the same peer filter as
/// `build_site_replication_config`); empty when site replication is not
/// enabled. Read by the bucket usecase so an S3 replication-config edit keeps
/// exactly the reconciler-owned rules (issue #1948); a state-read failure
/// propagates so the edit fails closed.
pub(crate) async fn site_replication_remote_peer_deployment_ids() -> S3Result<HashSet<String>> {
let state = load_site_replication_state().await?;
if !state.enabled() {
return Ok(HashSet::new());
}
let local_peer = current_local_runtime_peer(&state);
Ok(state
.peers
.values()
.filter(|peer| {
peer.deployment_id != local_peer.deployment_id && !same_identity_endpoint(&peer.endpoint, &local_peer.endpoint)
})
.map(|peer| peer.deployment_id.clone())
.collect())
}
async fn load_site_replication_state_no_lock(store: Arc<ECStore>) -> S3Result<SiteReplicationState> {
match read_config_no_lock(store, SITE_REPLICATION_STATE_PATH).await {
Ok(data) => parse_site_replication_state(&data),
+97 -33
View File
@@ -38,9 +38,9 @@ use super::storage_api::bucket_usecase::bucket::{
metadata_sys,
policy_sys::PolicySys,
replication::{
ReplicationTargetValidationError, invalid_replication_config_status_field, is_site_replication_rule,
merge_incoming_replication_config, replication_target_arns, should_remove_replication_target,
unsupported_replication_config_field, validate_replication_config_structure, validate_replication_config_target_arns,
ReplicationTargetValidationError, invalid_replication_config_status_field, merge_user_replication_config,
replication_target_arns, should_remove_replication_target, unsupported_replication_config_field,
validate_replication_config_structure, validate_replication_config_target_arns,
},
target::{BucketTargetType, BucketTargets},
utils::serialize,
@@ -66,6 +66,7 @@ use super::storage_api::bucket_usecase::{
};
use crate::admin::handlers::site_replication::{
site_replication_bucket_meta_hook, site_replication_delete_bucket_hook, site_replication_make_bucket_hook,
site_replication_remote_peer_deployment_ids,
};
use crate::app::object_data_cache::invalidate_object_data_cache_bucket_after_delete;
use crate::app::runtime_sources::{
@@ -625,36 +626,38 @@ async fn validate_bucket_replication_update(bucket: &str, config: &ReplicationCo
/// Defense in depth for site-replication-managed buckets (issue #1948): an S3
/// PutBucketReplication replaces the operator-authored rules but must not wipe
/// the local `site-repl-*` rules the reconciler owns — until its next pass
/// (600s period) every peer link on this bucket would be silently dead. The
/// same merge also drops incoming `site-repl-*` impostor rules, matching the
/// peer bucket-meta ingestion path. Buckets without site-replication rules
/// keep the verbatim overwrite semantics.
/// the rules the reconciler derived for the current remote peers
/// (`site_peer_deployment_ids`) — until its next pass (600s period) every
/// peer link on this bucket would be silently dead. The same merge also drops
/// incoming impostors of those rules. An empty peer set (site replication
/// disabled) keeps the verbatim overwrite semantics: rule ids are not
/// reserved, so an operator's own `site-repl-*` rule is ordinary state there.
fn merge_user_replication_config_update(
incoming: ReplicationConfiguration,
existing: Option<ReplicationConfiguration>,
site_peer_deployment_ids: &HashSet<String>,
) -> ReplicationConfiguration {
let has_site_rules = existing
.as_ref()
.is_some_and(|config| config.rules.iter().any(is_site_replication_rule));
if !has_site_rules {
if site_peer_deployment_ids.is_empty() {
return incoming;
}
// `existing` holds at least one site-replication rule the merge keeps, so
// the merged rule set is non-empty; the fallback only guards the type.
merge_incoming_replication_config(Some(incoming.clone()), existing).unwrap_or(incoming)
// `incoming` passed structure validation, so it holds at least one rule;
// `None` is only reachable when every incoming rule impersonates a
// reconciler rule, and then the stored reconciler rules are what remains.
merge_user_replication_config(Some(incoming.clone()), existing, site_peer_deployment_ids).unwrap_or(incoming)
}
/// Split of an S3 DeleteBucketReplication on the stored config (issue #1948):
/// the operator-authored rules are removed, the local `site-repl-*` rules
/// survive (`None` means nothing survives and the config is deleted), and the
/// returned ARNs are the ones whose bucket targets may be garbage-collected —
/// never an ARN a surviving site-replication rule still points at.
/// the operator-authored rules are removed, the rules the reconciler derived
/// for the current remote peers survive (`None` means nothing survives and
/// the config is deleted), and the returned ARNs are the ones whose bucket
/// targets may be garbage-collected — never an ARN a surviving reconciler
/// rule still points at.
fn split_replication_config_for_user_delete(
config: ReplicationConfiguration,
site_peer_deployment_ids: &HashSet<String>,
) -> (Option<ReplicationConfiguration>, HashSet<String>) {
let mut removable_arns = replication_target_arns(&config);
let remaining = merge_incoming_replication_config(None, Some(config));
let remaining = merge_user_replication_config(None, Some(config), site_peer_deployment_ids);
if let Some(remaining) = remaining.as_ref() {
for rule in &remaining.rules {
removable_arns.remove(rule.destination.bucket.trim());
@@ -1644,7 +1647,8 @@ impl DefaultBucketUsecase {
Err(err) => return Err(ApiError::from(err).into()),
};
let (remaining_config, updated_targets) = if let Some(config) = replication_config.as_ref() {
let (remaining, removable_arns) = split_replication_config_for_user_delete(config.clone());
let site_peers = site_replication_remote_peer_deployment_ids().await?;
let (remaining, removable_arns) = split_replication_config_for_user_delete(config.clone(), &site_peers);
let targets = replication_targets_without_arns(&bucket, &removable_arns).await?;
(remaining, targets)
} else {
@@ -2543,7 +2547,9 @@ impl DefaultBucketUsecase {
Err(StorageError::ConfigNotFound) => None,
Err(err) => return Err(ApiError::from(err).into()),
};
let replication_configuration = merge_user_replication_config_update(replication_configuration, existing_config);
let site_peers = site_replication_remote_peer_deployment_ids().await?;
let replication_configuration =
merge_user_replication_config_update(replication_configuration, existing_config, &site_peers);
let data = serialize_config(&replication_configuration)?;
update_bucket_config_for_incarnation(&bucket, BUCKET_REPLICATION_CONFIG, data, expected_incarnation_id)
.await
@@ -3180,6 +3186,10 @@ mod tests {
rule
}
fn site_peers(deployment_ids: &[&str]) -> HashSet<String> {
deployment_ids.iter().map(|id| id.to_string()).collect()
}
#[test]
fn put_replication_merge_preserves_site_replication_rules() {
let existing = ReplicationConfiguration {
@@ -3193,21 +3203,75 @@ mod tests {
role: String::new(),
rules: vec![
replication_rule_with_id("arn:rustfs:replication:us-east-1:new:bucket", "new-user-rule", 1),
replication_rule_with_id("arn:rustfs:replication::forged-dep:bucket", "site-repl-forged", 2),
replication_rule_with_id("arn:rustfs:replication::forged-dep:bucket", "site-repl-peer-dep", 2),
replication_rule_with_id("arn:rustfs:replication::other-dep:bucket", "site-repl-other", 3),
],
};
let merged = merge_user_replication_config_update(incoming, Some(existing));
let merged = merge_user_replication_config_update(incoming, Some(existing), &site_peers(&["peer-dep"]));
let ids: Vec<_> = merged
let rules: Vec<_> = merged
.rules
.iter()
.map(|rule| rule.id.as_deref().unwrap_or_default())
.map(|rule| (rule.id.as_deref().unwrap_or_default(), rule.destination.bucket.as_str()))
.collect();
assert_eq!(
ids,
vec!["new-user-rule", "site-repl-peer-dep"],
"user rules replaced, local site-replication rule preserved, forged incoming site-repl rule dropped"
rules,
vec![
("new-user-rule", "arn:rustfs:replication:us-east-1:new:bucket"),
("site-repl-other", "arn:rustfs:replication::other-dep:bucket"),
("site-repl-peer-dep", "arn:rustfs:replication::peer-dep:bucket"),
],
"user rules replaced, the reconciler rule for the current peer kept over the incoming impostor, \
a site-repl-* id that names no current peer is ordinary operator state"
);
}
// Rule ids do not reserve `site-repl-*`: outside site replication an
// owner's `site-repl-user` rule is ordinary state, so PUT stores it
// verbatim and DELETE removes it and garbage-collects its target.
#[test]
fn put_then_delete_replication_without_site_replication_treats_site_repl_id_as_user_rule() {
let user_arn = "arn:minio:replication:us-east-1:2f1c-remote:bucket";
let incoming = ReplicationConfiguration {
role: String::new(),
rules: vec![replication_rule_with_id(user_arn, "site-repl-user", 1)],
};
let stored = merge_user_replication_config_update(incoming.clone(), None, &HashSet::new());
assert_eq!(stored, incoming, "PUT on a non-site-replication bucket is verbatim");
let (remaining, removable) = split_replication_config_for_user_delete(stored, &HashSet::new());
assert!(remaining.is_none(), "DELETE must remove the operator's site-repl-* rule");
assert_eq!(removable, HashSet::from([user_arn.to_string()]));
}
// Under site replication only a rule the reconciler would derive — id
// `site-repl-<peer>` for a current peer, destination ARN naming the same
// peer — is reconciler-owned. Everything else is operator state.
#[test]
fn delete_replication_split_keeps_only_reconciler_derived_rules() {
let peer_arn = "arn:rustfs:replication::peer-dep:bucket";
let user_arn = "arn:minio:replication:us-east-1:2f1c-remote:bucket";
let config = ReplicationConfiguration {
role: String::new(),
rules: vec![
replication_rule_with_id(user_arn, "site-repl-user", 1),
replication_rule_with_id(user_arn, "site-repl-peer-dep", 2),
replication_rule_with_id("arn:rustfs:replication::gone-dep:bucket", "site-repl-gone-dep", 3),
replication_rule_with_id(peer_arn, "site-repl-peer-dep", 4),
],
};
let (remaining, removable) = split_replication_config_for_user_delete(config, &site_peers(&["peer-dep"]));
let remaining = remaining.expect("the reconciler-derived rule must survive");
assert_eq!(remaining.rules.len(), 1);
assert_eq!(remaining.rules[0].destination.bucket, peer_arn);
assert_eq!(
removable,
HashSet::from([user_arn.to_string(), "arn:rustfs:replication::gone-dep:bucket".to_string()]),
"targets of operator rules and of a removed peer are garbage-collected"
);
}
@@ -3230,7 +3294,7 @@ mod tests {
)],
};
let merged = merge_user_replication_config_update(incoming.clone(), Some(existing));
let merged = merge_user_replication_config_update(incoming.clone(), Some(existing), &HashSet::new());
assert_eq!(merged.role, incoming.role);
assert_eq!(merged.rules, incoming.rules, "non-SR buckets keep the verbatim overwrite semantics");
@@ -3248,7 +3312,7 @@ mod tests {
],
};
let (remaining, removable) = split_replication_config_for_user_delete(config);
let (remaining, removable) = split_replication_config_for_user_delete(config, &site_peers(&["peer-dep"]));
let remaining = remaining.expect("site-replication rules must survive a user delete");
let ids: Vec<_> = remaining
@@ -3271,7 +3335,7 @@ mod tests {
],
};
let (remaining, removable) = split_replication_config_for_user_delete(config);
let (remaining, removable) = split_replication_config_for_user_delete(config, &site_peers(&["peer-dep"]));
assert!(remaining.is_some());
assert!(
@@ -3288,7 +3352,7 @@ mod tests {
rules: vec![replication_rule_with_id(user_arn, "user-rule", 1)],
};
let (remaining, removable) = split_replication_config_for_user_delete(config);
let (remaining, removable) = split_replication_config_for_user_delete(config, &site_peers(&["peer-dep"]));
assert!(remaining.is_none(), "without site-replication rules the whole config is deleted");
assert_eq!(removable, HashSet::from([user_arn.to_string()]));
+1 -1
View File
@@ -614,7 +614,7 @@ pub(crate) mod bucket {
use crate::storage::storage_api::ecstore_bucket::replication as replication_contracts;
pub(crate) use replication_contracts::{is_site_replication_rule, merge_incoming_replication_config};
pub(crate) use replication_contracts::merge_user_replication_config;
type ReplicationObjectBridge = crate::storage::storage_api::ecstore_bucket::replication::ReplicationObjectBridge;
pub(crate) type DeleteReplicationConfigSnapshot =