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.
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
quic: support for server-preferred address behind DNAT #33774
Changes from 2 commits
7ca7daf
78abeb1
bb84625
e25ccb4
93f21e6
b8a812b
b54cc44
2a1b89c
b21946f
6eb2c6c
f006088
19c2d66
cf30494
c7bf130
81b10a2
9c8cfba
d8adc70
59ded87
8c7e972
155e2ef
40d2ab5
81241b7
39ca689
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -2,6 +2,8 @@ syntax = "proto3"; | |||
|
||||
package envoy.extensions.quic.server_preferred_address.v3; | ||||
|
||||
import "envoy/config/core/v3/address.proto"; | ||||
|
||||
import "xds/annotations/v3/status.proto"; | ||||
|
||||
import "udpa/annotations/status.proto"; | ||||
|
@@ -16,20 +18,51 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |||
// [#extension: envoy.quic.server_preferred_address.fixed] | ||||
|
||||
// Configuration for FixedServerPreferredAddressConfig. | ||||
// [#next-free-field: 7] | ||||
message FixedServerPreferredAddressConfig { | ||||
// [#comment:TODO(danzh2010): discuss with API shepherds before removing WiP status.] | ||||
|
||||
option (xds.annotations.v3.message_status).work_in_progress = true; | ||||
|
||||
oneof ipv4_type { | ||||
// String representation of IPv4 address, i.e. "127.0.0.2". | ||||
// The listener's port will be used. | ||||
// If not specified, none will be configured. | ||||
string ipv4_address = 1; | ||||
|
||||
// The IPv4 address to advertise to clients for Server Preferred Address. | ||||
config.core.v3.SocketAddress ipv4_address_and_port = 3; | ||||
} | ||||
|
||||
oneof ipv4_dnat_type { | ||||
// If there is a DNAT between the client and Envoy, the address that Envoy will observe | ||||
// server preferred address IPv4 packets being sent to. If this is not specified, it is assumed | ||||
// there is no DNAT and the server preferred address packets will be sent to the address advertised | ||||
// to clients for server preferred address. | ||||
// | ||||
// String representation of IPv4 address, i.e. "127.0.0.2" | ||||
// The listener's port will be used. | ||||
string ipv4_dnat_address = 5; | ||||
} | ||||
|
||||
oneof ipv6_type { | ||||
// String representation of IPv6 address, i.e. "::1". | ||||
// The listener's port will be used. | ||||
// If not specified, none will be configured. | ||||
string ipv6_address = 2; | ||||
|
||||
// The IPv6 address to advertise to clients for Server Preferred Address. | ||||
config.core.v3.SocketAddress ipv6_address_and_port = 4; | ||||
} | ||||
|
||||
oneof ipv6_dnat_type { | ||||
// If there is a DNAT between the client and Envoy, the address that Envoy will observe | ||||
// server preferred address IPv6 packets being sent to. If this is not specified, it is assumed | ||||
// there is no DNAT and the server preferred address packets will be sent to the address advertised | ||||
// to clients for server preferred address. | ||||
// | ||||
// String representation of IPv6 address, i.e. "::1" | ||||
// The listener's port will be used. | ||||
string ipv6_dnat_address = 6; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Line 63 in 21b406b
Is it necessary in this case? Do we anticipate more fields to select from? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to mirror the other field here. It'll probably need a similar set of options as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking about this a little further, I think using the config.core.v3.Address is a better type for this data, and in the cases where we currently cannot specify a port, ignore it (or throw a validation error if it isn't zero). Then we can deprecate the existing string version, and stop using the oneof's at all. Wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @abeyad and @RyanTheOptimist would it be better like this (plus the equivalent for ipv6)?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to make sure I understand,
is that correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That SGTM, thanks @ggreenway ! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need port in non-DNAT case? Even in DNAT case, the client doesn't see a different port right? Only the translation unit will translate [SPA:443] to something else [configured_address:port], is this correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the DNAT is translating both IP and port, then the client-advertised address may need a different port than the listener. Also, I'm considering adding support for quic listeners to support There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. QUICHE doesn't support multiple writers for a server connection. Is there a use case for advertising a different port for SPA? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use case is a host in AWS with a single NIC and IP, with both an NLB for initial connection load balancing, and an external EIP for SPA traffic. If the two paths go to different ports, envoy/quiche can easily differentiate which path the traffic is coming from. If they go to the same port, envoy/quiche sees the client attempting SPA as the client trying to do a voluntary migration during the initial handshake. |
||||
} | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,14 +15,28 @@ class EnvoyQuicServerPreferredAddressConfig { | |
public: | ||
virtual ~EnvoyQuicServerPreferredAddressConfig() = default; | ||
|
||
struct Addresses { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Brief comment, please. |
||
// Addresses that client is requested to use. | ||
quic::QuicSocketAddress ipv4_; | ||
quic::QuicSocketAddress ipv6_; | ||
|
||
// If destination NAT is applied between the client and Envoy, the addresses that | ||
// Envoy will see for client traffic to the server preferred address. If this is not | ||
// set, Envoy will expect to receive server preferred address traffic on the above addresses. | ||
// | ||
// A DNAT address will be ignored if the corresponding SPA address is not set. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is super clear. Thanks! |
||
quic::QuicSocketAddress dnat_ipv4_; | ||
quic::QuicSocketAddress dnat_ipv6_; | ||
}; | ||
|
||
/** | ||
* Called during config loading. | ||
* @param local_address the configured default listening address. | ||
* Returns a pair of the server preferred addresses in form of {IPv4, IPv6} which will be used for | ||
* the entire life time of the QUIC listener. An uninitialized address value means no preferred | ||
* address for that address family. | ||
*/ | ||
virtual std::pair<quic::QuicSocketAddress, quic::QuicSocketAddress> | ||
virtual Addresses | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this interface needs to be changed? And why the proto struct is more preferrable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it is now returning 4 addresses instead of two. And a |
||
getServerPreferredAddresses(const Network::Address::InstanceConstSharedPtr& local_address) PURE; | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,75 @@ | ||
#include "source/extensions/quic/server_preferred_address/fixed_server_preferred_address_config.h" | ||
|
||
#include "source/common/network/utility.h" | ||
#include "source/common/quic/envoy_quic_utils.h" | ||
|
||
namespace Envoy { | ||
namespace Quic { | ||
|
||
std::pair<quic::QuicSocketAddress, quic::QuicSocketAddress> | ||
namespace { | ||
|
||
quic::QuicSocketAddress | ||
ipOrAddressToAddress(const FixedServerPreferredAddressConfig::QuicSocketOrIpAddress& address, | ||
int32_t port) { | ||
return absl::visit( | ||
[&](const auto& arg) -> quic::QuicSocketAddress { | ||
using T = std::decay_t<decltype(arg)>; | ||
if constexpr (std::is_same_v<T, quic::QuicSocketAddress>) { | ||
return arg; | ||
} else if constexpr (std::is_same_v<T, quic::QuicIpAddress>) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Since each of these branches is returning, let's remove the "} else" which in particular removes the indentation for the last case. |
||
return quic::QuicSocketAddress(arg, port); | ||
} else { | ||
IS_ENVOY_BUG(fmt::format("Unhandled type in variant visitor: {}", typeof(arg))); | ||
return {}; | ||
} | ||
}, | ||
address); | ||
} | ||
|
||
quic::QuicIpAddress parseIp(const std::string& addr, absl::string_view version, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: "version" => "ip_version" or "address_family" |
||
const Protobuf::Message& message) { | ||
quic::QuicIpAddress ip; | ||
if (!ip.FromString(addr)) { | ||
ProtoExceptionUtil::throwProtoValidationException( | ||
absl::StrCat("bad ", version, " server preferred address: ", addr), message); | ||
} | ||
return ip; | ||
} | ||
|
||
quic::QuicSocketAddress parseSocketAddress(const envoy::config::core::v3::SocketAddress& addr, | ||
Network::Address::IpVersion version, | ||
absl::string_view version_str, | ||
const Protobuf::Message& message) { | ||
// There's no utility to convert from a `SocketAddress`, so wrap it in an `Address` to make use of | ||
// existing helpers. | ||
envoy::config::core::v3::Address outer; | ||
*outer.mutable_socket_address() = addr; | ||
auto envoy_addr = Network::Utility::protobufAddressToAddress(outer); | ||
if (envoy_addr == nullptr) { | ||
ProtoExceptionUtil::throwProtoValidationException( | ||
absl::StrCat("bad ", version_str, " server preferred address: ", addr), message); | ||
} | ||
if (envoy_addr->ip() == nullptr || envoy_addr->ip()->version() != version) { | ||
ProtoExceptionUtil::throwProtoValidationException( | ||
absl::StrCat("wrong address type for ", version_str, " server preferred address: ", addr), | ||
message); | ||
} | ||
|
||
return envoyIpAddressToQuicSocketAddress(envoy_addr->ip()); | ||
} | ||
|
||
} // namespace | ||
|
||
EnvoyQuicServerPreferredAddressConfig::Addresses | ||
FixedServerPreferredAddressConfig::getServerPreferredAddresses( | ||
const Network::Address::InstanceConstSharedPtr& local_address) { | ||
int32_t port = local_address->ip()->port(); | ||
return {quic::QuicSocketAddress(ip_v4_, port), quic::QuicSocketAddress(ip_v6_, port)}; | ||
Addresses addresses; | ||
addresses.ipv4_ = ipOrAddressToAddress(ip_v4_, port); | ||
addresses.ipv6_ = ipOrAddressToAddress(ip_v6_, port); | ||
addresses.dnat_ipv4_ = quic::QuicSocketAddress(dnat_ip_v4_, port); | ||
addresses.dnat_ipv6_ = quic::QuicSocketAddress(dnat_ip_v6_, port); | ||
return addresses; | ||
} | ||
|
||
Quic::EnvoyQuicServerPreferredAddressConfigPtr | ||
|
@@ -18,20 +80,47 @@ FixedServerPreferredAddressConfigFactory::createServerPreferredAddressConfig( | |
MessageUtil::downcastAndValidate<const envoy::extensions::quic::server_preferred_address::v3:: | ||
FixedServerPreferredAddressConfig&>(message, | ||
validation_visitor); | ||
quic::QuicIpAddress ip_v4, ip_v6; | ||
if (config.has_ipv4_address()) { | ||
if (!ip_v4.FromString(config.ipv4_address())) { | ||
ProtoExceptionUtil::throwProtoValidationException( | ||
absl::StrCat("bad v4 server preferred address: ", config.ipv4_address()), message); | ||
} | ||
FixedServerPreferredAddressConfig::QuicSocketOrIpAddress ip_v4, ip_v6; | ||
switch (config.ipv4_type_case()) { | ||
case envoy::extensions::quic::server_preferred_address::v3::FixedServerPreferredAddressConfig:: | ||
kIpv4Address: | ||
ip_v4 = parseIp(config.ipv4_address(), "v4", message); | ||
break; | ||
case envoy::extensions::quic::server_preferred_address::v3::FixedServerPreferredAddressConfig:: | ||
kIpv4AddressAndPort: | ||
ip_v4 = parseSocketAddress(config.ipv4_address_and_port(), Network::Address::IpVersion::v4, | ||
"v4", message); | ||
break; | ||
case envoy::extensions::quic::server_preferred_address::v3::FixedServerPreferredAddressConfig:: | ||
IPV4_TYPE_NOT_SET: | ||
break; | ||
} | ||
if (config.has_ipv6_address()) { | ||
if (!ip_v6.FromString(config.ipv6_address())) { | ||
ProtoExceptionUtil::throwProtoValidationException( | ||
absl::StrCat("bad v6 server preferred address: ", config.ipv6_address()), message); | ||
} | ||
|
||
switch (config.ipv6_type_case()) { | ||
case envoy::extensions::quic::server_preferred_address::v3::FixedServerPreferredAddressConfig:: | ||
kIpv6Address: | ||
ip_v6 = parseIp(config.ipv6_address(), "v6", message); | ||
break; | ||
case envoy::extensions::quic::server_preferred_address::v3::FixedServerPreferredAddressConfig:: | ||
kIpv6AddressAndPort: | ||
ip_v6 = parseSocketAddress(config.ipv6_address_and_port(), Network::Address::IpVersion::v6, | ||
"v6", message); | ||
break; | ||
case envoy::extensions::quic::server_preferred_address::v3::FixedServerPreferredAddressConfig:: | ||
IPV6_TYPE_NOT_SET: | ||
break; | ||
} | ||
return std::make_unique<FixedServerPreferredAddressConfig>(ip_v4, ip_v6); | ||
|
||
quic::QuicIpAddress dnat_v4, dnat_v6; | ||
if (config.has_ipv4_dnat_address()) { | ||
dnat_v4 = parseIp(config.ipv4_dnat_address(), "dnat v4", message); | ||
} | ||
|
||
if (config.has_ipv6_dnat_address()) { | ||
dnat_v6 = parseIp(config.ipv6_dnat_address(), "dnat v6", message); | ||
} | ||
|
||
return std::make_unique<FixedServerPreferredAddressConfig>(ip_v4, ip_v6, dnat_v4, dnat_v6); | ||
} | ||
|
||
REGISTER_FACTORY(FixedServerPreferredAddressConfigFactory, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why do we need a string and a struct version of this field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string version does not allow setting the port. I think it was done this way because we don't yet support multiple sockets on a quic listener; it has to get the original and spa traffic on the same socket, bound to
0.0.0.0:443
or similar, so the SPA address must use the same port, and so it isn't allowed to be configured here.I am hoping to allow multiple sockets in the future, but this is what we have for now.
But with DNAT, the client's view of the destination may not be what the server receives, so if the DNAT also does port translation, we can now support having a different port in the address advertised to clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a string version because we don't want to make port part of the config as the port is supposed to be the same as the listening port.
With DNAT do we need to advertise a different port in transport parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you added a new knob to allow a different port. I think we can keep the old behavior of the string form with a comment "if it is desired to advertise a different port, use the struct form".
As to the new knob, how about defining a struct
And then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the current restriction on the port being equal is an implementation restriction, that may be alleviated in the future, would it be better to use
SocketAddress
for both addresses, and document that the port is currently ignored, so that we don't have to change proto in the future to allow setting the port if/when that happens?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG. And please also emit an error during config validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a commit with what I think we've agreed on for the config. @danzh2010 @abeyad can you please take a look and confirm this looks roughly correct; then I'll make the related code changes (and add the validation that the port == 0 in the cases where it isn't/can't be used).