From e82789ea4bf7a19eb6f40a93d795a12b3e9567ff Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 29 Apr 2026 00:51:10 -0700 Subject: [PATCH] rust(volume): strip grpc-port suffix from master URL before HTTP lookup (#9276) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * rust(volume): strip grpc-port suffix from master URL before HTTP lookup The volume server stores `master_url` in SeaweedFS's canonical `host:httpPort.grpcPort` form (e.g. `node-a:5300.5310`). When `lookup_volume` builds the master `/dir/lookup` URL, the appended gRPC port turns the URL into `http://node-a:5300.5310/...`, which reqwest rejects with "builder error". Every replicated write, batch-delete lookup, and proxy/redirect read then fails. Mirror Go's `pb.ServerAddress.ToHttpAddress()` with a new `to_http_address` helper and apply it inside `lookup_volume`, the single funnel for all three HTTP master lookups in the Rust volume server. Other consumers of `master_url` already go through gRPC and use `to_grpc_address` / `parse_grpc_address`. Includes a regression test that mocks a master HTTP server and calls `lookup_volume` with a `host:port.grpcPort` master URL — without the fix it reproduces the exact "lookup request failed: builder error" from issue #9274. Fixes #9274 * rust(volume): only strip dotted suffix from address when both ports are numeric Previously `to_http_address` rewrote any `host:foo.bar` to `host:foo`, which would silently drop the suffix on malformed config (e.g. a hostname like `host:abc.def` or `host:99999.19333`). Validate that both halves of the dotted suffix parse as `u16` before stripping — mirrors the validation that `to_grpc_address` already does in the inverse direction. Also slice the input directly instead of going through `format!`, since the result is just a prefix of `addr`. Adds a test that asserts non-numeric / out-of-range dotted suffixes are preserved unchanged. * rust(volume): strip grpc-port suffix from peer URLs in replicate / proxy / redirect In normal operation the master returns `Location.url` as plain `host:port` and the gRPC port arrives in a separate field. But the volume server already has defensive logic (`grpc_address_for_location`) for the `host:httpPort.grpcPort` form on those same URLs, which implies a code path where peer URLs do carry the suffix. Apply `to_http_address` to `loc.url` / `target.url` before building HTTP URLs in `do_replicated_request`, `proxy_request`, and `redirect_request` to keep replicate-write, proxy-read, and redirect paths from hitting the same `lookup request failed: builder error` mode that #9274 documented for master lookups. Adds a unit test exercising `redirect_request` with a `.grpcPort` suffix on `target.url`. * rust(volume): return Cow from to_http_address to skip allocation on the no-suffix path Most addresses pass through `to_http_address` unchanged (master and peer URLs are normally plain `host:port`), so the previous String return type allocated on every call for nothing. Switch to `Cow`: the common pass-through borrows from the input, and only the rewrite branch allocates. Call sites use the result via `format!`/`Display`, which both work transparently with `Cow`. Adds a test asserting the variant is Borrowed in the no-rewrite cases and Owned only when the suffix is stripped. * rust(volume): cover bracketed IPv6 literals in to_http_address tests The current implementation already handles IPv6 correctly because `rfind(':')` lands on the colon after the closing bracket, leaving the dotted suffix logic unchanged. Add explicit test coverage so the behavior is locked down for dual-stack deployments — both the strip case (`[::1]:9333.19333` -> `[::1]:9333`) and the various passthrough cases (no suffix, non-numeric, out-of-range, trailing colon). * rust(volume): parse both ports in one tuple pattern match in to_http_address Combine the two `is_ok()` checks into a single `if let (Ok(_), Ok(_))` tuple match — equivalent semantics, slightly tighter expression of intent. No behavior change. --- seaweed-volume/src/server/handlers.rs | 99 +++++++++++++++++-- seaweed-volume/src/server/volume_server.rs | 107 +++++++++++++++++++++ 2 files changed, 200 insertions(+), 6 deletions(-) diff --git a/seaweed-volume/src/server/handlers.rs b/seaweed-volume/src/server/handlers.rs index a7d021dad..d324a587b 100644 --- a/seaweed-volume/src/server/handlers.rs +++ b/seaweed-volume/src/server/handlers.rs @@ -16,7 +16,7 @@ use axum::response::{IntoResponse, Response}; use serde::{Deserialize, Serialize}; use super::grpc_client::{build_grpc_endpoint, GRPC_MAX_MESSAGE_SIZE}; -use super::volume_server::{normalize_outgoing_http_url, VolumeServerState}; +use super::volume_server::{normalize_outgoing_http_url, to_http_address, VolumeServerState}; use crate::config::ReadMode; use crate::metrics; use crate::pb::volume_server_pb; @@ -357,9 +357,12 @@ async fn lookup_volume( master_url: &str, volume_id: u32, ) -> Result, String> { + // master_url may be in SeaweedFS's "host:httpPort.grpcPort" form (mirrors + // Go's pb.ServerAddress); strip the gRPC port before building the HTTP URL. + let master_http = to_http_address(master_url); let url = normalize_outgoing_http_url( scheme, - &format!("{}/dir/lookup?volumeId={}", master_url, volume_id), + &format!("{}/dir/lookup?volumeId={}", master_http, volume_id), )?; let resp = client .get(&url) @@ -528,9 +531,12 @@ async fn do_replicated_request( let mut futures = Vec::new(); for loc in remote_locations { + // Defensively strip a `.grpcPort` suffix in case loc.url carries it + // (e.g. from an older master); see lookup_volume for the same fix. + let peer_http = to_http_address(&loc.url); let url = normalize_outgoing_http_url( &state.outgoing_http_scheme, - &format!("{}{}?{}", loc.url, path, new_query), + &format!("{}{}?{}", peer_http, path, new_query), )?; let client = state.http_client.clone(); @@ -698,13 +704,15 @@ async fn proxy_request( ) -> Response { // Build target URL, adding proxied=true query param let path = info.path.trim_start_matches('/'); + // Defensively strip a `.grpcPort` suffix in case target.url carries it. + let target_http = to_http_address(&target.url); let raw_target = if info.original_query.is_empty() { - format!("{}/{}?proxied=true", target.url, path) + format!("{}/{}?proxied=true", target_http, path) } else { format!( "{}/{}?{}&proxied=true", - target.url, path, info.original_query + target_http, path, info.original_query ) }; let target_url = match normalize_outgoing_http_url(&state.outgoing_http_scheme, &raw_target) { @@ -771,9 +779,11 @@ fn redirect_request(info: &ProxyRequestInfo, target: &VolumeLocation, scheme: &s query_params.push("proxied=true".to_string()); let query = query_params.join("&"); + // Defensively strip a `.grpcPort` suffix in case target.url carries it. + let target_http = to_http_address(&target.url); let raw_target = format!( "{}/{},{}?{}", - target.url, &info.vid_str, &info.fid_str, query + target_http, &info.vid_str, &info.fid_str, query ); let location = match normalize_outgoing_http_url(scheme, &raw_target) { Ok(url) => url, @@ -3920,4 +3930,81 @@ mod tests { "https://volume.internal:8080/3,01637037d6?collection=photos&proxied=true" ); } + + /// Defensive coverage for peer-to-peer redirects: if a master ever returns + /// `Location.url` already encoded in `host:httpPort.grpcPort` form, + /// `redirect_request` must still emit a valid HTTP Location header. + #[test] + fn test_redirect_request_strips_grpc_port_from_target_url() { + let info = ProxyRequestInfo { + original_headers: HeaderMap::new(), + original_query: String::new(), + path: "/3,01637037d6".to_string(), + vid_str: "3".to_string(), + fid_str: "01637037d6".to_string(), + }; + let target = VolumeLocation { + url: "volume.internal:8080.18080".to_string(), + public_url: "volume.public:8080.18080".to_string(), + grpc_port: 18080, + }; + + let response = redirect_request(&info, &target, "http"); + assert_eq!(response.status(), StatusCode::MOVED_PERMANENTLY); + assert_eq!( + response.headers().get(header::LOCATION).unwrap(), + "http://volume.internal:8080/3,01637037d6?proxied=true" + ); + } + + /// Regression test for issue #9274. + /// + /// SeaweedFS encodes a server's gRPC port by appending `.grpcPort` to + /// the HTTP port: e.g. `master.local:9333.19333` (mirrors Go's + /// `pb.ServerAddress`). When the volume server's `--mserver` flag is + /// configured this way, replication writes look up the peer locations + /// from the master over HTTP. Prior to the fix, the lookup URL became + /// `http://host:9333.19333/dir/lookup?volumeId=...` — an invalid port + /// that reqwest rejects with a "builder error", causing every + /// replicated write to fail. + #[tokio::test] + async fn test_lookup_volume_strips_grpc_port_from_master_url() { + use axum::{routing::get, Router}; + + let app = Router::new().route( + "/dir/lookup", + get(|axum::extract::Query(params): axum::extract::Query>| async move { + assert_eq!(params.get("volumeId").map(String::as_str), Some("31")); + axum::Json(serde_json::json!({ + "volumeOrFileId": "31", + "locations": [ + {"url": "10.0.0.2:5301", "publicUrl": "10.0.0.2:5301", "grpcPort": 5311} + ] + })) + }), + ); + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + + // Mirrors how main.rs receives masters: a `host:httpPort.grpcPort` token. + // The grpc-port part is fictitious here (the master is HTTP-only in + // this test) — what matters is that the volume server must not pass + // it through to reqwest. + let master_url = format!("127.0.0.1:{}.{}", addr.port(), 19999); + let client = reqwest::Client::new(); + + let locations = super::lookup_volume(&client, "http", &master_url, 31) + .await + .expect("lookup_volume should succeed even when master_url carries a .grpcPort suffix"); + + assert_eq!(locations.len(), 1); + assert_eq!(locations[0].url, "10.0.0.2:5301"); + assert_eq!(locations[0].grpc_port, 5311); + + server.abort(); + } } diff --git a/seaweed-volume/src/server/volume_server.rs b/seaweed-volume/src/server/volume_server.rs index 90436dc01..b3dccf353 100644 --- a/seaweed-volume/src/server/volume_server.rs +++ b/seaweed-volume/src/server/volume_server.rs @@ -134,6 +134,38 @@ pub fn normalize_outgoing_http_url(scheme: &str, raw_target: &str) -> Result `host:port` +/// - `host:port` -> `host:port` (unchanged) +/// - Anything that does not look like `host:port[.grpcPort]` is returned unchanged. +/// +/// Returns a `Cow` so the common (no-suffix) case borrows from `addr` +/// without allocating; only the rewrite branch produces a new `String`. +pub fn to_http_address(addr: &str) -> std::borrow::Cow<'_, str> { + let Some(ports_sep_index) = addr.rfind(':') else { + return std::borrow::Cow::Borrowed(addr); + }; + let ports = &addr[ports_sep_index + 1..]; + if let Some(dot_idx) = ports.rfind('.') { + let http_port = &ports[..dot_idx]; + let grpc_port = &ports[dot_idx + 1..]; + // Only strip the suffix when both parts parse as real ports — leave + // anything else (e.g. "host:abc.def") untouched so bad config surfaces + // rather than being silently rewritten. Mirrors the validation already + // done in `to_grpc_address` for the inverse direction. + if let (Ok(_), Ok(_)) = (http_port.parse::(), grpc_port.parse::()) { + return std::borrow::Cow::Owned( + addr[..ports_sep_index + 1 + dot_idx].to_string(), + ); + } + } + std::borrow::Cow::Borrowed(addr) +} + fn request_remote_addr(request: &Request) -> Option { request .extensions() @@ -392,3 +424,78 @@ pub fn build_public_router(state: Arc) -> Router { .layer(middleware::from_fn(common_headers_middleware)) .with_state(state) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_to_http_address_strips_grpc_port_suffix() { + assert_eq!(to_http_address("10.0.0.1:9333.19333"), "10.0.0.1:9333"); + assert_eq!( + to_http_address("master.local:9333.19333"), + "master.local:9333" + ); + assert_eq!(to_http_address("10.85.183.6:5300.6300"), "10.85.183.6:5300"); + } + + #[test] + fn test_to_http_address_passthrough_without_grpc_suffix() { + assert_eq!(to_http_address("10.0.0.1:9333"), "10.0.0.1:9333"); + assert_eq!(to_http_address("master.local:9333"), "master.local:9333"); + } + + #[test] + fn test_to_http_address_returns_input_when_unparseable() { + assert_eq!(to_http_address(""), ""); + assert_eq!(to_http_address("no-port"), "no-port"); + // Trailing colon: nothing after the separator, treat as unparseable. + assert_eq!(to_http_address("host:"), "host:"); + } + + #[test] + fn test_to_http_address_borrows_when_unchanged_and_owns_when_stripped() { + // The common case (no suffix) must not allocate. + let result = to_http_address("10.0.0.1:9333"); + assert!(matches!(result, std::borrow::Cow::Borrowed(_))); + // Stripping requires a new string. + let result = to_http_address("10.0.0.1:9333.19333"); + assert!(matches!(result, std::borrow::Cow::Owned(_))); + assert_eq!(result, "10.0.0.1:9333"); + // Unparseable / passthrough also borrows. + let result = to_http_address("host:abc.def"); + assert!(matches!(result, std::borrow::Cow::Borrowed(_))); + } + + #[test] + fn test_to_http_address_keeps_non_numeric_dotted_suffix() { + // The dotted form is only valid when both sides are real port numbers. + // Otherwise the address is malformed config (e.g. a hostname like + // "host:abc.def"), and silently rewriting it would just hide the bug. + assert_eq!(to_http_address("host:abc.def"), "host:abc.def"); + assert_eq!(to_http_address("host:9333.notaport"), "host:9333.notaport"); + assert_eq!(to_http_address("host:notaport.19333"), "host:notaport.19333"); + // Out-of-range ports must not be silently truncated either. + assert_eq!(to_http_address("host:99999.19333"), "host:99999.19333"); + } + + #[test] + fn test_to_http_address_handles_bracketed_ipv6_literals() { + // The function uses `rfind(':')`, so for bracketed IPv6 the port + // separator is correctly identified as the colon AFTER the closing + // bracket — making IPv4 and IPv6 behave the same. + assert_eq!(to_http_address("[::1]:9333.19333"), "[::1]:9333"); + assert_eq!( + to_http_address("[2001:db8::10]:5300.6300"), + "[2001:db8::10]:5300" + ); + // Plain bracketed IPv6 without a dotted suffix is borrowed unchanged. + let result = to_http_address("[2001:db8::1]:9333"); + assert!(matches!(result, std::borrow::Cow::Borrowed(_))); + assert_eq!(result, "[2001:db8::1]:9333"); + // Non-numeric / out-of-range / missing suffix all preserve the input. + assert_eq!(to_http_address("[::1]:"), "[::1]:"); + assert_eq!(to_http_address("[::1]:abc.def"), "[::1]:abc.def"); + assert_eq!(to_http_address("[::1]:99999.19333"), "[::1]:99999.19333"); + } +}