fix(replication): make bucket replication rules editable from clients (#5715)

* fix(replication): accept explicit STANDARD destination storage class The replication engine never reads Rule.Destination.StorageClass (replica placement comes from the bucket-target config or the source object), yet the validator rejected any config carrying the field. The console's add-rule form always sends StorageClass=STANDARD, so every rule created through it failed with InvalidRequest. Tolerate exactly STANDARD as a no-op — semantically identical to omitting the field — and keep rejecting every other value, which would be silently ignored rather than honored. Document the deliberate omission from the replication capability contract. * feat(admin): support MinIO-style partial updates for set-remote-target set-remote-target?update=true previously replaced every stored field and required complete credentials in the body, so flipping a target's sync mode from the console forced operators to re-enter the secret key, and real mc replicate update bodies (madmin Clone() strips the secret) failed to deserialize at all. Adopt MinIO's TargetUpdateType contract: query params creds/sync/bandwidth/ path name the field groups to overlay onto the stored target, everything else keeps its persisted value, and unsupported groups (proxy, healthcheck, edge, edgeSyncBeforeExpiry) fail loudly. Credentials updates are skipped for site-replication peer targets — probed by both scheme derivations of the stored endpoint and the stored deployment id — because an operator never knows the site replicator's credentials, and a body-supplied deployment id is ignored on update since it anchors peer identity. madmin JSON aliases (bandwidthlimit, storageclass, resetID, deploymentID, sessionToken) let mc bodies parse under deny_unknown_fields. e2e: cover a credential-free sync-only update preserving the stored connection and the zero-ops no-op contract; align the missing-arn assertion with the earlier validation error. * chore(scripts): add two-site replication lab manager site_replication_smoke.py spawns and manages two local rustfs processes, pairs them via the site-replication admin API (idempotent), and verifies bidirectional object replication. Subcommands: up/down/restart/status/logs/ smoke/info/remove/clean. Stdlib-only; requests are SigV4-signed the same way as crates/e2e_test. * chore(scripts): rename direction-suffixed payload variables for typos check The typos linter reads the _ba suffix in payload_ba as a misspelling of "by"; use payload_a_to_b / payload_b_to_a instead. --------- Co-authored-by: overtrue <anzhengchao@gmail.com>
2026-08-10 23:26:53 +00:00 · 2026-08-05 09:50:31 +08:00
parent 4042bc0a5e
commit 15b9c1f4e3
4 changed files with 992 additions and 65 deletions
@@ -2754,7 +2754,7 @@ async fn test_set_remote_target_update_requires_arn() -> Result<(), Box<dyn Erro

    assert_eq!(status, StatusCode::BAD_REQUEST);
    assert!(body.contains("InvalidRequest"), "unexpected response: {body}");
-    assert!(body.to_ascii_lowercase().contains("arn is empty"), "unexpected response: {body}");
+    assert!(body.to_ascii_lowercase().contains("arn is required"), "unexpected response: {body}");

    Ok(())
 }
@@ -2812,6 +2812,128 @@ async fn test_set_remote_target_update_rejects_missing_target() -> Result<(), Bo
    Ok(())
 }

