Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

outbound: only build logical stacks for profiles with logical addrs #972

Merged
merged 40 commits into from
Apr 14, 2021
Merged

outbound: only build logical stacks for profiles with logical addrs #972

merged 40 commits into from
Apr 14, 2021

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Apr 9, 2021

Depends on #971

Currently, the proxy should only perform Destination.Get resolutions
for logical (named) addresses provided by Destination.GetProfile
lookups, and never for IP addresses. However, this is not guaranteed;
as the destination resolver still accepts a LookupAddr type which may
be either a logical named address or an IP address.

In order to remove support for IP lookups from the control plane's
Destination.Get implementation, we should ensure that the proxy will
never attempt to resolve an IP (see linkerd/linkerd2#5246). Now that
the Logical target type and the logical stacks are only constructed
when a profile is resolved (as of PR #963), we can now change the proxy
to ensure statically that this never happens.

This branch makes the following changes:

  • Currently (as of outbound: skip logical stacks when no profile is discovered #963), the Logical target type has an
    Option<LogicalAddr>. We attempt to unwrap the Option<Receiver>
    returned by a profile lookup, and build a logical stack when there is
    a profile. The Logical target is with Option<LogicalAddr> from
    that profile.

    However, this is not actually the ideal behavior: ideally, we would
    only contruct the logical stack when there is a resolved profile and
    the profile includes a logical address. Therefore, this branch changes
    the push_unwrap_logical function added in outbound: skip logical stacks when no profile is discovered #963 to use a custom
    filter predicate that only constructs the Logical target when the
    profile resolution is Some and the profile's addr field is
    Some. Now, Logical targets are only built when there is a known
    logical address.

  • The Destination.Get resolver requires a target type that implements
    Param<ConcreteAddr>. Currently, a ConcreteAddr is a newtype around
    Addr, which may be either a named address or an IP address. This
    branch changes ConcreteAddr to a newtype around NameAddr. This
    means that the resolver now ensures Destination.Get queries are only
    performed for named addresses at the type level.

  • Similarly, the Target type in linkerd-service-profiles is changed
    to store a NameAddr rather than an Addr.

  • Several of the outbound stack tests now test invalid behavior that
    should no longer happen --- building a logical stack and calling it
    with a target that doesn't include a logical address. I rewrote these
    tests so that the tests without logical addresses don't build a
    logical stack, and added a new test for calling the logical stack
    with a logical address. In the process, I did some cleanup and
    refactoring in the outbound TCP stack tests.

  • Finally, while debugging test failures, I noticed that the
    fmt::Debug output for some of the target types that include
    NameAddrs was a little unnecesarily verbose. I added new custom
    Debug impls that should make these a bit more concise.

This is the proxy component of linkerd/linkerd2#5246.

Signed-off-by: Eliza Weisman eliza@buoyant.io

hawkw added 30 commits April 2, 2021 12:43
@hawkw
EVERYTHING (except for ingress and tests) COMPILES
182370c 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
make TCP tests worky
742e170 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
http tests also worky now!
dd859df
@hawkw
un-comment all of ingress (oops)
f2d3d06 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
fix backwards if in detect::SkipByProfile
660b218 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
s/push_logical/push_profile
7756ff4 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
ingress compiles???
f33e6b6 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
clarify naming
859db4a 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
clearer naming in detect
eae1839 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
comments
57d2242 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
make clippy happy
5977345 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
bleh missed one
d5e8a6b 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Merge branch 'main' into eliza/require-profile
ae90726 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
review feedback
20a4e40 
(thanks @kleimkuhler)

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
undo @kleimkulher's suggestion
305f160 
sorry kevin it doesn't compile

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
whoopsy redundant clone
4550239 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
split: change traffic splits to require a profile
59233fe 
This branch builds on #963 by changing the traffic split service to
require its target type implement `Param<profiles::Receiver>` rather
than requiring `Param<Option<profiles::Receiver>>`. This lets us
simplify the implementation significantly by removing the "default" case
that's built when no profile was discovered.

Depends on #963

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
ConcreteAddrs are now always available
ce8905a 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
split: change traffic splits to require a profile
f7fb6bc 
This branch builds on #963 by changing the traffic split service to
require its target type implement `Param<profiles::Receiver>` rather
than requiring `Param<Option<profiles::Receiver>>`. This lets us
simplify the implementation significantly by removing the "default" case
that's built when no profile was discovered.

Depends on #963

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
ConcreteAddrs are now always available
4720c62 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
inbound: only build profile route stacks when a profile is resolved
b0556ff 
This is similar to PR #963 but on the inbound side. Since the inbound
proxy only requires logical targets in the HTTP profile route layers,
this change is much simpler --- protocol detection etc are all above the
profile resolution layer. We can simply add an `UnwrapOr` layer that
skips the profile route layers.

This change is much less important than the corresponding outbound
change, but the two changes together will permit us to simplify the
profile route layer by taking a `Param<profiles::Receiver>` rather than
a `Param<Option<profiles::Receiver>>`.

This does also require adding an additional `BoxResponse` layer to erase
differing response body types based on whether or not profile route
metrics are recorded.
@hawkw
Merge branch 'eliza/inbound-require-profile' into eliza/require-profi…
2129f80 
…le-iii
@hawkw
profiles: make receiver param in route_request non-optional
d345ee4 
This branch changes the `linkerd_service_profiles::http::route_request`
layer to require a `Param<profiles::Receiver>` rather than a
`Param<Option<profiles::Receiver>>` target type. This is possible
because the inbound and outbound logical destination stacks are now only
built when a profile is resolved (as of PRs #963 and #966).

Depends on #963
Depends on #966
@hawkw
outbound: move target types into stack modules
913cae3 
Depends on #967

This branch refactors the `linkerd-app-outbound` crate so that each
stack target type is now defined in the module for the stack that uses
it, rather than in one big `target` module. For example, `Logical` is
now defined in `logical.rs` and `Endpoint` is defined in `endpoint.rs`.

There should be no functional change in this PR --- it just moves code
around.
@hawkw
WIP remove ip addrs from concrete
ab00911 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
everything except ingress
593810f 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
fix ingress
4ccda73 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
put all the unwrapping in one place
df58f2d 
IMO this is nicer even if it does involve a `From` impl with a 3-tuple
in it :/

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
don't log watch receivers in log messages
7e86061 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
less wordy Debug format for name addrs
336ce32 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added 2 commits April 9, 2021 11:31
@hawkw
fixup tests
f9aac1c 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
make test actually do what it says it does
fae850f 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested review from olix0r and a team April 9, 2021 19:13
@hawkw hawkw self-assigned this Apr 9, 2021
Pothulapati added a commit to linkerd/linkerd2 that referenced this pull request Apr 12, 2021
@Pothulapati
destination: Remove support for IP Queries in Get API
b08e213 
Fixes #5246

This PR updates the destination to report an error when `Get`
is called for IP Queries. As the issue mentions, The proxies
are not using this API anymore and it helps to simplify and
remove unecessary logic.

This change is aimed for `stable-2.11`, and depends on
linkerd/linkerd2-proxy#972

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati Pothulapati mentioned this pull request Apr 12, 2021
Merged
kleimkuhler
kleimkuhler approved these changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few small comments but otherwise looks good.

linkerd/app/outbound/src/logical.rs Show resolved Hide resolved
linkerd/app/outbound/src/tcp/tests.rs Show resolved Hide resolved
@hawkw
outbound: move target types into stack modules
c3f5d48 
Depends on #967

This branch refactors the `linkerd-app-outbound` crate so that each
stack target type is now defined in the module for the stack that uses
it, rather than in one big `target` module. For example, `Logical` is
now defined in `logical.rs` and `Endpoint` is defined in `endpoint.rs`.

There should be no functional change in this PR --- it just moves code
around.
@hawkw hawkw force-pushed the eliza/require-profile-iv branch from 913cae3 to c3f5d48 Compare April 13, 2021 22:25
Base automatically changed from eliza/require-profile-iv to main April 13, 2021 22:46
olix0r
olix0r reviewed Apr 14, 2021
View reviewed changes
Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good. A few comments

linkerd/app/gateway/src/gateway.rs Outdated Show resolved Hide resolved
linkerd/app/gateway/src/lib.rs Outdated Show resolved Hide resolved
linkerd/app/inbound/src/http/mod.rs Outdated Show resolved Hide resolved
linkerd/app/outbound/src/logical.rs Show resolved Hide resolved
linkerd/app/outbound/src/logical.rs Outdated Show resolved Hide resolved
hawkw and others added 4 commits April 14, 2021 09:30
@hawkw @olix0r
Update linkerd/app/gateway/src/gateway.rs
a2406ce 
Co-authored-by: Oliver Gould <ver@buoyant.io>
@hawkw
inline unwrap_logical predicate
a1c478b 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
undo accidental whitespace change
be69a64
@hawkw
review feedback
3836595 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested a review from olix0r April 14, 2021 16:49
@hawkw
clippy placation
a8efe5f 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw merged commit 94c7796 into main Apr 14, 2021
@hawkw hawkw deleted the eliza/concrete-name-addrs branch April 14, 2021 17:21
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Apr 21, 2021
@olix0r
proxy: v2.142.0
c52d07f 
This release primarily improves protocol detection error messages in the
admin server so that logs clearly indicate when the client expected a
different TLS server identity than that of the running proxy.

A number of internal improvements have been made, especially eliminating
some potential runtime panics detected by oss-fuzz. It is not expected
that these panics could be triggered in typical cluster configurations.

---

* 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)
@olix0r olix0r mentioned this pull request Apr 21, 2021
Closed
olix0r added a commit to linkerd/linkerd2 that referenced this pull request May 12, 2021
@olix0r
proxy: v2.143.0
0d6eada 
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 olix0r mentioned this pull request May 12, 2021
Merged
olix0r added a commit to linkerd/linkerd2 that referenced this pull request May 18, 2021
@olix0r
proxy: v2.143.0 (#6107)
847b744 
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
Reviewers

@kleimkuhler kleimkuhler kleimkuhler approved these changes

@olix0r olix0r olix0r approved these changes

Assignees

@hawkw hawkw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hawkw @olix0r @kleimkuhler