inbound, outbound: `Param`-ify `listen::Addrs` by hawkw · Pull Request #955 · linkerd/linkerd2-proxy

hawkw · 2021-03-19T19:06:29Z

This PR changes the Services constructed by the inbound and outbound
stacks from always taking the listen::Addrs type as a target, to being
parameterized over a target type that implements Param for various
address types.

This required changing some middleware layers that were previously
hardcoded to listen::Addrs, and adding Param impls to
listen::Addrs for various address types. I also added a new
TcpAccept::from_local_addr constructor, which sets the target address
based on the server's listen address, rather than from the
SO_ORIGINAL_DST address. This is used by the admin server, which doesn't
configure its listening socket to include original dst addresses.
Previously, the From implementation for TcpAccept just always tried
the orig dst addr and fell back to the listen addr if it was not
present. Now, we have separate behavior for the admin server and the
proxy servers --- admin always uses the local addr, and the proxies only
try SO_ORIGINAL_DST or fail.

There's no functional change here, but this will set up for a future
change to make stacks generic over SO_ORIGINAL_DST detection, allowing
us to remove the currently quite awkward feature flagging.

Signed-off-by: Eliza Weisman <[email protected]>

linkerd/app/inbound/src/direct.rs

Signed-off-by: Eliza Weisman <[email protected]>

linkerd/app/inbound/src/direct.rs

linkerd/proxy/transport/src/addrs.rs

Co-authored-by: Oliver Gould <[email protected]>

Signed-off-by: Eliza Weisman <[email protected]>

olix0r · 2021-03-22T21:05:14Z

linkerd/app/inbound/src/lib.rs

+            .check_new_service::<T, I>()
            .push_switch(
-                PreventLoop::from(server_port),
+                PreventLoop::from(server_port).to_switch(),


Can we consolidate this switch to actually build a target type that holds the origdst? It looks like we have to handle this case with an error in at least 3 places... Not a blocker, though, as I understand followups may ease this...

I also find switches harder to understand with the impulse decoupled from the stack -- it'd probably be easier to follow by embedding the closure here directly...

Can we consolidate this switch to actually build a target type that holds the origdst? It looks like we have to handle this case with an error in at least 3 places... Not a blocker, though, as I understand followups may ease this...

the intent was to follow up with a branch that changes the whole stack to be based on Param<OrigDstAddr> rather than Param<Option<OrigDstAddr>>, so this is kind of an intermediate state; if you'd prefer to avoid that, we could try to land the whole change in one bigger branch when it's ready?

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)

…957) This branch introduces a new `transport::listen::Bind` trait that abstracts over binding and listening on a socket. Now, rather than owning an instance of the `BindTcp` type, `ServerConfig`s are passed as a parameter to a type implementing the `Bind` trait. This will eventually allow end-to-end testing without creating actual TCP sockets, as `Bind` produces a `Stream` of an IO type and an `Addrs` type. PR #955 changed the proxy stacks to be a target type which implements the `Param` trait for various types of address, rather than a fixed type. This PR also changes the inbound and outbound proxies to take targets which implement `Param<OrigDstAddr>`, rather than requiring `Param<Option<OrigDstAddr>>` and punting fallibility to every point where original dst addresses are used. The `BindTcp` type is now generic over a more general `GetAddrs` trait, rather than a `SO_ORIGINAL_DST`-specific `OrigDstAddr` trait. Rather than always producing a `listen::Addrs` with an `Option<OrigDstAddr>`, we now produce a variable addresses type, which may or may not incldue the original destination address (the admin and tap servers don't require SO_ORIGINAL_DST). This allows whether or not original destination addresses are included to be determined at compile time rather than at runtime. Finally, the incorrect feature flagging of mock original destination addresses has been fixed, The "mock-orig-dst" feature has been removed, and the integration tests now simply construct their own mock `GetAddrs` function that's used to configure the test proxy. In the future, we should be able to refactor the tests to avoid this by simply passing in their own mock `Bind` types which produce streams of in-memory mock connections, rather than actual TCP connections. Signed-off-by: Eliza Weisman <[email protected]> Co-authored-by: Oliver Gould <[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)

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)

hawkw added 3 commits March 19, 2021 11:33

transport: add Param impls for listen::Addrs

b3877fb

Signed-off-by: Eliza Weisman <[email protected]>

inbound: Param-ify listen::Addrs

ff0784c

Signed-off-by: Eliza Weisman <[email protected]>

outbound: Paramify listen::Addrs

8ff39e4

Signed-off-by: Eliza Weisman <[email protected]>

hawkw requested review from a team and olix0r March 19, 2021 19:06

olix0r reviewed Mar 19, 2021

View reviewed changes

linkerd/app/inbound/src/direct.rs Outdated Show resolved Hide resolved

s/A/T

9f6ccc5

Signed-off-by: Eliza Weisman <[email protected]>

hawkw requested a review from olix0r March 19, 2021 20:38

olix0r requested changes Mar 19, 2021

View reviewed changes

linkerd/app/inbound/src/direct.rs Outdated Show resolved Hide resolved

linkerd/app/inbound/src/direct.rs Outdated Show resolved Hide resolved

linkerd/app/inbound/src/direct.rs Outdated Show resolved Hide resolved

linkerd/proxy/transport/src/addrs.rs Outdated Show resolved Hide resolved

hawkw and others added 6 commits March 19, 2021 15:22

fix find-and-replace mistakes

6abfe0a

Co-authored-by: Oliver Gould <[email protected]>

get rid of TargetAddr

db0e631

Signed-off-by: Eliza Weisman <[email protected]>

additional find and replace mistake

958c744

Signed-off-by: Eliza Weisman <[email protected]>

whoops make admin compile

8a1b55d

Signed-off-by: Eliza Weisman <[email protected]>

[WIP] remove TargetAddr and just use orig dst

d7b21dc

fix admin failing with missing orig dst addrs

63d2631

Signed-off-by: Eliza Weisman <[email protected]>

hawkw requested a review from olix0r March 22, 2021 18:18

olix0r approved these changes Mar 22, 2021

View reviewed changes

hawkw merged commit fce8c4a into main Mar 23, 2021

hawkw deleted the eliza/orig-dst-param branch March 23, 2021 21:16

hawkw mentioned this pull request Mar 26, 2021

transport: introduce Bind trait and move orig dst to the type level #957

Merged

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

inbound, outbound: `Param`-ify `listen::Addrs`#955

inbound, outbound: `Param`-ify `listen::Addrs`#955
hawkw merged 10 commits intomainfrom
eliza/orig-dst-param

hawkw commented Mar 19, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olix0r Mar 22, 2021

Uh oh!

hawkw Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

hawkw commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olix0r Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hawkw commented Mar 19, 2021 •

edited

Loading