fix(server): hyper knobs, decoy fidelity, cache + builder fixes (SRV-04..SRV-10)
- SRV-04: TokioTimer on h1+h2 builder, header_read_timeout 10s, h1 keep-alive on, h2 keep-alive 30s/10s; concurrency boundary documented - SRV-05: with_decoy rebuild keeps extra routes (clone, not take) - SRV-07: method_not_allowed_fallback serves the nginx-shaped 405 - SRV-08: UTF-8 percent-decoding, literal '+', tokio::fs syscalls - SRV-09: /openapi.json cached at construction, generic 500 body, to_openapi returns Result (expect removed) - SRV-10: ChannelsPolicy extension injection point on the WS upgrade; single token resolution via route ordering (WS layer before the router-wide auth route_layer) Verification: cargo test 265 passed; --all-features server::/to_openapi:: green; clippy + fmt clean on touched files (remaining tree noise is a parallel agent's in-flight from_mcp/from_wss/forward work)
This commit is contained in:
+246
-18
@@ -20,8 +20,21 @@
|
||||
//! handler on a reserved path and break the "default surface wins"
|
||||
//! rule. Same-path-different-method merges outside the reserved set
|
||||
//! remain legal (axum composes the `MethodRouter`).
|
||||
//!
|
||||
//! ## Connection knobs and boundaries
|
||||
//!
|
||||
//! [`HttpAdapter::serve_io`] configures the hyper auto builder with a
|
||||
//! tokio timer and timeouts (values in the method doc). The **concurrency
|
||||
//! cap is not a knob of this crate**: [`ProtocolHandler::handle`] serves
|
||||
//! exactly one accepted bidirectional stream per call, and the accept
|
||||
//! loop — how many streams are handled concurrently, on which tasks —
|
||||
//! belongs to the consumer that owns the `Connection` (or the
|
||||
//! assembly-layer listener). Timeout/cap interaction: a request can
|
||||
//! hold its connection for the whole handler runtime; only the header
|
||||
//! phase and idle keep-alive windows are bounded here.
|
||||
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
|
||||
use alkcall::core::auth::AuthContext;
|
||||
use alkcall::core::types::{Connection, HandlerError, StreamError};
|
||||
@@ -29,13 +42,14 @@ use alkcall::registry::registration::OperationRegistry;
|
||||
use async_trait::async_trait;
|
||||
use axum::routing::get;
|
||||
use axum::Router;
|
||||
use hyper_util::rt::{TokioExecutor, TokioIo};
|
||||
use hyper_util::rt::{TokioExecutor, TokioIo, TokioTimer};
|
||||
use hyper_util::server::conn::auto::Builder as HyperBuilder;
|
||||
use hyper_util::service::TowerToHyperService;
|
||||
use parking_lot::Mutex;
|
||||
use tracing::error;
|
||||
|
||||
use super::auth::bearer_auth_middleware;
|
||||
use super::decoy::decoy_fallback;
|
||||
use super::decoy::{decoy_fallback, decoy_method_not_allowed};
|
||||
use super::healthz::healthz;
|
||||
use super::state::{DecoyConfig, RouterState};
|
||||
|
||||
@@ -72,6 +86,7 @@ pub struct HttpAdapter {
|
||||
extra_routes: Option<Router>,
|
||||
alpn: &'static [u8],
|
||||
router: Router,
|
||||
openapi_doc: CachedOpenAPIDoc,
|
||||
}
|
||||
|
||||
impl HttpAdapter {
|
||||
@@ -95,10 +110,12 @@ impl HttpAdapter {
|
||||
alpn: &'static [u8],
|
||||
) -> Self {
|
||||
let decoy = DecoyConfig::default();
|
||||
let openapi_doc = CachedOpenAPIDoc::new(®istry);
|
||||
let state = RouterState {
|
||||
registry: Arc::clone(®istry),
|
||||
identity_provider: Arc::clone(&identity_provider),
|
||||
decoy: decoy.clone(),
|
||||
openapi_doc: openapi_doc.clone(),
|
||||
};
|
||||
let router = build_router(state, None);
|
||||
Self {
|
||||
@@ -108,6 +125,7 @@ impl HttpAdapter {
|
||||
extra_routes: None,
|
||||
alpn,
|
||||
router,
|
||||
openapi_doc,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -117,8 +135,13 @@ impl HttpAdapter {
|
||||
registry: Arc::clone(&self.registry),
|
||||
identity_provider: Arc::clone(&self.identity_provider),
|
||||
decoy,
|
||||
openapi_doc: self.openapi_doc.clone(),
|
||||
};
|
||||
self.router = build_router(state, self.extra_routes.take());
|
||||
// `extra_routes` is borrowed, not consumed (SRV-05): a builder
|
||||
// call after `with_extra_routes` must keep the custom routes in
|
||||
// the rebuild — `with_extra_routes` stores a verified clone, so
|
||||
// re-merging it is safe.
|
||||
self.router = build_router(state, self.extra_routes.clone());
|
||||
self
|
||||
}
|
||||
|
||||
@@ -127,6 +150,7 @@ impl HttpAdapter {
|
||||
registry: Arc::clone(&self.registry),
|
||||
identity_provider: Arc::clone(&self.identity_provider),
|
||||
decoy: self.decoy.clone(),
|
||||
openapi_doc: self.openapi_doc.clone(),
|
||||
};
|
||||
self.router = build_router(state, Some(routes.clone()));
|
||||
self.extra_routes = Some(routes);
|
||||
@@ -171,13 +195,21 @@ fn build_router(state: RouterState, extra_routes: Option<Router>) -> Router {
|
||||
|
||||
let default: Router<RouterState> = Router::new()
|
||||
.merge(crate::gateway::routes::gateway_router())
|
||||
// The openapi handler's state is the pre-serialized projection
|
||||
// cache (SRV-09), threaded through `RouterState` below.
|
||||
.route("/openapi.json", get(openapi_json_handler))
|
||||
.route("/healthz", get(healthz))
|
||||
// The WS upgrade route carries its own bearer middleware
|
||||
// (`ws_bearer_auth` — 401 without a resolvable token, because a WS
|
||||
// session without an identity cannot run AccessControl::check); the
|
||||
// shared bearer_auth_middleware only stashes a permissive
|
||||
// Option<Identity> for the gateway endpoints.
|
||||
// session without an identity cannot run AccessControl::check).
|
||||
// This route is registered BEFORE the router-wide
|
||||
// bearer_auth_middleware route_layer below (route_layer applies
|
||||
// only to routes registered before it), so the WS path resolves
|
||||
// the token exactly once — enforced (401) — while the gateway
|
||||
// endpoints resolve once, permissively (no enforcement; the
|
||||
// dispatch's AccessControl decides). See SRV-10: the previous
|
||||
// mux nested this route inside the shared layer and resolved
|
||||
// the same token twice.
|
||||
.route(
|
||||
WS_UPGRADE_PATH,
|
||||
get(crate::websocket::ws_upgrade_handler).route_layer(from_fn_with_state(
|
||||
@@ -186,6 +218,7 @@ fn build_router(state: RouterState, extra_routes: Option<Router>) -> Router {
|
||||
)),
|
||||
)
|
||||
.fallback(decoy_fallback)
|
||||
.method_not_allowed_fallback(decoy_method_not_allowed)
|
||||
.merge(mcp_router);
|
||||
|
||||
let with_extras = match extra_routes {
|
||||
@@ -198,10 +231,12 @@ fn build_router(state: RouterState, extra_routes: Option<Router>) -> Router {
|
||||
};
|
||||
|
||||
// Applied after the merges (ADR-046 §4): the bearer-auth layer wraps
|
||||
// the extra routes too, so the documented default — custom routes
|
||||
// carry the same auth — holds. A route that wants to opt out carries
|
||||
// its own inner auth layer, which runs innermost (last-applied wins
|
||||
// for extension stashes it inserts).
|
||||
// the extra routes and every default-surface route registered above
|
||||
// except the WS upgrade route (registered earlier with its own
|
||||
// enforced layer, so the token resolves once per request — SRV-10).
|
||||
// A route that wants to opt out carries its own inner auth layer,
|
||||
// which runs innermost (last-applied wins for extension stashes it
|
||||
// inserts).
|
||||
let with_extras =
|
||||
with_extras.route_layer(from_fn_with_state(auth_state, bearer_auth_middleware));
|
||||
|
||||
@@ -370,6 +405,26 @@ impl alkcall::core::types::ProtocolHandler for HttpAdapter {
|
||||
}
|
||||
|
||||
impl HttpAdapter {
|
||||
/// Serve one accepted bidirectional stream as a single HTTP
|
||||
/// connection.
|
||||
///
|
||||
/// Hyper knobs (SRV-04) — hyper 1.11 *silently ignores* the
|
||||
/// header-read timeout unless a timer is set, so the timer is
|
||||
/// explicit and the values chosen are:
|
||||
///
|
||||
/// - `header_read_timeout` = **10 s** (tighter than hyper's 30 s
|
||||
/// default; bounds the slow-loris window where a client drips
|
||||
/// request header bytes)
|
||||
/// - h1 `keep_alive` = **enabled** (default; normal client reuse),
|
||||
/// with hyper's default 30 s idle header-read window applying
|
||||
/// per-request
|
||||
/// - h2 `keep_alive_interval` = **30 s**, `keep_alive_timeout` =
|
||||
/// **10 s** — a peer that fails to ack pings for 10 s is dropped,
|
||||
/// so half-open h2 connections do not accumulate
|
||||
///
|
||||
/// No concurrency cap is set here — see the module doc: the accept
|
||||
/// loop is the consumer's, and the per-connection "cap" is one
|
||||
/// stream per `handle` call.
|
||||
async fn serve_io<I>(&self, io: I) -> Result<(), HandlerError>
|
||||
where
|
||||
I: tokio::io::AsyncRead + tokio::io::AsyncWrite + Send + Unpin + 'static,
|
||||
@@ -377,11 +432,28 @@ impl HttpAdapter {
|
||||
let io = TokioIo::new(io);
|
||||
let service = TowerToHyperService::new(self.router.clone());
|
||||
|
||||
const HEADER_READ_TIMEOUT: Duration = Duration::from_secs(10);
|
||||
const H2_KEEP_ALIVE_INTERVAL: Duration = Duration::from_secs(30);
|
||||
const H2_KEEP_ALIVE_TIMEOUT: Duration = Duration::from_secs(10);
|
||||
|
||||
#[cfg_attr(not(feature = "h2"), allow(unused_mut))]
|
||||
let mut builder = HyperBuilder::new(TokioExecutor::new());
|
||||
#[cfg(feature = "http1")]
|
||||
{
|
||||
builder
|
||||
.http1()
|
||||
.timer(TokioTimer::new())
|
||||
.keep_alive(true)
|
||||
.header_read_timeout(HEADER_READ_TIMEOUT);
|
||||
}
|
||||
#[cfg(feature = "h2")]
|
||||
{
|
||||
builder.http2().enable_connect_protocol();
|
||||
builder
|
||||
.http2()
|
||||
.timer(TokioTimer::new())
|
||||
.enable_connect_protocol()
|
||||
.keep_alive_interval(H2_KEEP_ALIVE_INTERVAL)
|
||||
.keep_alive_timeout(H2_KEEP_ALIVE_TIMEOUT);
|
||||
}
|
||||
|
||||
let conn = builder.serve_connection_with_upgrades(io, service);
|
||||
@@ -399,20 +471,77 @@ fn stream_error_to_handler(e: StreamError) -> HandlerError {
|
||||
HandlerError::from(e)
|
||||
}
|
||||
|
||||
/// Serialized-to-bytes cache of the `/openapi.json` projection
|
||||
/// (SRV-09): built once per `HttpAdapter` from the registry at
|
||||
/// construction instead of re-projecting + re-serializing per request.
|
||||
/// The registry is defined not to mutate after assembly (the assembly
|
||||
/// layer registers handlers before serving), so the doc is fixed for
|
||||
/// the adapter's lifetime; a rebuild via [`HttpAdapter::with_decoy`] /
|
||||
/// `with_extra_routes` re-derives it from the same registry.
|
||||
///
|
||||
/// Serialization happens once, here — so a serde failure surfaces at
|
||||
/// construction as a tracing error and the handler answers `/openapi.json`
|
||||
/// with a generic 500 (no serde internals on the wire) rather than
|
||||
/// echoing an error to unauthenticated callers.
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct CachedOpenAPIDoc {
|
||||
inner: Arc<Mutex<Option<CachedOpenAPIDocInner>>>,
|
||||
}
|
||||
|
||||
struct CachedOpenAPIDocInner {
|
||||
bytes: axum::body::Bytes,
|
||||
}
|
||||
|
||||
impl CachedOpenAPIDoc {
|
||||
pub(crate) fn new(registry: &OperationRegistry) -> Self {
|
||||
Self {
|
||||
inner: Arc::new(Mutex::new(None)),
|
||||
}
|
||||
.with_registry(registry)
|
||||
}
|
||||
|
||||
fn with_registry(self, registry: &OperationRegistry) -> Self {
|
||||
let spec = match crate::adapters::to_openapi(registry) {
|
||||
Ok(spec) => spec,
|
||||
Err(e) => {
|
||||
error!("openapi.json projection failed; endpoint will return 500: {e}");
|
||||
return self;
|
||||
}
|
||||
};
|
||||
match serde_json::to_vec(&spec.raw) {
|
||||
Ok(bytes) => {
|
||||
*self.inner.lock() = Some(CachedOpenAPIDocInner {
|
||||
bytes: axum::body::Bytes::from(bytes),
|
||||
});
|
||||
}
|
||||
Err(e) => {
|
||||
error!("openapi.json serialization failed; endpoint will return 500: {e}");
|
||||
}
|
||||
}
|
||||
self
|
||||
}
|
||||
|
||||
fn bytes(&self) -> Option<axum::body::Bytes> {
|
||||
self.inner.lock().as_ref().map(|c| c.bytes.clone())
|
||||
}
|
||||
}
|
||||
|
||||
/// `GET /openapi.json` — the `to_openapi` projection of the local
|
||||
/// operation registry (ADR-042, ADR-045): the fixed 6-endpoint gateway
|
||||
/// doc. Served under the bearer-auth route layer like every other
|
||||
/// gateway endpoint.
|
||||
/// gateway endpoint. The serialized doc is cached at adapter
|
||||
/// construction ([`CachedOpenAPIDoc`]); a cache miss (projection or
|
||||
/// serialization failed at construction) answers with a **generic**
|
||||
/// `500` body — no serde internals on the wire.
|
||||
async fn openapi_json_handler(
|
||||
axum::extract::State(registry): axum::extract::State<Arc<OperationRegistry>>,
|
||||
axum::extract::State(doc): axum::extract::State<CachedOpenAPIDoc>,
|
||||
) -> axum::response::Response {
|
||||
use axum::response::IntoResponse;
|
||||
let spec = crate::adapters::to_openapi(®istry);
|
||||
match serde_json::to_vec(&spec.raw) {
|
||||
Ok(bytes) => ([(http::header::CONTENT_TYPE, "application/json")], bytes).into_response(),
|
||||
Err(e) => (
|
||||
match doc.bytes() {
|
||||
Some(bytes) => ([(http::header::CONTENT_TYPE, "application/json")], bytes).into_response(),
|
||||
None => (
|
||||
axum::http::StatusCode::INTERNAL_SERVER_ERROR,
|
||||
format!("failed to serialize gateway spec: {e}"),
|
||||
"internal server error",
|
||||
)
|
||||
.into_response(),
|
||||
}
|
||||
@@ -811,6 +940,7 @@ mod tests {
|
||||
registry: empty_registry(),
|
||||
identity_provider: idp,
|
||||
decoy: DecoyConfig::default(),
|
||||
openapi_doc: CachedOpenAPIDoc::new(&OperationRegistry::new()),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -829,6 +959,14 @@ mod tests {
|
||||
.unwrap()
|
||||
}
|
||||
|
||||
async fn get_with_bearer_with_method(
|
||||
app: Router,
|
||||
request: axum::http::Request<axum::body::Body>,
|
||||
) -> axum::http::Response<axum::body::Body> {
|
||||
use tower::ServiceExt;
|
||||
app.oneshot(request).await.unwrap()
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn extra_routes_resolve_bearer_identity_through_the_default_auth() {
|
||||
let extra = Router::new().route(
|
||||
@@ -913,4 +1051,94 @@ mod tests {
|
||||
HttpAdapter::new(static_provider(), empty_registry()).with_extra_routes(extra);
|
||||
assert!(adapter.router().has_routes());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn second_with_decoy_keeps_extra_routes() {
|
||||
let extra = Router::new().route("/v1/ping", get(|| async { "pong" }));
|
||||
let adapter = HttpAdapter::new(static_provider(), empty_registry())
|
||||
.with_extra_routes(extra)
|
||||
.with_decoy(DecoyConfig::Redirect {
|
||||
to: "https://example.com".to_string(),
|
||||
});
|
||||
|
||||
let request = axum::http::Request::builder()
|
||||
.uri("/v1/ping")
|
||||
.body(axum::body::Body::empty())
|
||||
.unwrap();
|
||||
let response = get_with_bearer(adapter.router().clone(), "/v1/ping", None).await;
|
||||
drop(request);
|
||||
assert_eq!(
|
||||
response.status(),
|
||||
axum::http::StatusCode::OK,
|
||||
"a second builder call must not drop the extra routes (SRV-05)"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn method_mismatch_on_registered_path_serves_decoy_405() {
|
||||
let adapter = HttpAdapter::new(static_provider(), empty_registry());
|
||||
let request = axum::http::Request::builder()
|
||||
.method(axum::http::Method::OPTIONS)
|
||||
.uri("/search")
|
||||
.body(axum::body::Body::empty())
|
||||
.unwrap();
|
||||
let response = get_with_bearer_with_method(adapter.router().clone(), request).await;
|
||||
assert_eq!(
|
||||
response.status(),
|
||||
axum::http::StatusCode::METHOD_NOT_ALLOWED
|
||||
);
|
||||
let server = response
|
||||
.headers()
|
||||
.get(axum::http::header::SERVER)
|
||||
.map(|v| v.to_str().unwrap().to_string());
|
||||
assert_eq!(
|
||||
server.as_deref(),
|
||||
Some("nginx"),
|
||||
"405 must carry the decoy Server header, not axum's bare 405 (SRV-07)"
|
||||
);
|
||||
let body = axum::body::to_bytes(response.into_body(), usize::MAX)
|
||||
.await
|
||||
.unwrap();
|
||||
let body = String::from_utf8_lossy(&body);
|
||||
assert!(body.contains("405 Not Allowed"), "got: {body}");
|
||||
assert!(
|
||||
!body.contains("axum") && !body.contains("alk"),
|
||||
"got: {body}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn openapi_json_is_cached_and_generic_on_cache_miss() {
|
||||
let adapter = HttpAdapter::new(static_provider(), empty_registry());
|
||||
let first = openapi_json_handler(axum::extract::State(adapter.openapi_doc.clone())).await;
|
||||
let second = openapi_json_handler(axum::extract::State(adapter.openapi_doc.clone())).await;
|
||||
let first_bytes = axum::body::to_bytes(first.into_body(), usize::MAX)
|
||||
.await
|
||||
.unwrap();
|
||||
let second_bytes = axum::body::to_bytes(second.into_body(), usize::MAX)
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
&first_bytes[..],
|
||||
&second_bytes[..],
|
||||
"cached doc is byte-stable"
|
||||
);
|
||||
|
||||
let miss = CachedOpenAPIDoc {
|
||||
inner: Arc::new(Mutex::new(None)),
|
||||
};
|
||||
let response = openapi_json_handler(axum::extract::State(miss)).await;
|
||||
assert_eq!(
|
||||
response.status(),
|
||||
axum::http::StatusCode::INTERNAL_SERVER_ERROR
|
||||
);
|
||||
let body = axum::body::to_bytes(response.into_body(), usize::MAX)
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
&body[..],
|
||||
b"internal server error",
|
||||
"the 500 body is the fixed generic string, no serde internals"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user