Update profile response to include a logical address by olix0r · Pull Request #954 · linkerd/linkerd2-proxy

olix0r · 2021-03-18T21:12:56Z

Profile responses optionally include a service name, but we only ever
use it in the context of an address (i.e. with a port).

This change introduces a profiles::LookupAddr param type that is used
as input into the profile client -- the lookup address may hold either a
named or numbered address. Service profile responses now include a
LogicalAddr that must be a named address.

This sets up for requiring that concrete addresses are named.

ebf67d6 moved endpoint construction logic out of the logical stacks. In doing so, the gateway was incorrectly configured to mark endpoints with disabled identity, preventing mTLS between the gateway and destination service. This change moves endpoint construction into each logical stack so that it need not be configured on each instantiation.

In order to make the server and logical stack more flexible, we need to make stacks generic over their input target type. This change modifies the innermost `tcp::connect` stack to not depend on a concrete target type, instead relying on `Param` constraints. A new `tcp::Connect` target type is introduced to be constructed by the TCP connect stack.

THe HTTP endpoint stack is coupled to the Endpoint target type. In order to make the outbound stack more flexible so that the endpoint stack can be used without the logical stack, this change updates the outbound HTTP endpoint stack to be generic over its target type.

The HTTP server stack is unnecessarily bound to a target type. In order to support more flexible stack composition, this change makes the outbound HTTP server stack generic over its target type.

Profile responses optionally include a service name, but we only ever use it in the context of an address (i.e. with a port). This change introduces a `profiles::LookupAddr` param type that is used as input into the profile client -- the lookup address may hold either a named or numbered address. Service profile responses now include a `LogicalAddr` that must be a named address. This sets up for requiring that concrete addresses are named.

hawkw

overall, this looks good to me! the change is actually much more straightforward than it seems from the changed lines count; it just touches a lot of files.

i did wonder if it makes sense to get rid of the type parameter on GetProfile, but that could also happen separately.

hawkw · 2021-03-18T22:41:02Z

linkerd/app/gateway/src/lib.rs

        io::AsyncRead + io::AsyncWrite + tls::HasNegotiatedProtocol + Send + Unpin + 'static,
    O::Future: Send + Unpin + 'static,
-    P: profiles::GetProfile<profiles::LogicalAddr> + Clone + Send + Sync + Unpin + 'static,
+    P: profiles::GetProfile<profiles::LookupAddr> + Clone + Send + Sync + Unpin + 'static,


AFAICT, there are now no impls of GetProfile for anything other than a LookupAddr:

:# eliza on butterfly in linkerd2-proxy on  ver/profile-addr [$?] via ⚙️ v1.49.0 :; rg "GetProfile<" linkerd/service-profiles/src/default.rs 25: S: GetProfile<T>, linkerd/service-profiles/src/discover.rs 14: G: GetProfile<F::Request> + Clone, 34: G: GetProfile<T>, linkerd/service-profiles/src/lib.rs 54:pub trait GetProfile<T> { 68:impl<T, S> GetProfile<T> for S 84: P: GetProfile<T>, linkerd/app/outbound/src/discover.rs 28: P: profiles::GetProfile<profiles::LookupAddr> + Clone + Send + 'static, linkerd/app/outbound/src/lib.rs 113: P: profiles::GetProfile<profiles::LookupAddr> + Clone + Send + 'static, 141: P: profiles::GetProfile<profiles::LookupAddr> + Clone + Send + Sync + Unpin + 'static, linkerd/app/outbound/src/ingress.rs 44: P: profiles::GetProfile<profiles::LookupAddr> + Clone + Send + Sync + Unpin + 'static, linkerd/app/gateway/src/lib.rs 87: P: profiles::GetProfile<profiles::LookupAddr> + Clone + Send + Sync + Unpin + 'static, linkerd/app/inbound/src/lib.rs 138: P: profiles::GetProfile<profiles::LookupAddr> + Clone + Send + Sync + 'static, 228: P: profiles::GetProfile<profiles::LookupAddr> + Clone + Send + Sync + 'static, linkerd/app/inbound/src/http/mod.rs 118: P: profiles::GetProfile<profiles::LookupAddr> + Clone + Send + Sync + 'static,

Should we maybe just remove the type parameter and change GetProfile to

pub trait GetProfile { type Error: Into<Error>; type Future: Future<Output = Result<Option<Receiver>, Self::Error>>; fn get_profile(&mut self, target: LookupAddr) -> Self::Future; }