+async fn send_set_replication_target_update_request(
+    source_env: &RustFSTestEnvironment,
+    source_bucket: &str,
+    ops: &[&str],
+    body: serde_json::Value,
+) -> Result<reqwest::Response, Box<dyn Error + Send + Sync>> {
+    let mut url = format!(
+        "{}/rustfs/admin/v3/set-remote-target?bucket={}&update=true",
+        source_env.url,
+        urlencoding::encode(source_bucket)
+    );
+    for op in ops {
+        url.push_str(&format!("&{op}=true"));
+    }
+    signed_request(
+        http::Method::PUT,
+        &url,
+        &source_env.access_key,
+        &source_env.secret_key,
+        Some(body.to_string().into_bytes()),
+        Some("application/json"),
+    )
+    .await
+}
+
+async fn fetch_single_target(
+    env: &RustFSTestEnvironment,
+    bucket: &str,
+) -> Result<serde_json::Value, Box<dyn Error + Send + Sync>> {
+    let response = list_replication_targets_request(env, Some(bucket)).await?;
+    assert_eq!(response.status(), StatusCode::OK);
+    let mut targets: Vec<serde_json::Value> = response.json().await?;
+    assert_eq!(targets.len(), 1, "expected exactly one remote target");
+    Ok(targets.remove(0))
+}
+
+#[tokio::test]
+#[serial]
+async fn test_set_remote_target_partial_update_preserves_credentials() -> Result<(), Box<dyn Error + Send + Sync>> {
+    init_logging();
+
+    let mut source_env = RustFSTestEnvironment::new().await?;
+    source_env
+        .start_rustfs_server_with_env(vec![], LOOPBACK_REPLICATION_TARGET_ENV)
+        .await?;
+
+    let mut target_env = RustFSTestEnvironment::new().await?;
+    target_env.start_rustfs_server_without_cleanup(vec![]).await?;
+
+    let source_bucket = "replication-partial-update-src";
+    let target_bucket = "replication-partial-update-dst";
+
+    let source_client = source_env.create_s3_client();
+    let target_client = target_env.create_s3_client();
+
+    source_client.create_bucket().bucket(source_bucket).send().await?;
+    target_client.create_bucket().bucket(target_bucket).send().await?;
+
+    enable_bucket_versioning(&source_env, source_bucket).await?;
+    enable_bucket_versioning(&target_env, target_bucket).await?;
+
+    let arn = set_replication_target(&source_env, source_bucket, &target_env, target_bucket).await?;
+
+    // A sync-only update whose body omits credentials entirely must succeed and
+    // leave the stored connection settings untouched.
+    let response = send_set_replication_target_update_request(
+        &source_env,
+        source_bucket,
+        &["sync"],
+        serde_json::json!({
+            "arn": arn,
+            "type": "replication",
+            "replicationSync": true
+        }),
+    )
+    .await?;
+    assert_eq!(response.status(), StatusCode::OK, "sync-only update failed: {}", response.text().await?);
+
+    let target = fetch_single_target(&source_env, source_bucket).await?;
+    assert_eq!(target["replicationSync"], serde_json::json!(true));
+    assert_eq!(target["endpoint"], serde_json::json!(target_env.address));
+    assert_eq!(target["credentials"]["accessKey"], serde_json::json!(target_env.access_key));
+
+    // An update naming no field groups is a no-op: a body carrying a different
+    // endpoint and credentials must not leak into the stored target.
+    let response = send_set_replication_target_update_request(
+        &source_env,
+        source_bucket,
+        &[],
+        serde_json::json!({
+            "arn": arn,
+            "type": "replication",
+            "endpoint": "203.0.113.1:9000",
+            "credentials": { "accessKey": "other-access", "secretKey": "other-secret" },
+            "targetbucket": "elsewhere",
+            "secure": false,
+            "replicationSync": false
+        }),
+    )
+    .await?;
+    assert_eq!(response.status(), StatusCode::OK, "no-op update failed: {}", response.text().await?);
+
+    let target = fetch_single_target(&source_env, source_bucket).await?;
+    assert_eq!(
+        target["replicationSync"],
+        serde_json::json!(true),
+        "no-op update must not change sync mode"
+    );
+    assert_eq!(
+        target["endpoint"],
+        serde_json::json!(target_env.address),
+        "no-op update must not change endpoint"
+    );
+    assert_eq!(
+        target["credentials"]["accessKey"],
+        serde_json::json!(target_env.access_key),
+        "no-op update must not change credentials"
+    );
+
+    Ok(())
+}
+
 #[tokio::test]
 #[serial]
 async fn test_set_remote_target_rejects_invalid_target_url() -> Result<(), Box<dyn Error + Send + Sync>> {
@@ -20,7 +20,7 @@ use s3s::dto::DeleteReplicationStatus;
 use s3s::dto::Destination;
 use s3s::dto::{
    ExistingObjectReplicationStatus, ReplicaModificationsStatus, ReplicationConfiguration, ReplicationRule,
-    ReplicationRuleStatus, ReplicationRules,
+    ReplicationRuleStatus, ReplicationRules, StorageClass,
 };
 use serde::{Deserialize, Serialize};
 use std::collections::{BTreeMap, HashMap, HashSet};
@@ -28,6 +28,10 @@ use uuid::Uuid;

 pub const REPLICATION_CAPABILITY_CONTRACT_VERSION: u32 = 1;

+// `Rule.Destination.StorageClass` is deliberately absent from both lists below:
+// clients should keep omitting it, but the validator tolerates an explicit
+// `STANDARD` as a no-op (see `unsupported_replication_config_field`) because the
+// console's rule form always sends it.
 pub const REPLICATION_WRITABLE_FIELDS: &[&str] = &[
    "Role",
    "Rule.ID",
@@ -164,7 +168,18 @@ pub fn unsupported_replication_config_field(config: &ReplicationConfiguration) -
        if rule.destination.replication_time.is_some() {
            return Some("Destination.ReplicationTime");
        }
-        if rule.destination.storage_class.is_some() {
+        // The replication engine never reads this field (replica placement comes from
+        // the bucket-target config or the source object), so an explicit STANDARD —
+        // which the console's rule form always sends — is indistinguishable from
+        // omitting it and is tolerated as a no-op. Any other value would be silently
+        // ignored rather than honored, so it stays rejected. Exact match: S3 storage
+        // class enums are case-sensitive.
+        if rule
+            .destination
+            .storage_class
+            .as_ref()
+            .is_some_and(|class| class.as_str() != StorageClass::STANDARD)
+        {
            return Some("Destination.StorageClass");
        }
    }
@@ -952,6 +967,47 @@ mod tests {
        config.rules[0].destination.storage_class =
            Some(s3s::dto::StorageClass::from_static(s3s::dto::StorageClass::STANDARD_IA));
        assert_eq!(unsupported_replication_config_field(&config), Some("Destination.StorageClass"));
+
+        // The exact-match contract is deliberate: S3 storage class enums are
+        // case-sensitive, so a lowercase variant must stay rejected.
+        config.rules[0].destination.storage_class = Some(StorageClass::from("standard".to_string()));
+        assert_eq!(unsupported_replication_config_field(&config), Some("Destination.StorageClass"));
+
+        // Explicit STANDARD is a no-op (the engine never reads the field) and must
+        // pass: the console's rule form always sends it, and rejecting it makes the
+        // form unusable.
+        config.rules[0].destination.storage_class = Some(StorageClass::from_static(StorageClass::STANDARD));
+        assert_eq!(unsupported_replication_config_field(&config), None);
+    }
+
+    #[test]
+    fn explicit_standard_storage_class_is_accepted_from_wire_xml() {
+        // The exact request shape the console's add-replication-rule form sends:
+        // a rule whose Destination carries <StorageClass>STANDARD</StorageClass>.
+        let xml = br#"
+            <ReplicationConfiguration>
+              <Role></Role>
+              <Rule>
+                <ID>console-rule</ID>
+                <Status>Enabled</Status>
+                <Priority>1</Priority>
+                <DeleteMarkerReplication><Status>Enabled</Status></DeleteMarkerReplication>
+                <Destination>
+                  <Bucket>arn:aws:s3:::destination</Bucket>
+                  <StorageClass>STANDARD</StorageClass>
+                </Destination>
+              </Rule>
+            </ReplicationConfiguration>
+        "#;
+        let mut deserializer = Deserializer::new(xml);
+        let config = <ReplicationConfiguration as s3s::xml::Deserialize>::deserialize(&mut deserializer)
+            .expect("console-shaped config should parse");
+        deserializer
+            .expect_eof()
+            .expect("console-shaped config should consume the whole body");
+
+        assert_eq!(config.rules[0].destination.storage_class.as_ref().map(|c| c.as_str()), Some("STANDARD"));
+        assert_eq!(unsupported_replication_config_field(&config), None);
    }

    #[test]