Split the inbound server into multiple modules #1179

olix0r · 2021-08-02T15:20:18Z

The Inbound::push_server stack builder is responsible for building all of the inbound stacks: the HTTP stack, the TCP forwarding stack, and the direct/gateway stack. Furthermore, most of these stacks rely on fixed target types (which makes them inflexible).

This change refactors the inbound stack to setup for upcoming policy changes:

A 'detect' stack is split out into a push_detect helper that only deals with protocol detection and dispatching connections to either the TCP forwarding stack or the HTTP handling stack.
An 'accept' stack is split out into a push_accept helper that is responsible for extracting relevant connection metadata (client and orig dst addrs) and determining the policy to use for the connection.
Each stack/module defines its own target types and other modules rely on param constraints instead of fixed target types.
A port_policies module is introduced to model the existing per-port policies: whether identity is required and whether protocol detection should be skipped. Various auxiliary modules have been eliminated in favor of inlined target filters. This helps to make these stacks easier to reason about, as relevant logic isn't spread over multiple files.

Depends on #1174

Following #1169, this change modifies the `detect` middleware to use `ExtractParam`. In doing this, it makes sense to modify the default implementations of `ExtractParam` so that we can provide a static configuration to the module that is cloned for all targets without modification. This change does not alter any functionality, but sets up to make the HTTP detection layer configured per-target.

We have BoxHttp and BoxNewHttp aliases that help reduce type boilerplate. This change adds similar aliases for TCP-forwarding stacks.

olix0r · 2021-08-02T21:19:25Z

linkerd/app/admin/src/server/mod.rs

@@ -150,13 +150,6 @@ impl<M> Admin<M> {
    }
 }