or am I missing something?

seems like a fine change but it will touch more than this pr needs to.

👍 i can throw together a PR after this lands

hawkw · 2021-03-18T22:51:00Z

linkerd/service-profiles/src/lib.rs

+impl fmt::Display for LookupAddr {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
+impl FromStr for LookupAddr {
+    type Err = <Addr as FromStr>::Err;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        Addr::from_str(s).map(LookupAddr)
+    }
+}
+
+impl From<Addr> for LookupAddr {
+    fn from(a: Addr) -> Self {
+        Self(a)
+    }
+}
+
+impl Into<Addr> for LookupAddr {
+    fn into(self) -> Addr {
+        self.0
+    }
+}
+
+// === impl LogicalAddr ===
+
+impl fmt::Display for LogicalAddr {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
+impl FromStr for LogicalAddr {
+    type Err = <NameAddr as FromStr>::Err;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        NameAddr::from_str(s).map(LogicalAddr)
+    }
+}
+
+impl From<NameAddr> for LogicalAddr {
+    fn from(na: NameAddr) -> Self {
+        Self(na)
+    }
+}
+
+impl Into<NameAddr> for LogicalAddr {
+    fn into(self) -> NameAddr {
+        self.0
+    }
+}


some of these trivial impls can probably be #[inline], though I'm not sure if it it actually matters; i just noticed we added the attribute on other code in this PR...

hawkw · 2021-03-18T22:53:50Z

linkerd/service-profiles/src/lib.rs

+#[derive(Clone, Debug, Hash, Eq, PartialEq)]
+pub struct LookupAddr(pub Addr);
+
+/// A bound logical service address


nit: should the comment maybe make it clearer that this is part of the profile returned by a lookup, in contrast to LookupAddr, which is the input?

hawkw · 2021-03-18T22:55:35Z

linkerd/service-profiles/src/lib.rs

+impl fmt::Display for LookupAddr {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
+impl FromStr for LookupAddr {
+    type Err = <Addr as FromStr>::Err;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        Addr::from_str(s).map(LookupAddr)
+    }
+}
+
+impl From<Addr> for LookupAddr {
+    fn from(a: Addr) -> Self {
+        Self(a)
+    }
+}
+
+impl Into<Addr> for LookupAddr {
+    fn into(self) -> Addr {
+        self.0
+    }
+}
+
+// === impl LogicalAddr ===
+
+impl fmt::Display for LogicalAddr {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
+impl FromStr for LogicalAddr {
+    type Err = <NameAddr as FromStr>::Err;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        NameAddr::from_str(s).map(LogicalAddr)
+    }
+}
+
+impl From<NameAddr> for LogicalAddr {
+    fn from(na: NameAddr) -> Self {
+        Self(na)
+    }
+}
+
+impl Into<NameAddr> for LogicalAddr {
+    fn into(self) -> NameAddr {
+        self.0
+    }
+}


a lot of these are pretty trivial, maybe they should be marked as #[inline]able? not sure if this matters at all, just noticed that we added the attribute elsewhere in this branch...

