Decouple client connection metadata from the I/O type#1426
Merged
Conversation
Instead of using tower's `MakeConnection` trait, we use our own trait so that we can introduce connection metadata in a followup change.
Some information about client connections may not be known a priori. For example, the local socket address is only known after the connection is established and the TLS handshake may negotiate additional information (like the ALPN protocol). We can use traits on the I/O type, but this means that all wrappers types need implement all of these traits. This is cumbersome. This change replaces the `tower::make::MakeConnection` helper trait with a new `linkerd_stack::MakeConnection` trait that explicity returns a `Metadata` instance so that connectors can return arbitrary information. Now, the TCP connector returns a `Local<ClientAddr>` and the TLS connector conditionally includes the negotiated protocol. This change sets up the ability for the HTTP client to negotiate protocol upgrading via ALPN instead of using per-request headers.
olix0r
commented
Jan 3, 2022
hawkw
approved these changes
Jan 3, 2022
Contributor
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
this looks great --- not having to implement a bunch of additional traits on every I/O wrapper type seems much nicer IMO. i commented on several minor style nits, but I didn't see any blockers! :)
Comment on lines
+40
to
+42
| let local_addr = io.local_addr()?; | ||
| debug!( | ||
| local.addr = %io.local_addr().expect("cannot load local addr"), | ||
| local.addr = %local_addr, |
Contributor
There was a problem hiding this comment.
this will now bail rather than panicking if the local address is missing...is that what we want? should we be logging an error or something before bailing?
Member
Author
There was a problem hiding this comment.
This will manifest as a connection error and be logged where those errors are handled.
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Jan 6, 2022
In addition to dependency updates, this change updates the inbound proxy to handle opaquely transported HTTP traffic. This fixes an issue encountered when a `Service`'s opaque ports annotation does not match that of the pods in the service. This situation should be rare. --- * Handle HTTP traffic over opaque transport connections (linkerd/linkerd2-proxy#1416) * build(deps): bump tracing-subscriber from 0.3.3 to 0.3.4 (linkerd/linkerd2-proxy#1421) * build(deps): bump pin-project from 1.0.8 to 1.0.9 (linkerd/linkerd2-proxy#1422) * build(deps): bump tracing-subscriber from 0.3.4 to 0.3.5 (linkerd/linkerd2-proxy#1423) * build(deps): bump pin-project from 1.0.9 to 1.0.10 (linkerd/linkerd2-proxy#1425) * build(deps): bump http from 0.2.5 to 0.2.6 (linkerd/linkerd2-proxy#1424) * build(deps): bump serde_json from 1.0.73 to 1.0.74 (linkerd/linkerd2-proxy#1427) * Decouple client connection metadata from the I/O type (linkerd/linkerd2-proxy#1426) * tests: rename 'metrics' addr to 'admin' (linkerd/linkerd2-proxy#1429)
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Jan 6, 2022
In addition to dependency updates, this change updates the inbound proxy to handle opaquely transported HTTP traffic. This fixes an issue encountered when a `Service`'s opaque ports annotation does not match that of the pods in the service. This situation should be rare. --- * Handle HTTP traffic over opaque transport connections (linkerd/linkerd2-proxy#1416) * build(deps): bump tracing-subscriber from 0.3.3 to 0.3.4 (linkerd/linkerd2-proxy#1421) * build(deps): bump pin-project from 1.0.8 to 1.0.9 (linkerd/linkerd2-proxy#1422) * build(deps): bump tracing-subscriber from 0.3.4 to 0.3.5 (linkerd/linkerd2-proxy#1423) * build(deps): bump pin-project from 1.0.9 to 1.0.10 (linkerd/linkerd2-proxy#1425) * build(deps): bump http from 0.2.5 to 0.2.6 (linkerd/linkerd2-proxy#1424) * build(deps): bump serde_json from 1.0.73 to 1.0.74 (linkerd/linkerd2-proxy#1427) * Decouple client connection metadata from the I/O type (linkerd/linkerd2-proxy#1426) * tests: rename 'metrics' addr to 'admin' (linkerd/linkerd2-proxy#1429)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Some information about client connections may not be known a priori. For
example, the local socket address is only known after the connection is
established and the TLS handshake may negotiate additional information
(like the ALPN protocol).
We can use traits on the I/O type, but this means that all wrappers
types need implement all of these traits. This is cumbersome.
This change replaces the
tower::make::MakeConnectionhelper trait witha new
linkerd_stack::MakeConnectiontrait that explicity returns aMetadatainstance so that connectors can return arbitrary information.Now, the TCP connector returns a
Local<ClientAddr>and the TLSconnector conditionally includes the negotiated protocol.
This change sets up the ability for the HTTP client to negotiate
protocol upgrading via ALPN instead of using per-request headers.
Changes are broken into distinct commits. No functional changes.