Decouple server config from OrigDstAddr by olix0r · Pull Request #962 · linkerd/linkerd2-proxy

olix0r · 2021-03-31T04:06:00Z

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.

7d2bdbb fixed an issue with TLS detection. This change adds a test for this behavior. In order to implement a test, this change modifies the `detect` helper to not be responsible for SNI matching & TLS termination. The `detect` helper is replaced with a `detect_sni` function that only reads the ClientHello without terminating TLS. A test has been added for the `detect_sni` helper.

The HTTP detect module may fail to detect an HTTP protocol if the first read does not return at least 14 bytes. This has caused spurious failures as described in linkerd/linkerd2#5672. This change updates the admin server's protocol detection to handle these connections as HTTP/1 so that the HTTP server can try to process them and fail on its own if the connection really is not HTTP.

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.

hawkw

this looks great, I think this factoring is much better than what I ended up with. I had some minor non-blocking nits, and also noticed one accidental change that i think we definitely need to fix, but besides that, I'm happy to merge this into my branch.

linkerd/app/src/env.rs

hawkw · 2021-03-31T17:49:34Z

linkerd/app/src/lib.rs

+        bind_in: BIn,
+        bind_out: BOut,
+        bind_adm: BAdm,


hmmm...this is not my favorite thing in the world, the big pile of positionals feels like it's a little hard to follow at the callsite? Maybe it would be better if there was an arguments struct like:

pub struct BindConfig<In, Out, Adm> { pub bind_inbound: In, pub bind_outbound: Out, pub admin: Adm, }

to fake calling build with named params, like:

let config = ...; let bind_inbound = ...; let bind_outbound = ...; let bind_admin = ...; let app = config.build(BindConfig { bind_inbound, bind_outbound, bind_admin })?;

but...it's probably not really worth the complexity; it's not like this is a library API or even a function that we call all that often (i think it gets called...twice in the entire codebase?)

hawkw · 2021-03-31T17:51:03Z

linkerd/proxy/transport/src/listen.rs

 where
-    A: GetAddrs<TcpStream> + Send + Sync + 'static,
-    A::Addrs: Send + Sync + 'static,
+    T: Param<ListenAddr> + Param<Keepalive>,


nice...although OTTOH, as far as i can tell, we only ever call this with ServerConfig...maybe instead of making Bind generic over an arbitrary params type, we could just have a BindConfig with a Keepalive and a ListenAddr in this module (solving the problem of ServerConfig being defined in a higher-up crate), and have Bind::bind take a BindConfig? That might be simpler than having a more generic solution that we don't actually need.

but...this doesn't add so much complexity that i care all that strongly...

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

linkerd/proxy/transport/src/orig_dst.rs

+#[derive(Copy, Clone, Debug, Default)]
+pub struct BindWithOrigDst<B = listen::BindTcp> {
+    inner: B,
+}
+
+#[derive(Clone, Debug)]
+pub struct Addrs<A = listen::Addrs> {
+    pub inner: A,
+    pub orig_dst: OrigDstAddr,
+}


very elegant, i approve of this factoring <3

hawkw · 2021-03-31T17:58:25Z

linkerd/proxy/transport/src/orig_dst.rs

+            Ok((addrs, tcp))
+        });
+
+        Ok((addr, Box::pin(incoming)))


this does mean we're Box::pinning the incoming stream twice, so every time we poll it we dereference two trait object pointers. Since this map doesn't capture anything, I don't think we need to box again; we could change Incoming to

type Incoming = futures::stream::Map< T::Incoming, fn(io::Result<(B::Addrs, B::Io)>-> io::Result<(Self::Addrs, Self::Io)> >;

which is not all that much worse of a type and avoids a second trait object pointer deref. Not sure if this matters though...

I tried this and it doesn't work as you'd hope because the closure captures part of the type signature... if you can shave off the box, I'm open to it, though

hawkw · 2021-03-31T18:05:02Z

linkerd2-proxy/src/main.rs

+        let app = match async move {
+            config
+                .build(bind, bind, BindTcp::default(), shutdown_tx, trace?)
+                .await
+        }
+        .await


oh, wow, huh, this sure is some formatting...

hawkw · 2021-03-31T18:06:06Z

linkerd/proxy/transport/src/orig_dst.rs

+impl<A> Param<Remote<ClientAddr>> for Addrs<A>
+where
+    A: Param<Remote<ClientAddr>>,
+{
+    fn param(&self) -> Remote<ClientAddr> {
+        self.inner.param()
+    }
+}
+
+impl<A> Param<Local<ServerAddr>> for Addrs<A>
+where
+    A: Param<Local<ServerAddr>>,
+{
+    fn param(&self) -> Local<ServerAddr> {
+        self.inner.param()
+    }
+}


sad that these can't just be a impl<A, T> Param<T> for Addrs<A> where A: Param<T>, but i guess there's no way to convince rustc that that wouldn't conflict with Param<OrigDstAddrs>...

hawkw · 2021-03-31T18:07:25Z

linkerd/proxy/transport/src/listen.rs

+            let tcp = res?;
+            super::set_nodelay_or_warn(&tcp);
+            super::set_keepalive_or_warn(&tcp, keepalive);
+            let client = Remote(ClientAddr(tcp.peer_addr()?));


small nitpick: I put the "accepted" trace event here once we have the addrs, and we don't have that any more...nbd but it might be worth having

hawkw · 2021-03-31T18:09:36Z

linkerd/app/outbound/src/tcp/tests.rs

+    let svc = server.new_service(orig_dst::Addrs {
+        orig_dst: OrigDstAddr(ep1),
+        inner: listen::Addrs {
+            server: Local(ServerAddr(([127, 0, 0, 1], 4140).into())),
+            client: Remote(ClientAddr(([127, 0, 0, 1], 666).into())),
+        },
+    });


nit/TIOLI: maybe there should be an orig_dst::Addrs::new(OrigDstAddr, Local<ServerAddr>, Remote<ClientAddr>) constructor to make this slightly less wordy? not a big deal.

olix0r added 4 commits March 30, 2021 08:36

Merge branch 'main' into ver/eliza/odl

0203d04

olix0r requested a review from a team March 31, 2021 04:06

olix0r mentioned this pull request Mar 31, 2021

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

Merged

Remove unneeded test helper

0a86b80

hawkw approved these changes Mar 31, 2021

View reviewed changes

fix inadvertant metrics retain idle env var change

aad758d

hawkw merged commit b695009 into eliza/orig-dst-layer Mar 31, 2021

hawkw deleted the ver/eliza/odl branch March 31, 2021 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple server config from OrigDstAddr #962

Decouple server config from OrigDstAddr #962
hawkw merged 6 commits intoeliza/orig-dst-layerfrom
ver/eliza/odl

olix0r commented Mar 31, 2021

Uh oh!

hawkw left a comment

Uh oh!

Uh oh!

hawkw Mar 31, 2021

Uh oh!

hawkw Mar 31, 2021

Uh oh!

hawkw Mar 31, 2021

Uh oh!

hawkw Mar 31, 2021

Uh oh!

olix0r Mar 31, 2021

Uh oh!

hawkw Mar 31, 2021

Uh oh!

hawkw Mar 31, 2021

Uh oh!

hawkw Mar 31, 2021

Uh oh!

hawkw Mar 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

olix0r commented Mar 31, 2021

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants