mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-22 12:26:37 +00:00
fix(admin): use madmin key names in list-remote-targets response
The admin list-remote-targets response serialized five fields under the persisted snake_case keys (bandwidth_limit, storage_class, reset_id, deployment_id, credentials.session_token), which madmin/mc decode to zero values, blanking the bandwidth and reset-id columns of mc replicate ls. Rename them to the madmin JSON tags (bandwidth, storageclass, resetID, deploymentID, sessionToken) in remote_target_admin_json only, following the existing Go-duration overwrite precedent there. Symmetrically, BucketTarget deserialization only aliased bandwidth, so a MinIO-written bucket-targets.json silently dropped the other four fields to defaults on migration; add serde aliases for them. Aliases and the response-layer rename leave the persisted bucket-targets.json and the msgpack struct-map wire format untouched, pinned by new tests. Refs rustfs/backlog#1951, spec rustfs/backlog#1946.
This commit is contained in:
@@ -32,6 +32,10 @@ pub struct Credentials {
|
|||||||
pub access_key: String,
|
pub access_key: String,
|
||||||
#[serde(rename = "secretKey")]
|
#[serde(rename = "secretKey")]
|
||||||
pub secret_key: String,
|
pub secret_key: String,
|
||||||
|
// The aliases accept madmin's JSON tags (MinIO-written bucket-targets
|
||||||
|
// metadata and mc request bodies) without changing the snake_case
|
||||||
|
// persisted/peer wire format this struct serializes to.
|
||||||
|
#[serde(alias = "sessionToken")]
|
||||||
pub session_token: Option<String>,
|
pub session_token: Option<String>,
|
||||||
pub expiration: Option<Timestamp>,
|
pub expiration: Option<Timestamp>,
|
||||||
}
|
}
|
||||||
@@ -207,7 +211,7 @@ pub struct BucketTarget {
|
|||||||
|
|
||||||
#[serde(rename = "replicationSync", default)]
|
#[serde(rename = "replicationSync", default)]
|
||||||
pub replication_sync: bool,
|
pub replication_sync: bool,
|
||||||
#[serde(default)]
|
#[serde(alias = "storageclass", default)]
|
||||||
pub storage_class: String,
|
pub storage_class: String,
|
||||||
#[serde(rename = "skipTlsVerify", default)]
|
#[serde(rename = "skipTlsVerify", default)]
|
||||||
pub skip_tls_verify: bool,
|
pub skip_tls_verify: bool,
|
||||||
@@ -220,7 +224,7 @@ pub struct BucketTarget {
|
|||||||
|
|
||||||
#[serde(rename = "resetBeforeDate", with = "time::serde::rfc3339::option", default)]
|
#[serde(rename = "resetBeforeDate", with = "time::serde::rfc3339::option", default)]
|
||||||
pub reset_before_date: Option<OffsetDateTime>,
|
pub reset_before_date: Option<OffsetDateTime>,
|
||||||
#[serde(default)]
|
#[serde(alias = "resetID", default)]
|
||||||
pub reset_id: String,
|
pub reset_id: String,
|
||||||
#[serde(rename = "totalDowntime", with = "duration_seconds", default)]
|
#[serde(rename = "totalDowntime", with = "duration_seconds", default)]
|
||||||
pub total_downtime: Duration,
|
pub total_downtime: Duration,
|
||||||
@@ -233,7 +237,7 @@ pub struct BucketTarget {
|
|||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
pub latency: LatencyStat,
|
pub latency: LatencyStat,
|
||||||
|
|
||||||
#[serde(default)]
|
#[serde(alias = "deploymentID", default)]
|
||||||
pub deployment_id: String,
|
pub deployment_id: String,
|
||||||
|
|
||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
@@ -531,6 +535,84 @@ mod tests {
|
|||||||
assert_eq!(value["totalDowntime"], 90);
|
assert_eq!(value["totalDowntime"], 90);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn bucket_target_persisted_wire_keys_stay_snake_case() {
|
||||||
|
// bucket-targets.json (persisted via `serde_json::to_vec(&BucketTargets)`
|
||||||
|
// in the admin set/remove handlers) and the msgpack struct-map form
|
||||||
|
// (`BucketTargets::marshal_msg`) both come straight from this struct's
|
||||||
|
// serde field names. madmin naming is applied only in the admin
|
||||||
|
// response layer (`remote_target_admin_json`); renaming here would
|
||||||
|
// silently break every existing deployment's persisted metadata.
|
||||||
|
let targets = BucketTargets {
|
||||||
|
targets: vec![BucketTarget {
|
||||||
|
credentials: Some(Credentials {
|
||||||
|
access_key: "ak".to_string(),
|
||||||
|
secret_key: "sk".to_string(),
|
||||||
|
session_token: Some("token".to_string()),
|
||||||
|
expiration: None,
|
||||||
|
}),
|
||||||
|
bandwidth_limit: 5,
|
||||||
|
storage_class: "STANDARD".to_string(),
|
||||||
|
reset_id: "reset-1".to_string(),
|
||||||
|
deployment_id: "deploy-1".to_string(),
|
||||||
|
..Default::default()
|
||||||
|
}],
|
||||||
|
};
|
||||||
|
|
||||||
|
let json = serde_json::to_value(&targets).expect("targets should serialize to JSON");
|
||||||
|
let msgpack: serde_json::Value =
|
||||||
|
rmp_serde::from_slice(&targets.marshal_msg().expect("targets should marshal to msgpack"))
|
||||||
|
.expect("msgpack struct map should decode into a JSON value");
|
||||||
|
|
||||||
|
for (wire, entry) in [("JSON", &json["targets"][0]), ("msgpack", &msgpack["targets"][0])] {
|
||||||
|
assert_eq!(entry["bandwidth_limit"], 5, "{wire} key `bandwidth_limit` must stay");
|
||||||
|
assert_eq!(entry["storage_class"], "STANDARD", "{wire} key `storage_class` must stay");
|
||||||
|
assert_eq!(entry["reset_id"], "reset-1", "{wire} key `reset_id` must stay");
|
||||||
|
assert_eq!(entry["deployment_id"], "deploy-1", "{wire} key `deployment_id` must stay");
|
||||||
|
assert_eq!(entry["credentials"]["session_token"], "token", "{wire} key `session_token` must stay");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn minio_written_bucket_targets_json_populates_madmin_named_fields() {
|
||||||
|
// A MinIO-written bucket-targets.json carries madmin's JSON tags
|
||||||
|
// (`bandwidth`, `storageclass`, `resetID`, `deploymentID`,
|
||||||
|
// `credentials.sessionToken`). On migration these must land in the
|
||||||
|
// matching fields instead of silently defaulting (backlog#1946).
|
||||||
|
let targets: BucketTargets = serde_json::from_value(serde_json::json!({
|
||||||
|
"targets": [{
|
||||||
|
"sourcebucket": "src",
|
||||||
|
"endpoint": "minio.example:9000",
|
||||||
|
"credentials": {
|
||||||
|
"accessKey": "ak",
|
||||||
|
"secretKey": "sk",
|
||||||
|
"sessionToken": "minio-session-token"
|
||||||
|
},
|
||||||
|
"targetbucket": "dst",
|
||||||
|
"type": "replication",
|
||||||
|
"replicationSync": true,
|
||||||
|
"bandwidth": 107374182400i64,
|
||||||
|
"storageclass": "STANDARD",
|
||||||
|
"resetID": "reset-789",
|
||||||
|
"deploymentID": "deploy-123"
|
||||||
|
}]
|
||||||
|
}))
|
||||||
|
.expect("MinIO-written bucket-targets.json must deserialize");
|
||||||
|
|
||||||
|
let target = &targets.targets[0];
|
||||||
|
assert_eq!(target.bandwidth_limit, 107374182400);
|
||||||
|
assert_eq!(target.storage_class, "STANDARD");
|
||||||
|
assert_eq!(target.reset_id, "reset-789");
|
||||||
|
assert_eq!(target.deployment_id, "deploy-123");
|
||||||
|
assert_eq!(
|
||||||
|
target
|
||||||
|
.credentials
|
||||||
|
.as_ref()
|
||||||
|
.and_then(|credentials| credentials.session_token.as_deref()),
|
||||||
|
Some("minio-session-token")
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_bucket_target_debug_redacts_credentials() {
|
fn test_bucket_target_debug_redacts_credentials() {
|
||||||
let target = BucketTarget {
|
let target = BucketTarget {
|
||||||
|
|||||||
@@ -374,8 +374,10 @@ impl RemoteTargetRequest {
|
|||||||
/// Admin-response encoding of a remote target: the persisted bucket-targets
|
/// Admin-response encoding of a remote target: the persisted bucket-targets
|
||||||
/// format keeps `healthCheckDuration`/`totalDowntime` in seconds and the
|
/// format keeps `healthCheckDuration`/`totalDowntime` in seconds and the
|
||||||
/// `latency` stats in milliseconds, but madmin decodes all of them as Go
|
/// `latency` stats in milliseconds, but madmin decodes all of them as Go
|
||||||
/// `time.Duration` (nanoseconds) — re-encode just those fields without
|
/// `time.Duration` (nanoseconds) — and it looks the fields up under its own
|
||||||
/// touching the persistence wire format.
|
/// JSON tags (`bandwidth`, `storageclass`, `resetID`, `deploymentID`,
|
||||||
|
/// `credentials.sessionToken`), not the persisted snake_case keys. Re-encode
|
||||||
|
/// just those fields here without touching the persistence wire format.
|
||||||
fn remote_target_admin_json(target: &BucketTarget) -> Result<serde_json::Value, serde_json::Error> {
|
fn remote_target_admin_json(target: &BucketTarget) -> Result<serde_json::Value, serde_json::Error> {
|
||||||
fn go_duration_nanos(duration: Duration) -> serde_json::Value {
|
fn go_duration_nanos(duration: Duration) -> serde_json::Value {
|
||||||
// Saturate instead of truncating: >u64::MAX nanoseconds (~584 years)
|
// Saturate instead of truncating: >u64::MAX nanoseconds (~584 years)
|
||||||
@@ -383,6 +385,12 @@ fn remote_target_admin_json(target: &BucketTarget) -> Result<serde_json::Value,
|
|||||||
u64::try_from(duration.as_nanos()).unwrap_or(u64::MAX).into()
|
u64::try_from(duration.as_nanos()).unwrap_or(u64::MAX).into()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn rename_key(value: &mut serde_json::Value, from: &str, to: &str) {
|
||||||
|
if let Some(moved) = value.as_object_mut().and_then(|object| object.remove(from)) {
|
||||||
|
value[to] = moved;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
let mut value = serde_json::to_value(target)?;
|
let mut value = serde_json::to_value(target)?;
|
||||||
value["healthCheckDuration"] = go_duration_nanos(target.health_check_duration);
|
value["healthCheckDuration"] = go_duration_nanos(target.health_check_duration);
|
||||||
value["totalDowntime"] = go_duration_nanos(target.total_downtime);
|
value["totalDowntime"] = go_duration_nanos(target.total_downtime);
|
||||||
@@ -391,6 +399,11 @@ fn remote_target_admin_json(target: &BucketTarget) -> Result<serde_json::Value,
|
|||||||
"avg": go_duration_nanos(target.latency.avg),
|
"avg": go_duration_nanos(target.latency.avg),
|
||||||
"max": go_duration_nanos(target.latency.max),
|
"max": go_duration_nanos(target.latency.max),
|
||||||
});
|
});
|
||||||
|
rename_key(&mut value, "bandwidth_limit", "bandwidth");
|
||||||
|
rename_key(&mut value, "storage_class", "storageclass");
|
||||||
|
rename_key(&mut value, "reset_id", "resetID");
|
||||||
|
rename_key(&mut value, "deployment_id", "deploymentID");
|
||||||
|
rename_key(&mut value["credentials"], "session_token", "sessionToken");
|
||||||
Ok(value)
|
Ok(value)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1473,7 +1486,7 @@ mod tests {
|
|||||||
parse_remote_target_update_ops, render_mrf_backlog, render_replication_diff, unique_replication_peers,
|
parse_remote_target_update_ops, render_mrf_backlog, render_replication_diff, unique_replication_peers,
|
||||||
validate_remote_target_tls_settings,
|
validate_remote_target_tls_settings,
|
||||||
};
|
};
|
||||||
use crate::admin::storage_api::bucket::target::{BucketTarget, LatencyStat};
|
use crate::admin::storage_api::bucket::target::{BucketTarget, Credentials as TargetCredentials, LatencyStat};
|
||||||
use crate::admin::storage_api::replication::{BucketStats, DurableMrfBacklog, MrfOpKind, MrfReplicateEntry};
|
use crate::admin::storage_api::replication::{BucketStats, DurableMrfBacklog, MrfOpKind, MrfReplicateEntry};
|
||||||
use http::Uri;
|
use http::Uri;
|
||||||
|
|
||||||
@@ -2268,6 +2281,44 @@ mod tests {
|
|||||||
assert_eq!(persisted["latency"]["max"], 250);
|
assert_eq!(persisted["latency"]["max"], 250);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn list_remote_targets_response_uses_madmin_key_names() {
|
||||||
|
// madmin's BucketTarget JSON tags are `bandwidth`, `storageclass`,
|
||||||
|
// `resetID`, `deploymentID`, and `credentials.sessionToken`
|
||||||
|
// (backlog#1946); the persisted snake_case keys decode to zero values
|
||||||
|
// in mc, blanking the bandwidth and reset-id columns of
|
||||||
|
// `mc replicate ls`.
|
||||||
|
let target = BucketTarget {
|
||||||
|
endpoint: "192.168.1.10:9000".to_string(),
|
||||||
|
target_bucket: "target".to_string(),
|
||||||
|
credentials: Some(TargetCredentials {
|
||||||
|
access_key: "access".to_string(),
|
||||||
|
secret_key: String::new(),
|
||||||
|
session_token: Some("session-token".to_string()),
|
||||||
|
expiration: None,
|
||||||
|
}),
|
||||||
|
bandwidth_limit: 107_374_182_400,
|
||||||
|
storage_class: "STANDARD".to_string(),
|
||||||
|
reset_id: "reset-123".to_string(),
|
||||||
|
deployment_id: "deploy-456".to_string(),
|
||||||
|
..Default::default()
|
||||||
|
};
|
||||||
|
|
||||||
|
let value = super::remote_target_admin_json(&target).expect("admin response should serialize");
|
||||||
|
|
||||||
|
assert_eq!(value["bandwidth"], 107_374_182_400i64);
|
||||||
|
assert_eq!(value["storageclass"], "STANDARD");
|
||||||
|
assert_eq!(value["resetID"], "reset-123");
|
||||||
|
assert_eq!(value["deploymentID"], "deploy-456");
|
||||||
|
assert_eq!(value["credentials"]["sessionToken"], "session-token");
|
||||||
|
// The madmin keys replace the snake_case ones rather than duplicating
|
||||||
|
// them next to each other.
|
||||||
|
for stale in ["bandwidth_limit", "storage_class", "reset_id", "deployment_id"] {
|
||||||
|
assert!(value.get(stale).is_none(), "admin response must not carry `{stale}`");
|
||||||
|
}
|
||||||
|
assert!(value["credentials"].get("session_token").is_none());
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn remote_target_admin_json_latency_round_trips_through_go_duration() {
|
fn remote_target_admin_json_latency_round_trips_through_go_duration() {
|
||||||
// Round trip: a madmin reader decodes the latency values as Go
|
// Round trip: a madmin reader decodes the latency values as Go
|
||||||
|
|||||||
Reference in New Issue
Block a user