destination: send Opaque protocol hint for opaque ports
#10301
Conversation
it doesn't actually need any of the changes in v0.8.0, but...why not...
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).
| @@ -402,6 +403,9 @@ func createWeightedAddr(address watcher.Address, opaquePorts map[uint32]struct{} | |||
| weightedAddr.ProtocolHint.OpaqueTransport = &pb.ProtocolHint_OpaqueTransport{ | |||
| InboundPort: port, | |||
| } | |||
| weightedAddr.ProtocolHint.Protocol = &pb.ProtocolHint_Opaque_{ | |||
| Opaque: &pb.ProtocolHint_Opaque{}, | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
I think this might read more clearly if we put the enableH2Upgrade block as an else if here. This helps communicate that these are mutually exclusive and that opaque takes precedence, rather than relying on ordering and overwriting. However, if we do that, we'll need to be a bit careful to make sure we properly handle the case when OpaqueProtocol is set but getInboundPort fails (what even IS the correct behavior in that case?).
There was a problem hiding this comment.
hmm, AFAICT if getInboundPort fails, that means that we couldn't parse a port from the pod spec's proxy container's env vars, or the pod doesn't have a proxy container. if there's no proxy at all, we shouldn't be enabling the opaque transport flag... but if the env var is malformed, what should we do? just skip the update entirely?
There was a problem hiding this comment.
Previously, if getInboundPort failed, we would log and leave the protocol hint set to H2 (if enabled). Not sure if that was the right behavior to begin with, but after this change the behavior would be to leave the protocol completely unset.
Would it make sense to still set ProtocolHint_Opaque, even if the getInboundPort fails? Is that a valid configuration? Can opaque traffic be sent without the tagged transport?
There was a problem hiding this comment.
Previously, if
getInboundPortfailed, we would log and leave the protocol hint set to H2 (if enabled). Not sure if that was the right behavior to begin with, but after this change the behavior would be to leave the protocol completely unset.
I think the new behavior is more correct. We should not be attempting an H2 upgrade if targeting a port that's marked as opaque, since we know that the proxy on the receiving end will not attempt a downgrade back to HTTP/1.
Would it make sense to still set ProtocolHint_Opaque, even if the getInboundPort fails? Is that a valid configuration? Can opaque traffic be sent without the tagged transport?
The Opaque protocol hint is meaningless without tagged transport, as it only configures what SessionProtocol is sent when using tagged transport. perhaps we should still send it in this case, but we should not be populating the ProtocolHint.OpaqueTransport field, as we don't know the port that connections should be directed to, and therefore cannot use opaque transport.
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>
The outbound proxy handles endpoints with the
opaque_transportflag byopening a direct connection to the inbound proxy's inbound listener
port, and sending a ProtoBuf
TransportHeaderincluding the target portof the originating outbound connection and an (optional)
SessionProtocoldescribing the protocol used on that connection.Currently, outbound proxies initiating direct connections will always
send
SessionProtocolvalues communicating the protocol as understoodby the outbound proxy. However, this is not always the desired behavior.
Direct connections with
TransportHeaders are used in two cases: forgateway connections, and for ports which are marked as opaque. When the
inbound port is marked as opaque, the presence of a
SessionProtocoltells 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
Destinationproxy API has been updated to add a newProtocolHint,Opaque, which indicates that an outbound proxy shouldnot send a
SessionProtocolwhen initiating a direct connection, evenif 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-apiv0.8.0.This branch updates the Destination controller's dependency on
linkerd2-proxy-apito v0.8.0, and changes the controller to send anOpaqueprotocol hint when the target port is marked as opaque on thedestination pod. This should override the
H2protocol hint that isadded 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
Opaqueprotocol hint).