outbound: Discover profiles for each unique TCP target#698
Merged
olix0r merged 9 commits intover/accept-profilesfrom Oct 10, 2020
Merged
outbound: Discover profiles for each unique TCP target#698olix0r merged 9 commits intover/accept-profilesfrom
olix0r merged 9 commits intover/accept-profilesfrom
Conversation
Member
Author
|
Depends on #697 |
The `AcceptHttp` service is stateful. It builds the underlying HTTP, HTTP/2, and TCP service stacks lazily the first time they're needed. This interacts badly with the derived implementation of `Clone`: if it's cloned before the state is set, the state will only be set for the clone, rather than for the original instance in the cache. Since `Prefix` clones and oneshots the underlying service (`AcceptHttp`), this means that we rebuild the HTTP, HTTP/2, and TCP stacks on every connection to the same orig dest. This basically breaks caching for everything other than service profile discovery, causing us to do new destination discovery for every connection to the same original destination. Which...isn't great. This branch fixes this issue by removing the `Clone` impl from `AcceptHttp` and putting it behind a buffer instead. In the future, it may be possible to fix this in a nicer way, but this resolves the issue for now.
Removes defunct identity tests per linkerd/linkerd2#4947
The proxy has classically discovered profiles & endpoints by looking at each HTTP request's headers. This change removes all per-request discovery. Instead, resolution is now done for each unique outbound TCP target (regardless of whether the connection serves HTTP or not). This eager resolution eliminates per-request cache binding; and supports using `TrafficSplit` with non-HTTP services. This has a few side effects: - The `l5d-dst-override` header is no longer honored. - When the application attempts to connect to a pod IP, the proxy no longer load balances these requests among all pods in the service. The proxy will now honor session-stickiness as selected by an application-level load balancer (see linkerd/linkerd2#4956). - TrafficSplits are only applied when a client targets a service's IP. - The proxy no longer performs DNS "canonicalization" to translate relative host header names to a fully-qualified form.
hawkw
approved these changes
Oct 9, 2020
Contributor
hawkw
left a comment
There was a problem hiding this comment.
i was surprised by how simple the actual functional change is; most of this is actually just the test changes. the change itself is very straightforward! this looks good to me 👍
| H: NewService<(HttpVersion, T)>, | ||
| { | ||
| pub fn new(target: T, server: Server, new_http: H, new_tcp: F, drain: drain::Watch) -> Self { | ||
| tracing::trace!("new AcceptHttp"); |
Contributor
There was a problem hiding this comment.
we probably don't need this, i stuck it in while debugging and forgot to take it out
Suggested change
| tracing::trace!("new AcceptHttp"); |
| Some(HttpVersion::Http1) => { | ||
| debug!("Handling as HTTP"); | ||
| let http1 = if let Some(svc) = self.http1.clone() { | ||
| trace!("HTTP service already exists"); |
Contributor
There was a problem hiding this comment.
these were also from debugging but i thought they might actually be worth having.
| pub profile: Option<profiles::Receiver>, | ||
| } | ||
|
|
||
| #[derive(Clone, Debug)] |
Contributor
There was a problem hiding this comment.
nit: while working on the test changes i realized that it might not be great to derive Debug for TcpLogical, since it has a MPSC receiver in it, which dumps a bunch of internal state that we don't care about...we might want a custom Debug impl for TcpLogical because of that.
not a blocker, can fix it later; just something i remembered while looking at this.
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.
The proxy has classically discovered profiles & endpoints by looking at
each HTTP request's headers. This change removes all per-request
discovery. Instead, resolution is now done for each unique outbound TCP
target (regardless of whether the connection serves HTTP or not). This
eager resolution eliminates per-request cache binding; and supports
using
TrafficSplitwith non-HTTP services.This has a few side effects:
l5d-dst-overrideheader is no longer honored.longer load balances these requests among all pods in the service.
The proxy will now honor session-stickiness as selected by an
application-level load balancer (see Session Stickiness only works with TLS backends linkerd2#4956).
relative host header names to a fully-qualified form.