outbound: Decouple endpoint & logical stacks #1002
Merged
Conversation
This file contains 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
In #963 we began to split the outbound stack so that logical stack was only built when a profile resolution was successful. However, this incorrectly handled the case when a profile resolution included an endpoint in its response. This change introduces a `outbound::switch_logical` module that takes an optional profile and builds either an endpoint stack or a logical stack. Logical stacks always have a profile resolution and (named) logical address. Endpoint stacks are built otherwise, optionally with metadata from the profile resolution. In order to test this--and to simplify/focus tests, in general--I've moved tests from `outbound/src/{tcp,http}/tests.rs` into the modules they test. Now each test more narrowly targets the stacks whose logic they test, rather than composing an entire outbound stack. It was becoming cumbersome to support mocking each component of the larger stack. Now, we more intentionaly test targeted logic in each stack (including the newly fixed endpoint/logical stack switching). Supersedes #988
olix0r
commented
May 10, 2021
| HSvc: svc::Service<http::Request<http::BoxBody>, Response = http::Response<http::BoxBody>>, | ||
| HSvc: Clone + Send + Sync + Unpin + 'static, | ||
| HSvc::Error: Into<Error>, | ||
| HSvc::Future: Send, | ||
| HTgt: From<(http::Version, T)> + svc::Param<http::Version> + 'static, | ||
| T: Param<SkipHttpDetection> + Clone + Send + Sync + 'static, | ||
| T: Param<Option<Skip>> + Clone + Send + Sync + 'static, |
There was a problem hiding this comment.
This change isn't strictly necessary. Can back it out if you'd prefer the other way. @hawkw
There was a problem hiding this comment.
no strong preference; one fewer type param is nicer IMO
hawkw
reviewed
May 10, 2021
There was a problem hiding this comment.
overall this looks good, and i'm onboard with this direction, although i still think it could be good to have tests that exercise the entire outbound stack in addition.
i left some comments, mostly on minor nits. i tried to read through all the tests, but there's a lot of code here, so it would be nice to get more eyes on them...seems right as far as i can tell!
| // Instantiate a service from the stack so that it instantiates the tracked inner service. | ||
| // | ||
| // The discover stack's buffer does not drive profile resolution (or the inner service) | ||
| // until the service is called?! So we drive this all on a background ask that gets canceled |
There was a problem hiding this comment.
The discover stack's buffer does not drive profile resolution (or the inner service) until the service is called?!
hmm, that seems...weird. should we try to change that behavior in a follow-up?
There was a problem hiding this comment.
Yeah... it's a little surprising. It would at least be good to understand it in more detail.
Co-authored-by: Kevin Leimkuhler <kevin@kleimkuhler.com>
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
May 12, 2021
This release simplifies internals so that endpoint-forwarding logic is completely distinct from handling of load balanced services. The ingress-mode outbound proxy has been simplified to *require* the `l5d-dst-override` header and to fail non-HTTP communication. This ensures that the ingress-mode proxy does not unexpectedly revert to insecure communication. Finally, a regression was recently introduced that caused all proxy logs to be output with ANSI control characters. Logs are now output in plaintext by default --- * discover: replace `linkerd-channel` with `tokio-util` `PollSender` (linkerd/linkerd2-proxy#969) * replace `linkerd-channel` with `tokio-stream` (linkerd/linkerd2-proxy#970) * concurrency-limit: use `tokio-util`'s `PollSemaphore` (linkerd/linkerd2-proxy#968) * http: Do not fail fuzz tests when all IO is not read (linkerd/linkerd2-proxy#973) * transport: Fix orig-dst compilation on non-Linux targets (linkerd/linkerd2-proxy#974) * Update trust-dns to fix possible panic (linkerd/linkerd2-proxy#975) * addr: fix `to_http_authority` panic with IPv6 (linkerd/linkerd2-proxy#976) * outbound: skip logical stacks when no profile is discovered (linkerd/linkerd2-proxy#963) * split: change traffic splits to require a profile (linkerd/linkerd2-proxy#964) * inbound: only build profile route stacks when a profile is resolved (linkerd/linkerd2-proxy#966) * profiles: make receiver param in `route_request` non-optional (linkerd/linkerd2-proxy#967) * outbound: move target types into stack modules (linkerd/linkerd2-proxy#971) * outbound: only build logical stacks for profiles with logical addrs (linkerd/linkerd2-proxy#972) * app: inbound: add fuzzer (linkerd/linkerd2-proxy#977) * admin: Fail connections when HTTP detection fails (linkerd/linkerd2-proxy#979) * reduce error boilerplate with `thiserror` (linkerd/linkerd2-proxy#980) * app: inbound: fuzzer: generalise fuzzers and solve coverage build (linkerd/linkerd2-proxy#978) * admin: Assume meshed connections are HTTP/2 (linkerd/linkerd2-proxy#982) * chore: Fix deprecations on nightly (linkerd/linkerd2-proxy#983) * fuzz: Add logging to fuzz targets (linkerd/linkerd2-proxy#985) * fuzz: don't panic when the proxy closes the conn (linkerd/linkerd2-proxy#986) * Commit lock files for fuzzers (linkerd/linkerd2-proxy#984) * fuzz: allow client requests to fail (linkerd/linkerd2-proxy#989) * tower: update dependency to 0.4.7 (linkerd/linkerd2-proxy#990) * outbound: Make the Endpoint::logical_addr field optional (linkerd/linkerd2-proxy#991) * trace: explicitly disable ANSI terminal colors (linkerd/linkerd2-proxy#994) * ingress: Require the l5d-dst-override header (linkerd/linkerd2-proxy#992) * outbound: Do not support TCP-forwarding in ingress-mode (linkerd/linkerd2-proxy#995) * Decouple tcp forward stack from Endpoint target (linkerd/linkerd2-proxy#996) * Pickup linkerd-await wrapper in docker build (linkerd/linkerd2-proxy#999) * docs: Add fuzzing report (linkerd/linkerd2-proxy#1000) * tests: Use io::Error in mocked connector (linkerd/linkerd2-proxy#1001) * outbound: Decouple endpoint & logical stacks (linkerd/linkerd2-proxy#1002)
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
May 18, 2021
This release simplifies internals so that endpoint-forwarding logic is completely distinct from handling of load balanced services. The ingress-mode outbound proxy has been simplified to *require* the `l5d-dst-override` header and to fail non-HTTP communication. This ensures that the ingress-mode proxy does not unexpectedly revert to insecure communication. Finally, a regression was recently introduced that caused all proxy logs to be output with ANSI control characters. Logs are now output in plaintext by default --- * discover: replace `linkerd-channel` with `tokio-util` `PollSender` (linkerd/linkerd2-proxy#969) * replace `linkerd-channel` with `tokio-stream` (linkerd/linkerd2-proxy#970) * concurrency-limit: use `tokio-util`'s `PollSemaphore` (linkerd/linkerd2-proxy#968) * http: Do not fail fuzz tests when all IO is not read (linkerd/linkerd2-proxy#973) * transport: Fix orig-dst compilation on non-Linux targets (linkerd/linkerd2-proxy#974) * Update trust-dns to fix possible panic (linkerd/linkerd2-proxy#975) * addr: fix `to_http_authority` panic with IPv6 (linkerd/linkerd2-proxy#976) * outbound: skip logical stacks when no profile is discovered (linkerd/linkerd2-proxy#963) * split: change traffic splits to require a profile (linkerd/linkerd2-proxy#964) * inbound: only build profile route stacks when a profile is resolved (linkerd/linkerd2-proxy#966) * profiles: make receiver param in `route_request` non-optional (linkerd/linkerd2-proxy#967) * outbound: move target types into stack modules (linkerd/linkerd2-proxy#971) * outbound: only build logical stacks for profiles with logical addrs (linkerd/linkerd2-proxy#972) * app: inbound: add fuzzer (linkerd/linkerd2-proxy#977) * admin: Fail connections when HTTP detection fails (linkerd/linkerd2-proxy#979) * reduce error boilerplate with `thiserror` (linkerd/linkerd2-proxy#980) * app: inbound: fuzzer: generalise fuzzers and solve coverage build (linkerd/linkerd2-proxy#978) * admin: Assume meshed connections are HTTP/2 (linkerd/linkerd2-proxy#982) * chore: Fix deprecations on nightly (linkerd/linkerd2-proxy#983) * fuzz: Add logging to fuzz targets (linkerd/linkerd2-proxy#985) * fuzz: don't panic when the proxy closes the conn (linkerd/linkerd2-proxy#986) * Commit lock files for fuzzers (linkerd/linkerd2-proxy#984) * fuzz: allow client requests to fail (linkerd/linkerd2-proxy#989) * tower: update dependency to 0.4.7 (linkerd/linkerd2-proxy#990) * outbound: Make the Endpoint::logical_addr field optional (linkerd/linkerd2-proxy#991) * trace: explicitly disable ANSI terminal colors (linkerd/linkerd2-proxy#994) * ingress: Require the l5d-dst-override header (linkerd/linkerd2-proxy#992) * outbound: Do not support TCP-forwarding in ingress-mode (linkerd/linkerd2-proxy#995) * Decouple tcp forward stack from Endpoint target (linkerd/linkerd2-proxy#996) * Pickup linkerd-await wrapper in docker build (linkerd/linkerd2-proxy#999) * docs: Add fuzzing report (linkerd/linkerd2-proxy#1000) * tests: Use io::Error in mocked connector (linkerd/linkerd2-proxy#1001) * outbound: Decouple endpoint & logical stacks (linkerd/linkerd2-proxy#1002)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In #963 we began to split the outbound stack so that logical stack was
only built when a profile resolution was successful. However, this
incorrectly handled the case when a profile resolution included an
endpoint in its response.
This change introduces a
outbound::switch_logicalmodule that takes anoptional profile and builds either an endpoint stack or a logical stack.
Logical stacks always have a profile resolution and (named) logical
address. Endpoint stacks are built otherwise, optionally with metadata
from the profile resolution.
In order to test this--and to simplify/focus tests, in general--I've
moved tests from
outbound/src/{tcp,http}/tests.rsinto the modulesthey test. Now each test more narrowly targets the stacks whose logic
they test, rather than composing an entire outbound stack. It was
becoming cumbersome to support mocking each component of the larger
stack. Now, we more intentionaly test targeted logic in each stack
(including the newly fixed endpoint/logical stack switching).
Supersedes #988