http: Simplify stacks and target types by olix0r · Pull Request #656 · linkerd/linkerd2-proxy

olix0r · 2020-09-15T01:41:33Z

Currently, various properties of HTTP requests are exposed into the
stack target type, meaning that stacks (including their resolutions)
cannot be shared across requests that vary by any of these settings and,
worse, that per-request determinations must be made to determine which
stack to use.

In order to bind stacks according to connection-level metadata,
where these request-level parameters cannot be known, we need to remove
this metadata from the stack target.

The HTTP server now applies URI normalization as an internal
implementation detail (because it's a detail of how hyper clients and
servers differ). This eliminates the ShouldNormalizeUri trait, as that
determination can be made entirely based on the properties of the
request without any additional stack-specific metadata.

The HTTP client now takes an updated Settings type that explicitly
handles the orig-proto upgrade logic (removing this from the client
stack).

Stack targets now hold an http::Version on the accept-side, and map
that to a per-endpoint http::client::Settings for use by the http
client.

Currently, various properties of HTTP requests are exposed into the stack target type, meaning that stacks (including their resolutions) cannot be shared across requests that vary by any of these settings and, worse, that per-request determinations must be made to determine which stack to use. In order to perform bind stacks according to connection-level metadata, where these request-level parameters cannot be known, we need to remove this metadata from the stack target. The HTTP server now applies URI normalization as an internal implementation detail (because it's a detail of how hyper clients and servers differ). This eliminates the `ShouldNormalizeUri` trait, as that determination can be made entirely based on the properties of the request without any additional stack-specific metadata. The HTTP client now takes an updated `Settings` type that explicitly handles the orig-proto upgrade logic (removing this from the client stack). Stack targets now hold an `http::Version` on the accept-side, and map that to a per-endpoint `http::client::Settings` for use by the http client.

hawkw

the new factoring makes a lot of sense to me --- this feels simpler and easier to understand. i had some style nits and suggestions, but didn't notice any problems.

linkerd/app/inbound/src/endpoint.rs

linkerd/proxy/http/src/client.rs

linkerd/app/outbound/src/endpoint.rs

linkerd/proxy/http/src/client.rs

linkerd/proxy/http/src/orig_proto.rs

hawkw · 2020-09-15T18:00:47Z

linkerd/proxy/http/src/orig_proto.rs

+                    if orig_proto == "HTTP/1.1" {
+                        Some(http::Version::HTTP_11)
+                    } else if orig_proto == "HTTP/1.0" {
+                        Some(http::Version::HTTP_10)
+                    } else {
+                        None
+                    }


nit/tioli: could be

Suggested change

if orig_proto == "HTTP/1.1" {

Some(http::Version::HTTP_11)

} else if orig_proto == "HTTP/1.0" {

Some(http::Version::HTTP_10)

} else {

None

}

match orig_proto {

"HTTP/1.1" => Some(http::Version::HTTP_11),

"HTTP/1.0" => Some(http::Version::HTTP_10),

_ => None,

}

...unless i'm missing something, of course?

orig_proto is a HeaderValue and not a string, so the matching form doesn't work

linkerd/proxy/transport/src/listen.rs

linkerd/proxy/transport/src/tls/accept.rs

hawkw · 2020-09-15T18:06:36Z

linkerd/proxy/http/src/h1.rs

+            if client.is_none() {
+                debug!("Caching new client");
+                *client = Some(hyper::Client::builder().set_host(absolute_form).build(
+                    HyperConnect::new(self.connect.clone(), self.target.clone(), absolute_form),
+                ));
+            }
+
+            client.as_ref().unwrap().request(req)


nit/TIOLI: there's actually an [Option::get_or_insert_with] method that basically implements this logic and avoids the unwrap:

Suggested change

if client.is_none() {

debug!("Caching new client");

*client = Some(hyper::Client::builder().set_host(absolute_form).build(

HyperConnect::new(self.connect.clone(), self.target.clone(), absolute_form),

));

}

client.as_ref().unwrap().request(req)

client

.get_or_insert_with(|| {

debug!("Caching new client");

hyper::Client::builder().set_host(absolute_form).build(

HyperConnect::new(self.connect.clone(), self.target.clone(), absolute_form),

)

})

.request(req)

Unfortunately, this doesn't play with the borrow checker and self -- because we're holding a mutable ref to the client, we can't pass a reference to self into the closure.

kleimkuhler

This looks good! I have a question about a change in Gateway, but otherwise this makes sense to me.

kleimkuhler · 2020-09-15T19:52:35Z

linkerd/app/gateway/src/lib.rs

                    .map(|n| NameAddr::from_str(n).unwrap().into())
                    .unwrap_or_else(|| socket_addr.into()),
-                http_settings: http::Settings::Http2,
+                http_version: http::Version::Http1,


Is this now Http1 because we know it should be upgraded to H2? If not, why this change?

it's a test, so it's not actually used

olix0r · 2020-09-15T21:10:24Z

So, this branch seems to impact CI time quite a bit.

Locally, I see:

`main`

#15 240.5     Finished release [optimized] target(s) in 4m 00s
#15 240.6       User time (seconds): 2043.05
#15 240.6       System time (seconds): 31.72
#15 240.6       Percent of CPU this job got: 863%
#15 240.6       Elapsed (wall clock) time (h:mm:ss or m:ss): 4:00.35
#15 240.6       Maximum resident set size (kbytes): 4573996

`ver/orig-proto`

#15 389.1     Finished release [optimized] target(s) in 6m 28s
#15 389.1       User time (seconds): 2143.76
#15 389.1       System time (seconds): 31.86
#15 389.1       Percent of CPU this job got: 559%
#15 389.1       Elapsed (wall clock) time (h:mm:ss or m:ss): 6:28.91
#15 389.1       Maximum resident set size (kbytes): 4328072

I'm not sure how this could have made it 30-40% worse... i would have expected the opposite.

Helps compile times for some reason?!

This release fixes a recent regression in multicluster gateway configurations that would forbid inbound gateway traffic. It also fixes URI normalization for orig-proto-upgrade requests that do not include a `Host` header. --- * http: Simplify stacks and target types (linkerd/linkerd2-proxy#656) * Make SkipDetect more generic as stack::MakeSwitch (linkerd/linkerd2-proxy#657) * introduce tests for isolated services (linkerd/linkerd2-proxy#655) * http: Put normalize_uri back on the stack (linkerd/linkerd2-proxy#659) * inbound: Apply loop detection on the connect stack (linkerd/linkerd2-proxy#660) * tracing: Elide redundant info in tracing contexts (linkerd/linkerd2-proxy#661) * outbound: Reorganize outbound stacks (linkerd/linkerd2-proxy#662) * app: Decouple stacks from listeners (linkerd/linkerd2-proxy#663) * inbound: Split HTTP detection stack from TLS (linkerd/linkerd2-proxy#664) * integration: Bundle tests in src (linkerd/linkerd2-proxy#665)

olix0r added 11 commits September 12, 2020 19:21

wip

0f0345b

Normalize with the original destination, if no host header is present

66e99fb

polish http client

07acd80

Embed NormalizeUri directly in the detect/server logic

12a1e84

wip: embed orig-proto in client

7f1b2ca

wip: inbound

5d540b6

wip: outbound

00f703b

wip: gateway

4e02d53

fixups from tests

99245fe

Commentary/clarity

9623a4a

olix0r requested a review from a team September 15, 2020 01:41

olix0r added 4 commits September 15, 2020 01:42

remove unused impl

a376bd6

revert unneeded change

6fa0224

Use Version enum to coalesce HTTP/1.1 and HTTP/1.0

9c48b71

Needless TryInto

b50d7de

hawkw approved these changes Sep 15, 2020

View reviewed changes

olix0r added 6 commits September 15, 2020 18:32

lifetime golf

693bcee

Use Version::into

8149c9d

h1 polish

d0aede0

more lifetime golf

e893233

orig_proto polish

c294efd

polish normalize_uri

0f6534e

kleimkuhler approved these changes Sep 15, 2020

View reviewed changes

olix0r added 2 commits September 15, 2020 20:09

avoid span clone

ad751db

fixup span

a72bdcb

Revert changes to stack ordering

02d56a0

Helps compile times for some reason?!

olix0r merged commit b59172a into main Sep 16, 2020

olix0r deleted the ver/orig-proto branch September 16, 2020 01:20

olix0r mentioned this pull request Sep 18, 2020

proxy: v2.110.0 linkerd/linkerd2#4987

Merged

Conversation

olix0r commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

olix0r Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hawkw Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

olix0r Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

kleimkuhler left a comment

Choose a reason for hiding this comment

Uh oh!

kleimkuhler Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

olix0r Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

olix0r commented Sep 15, 2020

main

ver/orig-proto

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olix0r commented Sep 15, 2020 •

edited

Loading

`main`

`ver/orig-proto`