quic: support for server-preferred address behind DNAT #33774
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.
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. 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. 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. @abeyad and @RyanTheOptimist would it be better like this (plus the equivalent for ipv6)? There was a problem hiding this comment. to make sure I understand, is that correct? There was a problem hiding this comment. That SGTM, thanks @ggreenway ! There was a problem hiding this comment. 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. 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. 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. 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 |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| load( | ||
| "//bazel:envoy_build_system.bzl", | ||
| "envoy_cc_test_library", | ||
| "envoy_package", | ||
| ) | ||
| load( | ||
| "//test/extensions:extensions_build_system.bzl", | ||
| "envoy_extension_cc_test", | ||
| ) | ||
|
|
||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| envoy_package() | ||
|
|
||
| envoy_extension_cc_test( | ||
| name = "fixed_server_preferred_address_test", | ||
| srcs = ["fixed_server_preferred_address_test.cc"], | ||
| extension_names = ["envoy.quic.server_preferred_address.fixed"], | ||
| tags = ["nofips"], | ||
| deps = [ | ||
| "//source/extensions/quic/server_preferred_address:fixed_server_preferred_address_config_lib", | ||
| "//test/mocks/protobuf:protobuf_mocks", | ||
| ], | ||
| ) |
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:443or 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
SocketAddressfor 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).