transport: introduce Bind trait and move orig dst to the type level by hawkw · Pull Request #957 · linkerd/linkerd2-proxy

hawkw · 2021-03-25T21:57:09Z

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, ServerConfigs 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]>

This fixes telemetry tests expecting the address to be 127.0.0.1:0 (which worked on my machine, for whatever reason?). Signed-off-by: Eliza Weisman <[email protected]>

olix0r

I need to give this a deeper read through, but my first impression is that we really should not need a GetAddrs trait to be exposed to all of the stacks... I.e., we should keep the same general signature for the stacks that they are constructed with a parameterized target and we shouldn't have to have all of the address filtering business in every stack.

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

how the HECK did i manage to break that, wtf?? Signed-off-by: Eliza Weisman <[email protected]>

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

hawkw · 2021-03-29T21:33:24Z

linkerd/app/inbound/src/lib.rs

+            + Param<OrigDstAddr>
+            + Clone
+            + Send,
+        B::Io: io::Peek + io::PeerAddr + Debug + Unpin,


we could avoid having to write these bounds out in as many places by just making them into bounds on the definition of the Bind trait, since it turns out everything requires these traits as well. I wasn't sure if it was worth making the trait definition more restrictive or not...

also, we could try to figure out a way to get rid of the PeerAddr trait and just get client addresses from the target always. IDK if this matters or is worth doing...

we need peeraddr for now at least, as the http server marks extensions with the client addr, but we can't have the http server created per-client (since we may need to do cached resolutions before this) -- so we'd either need to rework caching or have an Io type that supports extensions...

yeah, i took a look at that on Monday and i think that we're better off keeping it, and if we are going to get rid of it, that's out of scope for this PR.

@olix0r 7b16586 moves all the bounds on the Bind::Io type to the Bind trait so they're not repeated everywhere else (and removes some of the Bind::Addrs bounds that the Bind trait already requires).

We could also put the Param<Local<ServerAddr>> and Param<Remote<ClientAddr>> bounds on Bind::Addrs so they aren't repeated, but it felt a little weird to see it require only Param<OrigDstAddr> at use-sites; i could go either way on this...

olix0r · 2021-03-30T15:59:00Z

What does the mock-orig-dst feature do now? Is it necessary at all?

hawkw · 2021-03-30T16:47:07Z

What does the mock-orig-dst feature do now? Is it necessary at all?

It just controls whether or not the MockOrigDstAddr type exists. Since it's only used in tests, I thought it was better to have that type only be enabled by the test crates when building tests, and not in the app build.

olix0r · 2021-03-30T18:03:24Z

It just controls whether or not the MockOrigDstAddr type exists. Since it's only used in tests, I thought it was better to have that type only be enabled by the test crates when building tests, and not in the app build.

Can the type live in the test crate?

hawkw · 2021-03-30T18:06:05Z

It just controls whether or not the MockOrigDstAddr type exists. Since it's only used in tests, I thought it was better to have that type only be enabled by the test crates when building tests, and not in the app build.

Can the type live in the test crate?

...yes, now i feel a bit silly for not thinking of that!

Making the config types generic over the orig-dst strategy promulgates generics across all of the stacks. But the binding logic is only necessary at the very "top" of the stack where the accept loop is driven. This change eliminates the `GetAddrs` trait and modifies the `Bind` trait to take params that allow the binding implementation to be configured (i.e., from the environment). `OrigDstAddr` binding is provided as a `Bind` implementation that wraps the `BindTcp` strategy. This lets us eliminate the `mock-orig-dst` feature by moving the `MockOrigDst` helpers into the integration test suite.

olix0r · 2021-03-31T04:08:13Z

@hawkw I tried hacking together #962 on top of this to remove the orig-dst constraints from the config types, which hopefully simplifies things a bit.

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

hawkw · 2021-03-31T17:29:54Z

Dockerfile

-# may be set to `mock-orig-dst` for profiling builds, or to `multithreaded` to
-# enable the multithreaded Tokio runtime.


I removed this part of the comment since the mock-orig-dst feature is now removed and the multithreaded feature is enabled by default.

hawkw · 2021-03-31T17:30:10Z

It just controls whether or not the MockOrigDstAddr type exists. Since it's only used in tests, I thought it was better to have that type only be enabled by the test crates when building tests, and not in the app build.

Can the type live in the test crate?

...yes, now i feel a bit silly for not thinking of that!

@olix0r removed the feature flag in 97b9590, good call!

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

Signed-off-by: Eliza Weisman <[email protected]> ; Conflicts: ; linkerd/app/integration/src/proxy.rs ; linkerd/app/src/admin.rs ; linkerd/app/src/env.rs ; linkerd/app/src/lib.rs ; linkerd/proxy/transport/Cargo.toml ; linkerd/proxy/transport/src/listen.rs

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

olix0r · 2021-03-31T21:03:32Z

linkerd/proxy/transport/src/orig_dst.rs

+        let incoming = incoming.map(|res| {
+            let (inner, tcp) = res?;
+            let orig_dst = orig_dst_addr(&tcp)?;
+            let addrs = Addrs { inner, orig_dst };
+            Ok((addrs, tcp))
+        });
+
+        Ok((addr, Box::pin(incoming)))


btw, I tried to get rid of the box, but this closure captures B (types -- not values) so we get a expected fn pointer found closure compiler error. if you know of a way to avoid the box, by all means...

