Skip to content

Commit 966306b

Browse files
authored
outbound: implement OutboundPolicies backend request timeouts (#2419)
Depends on #2418 The latest proxy-api release, v0.10.0, adds fields to the `OutboundPolicies` API for configuring HTTP request timeouts, based on the proposed changes to HTTPRoute in kubernetes-sigs/gateway-api#1997. PR #2418 updates the proxy to depend on the new proxy-api release, and implements the `Rule.request_timeout` field added to the API. However, that branch does *not* add a timeout for the `RouteBackend.request_timeout` field. This branch changes the proxy to apply the backend request timeout when configured by the policy controller. This branch implements `RouteBackend.request_timeout` by adding an additional timeout layer in the `MatchedBackend` stack. This applies the per-backend timeout once a backend is selected for a route. I've also added stack tests for the interaction between the request and backend request timeouts. Note that once retries are added to client policy stacks, it may be necessary to move the backend request timeout to ensure it occurs "below" retries, depending on where the retry middleware ends up being located in the proxy stack.
1 parent 1a6225f commit 966306b

File tree

3 files changed

+125
-3
lines changed

3 files changed

+125
-3
lines changed

linkerd/app/outbound/src/http/logical/policy/route/backend.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ pub(crate) struct Backend<T, F> {
1616
pub(crate) route_ref: RouteRef,
1717
pub(crate) concrete: Concrete<T>,
1818
pub(crate) filters: Arc<[F]>,
19+
pub(crate) request_timeout: Option<std::time::Duration>,
1920
}
2021

2122
pub(crate) type MatchedBackend<T, M, F> = super::Matched<M, Backend<T, F>>;
@@ -37,6 +38,7 @@ impl<T: Clone, F> Clone for Backend<T, F> {
3738
route_ref: self.route_ref.clone(),
3839
filters: self.filters.clone(),
3940
concrete: self.concrete.clone(),
41+
request_timeout: self.request_timeout,
4042
}
4143
}
4244
}
@@ -107,6 +109,7 @@ where
107109
}| concrete,
108110
)
109111
.push(filters::NewApplyFilters::<Self, _, _>::layer())
112+
.push(http::NewTimeout::layer())
110113
.push(count_reqs::NewCountRequests::layer_via(ExtractMetrics {
111114
metrics: metrics.clone(),
112115
}))
@@ -116,6 +119,12 @@ where
116119
}
117120
}
118121

122+
impl<T, M, F> svc::Param<http::ResponseTimeout> for MatchedBackend<T, M, F> {
123+
fn param(&self) -> http::ResponseTimeout {
124+
http::ResponseTimeout(self.params.request_timeout)
125+
}
126+
}
127+
119128
impl<T> filters::Apply for Http<T> {
120129
#[inline]
121130
fn apply<B>(&self, req: &mut ::http::Request<B>) -> Result<()> {

linkerd/app/outbound/src/http/logical/policy/router.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,7 @@ where
179179
route_ref: route_ref.clone(),
180180
filters,
181181
concrete,
182+
request_timeout: rb.request_timeout,
182183
}
183184
};
184185

linkerd/app/outbound/src/http/logical/tests.rs