-impl<M: FmtMetrics + Clone, T> svc::NewService<T> for Admin<M> {


This can just be in-lined.. no need for a dedicated impl

olix0r · 2021-08-02T21:19:42Z

linkerd/app/admin/src/stack.rs

@@ -66,31 +78,29 @@ impl Config {

        let (ready, latch) = crate::server::Readiness::new();
        let admin = crate::server::Admin::new(report, ready, shutdown, trace);
-        let admin = svc::stack(admin)
-            .push(metrics.http_endpoint.to_layer::<classify::Response, _, Target>())
+        let admin = svc::stack(move |_| admin.clone())


olix0r · 2021-08-02T21:22:21Z

linkerd/app/core/src/metrics/mod.rs

@@ -86,7 +87,7 @@ pub struct StackLabels {
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct RouteLabels {
    direction: Direction,
-    target: Addr,
+    addr: profiles::LogicalAddr,


there's no need for the more general Addr type here -- we'll always have a LogicalAddr when we're dealing with routes.

olix0r · 2021-08-02T21:23:07Z

linkerd/app/core/src/config.rs

-/// Because ports are single `u16` values, we don't have to hash them; we can just use
-/// the integer values as hashes directly.
-#[derive(Default)]
-pub struct PortHasher(u16);


Not really a part of config in general -- this has been moved the port_policies module as an impl detail.

olix0r · 2021-08-02T21:24:01Z

linkerd/app/core/src/dst.rs

 use std::time::Duration;

 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct Route {
-    pub target: Addr,
+    pub addr: profiles::LogicalAddr,


routes always have a logical addr. no need to be more general

olix0r · 2021-08-02T21:33:19Z

linkerd/app/inbound/src/http/tests.rs

+impl Target {
+    const HTTP1: Self = Self(http::Version::Http1);
+    const H2: Self = Self(http::Version::H2);


Tests now use their own target type.

olix0r · 2021-08-02T21:33:54Z

linkerd/app/inbound/src/lib.rs


-    pub fn serve<B, G, GSvc, P>(


moved into its own module

olix0r · 2021-08-02T21:34:38Z

linkerd/app/inbound/src/port_policies.rs

+use thiserror::Error;
+
+#[derive(Clone, Debug)]
+pub struct PortPolicies {


replaces require_identity and prevent_loop

olix0r · 2021-08-02T21:35:30Z

linkerd/app/inbound/src/server.rs

+        let serve = async move {
+            let shutdown = self.runtime.drain.clone().signaled();
+
+            // Handles connections to ports that can't be determined to be HTTP.
+            let forward = self
+                .clone()
+                .into_tcp_connect(la.port())
+                .push_tcp_forward()
+                .into_stack()
+                .push_map_target(TcpEndpoint::from_param)
+                .instrument(|_: &_| debug_span!("tcp"))
+                .into_inner();
+
+            // Handles connections that target the inbound proxy port.
+            let direct = self
+                .clone()
+                .into_tcp_connect(la.port())
+                .push_tcp_forward()
+                .map_stack(|_, _, s| s.push_map_target(TcpEndpoint::from_param))
+                .push_direct(gateway)
+                .into_stack()
+                .instrument(|_: &_| debug_span!("direct"))
+                .into_inner();
+
+            // Handles HTTP connections.
+            let http = self
+                .into_tcp_connect(la.port())
+                .push_http_router(profiles)
+                .push_http_server();
+
+            // Determines how to handle an inbound connection, dispatching it to the appropriate
+            // stack.
+            let server = http
+                .push_detect(forward)
+                .push_accept(la.port(), direct)
+                .into_inner();
+
+            serve::serve(listen, server, shutdown).await


now we build all of the stacks with the server so that each stack can be tested independently.

olix0r · 2021-08-02T21:36:09Z

linkerd/app/src/env.rs

+                require_identity_for_inbound_ports
+                    .into_iter()
+                    .map(|p| (p, inbound::AllowPolicy::Authenticated))
+                    .chain(inbound_opaque_ports.into_iter().map(|p| {
+                        (
+                            p,
+                            inbound::AllowPolicy::Unauthenticated { skip_detect: true },
+                        )
+                    })),


require_id and opaque_ports configs are unified into port policies

olix0r · 2021-08-02T21:36:53Z

linkerd/app/inbound/src/http/router.rs

+// === impl Inbound ===
+
+impl<C> Inbound<C> {
+    pub fn push_http_router<T, P>(


moved from http/lib.rs

olix0r · 2021-08-02T21:37:30Z

linkerd/app/inbound/src/http/router.rs

+                    // If the target includes a logical named address and it exists in the set of
+                    // allowed discovery suffixes, use that address for discovery. Otherwise, fail
+                    // discovery (so that we skip the profile stack above).
+                    let addr = t.logical.ok_or_else(|| {
+                        DiscoveryRejected::new("inbound profile discovery requires DNS names")
+                    })?;
+                    if !allow_profile.matches(addr.name()) {
+                        tracing::debug!(
+                            %addr,
+                            suffixes = %allow_profile,
+                            "Rejecting discovery, address not in configured DNS suffixes",
+                        );
+                        return Err(DiscoveryRejected::new("address not in search DNS suffixes"));
+                    }
+                    Ok(profiles::LookupAddr(addr.into()))


replaces the allow_discovery module

olix0r · 2021-08-02T21:37:58Z

linkerd/app/inbound/src/http/router.rs

+        // Try to read a logical named address from the request. First check the canonical-dst
+        // header as set by the client proxy; otherwise fallback to the request's `:authority` or
+        // `host` headers. If these values include a numeric address, no logical name will be used.
+        // This value is used for profile discovery.
+        let logical = req
+            .headers()
+            .get(CANONICAL_DST_HEADER)
+            .and_then(|dst| {
+                dst.to_str().ok().and_then(|d| {
+                    Addr::from_str(d).ok().map(|a| {
+                        debug!("using {}", CANONICAL_DST_HEADER);
+                        a
+                    })
+                })
+            })
+            .or_else(|| http_request_authority_addr(req).ok())
+            .or_else(|| http_request_host_addr(req).ok())
+            .and_then(|a| a.into_name_addr());


now always builds a named address

hawkw

left a preliminary review, would like to take a closer look at a few things. overall, this feels good!

linkerd/app/inbound/src/accept.rs

hawkw · 2021-08-02T22:05:01Z

linkerd/app/inbound/src/detect.rs

+    /// Builds a stack that handles protocol detection according to the port's policy. If the
+    /// connection is determined to be HTTP, the inner stack is used; otherwise the connection is
+    /// passed to the provided 'forward' stack.
+    pub fn push_detect<T, I, NSvc, F, FSvc>(self, forward: F) -> Inbound<svc::BoxNewTcp<T, I>>


nit/tioli: might have called this push_detect_http so it's completely obvious what we're detecting here...not a blocker

it also detects TLS

ah, i missed that, sorry — can't read!

linkerd/app/inbound/src/detect.rs

linkerd/app/inbound/src/http/router.rs

linkerd/app/inbound/src/http/tests.rs

hawkw · 2021-08-02T23:05:42Z

linkerd/app/inbound/src/server.rs

+        let serve = async move {
+            let shutdown = self.runtime.drain.clone().signaled();
+
+            // Handles connections to ports that can't be determined to be HTTP.
+            let forward = self
+                .clone()
+                .into_tcp_connect(la.port())
+                .push_tcp_forward()
+                .into_stack()
+                .push_map_target(TcpEndpoint::from_param)
+                .instrument(|_: &_| debug_span!("tcp"))
+                .into_inner();
+
+            // Handles connections that target the inbound proxy port.
+            let direct = self
+                .clone()
+                .into_tcp_connect(la.port())
+                .push_tcp_forward()
+                .map_stack(|_, _, s| s.push_map_target(TcpEndpoint::from_param))
+                .push_direct(gateway)
+                .into_stack()
+                .instrument(|_: &_| debug_span!("direct"))
+                .into_inner();
+
+            // Handles HTTP connections.
+            let http = self
+                .into_tcp_connect(la.port())
+                .push_http_router(profiles)
+                .push_http_server();
+
+            // Determines how to handle an inbound connection, dispatching it to the appropriate
+            // stack.
+            let server = http
+                .push_detect(forward)
+                .push_accept(la.port(), direct)
+                .into_inner();
+
+            serve::serve(listen, server, shutdown).await


hawkw

okay, i feel good about this — commented on the changes that i think are necessary to fix the fuzz build.

linkerd/app/inbound/src/http/router.rs

linkerd/app/inbound/src/detect.rs

linkerd/app/inbound/src/http/mod.rs

The `Inbound::push_server` stack builder is responsible for building all of the inbound stacks: the HTTP stack, the TCP forwarding stack, and the direct/gateway stack. Furthermore, most of these stacks rely on fixed target types (which makes them inflexible). This change refactors the inbound stack to setup for upcoming policy changes: 1. A 'detect' stack is split out into a `push_detect` helper that only deals with protocol detection and dispatching connections to either the TCP forwarding stack or the HTTP handling stack. 2. An 'accept' stack is split out into a `push_accept` helper that is responsible for extracting relevant connection metadata (client and orig dst addrs) and determining the policy to use for the connection. 3. Each stack/module defines its own target types and other modules rely on param constraints instead of fixed target types. 4. A `port_policies` module is introduced to model the existing per-port policies: whether identity is required and whether protocol detection should be skipped. Various auxiliary modules have been eliminated in favor of inlined target filters. This helps to make these stacks easier to reason about, as relevant logic isn't spread over multiple files.

hawkw · 2021-08-03T17:25:52Z

Couple of tests appear to be failing?

This release includes only dependency updates an internal changes to support upcoming policy features. No user-facing changes are expected. --- * build(deps): bump tokio from 1.8.2 to 1.9.0 (linkerd/linkerd2-proxy#1162) * build(deps): bump hyper from 0.14.10 to 0.14.11 (linkerd/linkerd2-proxy#1163) * build(deps): bump codecov/codecov-action from 2.0.1 to 2.0.2 (linkerd/linkerd2-proxy#1165) * build(deps): bump futures from 0.3.15 to 0.3.16 (linkerd/linkerd2-proxy#1166) * build(deps): bump softprops/action-gh-release (linkerd/linkerd2-proxy#1167) * build(deps): bump softprops/action-gh-release from 0.1.6 to 1 (linkerd/linkerd2-proxy#1168) * stack: Introduce ExtractParam and InsertParam (linkerd/linkerd2-proxy#1169) * inbound: Consolidate port-based switching (linkerd/linkerd2-proxy#1170) * build(deps): bump socket2 from 0.4.0 to 0.4.1 (linkerd/linkerd2-proxy#1171) * build(deps): bump serde_json from 1.0.64 to 1.0.65 (linkerd/linkerd2-proxy#1172) * build(deps): bump async-trait from 0.1.50 to 0.1.51 (linkerd/linkerd2-proxy#1173) * Update to Rust v1.54.0 (linkerd/linkerd2-proxy#1175) * build(deps): bump serde_json from 1.0.65 to 1.0.66 (linkerd/linkerd2-proxy#1176) * Add BoxTcp and BoxNewTcp service type aliases (linkerd/linkerd2-proxy#1177) * http: Use ExtractParam to support dynamic detect configuration (linkerd/linkerd2-proxy#1174) * Split the inbound server into multiple modules (linkerd/linkerd2-proxy#1179)

PR #1179 changed the inbound HTTP fuzz logic, and added a mocked `Target` type similar to other stack tests. However, this introduced a bug, because the `Target::addr()` function, which returns the socket address that the mock TCP connector should expect the proxy to try to connect to, returns the (mocked) original destination address. Since the stack we build for the fuzz test maps the target IP address to localhost before calling into the connect stack (as the real proxy currently does), this means that we attempt to connect to an address the mock connector doesn't expect. Then, it panics and the fuzz test fails. This branch fixes this issue by changing `Target::addr` to return 127.0.0.1:80 instead of 192.0.2.2:80. This should match what the proxy will actually attempt to connect to. I also updated the fuzz logic to use exactly the same connect stack as the HTTP stack tests (including a timeout). Although this shouldn't be exercised, it makes the fuzz test more closely match the stack tests and thus the real life proxy. I wonder if we can eventually share more code between the tests and the fuzzers, but that seems like a follow-up. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

This release includes only dependency updates an internal changes to support upcoming policy features. No user-facing changes are expected. --- * build(deps): bump tokio from 1.8.2 to 1.9.0 (linkerd/linkerd2-proxy#1162) * build(deps): bump hyper from 0.14.10 to 0.14.11 (linkerd/linkerd2-proxy#1163) * build(deps): bump codecov/codecov-action from 2.0.1 to 2.0.2 (linkerd/linkerd2-proxy#1165) * build(deps): bump futures from 0.3.15 to 0.3.16 (linkerd/linkerd2-proxy#1166) * build(deps): bump softprops/action-gh-release (linkerd/linkerd2-proxy#1167) * build(deps): bump softprops/action-gh-release from 0.1.6 to 1 (linkerd/linkerd2-proxy#1168) * stack: Introduce ExtractParam and InsertParam (linkerd/linkerd2-proxy#1169) * inbound: Consolidate port-based switching (linkerd/linkerd2-proxy#1170) * build(deps): bump socket2 from 0.4.0 to 0.4.1 (linkerd/linkerd2-proxy#1171) * build(deps): bump serde_json from 1.0.64 to 1.0.65 (linkerd/linkerd2-proxy#1172) * build(deps): bump async-trait from 0.1.50 to 0.1.51 (linkerd/linkerd2-proxy#1173) * Update to Rust v1.54.0 (linkerd/linkerd2-proxy#1175) * build(deps): bump serde_json from 1.0.65 to 1.0.66 (linkerd/linkerd2-proxy#1176) * Add BoxTcp and BoxNewTcp service type aliases (linkerd/linkerd2-proxy#1177) * http: Use ExtractParam to support dynamic detect configuration (linkerd/linkerd2-proxy#1174) * Split the inbound server into multiple modules (linkerd/linkerd2-proxy#1179)

PR #1179 changed the inbound HTTP fuzz logic, and added a mocked `Target` type similar to other stack tests. However, this introduced a bug, because the `Target::addr()` function, which returns the socket address that the mock TCP connector should expect the proxy to try to connect to, returns the (mocked) original destination address. Since the stack we build for the fuzz test maps the target IP address to localhost before calling into the connect stack (as the real proxy currently does), this means that we attempt to connect to an address the mock connector doesn't expect. Then, it panics and the fuzz test fails. This branch fixes this issue by changing `Target::addr` to return 127.0.0.1:80 instead of 192.0.2.2:80. This should match what the proxy will actually attempt to connect to. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

This release includes only dependency updates an internal changes to support upcoming policy features. No user-facing changes are expected. --- * build(deps): bump tokio from 1.8.2 to 1.9.0 (linkerd/linkerd2-proxy#1162) * build(deps): bump hyper from 0.14.10 to 0.14.11 (linkerd/linkerd2-proxy#1163) * build(deps): bump codecov/codecov-action from 2.0.1 to 2.0.2 (linkerd/linkerd2-proxy#1165) * build(deps): bump futures from 0.3.15 to 0.3.16 (linkerd/linkerd2-proxy#1166) * build(deps): bump softprops/action-gh-release (linkerd/linkerd2-proxy#1167) * build(deps): bump softprops/action-gh-release from 0.1.6 to 1 (linkerd/linkerd2-proxy#1168) * stack: Introduce ExtractParam and InsertParam (linkerd/linkerd2-proxy#1169) * inbound: Consolidate port-based switching (linkerd/linkerd2-proxy#1170) * build(deps): bump socket2 from 0.4.0 to 0.4.1 (linkerd/linkerd2-proxy#1171) * build(deps): bump serde_json from 1.0.64 to 1.0.65 (linkerd/linkerd2-proxy#1172) * build(deps): bump async-trait from 0.1.50 to 0.1.51 (linkerd/linkerd2-proxy#1173) * Update to Rust v1.54.0 (linkerd/linkerd2-proxy#1175) * build(deps): bump serde_json from 1.0.65 to 1.0.66 (linkerd/linkerd2-proxy#1176) * Add BoxTcp and BoxNewTcp service type aliases (linkerd/linkerd2-proxy#1177) * http: Use ExtractParam to support dynamic detect configuration (linkerd/linkerd2-proxy#1174) * Split the inbound server into multiple modules (linkerd/linkerd2-proxy#1179) Signed-off-by: Sanni Michael <sannimichaelse@gmail.com>

olix0r added 30 commits July 30, 2021 18:46

touchup

9a8ad80

Merge branch 'main' into ver/http-extract

e5fd625

lessen boilerplate with a helper fn

fa5c53b

Add BoxTcp and BoxNewTcp service type aliases

b5a1e1c

We have BoxHttp and BoxNewHttp aliases that help reduce type boilerplate. This change adds similar aliases for TCP-forwarding stacks.

wip

0a8dd16

Merge branch 'ver/http-extract' into ver/inbound-port-policy

1ce428c

Merge branch 'ver/box-tcp' into ver/inbound-port-policy

b345c4d

wip

c3ac565

wip

16bbf7b

wip

4b18718

Merge branch 'main' into ver/inbound-port-policy

5636be8

wip

a94155a

fixup tests

80e4656

decouple tcp-forward from target type

454f730

fixup tests

4fb8b3a

wip: use existing inbound port config

7512ac0

Use port policies to inform skipping detection

2e9c77f

restore authentication policies

b6110d7

Decouple TcpEndpoint target type

361e0ad

fixup skip-detect check

8c84903

comment

3eb4fe0

touchup

9bbcfb2

build direct stack outside of server

28d0311

comment

d00dfea

Make Inbound::push_server decoupled from underlying stacks

3fd8a0e

Split the inbound server into accept and detect stacks

6ab426a

comments

cda59c4

inbound: split detect module

fd0cd6a

Split accept module; move server into its own module

89319f3

olix0r added 4 commits August 2, 2021 20:21

Add detect tests

2dfd006

Merge branch 'main' into ver/inbound-port-policy

afd9f01

-fixme

a5eb308

fix inbound http fuzzing

2ff0980

olix0r marked this pull request as ready for review August 2, 2021 20:53

olix0r requested a review from a team August 2, 2021 20:53

olix0r commented Aug 2, 2021

View reviewed changes

kill dead code

fe0f2ad

hawkw reviewed Aug 2, 2021

View reviewed changes

olix0r added 4 commits August 3, 2021 15:39

Rename target types in the http router

6025702

cleanup detect stack

68b563f

+comment

d7cb5b3

re-export http::Method for convenience

bcb8efe

hawkw approved these changes Aug 3, 2021

View reviewed changes

olix0r added 3 commits August 3, 2021 17:02

fixup fuzzer

af1fad1

Add target type docs

a699e8d

fixup error message

9e1c619

olix0r merged commit 2eb1671 into main Aug 3, 2021

olix0r deleted the ver/inbound-port-policy branch August 3, 2021 18:56

olix0r mentioned this pull request Aug 5, 2021

proxy: v2.151.0 linkerd/linkerd2#6607

Merged

hawkw mentioned this pull request Aug 10, 2021

fuzz: fix inbound fuzz logic expecting wrong IP address #1193

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the inbound server into multiple modules #1179

Split the inbound server into multiple modules #1179

olix0r commented Aug 2, 2021 •

edited

Loading

olix0r Aug 2, 2021

olix0r Aug 2, 2021

olix0r Aug 2, 2021

olix0r Aug 2, 2021

olix0r Aug 2, 2021

olix0r Aug 2, 2021

olix0r Aug 2, 2021

olix0r Aug 2, 2021

olix0r Aug 2, 2021

hawkw Aug 2, 2021

olix0r Aug 2, 2021

olix0r Aug 2, 2021

olix0r Aug 2, 2021

olix0r Aug 2, 2021

hawkw left a comment

hawkw Aug 2, 2021

olix0r Aug 3, 2021

hawkw Aug 3, 2021

hawkw Aug 2, 2021

hawkw left a comment

hawkw commented Aug 3, 2021

Split the inbound server into multiple modules #1179

Split the inbound server into multiple modules #1179

Conversation

olix0r commented Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw commented Aug 3, 2021

olix0r commented Aug 2, 2021 •

edited

Loading