handle `Opaque` protocol hints on endpoints by hawkw · Pull Request #2209 · linkerd/linkerd2-proxy

hawkw · 2023-02-02T21:19:58Z

Currently, when the outbound proxy makes a direct connection prefixed
with a TransportHeader in order to send HTTP traffic, it will always
send a SessionProtocol hint with the HTTP version as part of the
header. This instructs the inbound proxy to use that protocol, even if
the target port has a ServerPolicy that marks that port as opaque, which
can result in incorrect handling of that connection. See
linkerd/linkerd2#9888 for details.

In order to prevent this, linkerd/linkerd2-proxy-api#197 adds a new
ProtocolHint value to the protobuf endpoint metadata message. This
will allow the Destination controller to explicitly indicate to the
outbound proxy that a given endpoint is known to handle all connections
to a port as an opaque TCP stream, and that the proxy should not perform
a protocol upgrade or send a SessionProtocol in the transport header.
This branch updates the proxy to handle this new hint value, and adds
tests that the outbound proxy behaves as expected.

Once the Destination controller is updated to send this ProtocolHint
for ports that are marked as opaque on the inbound side, this will fix
linkerd/linkerd2#9888.

This branch is currently a draft because the linkerd2-proxy-api
changes have not been published yet. Once a new version of the proxy API
is published, we'll update this branch to depend on that, and this can
be merged.

