From d62a869e68c7cba3e8bba417bf80113f93662f5e Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 9 Mar 2018 16:25:19 -0800 Subject: [PATCH] Fix outbound HTTP/1 requests not using Destinations (#555) Commit 569d6939a799bb0df6bd4053de7d7e8ac6b49ab6 introduced a regression that caused the proxy to stop using the Destination service for outbound HTTP/1 requests with no authority in the request URI but a valid authority in the `Host:` header. The bug is due to some code in `Outbound::recognize` which assumed that a request had already been passed through `normalize_our_view_of_uri`. This was valid at one point while I was writing #492, as URIs were normalized prior to `recognize` and a request `Extension` was used to mark that they had been rewritten, and the host header and request URI could be assumed to be in agreement, but after merging #514 into the dev branch for #492, this behaviour changed and I forgot to update the logic in `recognize`. I've fixed the issue by adding the logic for routing on `Host:` headers back into `Outbound::recognize`. @seanmonstar added a test in `discovery.rs`, `outbound_http1_asks_controller_about_host`, which should exercise this case. I've added a couple more unit tests in that file to try and ensure we cover more of the different cases that can occur here. Fixes #552 --- proxy/src/outbound.rs | 21 +++++++++---- proxy/src/transparency/h1.rs | 2 +- proxy/tests/discovery.rs | 59 ++++++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/proxy/src/outbound.rs b/proxy/src/outbound.rs index 334601df4..b4269b32b 100644 --- a/proxy/src/outbound.rs +++ b/proxy/src/outbound.rs @@ -20,6 +20,7 @@ use control::discovery::Bind as BindTrait; use ctx; use fully_qualified_authority::{FullyQualifiedAuthority, NamedAddress}; use timeout::Timeout; +use transparency::h1; type BindProtocol = bind::BindProtocol, B>; @@ -87,12 +88,20 @@ where fn recognize(&self, req: &Self::Request) -> Option> { let proto = bind::Protocol::detect(req); - let local = req.uri().authority_part().map(|authority| { - FullyQualifiedAuthority::normalize( - authority, - &self.default_namespace) - - }); + // The request URI and Host: header have not yet been normalized + // by `NormalizeUri`, as we need to know whether the request will + // be routed by Host/authority or by SO_ORIGINAL_DST, in order to + // determine whether the service is reusable. + let local = req.uri().authority_part() + .cloned() + // Therefore, we need to check the host header as well as the URI + // for a valid authority, before we fall back to SO_ORIGINAL_DST. + .or_else(|| h1::authority_from_host(req)) + .map(|authority| { + FullyQualifiedAuthority::normalize( + &authority, + &self.default_namespace) + }); // If we can't fully qualify the authority as a local service, // and there is no original dst, then we have nothing! In that diff --git a/proxy/src/transparency/h1.rs b/proxy/src/transparency/h1.rs index f0b869d8f..f189e4ce0 100644 --- a/proxy/src/transparency/h1.rs +++ b/proxy/src/transparency/h1.rs @@ -46,7 +46,7 @@ pub fn normalize_our_view_of_uri(req: &mut http::Request) { /// Returns an Authority from a request's Host header. pub fn authority_from_host(req: &http::Request) -> Option { - req.headers().get(HOST).cloned() + req.headers().get(HOST) .and_then(|host| { host.to_str().ok() .and_then(|s| { diff --git a/proxy/tests/discovery.rs b/proxy/tests/discovery.rs index cad094499..2888b261f 100644 --- a/proxy/tests/discovery.rs +++ b/proxy/tests/discovery.rs @@ -18,6 +18,21 @@ fn outbound_asks_controller_api() { assert_eq!(client.get("/bye"), "bye"); } +#[test] +fn outbound_asks_controller_about_authority() { + let _ = env_logger::try_init(); + + let srv = server::new().route("/", "hello").route("/bye", "bye").run(); + let ctrl = controller::new() + .destination("disco.test.svc.cluster.local", srv.addr) + .run(); + let proxy = proxy::new().controller(ctrl).run(); + let client = client::new(proxy.outbound, "disco.test.svc.cluster.local"); + + assert_eq!(client.get("/"), "hello"); + assert_eq!(client.get("/bye"), "bye"); +} + #[test] fn outbound_reconnects_if_controller_stream_ends() { let _ = env_logger::try_init(); @@ -33,6 +48,50 @@ fn outbound_reconnects_if_controller_stream_ends() { assert_eq!(client.get("/recon"), "nect"); } +#[test] +fn outbound_http1_asks_controller_about_host() { + let _ = env_logger::try_init(); + + let srv = server::http1() + .route("/", "hello") + .route("/bye", "bye") + .run(); + let ctrl = controller::new() + .destination("disco.test.svc.cluster.local", srv.addr) + .run(); + let proxy = proxy::new() + .controller(ctrl) + // don't set srv as outbound(), so that SO_ORIGINAL_DST isn't + // used as a backup + .run(); + let client = client::http1(proxy.outbound, "disco.test.svc.cluster.local"); + + assert_eq!(client.get("/"), "hello"); + assert_eq!(client.get("/bye"), "bye"); +} + +#[test] +fn outbound_http1_asks_controller_api() { + let _ = env_logger::try_init(); + + let srv = server::http1() + .route("/", "hello") + .route("/bye", "bye") + .run(); + let ctrl = controller::new() + .destination("disco.test.svc.cluster.local", srv.addr) + .run(); + let proxy = proxy::new() + .controller(ctrl) + // don't set srv as outbound(), so that SO_ORIGINAL_DST isn't + // used as a backup + .run(); + let client = client::http1(proxy.outbound, "disco.test.svc.cluster.local"); + + assert_eq!(client.get("/"), "hello"); + assert_eq!(client.get("/bye"), "bye"); +} + #[test] fn outbound_updates_newer_services() { let _ = env_logger::try_init();