This release fixes a caching issue in the outbound proxy's "ingress mode" that could cause the incorrect client to be used for requests. This caching has been fixed so that clients cannot be incorrectly reused across logical destinations. --- * transport: introduce Bind trait and move orig dst to the type level (linkerd/linkerd2-proxy#957) * Initial fuzzer integration (linkerd/linkerd2-proxy#961) * Fix caching in ingress mode (linkerd/linkerd2-proxy#965)

This release fixes a caching issue in the outbound proxy's "ingress mode" that could cause the incorrect client to be used for requests. This caching has been fixed so that clients cannot be incorrectly reused across logical destinations. --- * transport: introduce Bind trait and move orig dst to the type level (linkerd/linkerd2-proxy#957) * Initial fuzzer integration (linkerd/linkerd2-proxy#961) * Fix caching in ingress mode (linkerd/linkerd2-proxy#965) Signed-off-by: Jijeesh <[email protected]>

This release fixes a caching issue in the outbound proxy's "ingress mode" that could cause the incorrect client to be used for requests. This caching has been fixed so that clients cannot be incorrectly reused across logical destinations. --- * transport: introduce Bind trait and move orig dst to the type level (linkerd/linkerd2-proxy#957) * Initial fuzzer integration (linkerd/linkerd2-proxy#961) * Fix caching in ingress mode (linkerd/linkerd2-proxy#965)

hawkw added 13 commits March 23, 2021 14:37

wip

20ec14d

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

wip 2

0219b6f

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

getting more and more worky...

8e1cdc5

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

okay make ingress work

7569dfe

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

update admin

0b9cc1f

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

update and stackify tap

71abe61

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

it almost works!!

31d249e

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

ALMOST works

137ba13

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

make it compiley

0ef3bd2

fix integration tests!

db04dae

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

put back accept spans

319ee12

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

pass GetAddrs into admin servers as well

f0b07c7

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

remove dead code

5ec3aa0

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

hawkw changed the title ~~[WIP] factor out SO_ORIGINAL_DST detection into a stack layer~~ factor out SO_ORIGINAL_DST detection into a stack layer Mar 26, 2021

hawkw marked this pull request as ready for review March 26, 2021 18:46

hawkw requested a review from a team March 26, 2021 18:46

use bound admin addr rather than configured addr

08dc9bd

This fixes telemetry tests expecting the address to be 127.0.0.1:0 (which worked on my machine, for whatever reason?). Signed-off-by: Eliza Weisman <[email protected]>

olix0r reviewed Mar 29, 2021

View reviewed changes

hawkw added 8 commits March 29, 2021 11:27

move GetAddrs to BindTcp, make Bind a trait

388db61

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

inbound: update to use Bind trait

7312c82

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

finish wiring up new bind

3c3d30c

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

remove some dead code

1dd53b8

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

Merge branch 'main' into eliza/orig-dst-layer

c56145d

smallerate diff

46642ce

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

fix PreventLoop *always* returning B (loop)

c73ab69

how the HECK did i manage to break that, wtf?? Signed-off-by: Eliza Weisman <[email protected]>

smallerate diff some more

fbe89a8

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

hawkw commented Mar 29, 2021

View reviewed changes

hawkw changed the title ~~factor out SO_ORIGINAL_DST detection into a stack layer~~ transport: introduce Bind trait and move orig dst to the type level Mar 29, 2021

hawkw requested a review from olix0r March 29, 2021 23:07

olix0r added 2 commits March 30, 2021 22:29

Merge branch 'main' into ver/eliza/odl

0203d04

olix0r and others added 2 commits March 31, 2021 15:07

Remove unneeded test helper

0a86b80

remove mock-orig-dst feature

97b9590

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

hawkw commented Mar 31, 2021

View reviewed changes

hawkw and others added 7 commits March 31, 2021 10:31

Merge branch 'main' into eliza/orig-dst-layer

c483cd3

simplify GetAdminAddrs a bit

0e503dc

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

fix inadvertant metrics retain idle env var change

aad758d

small test cleanup

4fb3474

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

put trait bounds on Bind instead of repeating them

7b16586

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

clarity: bind_adm -> bind_admin

421842d

olix0r reviewed Mar 31, 2021

View reviewed changes

Cleanup app building

65c8332

olix0r approved these changes Mar 31, 2021

View reviewed changes

hawkw merged commit 07e9249 into main Mar 31, 2021

hawkw deleted the eliza/orig-dst-layer branch March 31, 2021 21:30

olix0r mentioned this pull request Apr 7, 2021

proxy: v2.141.0 linkerd/linkerd2#5998

Merged

		# may be set to `mock-orig-dst` for profiling builds, or to `multithreaded` to
		# enable the multithreaded Tokio runtime.

Conversation

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

Uh oh!

Uh oh!

olix0r left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

olix0r Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

olix0r commented Mar 30, 2021

Uh oh!

hawkw commented Mar 30, 2021

Uh oh!

olix0r commented Mar 30, 2021

Uh oh!

hawkw commented Mar 30, 2021

Uh oh!

olix0r commented Mar 31, 2021

Uh oh!

hawkw Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

hawkw commented Mar 31, 2021

Uh oh!

olix0r Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hawkw commented Mar 25, 2021 •

edited

Loading