The outbound proxy handles endpoints with the `opaque_transport` flag by opening a direct connection to the inbound proxy's inbound listener port, and sending a ProtoBuf `TransportHeader` including the target port of the originating outbound connection and an (optional) `SessionProtocol` describing the protocol used on that connection. Currently, outbound proxies initiating direct connections will *always* send `SessionProtocol` values communicating the protocol as understood by the outbound proxy. However, this is not always the desired behavior. Direct connections with `TransportHeader`s are used in two cases: for gateway connections, and for ports which are marked as opaque. When the inbound port is marked as opaque, the presence of a `SessionProtocol` tells the inbound proxy to handle that connection as the indicated protocol, which results in incorrect behavior when the inbound proxy's ServerPolicy configures the target port as opaque (see #9888). Therefore, the `Destination` proxy API has been updated to add a new `ProtocolHint`, `Opaque`, which indicates that an outbound proxy should _not_ send a `SessionProtocol` when initiating a direct connection, even if the outbound proxy handled the connection as HTTP. This hint was added to the proxy API in linkerd/linkerd2-proxy-api#197, and released in `linkerd2-proxy-api` v0.8.0. This branch updates the Destination controller's dependency on `linkerd2-proxy-api` to v0.8.0, and changes the controller to send an `Opaque` protocol hint when the target port is marked as opaque on the destination pod. This should override the `H2` protocol hint that is added when the destination is meshed. I've also added a new test for this behavior. Fixes #9888 (along with linkerd/linkerd2-proxy#2209, which changes the proxy to actually handle the `Opaque` protocol hint).

mateiidavid

Nice, it looks good to me.

linkerd/app/integration/src/tests/direct.rs

Cargo.toml

linkerd/proxy/api-resolve/src/metadata.rs

olix0r

This is going to conflict with the other changes i have in flight so I'm going to have to be a little careful about merging this.

How can we make the change smaller? For instance, there's a lot of test infra change in this PR that i haven't really parsed.

olix0r · 2023-02-13T18:21:27Z

linkerd/app/outbound/src/tcp/opaque_transport.rs

+    fn http_ep(metadata: Metadata, version: http::Version) -> crate::http::Connect {
+        let e = crate::http::Endpoint::from((version, ep(metadata)));
+        crate::http::Connect::from((version, e))
+    }


This stuff is going to conflict with other work, especially. Can you decouple the tests from the specific target types used in the stack?

Okay, 7557676 changes these tests so that they no longer use the actual target types --- is that sufficient?

IMO, this is somewhat unfortunate, as the target types' Param impls were part of the code being tested here, and now, we need to ensure that the targets used in the tests actually behave the same as the real targets. But, hopefully that reduces the conflict with other changes?

Look at what's coming in
https://github.com/linkerd/linkerd2-proxy/pull/2210/files#diff-397930556d70c6560dae17b17cf26153f2b1398664e6e7b46112d5135a1ec4bb -- we don't have any reason to use Metadata at all.

IMO, this is somewhat unfortunate, as the target types' Param impls were part of the code being tested here,

This decoupling is intended to force this conversation. Why are these tests coupled to the caller context? Do we need tests in the caller module(s) that exercise how they set their parameters? Or does this indicate that the params are not factored properly so that the inputs to this stack are too fine grain?

hawkw · 2023-02-13T18:29:57Z

For instance, there's a lot of test infra change in this PR that i haven't really parsed.

We can remove the integration test changes if we don't care about having integration tests for this case; I wrote that initially while trying to reproduce the issue. If it's more desirable to have a smaller branch, I can back that out for now and we can consider merging those changes separately (or not at all)?

Co-authored-by: Matei David <matei@buoyant.io>

Co-authored-by: Oliver Gould <ver@buoyant.io>

olix0r · 2023-02-13T21:35:10Z

We can remove the integration test changes if we don't care about having integration tests for this case

My point was kind of the opposite: there are probably some parts of this that are relatively easy/uncontroversial to merge that don't actually touch the outbound stack. The outbound stack changes need to be considered in the scope of other changes, while much of the other changes (proxy-api bump, test infra stuff, etc) do not.

hawkw · 2023-02-13T21:43:50Z

We can remove the integration test changes if we don't care about having integration tests for this case

My point was kind of the opposite: there are probably some parts of this that are relatively easy/uncontroversial to merge that don't actually touch the outbound stack. The outbound stack changes need to be considered in the scope of other changes, while much of the other changes (proxy-api bump, test infra stuff, etc) do not.

Ooooh, gotcha. I'll pull that stuff out, then.

olix0r · 2023-02-14T03:49:36Z

#2226 includes some of those test changes

Currently, updates sent by the mock destination controller used in integration tests are constructed by a number of free functions with names like `destination_add_tls` and `destination_add_h2_hinted` and so on. These functions determine different aspects of the generated destination updates, such as protocol hints, labels, and TLS identity. This works acceptably for the current integration tests. However, in order to add new tests, it may be desirable to add additional destination configurations. For example, for testing the handling of various protocol hints on direct connections, it's necessary to set both a protocol hint *and* a TLS identity (direct connections require TLS). The current approach has separate functions for constructing updates with a protocol hint and updates with TLS. In order to make the construction of test destinations more flexible, this PR replaces the current approach with a builder. This way, the logic for constructing updates with different attributes can be composed more easily, without having to implement separate constructors for the entire matrix of (protocol hint, TLS identity, addr labels, addr set labels) combinations. This was factored out of PR #2209 so that the test framework changes can be merged separately.

Depends on #2232 This branch adds a new integration test for direct connections transporting hinted HTTP/2. This was factored out from #2209. I did *not* add the test that actually reproduces linkerd/linkerd2#9888 in this PR, as it would fail.

Currently, updates sent by the mock destination controller used in integration tests are constructed by a number of free functions with names like `destination_add_tls` and `destination_add_h2_hinted` and so on. These functions determine different aspects of the generated destination updates, such as protocol hints, labels, and TLS identity. This works acceptably for the current integration tests. However, in order to add new tests, it may be desirable to add additional destination configurations. For example, for testing the handling of various protocol hints on direct connections, it's necessary to set both a protocol hint *and* a TLS identity (direct connections require TLS). The current approach has separate functions for constructing updates with a protocol hint and updates with TLS. In order to make the construction of test destinations more flexible, this PR replaces the current approach with a builder. This way, the logic for constructing updates with different attributes can be composed more easily, without having to implement separate constructors for the entire matrix of (protocol hint, TLS identity, addr labels, addr set labels) combinations. This was factored out of PR #2209 so that the test framework changes can be merged separately.

This branch adds a new integration test for direct connections transporting hinted HTTP/2. This was factored out from #2209. I did *not* add the test that actually reproduces linkerd/linkerd2#9888 in this PR, as it would fail.

hawkw · 2023-02-15T22:55:47Z

Okay, I've opened a new PR for this change (#2237), which is updated to track the latest changes on main. I'm going to close this branch in favor of that one.

Currently, when the outbound proxy makes a direct connection prefixed with a `TransportHeader` in order to send HTTP traffic, it will always send a `SessionProtocol` hint with the HTTP version as part of the header. This instructs the inbound proxy to use that protocol, even if the target port has a ServerPolicy that marks that port as opaque, which can result in incorrect handling of that connection. See linkerd/linkerd2#9888 for details. In order to prevent this, linkerd/linkerd2-proxy-api#197 adds a new `ProtocolHint` value to the protobuf endpoint metadata message. This will allow the Destination controller to explicitly indicate to the outbound proxy that a given endpoint is known to handle all connections to a port as an opaque TCP stream, and that the proxy should not perform a protocol upgrade or send a `SessionProtocol` in the transport header. This branch updates the proxy to handle this new hint value, and adds tests that the outbound proxy behaves as expected. Along with linkerd/linkerd2#10301, this will fix linkerd/linkerd2#9888. I opened a new PR for this change rather than attempting to rebase my previous PR #2209, as it felt a bit easier to start with a new branch and just make the changes that were still relevant. Therefore, this closes #2209.

The outbound proxy handles endpoints with the `opaque_transport` flag by opening a direct connection to the inbound proxy's inbound listener port, and sending a ProtoBuf `TransportHeader` including the target port of the originating outbound connection and an (optional) `SessionProtocol` describing the protocol used on that connection. Currently, outbound proxies initiating direct connections will *always* send `SessionProtocol` values communicating the protocol as understood by the outbound proxy. However, this is not always the desired behavior. Direct connections with `TransportHeader`s are used in two cases: for gateway connections, and for ports which are marked as opaque. When the inbound port is marked as opaque, the presence of a `SessionProtocol` tells the inbound proxy to handle that connection as the indicated protocol, which results in incorrect behavior when the inbound proxy's ServerPolicy configures the target port as opaque (see #9888). Therefore, the `Destination` proxy API has been updated to add a new `ProtocolHint`, `Opaque`, which indicates that an outbound proxy should _not_ send a `SessionProtocol` when initiating a direct connection, even if the outbound proxy handled the connection as HTTP. This hint was added to the proxy API in linkerd/linkerd2-proxy-api#197, and released in `linkerd2-proxy-api` v0.8.0. This branch updates the Destination controller's dependency on `linkerd2-proxy-api` to v0.8.0, and changes the controller to send an `Opaque` protocol hint when the target port is marked as opaque on the destination pod. This should override the `H2` protocol hint that is added when the destination is meshed. I've also added a new test for this behavior. Fixes #9888 (along with linkerd/linkerd2-proxy#2209, which changes the proxy to actually handle the `Opaque` protocol hint).

The outbound proxy handles endpoints with the `opaque_transport` flag by opening a direct connection to the inbound proxy's inbound listener port, and sending a ProtoBuf `TransportHeader` including the target port of the originating outbound connection and an (optional) `SessionProtocol` describing the protocol used on that connection. Currently, outbound proxies initiating direct connections will *always* send `SessionProtocol` values communicating the protocol as understood by the outbound proxy. However, this is not always the desired behavior. Direct connections with `TransportHeader`s are used in two cases: for gateway connections, and for ports which are marked as opaque. When the inbound port is marked as opaque, the presence of a `SessionProtocol` tells the inbound proxy to handle that connection as the indicated protocol, which results in incorrect behavior when the inbound proxy's ServerPolicy configures the target port as opaque (see #9888). Therefore, the `Destination` proxy API has been updated to add a new `ProtocolHint`, `Opaque`, which indicates that an outbound proxy should _not_ send a `SessionProtocol` when initiating a direct connection, even if the outbound proxy handled the connection as HTTP. This hint was added to the proxy API in linkerd/linkerd2-proxy-api#197, and released in `linkerd2-proxy-api` v0.8.0. This branch updates the Destination controller's dependency on `linkerd2-proxy-api` to v0.8.0, and changes the controller to send an `Opaque` protocol hint when the target port is marked as opaque on the destination pod. This should override the `H2` protocol hint that is added when the destination is meshed. I've also added a new test for this behavior. Fixes #9888 (along with linkerd/linkerd2-proxy#2209, which changes the proxy to actually handle the `Opaque` protocol hint). Signed-off-by: Eric Anderson <eric@buoyant.io>

hawkw changed the title ~~add test reproducing linkerd/linkerd2#9888~~ handle Opaque protocol hints on endpoints Feb 7, 2023

This was referenced Feb 7, 2023

rename OpaqueTransport to TaggedTransport #2212

Closed

Proxy returns 404 when HTTP connection is marked as opaque linkerd/linkerd2#9888

Closed

hawkw marked this pull request as ready for review February 8, 2023 23:22

hawkw requested a review from a team as a code owner February 8, 2023 23:22

hawkw mentioned this pull request Feb 9, 2023

destination: send Opaque protocol hint for opaque ports linkerd/linkerd2#10301

Merged

mateiidavid approved these changes Feb 13, 2023

View reviewed changes

linkerd/app/integration/src/tests/direct.rs Show resolved Hide resolved

linkerd/app/integration/src/tests/direct.rs Show resolved Hide resolved

hawkw requested a review from olix0r February 13, 2023 17:19

olix0r reviewed Feb 13, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

olix0r reviewed Feb 13, 2023

View reviewed changes

linkerd/proxy/api-resolve/src/metadata.rs Outdated Show resolved Hide resolved

olix0r reviewed Feb 13, 2023

View reviewed changes

hawkw and others added 14 commits February 13, 2023 10:39

try to reproduce linkerd/linkerd2#9888

ad0273f

actually repro the bug

936064a

outbound does not send a SessionProtocol to opaque ports

f1be3d7

rename tests

182e9ed

add unit tests for opaque hints

55dd35e

add unit tests for session protocols

2f97fc6

clippy placation

369a0a1

fix module renaming

fed14e1

rustfmt

52a5c77

use published proxy-api

a86f0e0

Update linkerd/app/integration/src/tests/direct.rs

92d3975

Co-authored-by: Matei David <matei@buoyant.io>

Update linkerd/proxy/api-resolve/src/metadata.rs

6a394dc

Co-authored-by: Oliver Gould <ver@buoyant.io>

fix trailing whitespace

2a5b102

remove integration test changes

86c0f04

hawkw force-pushed the eliza/9888 branch from 05ddf6d to 86c0f04 Compare February 13, 2023 18:40

use fake endpoint types in tests

7557676

hawkw requested a review from olix0r February 13, 2023 19:03

hawkw mentioned this pull request Feb 14, 2023

integration: add destination update builders #2232

Merged

hawkw mentioned this pull request Feb 15, 2023

integration: add test for direct HTTP connections #2234

Merged

update to track latest main

6f8a968

hawkw mentioned this pull request Feb 15, 2023

outbound: handle Opaque protocol hints on endpoints #2237

Merged

hawkw closed this Feb 15, 2023

olix0r deleted the eliza/9888 branch March 7, 2023 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle `Opaque` protocol hints on endpoints#2209

handle `Opaque` protocol hints on endpoints#2209
hawkw wants to merge 16 commits intomainfrom
eliza/9888

hawkw commented Feb 2, 2023 •

edited

Loading

Uh oh!

mateiidavid left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olix0r left a comment

Uh oh!

olix0r Feb 13, 2023

Uh oh!

hawkw Feb 13, 2023

Uh oh!

olix0r Feb 13, 2023

Uh oh!

hawkw commented Feb 13, 2023

Uh oh!

olix0r commented Feb 13, 2023

Uh oh!

hawkw commented Feb 13, 2023

Uh oh!

olix0r commented Feb 14, 2023

Uh oh!

hawkw commented Feb 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

hawkw commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mateiidavid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olix0r left a comment

Choose a reason for hiding this comment

Uh oh!

olix0r Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

hawkw Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

olix0r Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

hawkw commented Feb 13, 2023

Uh oh!

olix0r commented Feb 13, 2023

Uh oh!

hawkw commented Feb 13, 2023

Uh oh!

olix0r commented Feb 14, 2023

Uh oh!

hawkw commented Feb 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hawkw commented Feb 2, 2023 •

edited

Loading