From 902b7efac1d2aaa13277ef62572c81cba6b41cd2 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Tue, 19 Jun 2018 14:01:27 -0700 Subject: [PATCH] proxy: Clarify Outbound::recognize (#1144) The comments in Outbound::recognize had become somewhat stale as the logic changed. Furthermore, this implementation may be easier to understand if broken into smaller pieces. This change reorganizes the Outbound:recognize method into helper methods--`destination`, `host_port`, and `normalize`--each with accompanying docstrings that more accurately reflect the current implementation. This also has the side-effect benefit of eliminating a string clone on every request. --- proxy/src/outbound.rs | 136 +++++++++++++++++++++++++----------------- 1 file changed, 81 insertions(+), 55 deletions(-) diff --git a/proxy/src/outbound.rs b/proxy/src/outbound.rs index 28dfb2d95248a..aae7ed871602f 100644 --- a/proxy/src/outbound.rs +++ b/proxy/src/outbound.rs @@ -35,10 +35,14 @@ const MAX_IN_FLIGHT: usize = 10_000; /// This default is used by Finagle. const DEFAULT_DECAY: Duration = Duration::from_secs(10); +/// Describes a destination for HTTP requests. #[derive(Clone, Debug, PartialEq, Eq, Hash)] pub enum Destination { - Hostname(DnsNameAndPort), - ImplicitOriginalDst(SocketAddr), + /// A logical, lazily-bound endpoint. + Name(DnsNameAndPort), + + /// A single, bound endpoint. + Addr(SocketAddr), } // ===== impl Outbound ===== @@ -54,6 +58,65 @@ impl Outbound { bind_timeout, } } + + + /// TODO: Return error when `HostAndPort::normalize()` fails. + /// TODO: Use scheme-appropriate default port. + fn normalize(authority: &http::uri::Authority) -> Option { + const DEFAULT_PORT: Option = Some(80); + HostAndPort::normalize(authority, DEFAULT_PORT).ok() + } + + /// Determines the logical host:port of the request. + /// + /// If the parsed URI includes an authority, use that. Otherwise, try to load the + /// authority from the `Host` header. + /// + /// The port is either parsed from the authority or a default of 80 is used. + fn host_port(req: &http::Request) -> Option { + // Note: Calls to `normalize` cannot be deduped without cloning `authority`. + req.uri() + .authority_part() + .and_then(Self::normalize) + .or_else(|| { + h1::authority_from_host(req) + .and_then(|h| Self::normalize(&h)) + }) + } + + /// Determines the destination for a request. + /// + /// Typically, a request's authority is used to produce a `Destination`. If the + /// authority addresses a DNS name, a `Destination::Name` is returned; and, otherwise, + /// it addresses a fixed IP address and a `Destination::Addr` is returned. The port is + /// inferred if not specified in the authority. + /// + /// If no authority is available, the `SO_ORIGINAL_DST` socket option is checked. If + /// it's available, it is used to return a `Destination::Addr`. This socket option is + /// typically set by `iptables(8)` in containerized environments like Kubernetes (as + /// configured by the `proxy-init` program). + /// + /// If none of this information is available, no `Destination` is returned. + fn destination(req: &http::Request) -> Option { + match Self::host_port(req) { + Some(HostAndPort { host: Host::DnsName(host), port }) => { + let dst = DnsNameAndPort { host, port }; + Some(Destination::Name(dst)) + } + + Some(HostAndPort { host: Host::Ip(ip), port }) => { + let dst = SocketAddr::from((ip, port)); + Some(Destination::Addr(dst)) + } + + None => { + req.extensions() + .get::>() + .and_then(|ctx| ctx.orig_dst_if_not_local()) + .map(Destination::Addr) + } + } + } } impl Clone for Outbound @@ -88,45 +151,11 @@ where choose::PowerOfTwoChoices, >>>>; + // Route the request by its destination AND PROTOCOL. This prevents HTTP/1 + // requests from being routed to HTTP/2 servers, and vice versa. fn recognize(&self, req: &Self::Request) -> Option { + let dest = Self::destination(req)?; let proto = bind::Protocol::detect(req); - - // 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 authority = 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)); - - // TODO: Return error when `HostAndPort::normalize()` fails. - let mut dest = match authority.as_ref() - .and_then(|auth| HostAndPort::normalize(auth, Some(80)).ok()) { - Some(HostAndPort { host: Host::DnsName(dns_name), port }) => - Some(Destination::Hostname(DnsNameAndPort { host: dns_name, port })), - Some(HostAndPort { host: Host::Ip(_), .. }) | - None => None, - }; - - if dest.is_none() { - dest = req.extensions() - .get::>() - .and_then(|ctx| { - ctx.orig_dst_if_not_local() - }) - .map(Destination::ImplicitOriginalDst) - }; - - // If there is no authority in the request URI or in the Host header, - // and there is no original dst, then we have nothing! In that case, - // return `None`, which results an "unrecognized" error. In practice, - // this shouldn't ever happen, since we expect the proxy to be run on - // Linux servers, with iptables setup, so there should always be an - // original destination. - let dest = dest?; - Some((dest, proto)) } @@ -141,16 +170,12 @@ where let &(ref dest, ref protocol) = key; debug!("building outbound {:?} client to {:?}", protocol, dest); - let resolve = match *dest { - Destination::Hostname(ref authority) => { - Discovery::NamedSvc(self.discovery.resolve( - authority, - self.bind.clone().with_protocol(protocol.clone()), - )) - }, - Destination::ImplicitOriginalDst(addr) => { - Discovery::ImplicitOriginalDst(Some((addr, self.bind.clone() - .with_protocol(protocol.clone())))) + let resolve = { + let proto = self.bind.clone().with_protocol(protocol.clone()); + match *dest { + Destination::Name(ref authority) => + Discovery::Name(self.discovery.resolve(authority, proto)), + Destination::Addr(addr) => Discovery::Addr(Some((addr, proto))), } }; @@ -172,8 +197,8 @@ where } pub enum Discovery { - NamedSvc(Resolution>), - ImplicitOriginalDst(Option<(SocketAddr, BindProtocol)>), + Name(Resolution>), + Addr(Option<(SocketAddr, BindProtocol)>), } impl Discover for Discovery @@ -190,9 +215,9 @@ where fn poll(&mut self) -> Poll, Self::DiscoverError> { match *self { - Discovery::NamedSvc(ref mut w) => w.poll() + Discovery::Name(ref mut w) => w.poll() .map_err(|_| BindError::Internal), - Discovery::ImplicitOriginalDst(ref mut opt) => { + Discovery::Addr(ref mut opt) => { // This "discovers" a single address for an external service // that never has another change. This can mean it floats // in the Balancer forever. However, when we finally add @@ -209,6 +234,7 @@ where } } } + #[derive(Copy, Clone, Debug)] pub enum BindError { External { addr: SocketAddr }, @@ -243,10 +269,10 @@ struct Dst(Destination); impl fmt::Display for Dst { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self.0 { - Destination::Hostname(ref name) => { + Destination::Name(ref name) => { write!(f, "{}:{}", name.host, name.port) } - Destination::ImplicitOriginalDst(ref addr) => addr.fmt(f), + Destination::Addr(ref addr) => addr.fmt(f), } } }