This release fixes two issues: 1. The inbound proxy could break non-meshed TLS connections when the initial ClientHello message was larger than 512 bytes or when the entire message was not received in the first data packet of the connection. TLS detection has been fixed to ensure that the entire message is preserved in these cases. 2. The admin server could emit warnings about HTTP detection failing in some innocuous situations, such as when the socket closes before a request is sent. These situations are now handled gracefully without logging warnings. --- * Update MAINTAINERS to point at the main repo (linkerd/linkerd2-proxy#950) * outbound: Configure endpoint construction in logical stack (linkerd/linkerd2-proxy#949) * outbound: Decouple the TCP connect stack from the target type (linkerd/linkerd2-proxy#951) * outbound: Make HTTP endpoint stack generic on its target (linkerd/linkerd2-proxy#952) * outbound: Make the HTTP server stack generic (linkerd/linkerd2-proxy#953) * Update profile response to include a logical address (linkerd/linkerd2-proxy#954) * inbound, outbound: `Param`-ify `listen::Addrs` (linkerd/linkerd2-proxy#955) * tls: Fix inbound I/O when TLS detection fails (linkerd/linkerd2-proxy#958) * tls: Test SNI detection (linkerd/linkerd2-proxy#959) * admin: Handle connections that fail protocol detection (linkerd/linkerd2-proxy#960)

This release fixes two issues: 1. The inbound proxy could break non-meshed TLS connections when the initial ClientHello message was larger than 512 bytes or when the entire message was not received in the first data packet of the connection. TLS detection has been fixed to ensure that the entire message is preserved in these cases. 2. The admin server could emit warnings about HTTP detection failing in some innocuous situations, such as when the socket closes before a request is sent. These situations are now handled gracefully without logging warnings. --- * Update MAINTAINERS to point at the main repo (linkerd/linkerd2-proxy#950) * outbound: Configure endpoint construction in logical stack (linkerd/linkerd2-proxy#949) * outbound: Decouple the TCP connect stack from the target type (linkerd/linkerd2-proxy#951) * outbound: Make HTTP endpoint stack generic on its target (linkerd/linkerd2-proxy#952) * outbound: Make the HTTP server stack generic (linkerd/linkerd2-proxy#953) * Update profile response to include a logical address (linkerd/linkerd2-proxy#954) * inbound, outbound: `Param`-ify `listen::Addrs` (linkerd/linkerd2-proxy#955) * tls: Fix inbound I/O when TLS detection fails (linkerd/linkerd2-proxy#958) * tls: Test SNI detection (linkerd/linkerd2-proxy#959) * admin: Handle connections that fail protocol detection (linkerd/linkerd2-proxy#960) Signed-off-by: Jijeesh <[email protected]>

This release fixes two issues: 1. The inbound proxy could break non-meshed TLS connections when the initial ClientHello message was larger than 512 bytes or when the entire message was not received in the first data packet of the connection. TLS detection has been fixed to ensure that the entire message is preserved in these cases. 2. The admin server could emit warnings about HTTP detection failing in some innocuous situations, such as when the socket closes before a request is sent. These situations are now handled gracefully without logging warnings. --- * Update MAINTAINERS to point at the main repo (linkerd/linkerd2-proxy#950) * outbound: Configure endpoint construction in logical stack (linkerd/linkerd2-proxy#949) * outbound: Decouple the TCP connect stack from the target type (linkerd/linkerd2-proxy#951) * outbound: Make HTTP endpoint stack generic on its target (linkerd/linkerd2-proxy#952) * outbound: Make the HTTP server stack generic (linkerd/linkerd2-proxy#953) * Update profile response to include a logical address (linkerd/linkerd2-proxy#954) * inbound, outbound: `Param`-ify `listen::Addrs` (linkerd/linkerd2-proxy#955) * tls: Fix inbound I/O when TLS detection fails (linkerd/linkerd2-proxy#958) * tls: Test SNI detection (linkerd/linkerd2-proxy#959) * admin: Handle connections that fail protocol detection (linkerd/linkerd2-proxy#960)

olix0r added 7 commits March 17, 2021 01:51

Handle disabled identity, especially in tests

fa0c93e

Merge branch 'ver/resolve-endpoints' into ver/http-generic

7e02b4e

outbound: Make the HTTP server stack generic

1a851e8

The HTTP server stack is unnecessarily bound to a target type. In order to support more flexible stack composition, this change makes the outbound HTTP server stack generic over its target type.

olix0r requested a review from a team March 18, 2021 21:12

olix0r marked this pull request as draft March 18, 2021 21:13

hawkw approved these changes Mar 18, 2021

View reviewed changes

Base automatically changed from ver/http-server to main March 18, 2021 23:28

Merge branch 'main' into ver/profile-addr

4bc765d

olix0r marked this pull request as ready for review March 19, 2021 00:02

olix0r merged commit 32e459b into main Mar 19, 2021

olix0r deleted the ver/profile-addr branch March 19, 2021 00:04

olix0r mentioned this pull request Mar 30, 2021

proxy: v2.140.0 linkerd/linkerd2#5974

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update profile response to include a logical address#954

Update profile response to include a logical address#954
olix0r merged 8 commits intomainfrom
ver/profile-addr

olix0r commented Mar 18, 2021

Uh oh!

hawkw left a comment

Uh oh!

hawkw Mar 18, 2021

Uh oh!

olix0r Mar 18, 2021

Uh oh!

hawkw Mar 18, 2021

Uh oh!

hawkw Mar 18, 2021

Uh oh!

hawkw Mar 18, 2021

Uh oh!

hawkw Mar 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

olix0r commented Mar 18, 2021

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

olix0r Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants