From b09e0471b4a2bd37b8e20682c6ffe7b611b32f6d Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Wed, 28 Apr 2021 01:13:38 +0000 Subject: [PATCH 1/3] outbound: Make the Endpoint::logical_addr field optional The `outbound::Endpoint::logical_addr` field must currently _always_ be set, even when it just duplicates the endpoint's target address and there is no "logical" address. This change makes this field optional and changes its type to require a `LogicalAddr` (as returned by a profile lookup). As a result of this, the `authority` endpoint metrics label is now only set when a logical address is present. --- linkerd/app/outbound/src/endpoint.rs | 22 ++++++++++++------- linkerd/app/outbound/src/ingress.rs | 2 +- .../app/outbound/src/tcp/opaque_transport.rs | 4 ++-- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/linkerd/app/outbound/src/endpoint.rs b/linkerd/app/outbound/src/endpoint.rs index 51a2fc53e3..7e8a6e603a 100644 --- a/linkerd/app/outbound/src/endpoint.rs +++ b/linkerd/app/outbound/src/endpoint.rs @@ -5,11 +5,12 @@ use crate::{ }; use linkerd_app_core::{ metrics, + profiles::LogicalAddr, proxy::{api_resolve::Metadata, http, resolve::map_endpoint::MapEndpoint}, svc::{self, Param}, tls, transport::{self, Remote, ServerAddr}, - transport_header, Addr, Conditional, + transport_header, Conditional, }; use std::net::SocketAddr; @@ -18,7 +19,7 @@ pub struct Endpoint

{ pub addr: Remote, pub tls: tls::ConditionalClientTls, pub metadata: Metadata, - pub logical_addr: Addr, + pub logical_addr: Option, pub protocol: P, } @@ -71,14 +72,14 @@ impl

From<(tls::NoClientTls, Logical

)> for Endpoint

{ addr: Remote(ServerAddr(logical.orig_dst.into())), metadata: Metadata::default(), tls: Conditional::None(reason), - logical_addr: logical.addr(), + logical_addr: Some(logical.logical_addr), protocol: logical.protocol, }, Some((addr, metadata)) => Self { addr: Remote(ServerAddr(addr)), tls: FromMetadata::client_tls(&metadata, reason), metadata, - logical_addr: logical.addr(), + logical_addr: Some(logical.logical_addr), protocol: logical.protocol, }, } @@ -91,7 +92,7 @@ impl

From<(tls::NoClientTls, Accept

)> for Endpoint

{ addr: Remote(ServerAddr(accept.orig_dst.into())), metadata: Metadata::default(), tls: Conditional::None(reason), - logical_addr: accept.orig_dst.0.into(), + logical_addr: None, protocol: accept.protocol, } } @@ -134,11 +135,16 @@ impl

Param for Endpoint

{ impl

Param for Endpoint

{ fn param(&self) -> metrics::OutboundEndpointLabels { + let target_addr = self.addr.into(); + let authority = self + .logical_addr + .as_ref() + .map(|LogicalAddr(a)| a.as_http_authority()); metrics::OutboundEndpointLabels { - authority: Some(self.logical_addr.to_http_authority()), + authority, labels: metrics::prefix_labels("dst", self.metadata.labels().iter()), server_id: self.tls.clone(), - target_addr: self.addr.into(), + target_addr, } } } @@ -206,7 +212,7 @@ impl MapEndpoint, Metadata> for FromMetad addr: Remote(ServerAddr(addr)), tls, metadata, - logical_addr: concrete.logical.addr(), + logical_addr: Some(concrete.logical.logical_addr.clone()), protocol: concrete.logical.protocol, } } diff --git a/linkerd/app/outbound/src/ingress.rs b/linkerd/app/outbound/src/ingress.rs index 2a8ba93ba1..710fcef36a 100644 --- a/linkerd/app/outbound/src/ingress.rs +++ b/linkerd/app/outbound/src/ingress.rs @@ -216,7 +216,7 @@ impl From<(tls::NoClientTls, Target)> for http::Endpoint { addr, metadata: Metadata::default(), tls: Conditional::None(reason), - logical_addr: dst, + logical_addr: None, protocol: version, } } diff --git a/linkerd/app/outbound/src/tcp/opaque_transport.rs b/linkerd/app/outbound/src/tcp/opaque_transport.rs index 408604dc87..a018acceb7 100644 --- a/linkerd/app/outbound/src/tcp/opaque_transport.rs +++ b/linkerd/app/outbound/src/tcp/opaque_transport.rs @@ -149,7 +149,7 @@ mod test { tls, transport::{Remote, ServerAddr}, transport_header::TransportHeader, - Addr, Conditional, + Conditional, }; use pin_project::pin_project; use std::task::Context; @@ -170,7 +170,7 @@ mod test { tls::NoClientTls::NotProvidedByServiceDiscovery, )), metadata, - logical_addr: Addr::Socket(([127, 0, 0, 2], 4321).into()), + logical_addr: None, protocol: (), } } From 19eec6bdb97409dc57684d81dbe2cee46e8e2142 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Wed, 28 Apr 2021 01:13:38 +0000 Subject: [PATCH 2/3] ingress: Require the l5d-dst-override header The outbound ingress-mode proxy generally relies on the `l5d-dst-override` header to be set, but it still tries to handle other cases (mostly to satisfy type constraints we needed previously). This is unnecessary. This change modifies the ingress-mode outbound proxy to require that the `l5d-dst-override` header be set to a named address (i.e., not an IP address). When this header isn't set, requests are failed a descriptive error is emitted. Furthermore, the inbound proxy no longer honors the `l5d-dst-override` header. It never should have supported this header, but this was likely copied from the outbound router. The header is only intended for ingresses. --- linkerd/app/core/src/lib.rs | 6 ++++-- linkerd/app/inbound/src/target.rs | 13 ++---------- linkerd/app/outbound/src/ingress.rs | 33 ++++++++++++----------------- 3 files changed, 20 insertions(+), 32 deletions(-) diff --git a/linkerd/app/core/src/lib.rs b/linkerd/app/core/src/lib.rs index 81e7d6fa4a..541b128807 100644 --- a/linkerd/app/core/src/lib.rs +++ b/linkerd/app/core/src/lib.rs @@ -80,13 +80,15 @@ pub fn is_discovery_rejected(err: &(dyn std::error::Error + 'static)) -> bool { } } -pub fn http_request_l5d_override_dst_addr(req: &http::Request) -> Result { +pub fn http_request_l5d_override_dst_name_addr( + req: &http::Request, +) -> Result { proxy::http::authority_from_header(req, DST_OVERRIDE_HEADER) .ok_or_else(|| { tracing::trace!("{} not in request headers", DST_OVERRIDE_HEADER); addr::Error::InvalidHost }) - .and_then(|a| Addr::from_authority_and_default_port(&a, DEFAULT_PORT)) + .and_then(|a| NameAddr::from_authority_with_default_port(&a, DEFAULT_PORT)) } pub fn http_request_authority_addr(req: &http::Request) -> Result { diff --git a/linkerd/app/inbound/src/target.rs b/linkerd/app/inbound/src/target.rs index 9c2c838034..b965a79434 100644 --- a/linkerd/app/inbound/src/target.rs +++ b/linkerd/app/inbound/src/target.rs @@ -1,14 +1,13 @@ use indexmap::IndexMap; use linkerd_app_core::{ - classify, dst, http_request_authority_addr, http_request_host_addr, - http_request_l5d_override_dst_addr, metrics, profiles, + classify, dst, http_request_authority_addr, http_request_host_addr, metrics, profiles, proxy::{http, tap}, stack_tracing, svc::{self, Param}, tls, transport::{self, addrs::*}, transport_header::TransportHeader, - Addr, Conditional, Error, CANONICAL_DST_HEADER, DST_OVERRIDE_HEADER, + Addr, Conditional, Error, CANONICAL_DST_HEADER, }; use std::{convert::TryInto, net::SocketAddr, str::FromStr, sync::Arc}; use tracing::debug; @@ -323,14 +322,6 @@ impl svc::stack::RecognizeRoute> for RequestTarget { }) }) }) - .or_else(|| { - http_request_l5d_override_dst_addr(req) - .ok() - .map(|override_addr| { - debug!("using {}", DST_OVERRIDE_HEADER); - override_addr - }) - }) .or_else(|| http_request_authority_addr(req).ok()) .or_else(|| http_request_host_addr(req).ok()) .unwrap_or_else(|| self.accept.tcp.target_addr.into()); diff --git a/linkerd/app/outbound/src/ingress.rs b/linkerd/app/outbound/src/ingress.rs index 710fcef36a..607945cd90 100644 --- a/linkerd/app/outbound/src/ingress.rs +++ b/linkerd/app/outbound/src/ingress.rs @@ -1,13 +1,13 @@ use crate::{http, stack_labels, tcp, trace_labels, Config, Outbound}; use linkerd_app_core::{ config::{ProxyConfig, ServerConfig}, - detect, discovery_rejected, drain, errors, http_request_l5d_override_dst_addr, http_tracing, - io, profiles, + detect, discovery_rejected, drain, errors, http_request_l5d_override_dst_name_addr, + http_tracing, io, profiles, proxy::api_resolve::Metadata, svc::{self, stack::Param}, tls, transport::{self, ClientAddr, OrigDstAddr, Remote, ServerAddr}, - Addr, AddrMatch, Conditional, Error, + AddrMatch, Conditional, Error, NameAddr, }; use std::convert::TryFrom; use thiserror::Error; @@ -156,7 +156,7 @@ struct AllowHttpProfile(AddrMatch); #[derive(Clone, Debug, PartialEq, Eq, Hash)] struct Target { - dst: Addr, + dst: NameAddr, version: http::Version, } @@ -169,8 +169,8 @@ impl svc::stack::Predicate for AllowHttpProfile { type Request = profiles::LookupAddr; fn check(&mut self, Target { dst, .. }: Target) -> Result { - if self.0.matches(&dst) { - Ok(profiles::LookupAddr(dst)) + if self.0.names().matches(dst.name()) { + Ok(profiles::LookupAddr(dst.into())) } else { Err(discovery_rejected().into()) } @@ -185,14 +185,10 @@ impl TryFrom<(Option, Target)> for http::Logical { (profile, Target { dst, version }): (Option, Target), ) -> Result { let profile = profile.ok_or(ProfileRequired)?; + let logical_addr = profile.borrow().addr.clone().ok_or(ProfileRequired)?; // XXX This is a hack to fix caching when an dst-override is set. - let orig_dst = if let Some(a) = dst.socket_addr() { - OrigDstAddr(a) - } else { - OrigDstAddr(([0, 0, 0, 0], dst.port()).into()) - }; - let logical_addr = profile.borrow().addr.clone().ok_or(ProfileRequired)?; + let orig_dst = OrigDstAddr(([0, 0, 0, 0], dst.port()).into()); Ok(Self { orig_dst, @@ -206,11 +202,7 @@ impl TryFrom<(Option, Target)> for http::Logical { impl From<(tls::NoClientTls, Target)> for http::Endpoint { fn from((reason, Target { dst, version }): (tls::NoClientTls, Target)) -> Self { // XXX This is a hack to fix caching when an dst-override is set. - let addr = if let Some(a) = dst.socket_addr() { - Remote(ServerAddr(a)) - } else { - Remote(ServerAddr(([0, 0, 0, 0], dst.port()).into())) - }; + let addr = Remote(ServerAddr(([0, 0, 0, 0], dst.port()).into())); Self { addr, @@ -234,8 +226,7 @@ impl svc::stack::RecognizeRoute> for TargetPerRequest { type Key = Target; fn recognize(&self, req: &http::Request) -> Result { - let dst = - http_request_l5d_override_dst_addr(req).unwrap_or_else(|_| self.0.orig_dst.0.into()); + let dst = http_request_l5d_override_dst_name_addr(req).map_err(|_| DstOverrideRequired)?; Ok(Target { dst, version: self.0.protocol, @@ -248,3 +239,7 @@ impl svc::stack::RecognizeRoute> for TargetPerRequest { #[derive(Debug, Error)] #[error("ingress routing requires a service profile")] struct ProfileRequired; + +#[derive(Debug, Error)] +#[error("ingress routing requires the l5d-dst-override header")] +struct DstOverrideRequired; From 7bf62a1c0abe0277af5fa60c0c19568ab68ca913 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Thu, 29 Apr 2021 00:56:23 +0000 Subject: [PATCH 3/3] Drop unused ClientAddr param from ingress --- linkerd/app/outbound/src/ingress.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/linkerd/app/outbound/src/ingress.rs b/linkerd/app/outbound/src/ingress.rs index 607945cd90..6764ff0ab8 100644 --- a/linkerd/app/outbound/src/ingress.rs +++ b/linkerd/app/outbound/src/ingress.rs @@ -6,7 +6,7 @@ use linkerd_app_core::{ proxy::api_resolve::Metadata, svc::{self, stack::Param}, tls, - transport::{self, ClientAddr, OrigDstAddr, Remote, ServerAddr}, + transport::{self, OrigDstAddr, Remote, ServerAddr}, AddrMatch, Conditional, Error, NameAddr, }; use std::convert::TryFrom; @@ -30,7 +30,7 @@ impl Outbound<()> { Service = impl svc::Service, > where - T: Param + Param> + Clone + Send + Sync + 'static, + T: Param + Clone + Send + Sync + 'static, I: io::AsyncRead + io::AsyncWrite + io::PeerAddr + std::fmt::Debug + Send + Unpin + 'static, N: svc::NewService + Clone + Send + Sync + 'static, NSvc: svc::Service>, Response = ()>