Lines changed: 115 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -306,9 +306,9 @@ async fn route_request_timeout() {
306306

307307
let (_route_tx, routes) = {
308308
let backend = default_backend(&dest);
309-
let mut route = default_route(backend.clone());
310-
// set the request timeout on the route.
311-
route.rules[0].policy.request_timeout = Some(REQUEST_TIMEOUT);
309+
// Set a request timeout for the route, and no backend request timeout
310+
// on the backend.
311+
let route = timeout_route(backend.clone(), Some(REQUEST_TIMEOUT), None);
312312
watch::channel(Routes::Policy(policy::Params::Http(policy::HttpParams {
313313
addr: dest.into(),
314314
meta: ParentRef(client_policy::Meta::new_default("parent")),
@@ -345,6 +345,88 @@ async fn route_request_timeout() {
345345
));
346346
}
347347

348+
#[tokio::test(flavor = "current_thread")]
349+
async fn backend_request_timeout() {
350+
tokio::time::pause();
351+
let _trace = trace::test::trace_init();
352+
// must be less than the `default_config` failfast timeout, or we'll hit
353+
// that instead.
354+
const ROUTE_REQUEST_TIMEOUT: Duration = std::time::Duration::from_secs(2);
355+
const BACKEND_REQUEST_TIMEOUT: Duration = std::time::Duration::from_secs(1);
356+
357+
let addr = SocketAddr::new([192, 0, 2, 41].into(), PORT);
358+
let dest: NameAddr = format!("{AUTHORITY}:{PORT}")
359+
.parse::<NameAddr>()
360+
.expect("dest addr is valid");
361+
let (svc, mut handle) = tower_test::mock::pair();
362+
let connect = HttpConnect::default().service(addr, svc);
363+
let resolve = support::resolver().endpoint_exists(dest.clone(), addr, Default::default());
364+
let (rt, _shutdown) = runtime();
365+
let stack = Outbound::new(default_config(), rt)
366+
.with_stack(connect)
367+
.push_http_cached(resolve)
368+
.into_inner();
369+
370+
let (_route_tx, routes) = {
371+
let backend = default_backend(&dest);
372+
// Set both a route request timeout and a backend request timeout.
373+
let route = timeout_route(
374+
backend.clone(),
375+
Some(ROUTE_REQUEST_TIMEOUT),
376+
Some(BACKEND_REQUEST_TIMEOUT),
377+
);
378+
watch::channel(Routes::Policy(policy::Params::Http(policy::HttpParams {
379+
addr: dest.into(),
380+
meta: ParentRef(client_policy::Meta::new_default("parent")),
381+
backends: Arc::new([backend]),
382+
routes: Arc::new([route]),
383+
failure_accrual: client_policy::FailureAccrual::None,
384+
})))
385+
};
386+
let target = Target {
387+
num: 1,
388+
version: http::Version::H2,
389+
routes,
390+
};
391+
let svc = stack.new_service(target);
392+
393+
handle.allow(1);
394+
let rsp = send_req(svc.clone(), http::Request::get("/"));
395+
serve_req(&mut handle, mk_rsp(StatusCode::OK, "good")).await;
396+
assert_eq!(
397+
rsp.await.expect("request must succeed").status(),
398+
http::StatusCode::OK
399+
);
400+
401+
// Now, time out...
402+
let rsp = send_req(svc.clone(), http::Request::get("/"));
403+
// Wait until we actually get the request --- this timeout only starts once
404+
// the service has been acquired.
405+
handle.allow(1);
406+
let (_, send_rsp) = handle
407+
.next_request()
408+
.await
409+
.expect("service must receive request");
410+
tokio::time::sleep(BACKEND_REQUEST_TIMEOUT + Duration::from_millis(1)).await;
411+
// Still send a response, so that if we didn't hit the backend timeout
412+
// timeout, we don't hit the route timeout and succeed incorrectly.
413+
send_rsp.send_response(mk_rsp(StatusCode::OK, "good"));
414+
let error = rsp.await.expect_err("request must fail with a timeout");
415+
assert!(errors::is_caused_by::<http::timeout::ResponseTimeoutError>(
416+
error.as_ref()
417+
));
418+
419+
// The route request timeout should still apply to time spent before
420+
// the backend is acquired.
421+
let rsp = send_req(svc.clone(), http::Request::get("/"));
422+
tokio::time::sleep(ROUTE_REQUEST_TIMEOUT + Duration::from_millis(1)).await;
423+
handle.allow(1);
424+
let error = rsp.await.expect_err("request must fail with a timeout");
425+
assert!(errors::is_caused_by::<http::timeout::ResponseTimeoutError>(
426+
error.as_ref()
427+
));
428+
}
429+
348430
#[derive(Clone, Debug)]
349431
struct Target {
350432
num: usize,
@@ -518,3 +600,33 @@ fn default_route(backend: client_policy::Backend) -> client_policy::http::Route
518600
}],
519601
}
520602
}
603+
604+
fn timeout_route(
605+
backend: client_policy::Backend,
606+
route_timeout: Option<Duration>,
607+
backend_timeout: Option<Duration>,
608+
) -> client_policy::http::Route {
609+
use client_policy::{
610+
http::{self, Filter, Policy, Route, Rule},
611+
Meta, RouteBackend, RouteDistribution,
612+
};
613+
use once_cell::sync::Lazy;
614+
static NO_FILTERS: Lazy<Arc<[Filter]>> = Lazy::new(|| Arc::new([]));
615+
Route {
616+
hosts: vec![],
617+
rules: vec![Rule {
618+
matches: vec![http::r#match::MatchRequest::default()],
619+
policy: Policy {
620+
meta: Meta::new_default("test_route"),
621+
filters: NO_FILTERS.clone(),
622+
failure_policy: Default::default(),
623+
request_timeout: route_timeout,
624+
distribution: RouteDistribution::FirstAvailable(Arc::new([RouteBackend {
625+
filters: NO_FILTERS.clone(),
626+
backend,
627+
request_timeout: backend_timeout,
628+
}])),
629+
},
630+
}],
631+
}
632+
}

0 commit comments

Comments
 (0)