From 278aa2adb61b1d1ba56e6985f04e79a36d6d9988 Mon Sep 17 00:00:00 2001 From: Simo Lin <25425177+slin1237@users.noreply.github.com> Date: Thu, 11 Jun 2026 14:06:47 -0700 Subject: [PATCH] fix(model_gateway): bound HTTP metrics path label to matched route The HTTP layer-1 metrics labeled each request by normalize_path_for_metrics(uri.path()), which only collapsed dynamic segments at path index > 2. Parameterized routes whose dynamic segment sits at index <= 2 (e.g. /v1/responses/{response_id}, /workers/{worker_id}, /v1/conversations/{conversation_id}, /v1/tokenizers/{tokenizer_id}) therefore passed the raw id through verbatim as the "path" label. Each distinct id was interned into the never-evicted global STRING_INTERNER and emitted as a distinct Prometheus series, letting attacker-controlled ids drive unbounded metric cardinality and unbounded interner growth. Label by axum's MatchedPath route template instead, falling back to a fixed "other" when no route matched, which bounds the label set to the registered route table. Remove the now-obsolete segment-based normalizer. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com> --- model_gateway/src/middleware/logging.rs | 6 +- model_gateway/src/middleware/metrics.rs | 214 ++++++++++++------------ 2 files changed, 106 insertions(+), 114 deletions(-) diff --git a/model_gateway/src/middleware/logging.rs b/model_gateway/src/middleware/logging.rs index 200840e37a..8346b04aca 100644 --- a/model_gateway/src/middleware/logging.rs +++ b/model_gateway/src/middleware/logging.rs @@ -11,7 +11,7 @@ use tower_http::trace::{MakeSpan, OnRequest, OnResponse, TraceLayer}; use tracing::{error, field::Empty, info, info_span, warn, Span}; use tracing_opentelemetry::OpenTelemetrySpanExt; -use super::{metrics::normalize_path_for_metrics, request_id::RequestId}; +use super::{metrics::matched_path_label, request_id::RequestId}; use crate::observability::{ metrics::{method_to_static_str, Metrics}, otel_trace::extract_trace_context_http, @@ -62,8 +62,8 @@ impl OnRequest for RequestLogger { } let method = method_to_static_str(request.method().as_str()); - let path = normalize_path_for_metrics(request.uri().path()); - Metrics::record_http_request(method, &path); + let path = matched_path_label(request.extensions()); + Metrics::record_http_request(method, path); // Log the request start info!( diff --git a/model_gateway/src/middleware/metrics.rs b/model_gateway/src/middleware/metrics.rs index 018a278e6c..52705a9815 100644 --- a/model_gateway/src/middleware/metrics.rs +++ b/model_gateway/src/middleware/metrics.rs @@ -2,8 +2,8 @@ //! //! `HttpMetricsLayer` wraps the inner service to record per-request //! duration plus the in-flight connection count via -//! `InFlightRequestTracker`. The path is normalized to bound metric -//! cardinality. +//! `InFlightRequestTracker`. The path label is the matched axum route +//! template (or `"other"` when unmatched) to bound metric cardinality. use std::{ pin::Pin, @@ -12,7 +12,10 @@ use std::{ time::Instant, }; -use axum::{extract::Request, response::Response}; +use axum::{ + extract::{MatchedPath, Request}, + response::Response, +}; use tower::{Layer, Service}; use crate::{ @@ -69,7 +72,7 @@ where fn call(&mut self, req: Request) -> Self::Future { let method = method_to_static_str(req.method().as_str()); - let path = normalize_path_for_metrics(req.uri().path()); + let path = matched_path_label(req.extensions()).to_owned(); let start = Instant::now(); let mut inner = self.inner.clone(); @@ -100,129 +103,118 @@ where } } -/// Normalize path for metrics to avoid high cardinality. -/// Replaces dynamic segments (IDs, UUIDs) with `{id}` placeholder. -/// Only allocates when normalization is needed; uses single-pass with byte offsets. -pub(super) fn normalize_path_for_metrics(path: &str) -> String { - let bytes = path.as_bytes(); - let mut segment_start = 0; - let mut segment_idx = 0; - let mut result: Option = None; - - for (pos, &b) in bytes.iter().enumerate() { - if b == b'/' || pos == bytes.len() - 1 { - // Determine segment end (include last char if not a slash) - let segment_end = if b == b'/' { pos } else { pos + 1 }; - let segment = &path[segment_start..segment_end]; - - // Check segments after index 2 for dynamic IDs - if segment_idx > 2 && !segment.is_empty() && is_dynamic_id(segment) { - // Initialize result with everything before this segment - let result = result.get_or_insert_with(|| { - let mut s = String::with_capacity(path.len()); - s.push_str(&path[..segment_start]); - s - }); - result.push_str("{id}"); - } else if let Some(ref mut r) = result { - // Already normalizing, append this segment as-is - r.push_str(segment); - } - - // Add slash after segment (except at end) - if b == b'/' { - if let Some(ref mut r) = result { - r.push('/'); - } - segment_start = pos + 1; - segment_idx += 1; - } - } - } - - result.unwrap_or_else(|| path.to_owned()) -} - -/// Check if segment looks like a dynamic ID (prefixed ID, UUID, or numeric). -#[inline] -fn is_dynamic_id(s: &str) -> bool { - // Prefixed IDs: resp_xxx, chatcmpl_xxx (len > 10 with underscore) - if s.len() > 10 && s.contains('_') { - return true; - } - // UUIDs: 32+ hex chars with dashes - if s.len() >= 32 && s.bytes().all(|b| b.is_ascii_hexdigit() || b == b'-') { - return true; - } - // Numeric IDs - !s.is_empty() && s.bytes().all(|b| b.is_ascii_digit()) +/// Bounded path label for HTTP metrics: the matched axum route template, or +/// `"other"` when no route matched. Labeling by raw request path would let +/// attacker-controlled URIs create unbounded distinct labels. +pub(super) fn matched_path_label(extensions: &http::Extensions) -> &str { + extensions + .get::() + .map_or("other", MatchedPath::as_str) } #[cfg(test)] mod tests { + use std::sync::{Arc, Mutex}; + + use axum::{body::Body, http::Request, routing::get, Router}; + use tower::{ServiceBuilder, ServiceExt}; + use super::*; + use crate::observability::metrics::interner_size; #[test] - fn test_normalize_path_no_ids() { - // Common API paths should pass through unchanged - assert_eq!( - normalize_path_for_metrics("/v1/chat/completions"), - "/v1/chat/completions" - ); - assert_eq!( - normalize_path_for_metrics("/v1/completions"), - "/v1/completions" - ); - assert_eq!(normalize_path_for_metrics("/v1/models"), "/v1/models"); - assert_eq!(normalize_path_for_metrics("/health"), "/health"); + fn matched_path_label_defaults_to_other_when_absent() { + // No routing has run, so there is no MatchedPath extension. + let extensions = http::Extensions::new(); + assert_eq!(matched_path_label(&extensions), "other"); } - #[test] - fn test_normalize_path_with_prefixed_id() { - // Prefixed IDs (resp_xxx, chatcmpl_xxx) should be normalized - assert_eq!( - normalize_path_for_metrics("/v1/responses/resp_abc123def456"), - "/v1/responses/{id}" - ); - assert_eq!( - normalize_path_for_metrics("/v1/chat/completions/chatcmpl_abc123xyz"), - "/v1/chat/completions/{id}" - ); + /// Drive `request_uri` through a router that has one dynamic route and a + /// fallback, returning the label `matched_path_label` observes at a + /// `Router::layer`-applied middleware. The layer is the outermost wrap (it + /// also covers the fallback) so both the matched and unmatched branches are + /// exercised at the same layer the production metrics middleware uses. + async fn label_at_layer(request_uri: &str) -> String { + let captured: Arc>> = Arc::new(Mutex::new(None)); + let sink = captured.clone(); + + let app = Router::new() + .route("/v1/responses/{response_id}", get(|| async { "ok" })) + .fallback(|| async { "fallback" }) + .layer( + ServiceBuilder::new().map_request(move |req: Request| { + *sink.lock().unwrap() = Some(matched_path_label(req.extensions()).to_owned()); + req + }), + ); + + let response = app + .oneshot( + Request::builder() + .uri(request_uri) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert!(response.status().is_success()); + + let label = captured.lock().unwrap().clone(); + label.expect("label-capturing layer ran") } - #[test] - fn test_normalize_path_with_uuid() { + #[tokio::test] + async fn matched_route_uses_template_label() { + // A matched dynamic route is labeled by its template, never the raw id. assert_eq!( - normalize_path_for_metrics("/v1/responses/550e8400-e29b-41d4-a716-446655440000"), - "/v1/responses/{id}" + label_at_layer("/v1/responses/resp_abc123").await, + "/v1/responses/{response_id}" ); } - #[test] - fn test_normalize_path_with_numeric_id() { - assert_eq!( - normalize_path_for_metrics("/v1/workers/12345"), - "/v1/workers/{id}" - ); + #[tokio::test] + async fn unmatched_path_collapses_to_other() { + // An unmatched path must collapse to "other", not echo the raw URI. + assert_eq!(label_at_layer("/totally/unregistered/aaaa").await, "other"); } - #[test] - fn test_is_dynamic_id() { - // Prefixed IDs - assert!(is_dynamic_id("resp_abc123def")); - assert!(is_dynamic_id("chatcmpl_xyz789")); - assert!(!is_dynamic_id("short_id")); // Too short - - // UUIDs - assert!(is_dynamic_id("550e8400-e29b-41d4-a716-446655440000")); - assert!(is_dynamic_id("550e8400e29b41d4a716446655440000")); // No dashes - - // Numeric - assert!(is_dynamic_id("12345")); - assert!(!is_dynamic_id("")); // Empty - - // Regular words - assert!(!is_dynamic_id("completions")); - assert!(!is_dynamic_id("chat")); + #[tokio::test] + async fn distinct_ids_on_matched_route_do_not_grow_interner() { + use crate::observability::inflight_tracker::InFlightRequestTracker; + + // Drive the real `HttpMetricsLayer`. Every request matches the dynamic + // route `/v1/responses/{response_id}`, so each distinct id must record + // the bounded template label and leave the never-evicted interner flat. + let app = Router::new() + .route("/v1/responses/{response_id}", get(|| async { "ok" })) + .layer(HttpMetricsLayer::new(InFlightRequestTracker::new())); + + let send = |uri: String| { + let app = app.clone(); + async move { + let response = app + .oneshot(Request::builder().uri(uri).body(Body::empty()).unwrap()) + .await + .unwrap(); + assert!(response.status().is_success()); + } + }; + + // Warm up so the template label and the empty error_code are interned. + send("/v1/responses/resp_warmup".to_owned()).await; + let size_before = interner_size(); + + const ITERS: usize = 1000; + for i in 0..ITERS { + send(format!("/v1/responses/resp_{i}")).await; + } + + // Slack tolerates strings unrelated parallel tests may intern; an + // unbounded label would instead grow the interner by ~ITERS. + let growth = interner_size().saturating_sub(size_before); + assert!( + growth < 100, + "interner grew by {growth} for {ITERS} distinct request ids" + ); } }