quic: support for server-preferred address behind DNAT #33774

ggreenway · 2024-04-24T22:51:15Z

Commit Message:
Additional Description:
Risk Level: Low
Testing: Added integration test
Docs Changes: Documented in proto
Release Notes: added
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only · 2024-04-24T22:51:26Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #33774 was opened by ggreenway.

see: more, trace.

Signed-off-by: Greg Greenway <ggreenway@apple.com>

RyanTheOptimist

Nice! Thanks for doing this.

RyanTheOptimist · 2024-04-24T23:56:33Z

...nvoy/extensions/quic/server_preferred_address/v3/fixed_server_preferred_address_config.proto

    // 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;


Out of curiosity, why do we need a string and a struct version of this field?

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.

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?

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

DNatAddressPair { // Port will be used. config.core.v3.SocketAddress address_to_advertise; // Only the IP part. Different port is not supported yet. string dnat_internal_address; };

And then

oneof ipv4_type { // String representation of IPv4 address, i.e. "127.0.0.2". // Port will not be used. string ipv4_address = 1; DNatAddressPair ipv4_addresses_with_dnat = 2; }

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?

SG. And please also emit an error during config validation.

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).

RyanTheOptimist · 2024-04-24T23:58:06Z

source/common/quic/envoy_quic_server_preferred_address_config_factory.h

@@ -15,14 +15,28 @@ class EnvoyQuicServerPreferredAddressConfig {
 public:
  virtual ~EnvoyQuicServerPreferredAddressConfig() = default;

+  struct Addresses {


nit: Brief comment, please.

RyanTheOptimist · 2024-04-24T23:58:45Z

source/common/quic/envoy_quic_server_preferred_address_config_factory.h

+    // 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.


This comment is super clear. Thanks!

RyanTheOptimist · 2024-04-25T00:00:04Z

source/extensions/quic/server_preferred_address/fixed_server_preferred_address_config.cc

+        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>) {


nit: Since each of these branches is returning, let's remove the "} else" which in particular removes the indentation for the last case.

RyanTheOptimist · 2024-04-25T00:01:32Z

source/extensions/quic/server_preferred_address/fixed_server_preferred_address_config.cc

+      address);
+}
+
+quic::QuicIpAddress parseIp(const std::string& addr, absl::string_view version,


nit: "version" => "ip_version" or "address_family"

Signed-off-by: Greg Greenway <ggreenway@apple.com>

abeyad · 2024-04-25T19:08:44Z

...nvoy/extensions/quic/server_preferred_address/v3/fixed_server_preferred_address_config.proto

+    //
+    // String representation of IPv6 address, i.e. "::1"
+    // The listener's port will be used.
+    string ipv6_dnat_address = 6;


oneof is now discouraged:

envoy/api/STYLE.md

Line 63 in 21b406b

* Prefer multiple fields with defined precedence over boolean overloads of fields or

Is it necessary in this case? Do we anticipate more fields to select from?

I'm trying to mirror the other field here. It'll probably need a similar set of options as ipv6_address is getting in this PR. See #33774 (comment) for discussion on why it is this way.

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?

@abeyad and @RyanTheOptimist would it be better like this (plus the equivalent for ipv6)?

oneof ipv4_type { // Deprecated string ipv4_address = 1; } // The IPv4 address to advertise to clients for Server Preferred Address. config.core.v3.SocketAddress ipv4_address_and_port = 3; // Port isn't yet available; must be zero or ignored config.core.v3.SocketAddress ipv4_dnat_address = 5;

to make sure I understand, ipv4_dnat_type and ipv6_dnat_type would go away, and we'd just have:

config.core.v3.Address ipv4_dnat_address; config.core.v3.Address ipv6_dnat_address;

is that correct?

That SGTM, thanks @ggreenway !

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?

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 additional_addresses; if that were true then in the non-DNAT case, the client-advertised address may also need a different port, and the DNAT address may need a different port.

QUICHE doesn't support multiple writers for a server connection. Is there a use case for advertising a different port for SPA?

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.

Signed-off-by: Greg Greenway <ggreenway@apple.com>

RyanTheOptimist · 2024-04-30T00:07:47Z

/wait

Signed-off-by: Greg Greenway <ggreenway@apple.com>

danzh2010 · 2024-04-30T17:13:31Z

...nvoy/extensions/quic/server_preferred_address/v3/fixed_server_preferred_address_config.proto

 message FixedServerPreferredAddressConfig {
  // [#comment:TODO(danzh2010): discuss with API shepherds before removing WiP status.]

  option (xds.annotations.v3.message_status).work_in_progress = true;

+  // Addreses for server preferred address for a single address family (IPv4 or IPv6).
+  message Addresses {


nits: how about AddressPair?

danzh2010 · 2024-04-30T17:15:45Z

...nvoy/extensions/quic/server_preferred_address/v3/fixed_server_preferred_address_config.proto

    // If not specified, none will be configured.
    string ipv4_address = 1;
+
+    // The IPv4 address to advertise to clients for Server Preferred Address.
+    Addresses ipv4_config = 3;


nits: how about ipv4_addresses_with_dnat

Because if we allow setting a different port, without dnat, then this config will be used in a non-dnat case, and the dnat address in the message will be left unset. I'm trying to future-proof the config so we don't need to change it again.

abeyad · 2024-04-30T17:32:58Z

...nvoy/extensions/quic/server_preferred_address/v3/fixed_server_preferred_address_config.proto

    // If not specified, none will be configured.
    string ipv6_address = 2;
+
+    // The IPv6 address to advertise to clients for Server Preferred Address.
+    Addresses ipv6_config = 4;


The structure of all this LGTM.

I'm unsure if we should try to move the new Address type fields out of the oneof though, in keeping with the new API style guidance discouraging oneof in favor of defining precedence rules (e.g. this PR does that for an existing API https://github.com/envoyproxy/envoy/pull/33749/files#diff-e18722c3190aeb921472834ba2f4725bcfa5dfae5c024d9d7aca8cd2ecefeaee).

That would mean keeping the oneof, but defining ipv4_config and ipv6_config as separate fields outside of the oneof and using precedence rules that if ipv6_config (or ipv4_config) is defined, use that, otherwise use ipv6_address (or ipv4_address).

Would that work?

Pushed another version, which I think changes it as you're requesting. PTAL.

Are any of the normal rules for changing protos relaxed due to option (xds.annotations.v3.message_status).work_in_progress = true;? Could we remove the oneof wrapper, since nothing else will ever go into it?

LGTM, thanks Greg!

If it's marked work_in_progress, we could remove the oneof AFAIU. In https://github.com/envoyproxy/envoy/blob/main/api/STYLE.md, we have this clause:

Use a (xds.annotations.v3.file_status).work_in_progress, (xds.annotations.v3.message_status).work_in_progress, or (xds.annotations.v3.field_status).work_in_progress option annotation for files, messages, or fields, respectively, that are considered work in progress and are not subject to the threat model or the breaking change policy.

So we should be able to change the oneof since it's marked as work_in_progress.

cc @envoyproxy/api-shepherds in case there is a differing opinion on this.

The current structure looks good, other than a nits for renaming. Having oneof also makes sense to me, is there a reason why we want to remove it?

oneof is now discouraged by the API style guide: https://github.com/envoyproxy/envoy/blob/main/api/STYLE.md?plain=1#L63-L90

@danzh2010 I didn't want to change the names to be overly specific in case we need to add new fields, and they don't work well with those very specific names. For Addresses, maybe is could be AddressFamilyConfig?

AddressFamilyConfig sounds better.

Changed name to AddressFamilyConfig

Signed-off-by: Greg Greenway <ggreenway@apple.com>

abeyad

/lgtm api

Signed-off-by: Greg Greenway <ggreenway@apple.com>

RyanTheOptimist · 2024-05-06T15:01:27Z

@danzh2010 does this LG to you?

Signed-off-by: Greg Greenway <ggreenway@apple.com>

...nvoy/extensions/quic/server_preferred_address/v3/fixed_server_preferred_address_config.proto

danzh2010 · 2024-05-06T18:56:09Z

source/common/quic/envoy_quic_server_preferred_address_config_factory.h

  /**
   * 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


Why this interface needs to be changed? And why the proto struct is more preferrable?

Because it is now returning 4 addresses instead of two. And a std::tuple of 4 elements is harder to read than a struct with four elements.

Signed-off-by: Greg Greenway <ggreenway@apple.com>

danzh2010

LGTM, along aside a small nit. Thanks for contributing to this API!

source/extensions/quic/server_preferred_address/fixed_server_preferred_address_config.cc

Signed-off-by: Greg Greenway <ggreenway@apple.com>

abeyad

/lgtm api

ggreenway · 2024-05-07T20:23:27Z

@RyanTheOptimist please review

RyanTheOptimist

Thanks for doing this!

quic: support for server-preferred address behind DNAT

7ca7daf

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway requested review from alyssawilk and mattklein123 as code owners April 24, 2024 22:51

repokitteh-read-only bot added the api label Apr 24, 2024

repokitteh-read-only bot assigned abeyad Apr 24, 2024

fixes

78abeb1

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway assigned RyanTheOptimist Apr 24, 2024

fix

bb84625

Signed-off-by: Greg Greenway <ggreenway@apple.com>

RyanTheOptimist reviewed Apr 25, 2024

View reviewed changes

ggreenway added 2 commits April 24, 2024 17:10

add assert to test code

e25ccb4

Signed-off-by: Greg Greenway <ggreenway@apple.com>

review comments

93f21e6

Signed-off-by: Greg Greenway <ggreenway@apple.com>

RyanTheOptimist previously approved these changes Apr 25, 2024

View reviewed changes

ggreenway enabled auto-merge (squash) April 25, 2024 15:13

add coverage

b8a812b

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway dismissed RyanTheOptimist’s stale review via b8a812b April 25, 2024 18:37

abeyad reviewed Apr 25, 2024

View reviewed changes

ggreenway added 2 commits April 25, 2024 12:39

fix format

b54cc44

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/main' into spa-dnat

2a1b89c

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot added the waiting label Apr 30, 2024

update config proto based on PR feedback

b21946f

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot removed the waiting label Apr 30, 2024

danzh2010 reviewed Apr 30, 2024

View reviewed changes

abeyad reviewed Apr 30, 2024

View reviewed changes

ggreenway added 2 commits April 30, 2024 10:39

more config

6eb2c6c

Signed-off-by: Greg Greenway <ggreenway@apple.com>

fixes

f006088

Signed-off-by: Greg Greenway <ggreenway@apple.com>

abeyad previously approved these changes May 1, 2024

View reviewed changes

repokitteh-read-only bot removed the api label May 1, 2024

revert accidental change

19c2d66

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway dismissed abeyad’s stale review via 19c2d66 May 1, 2024 03:18

ggreenway added 6 commits May 1, 2024 08:15

proto format

cf30494

Signed-off-by: Greg Greenway <ggreenway@apple.com>

spelling

c7bf130

Signed-off-by: Greg Greenway <ggreenway@apple.com>

fix single backtick proto format error

81b10a2

Signed-off-by: Greg Greenway <ggreenway@apple.com>

build link in release notes

9c8cfba

Signed-off-by: Greg Greenway <ggreenway@apple.com>

add missing validation of spa port being zero for non-dnat

d8adc70

Signed-off-by: Greg Greenway <ggreenway@apple.com>

more tests

59ded87

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot added the api label May 1, 2024

ggreenway added 2 commits May 1, 2024 09:06

fix format of release notes

8c7e972

Signed-off-by: Greg Greenway <ggreenway@apple.com>

more proto format fixes

155e2ef

Signed-off-by: Greg Greenway <ggreenway@apple.com>

rename proto message

40d2ab5

Signed-off-by: Greg Greenway <ggreenway@apple.com>

danzh2010 reviewed May 6, 2024

View reviewed changes

remote mention of listener port in docs

81241b7

Signed-off-by: Greg Greenway <ggreenway@apple.com>

danzh2010 reviewed May 6, 2024

View reviewed changes

source/extensions/quic/server_preferred_address/fixed_server_preferred_address_config.cc Outdated Show resolved Hide resolved

auto->type

39ca689

Signed-off-by: Greg Greenway <ggreenway@apple.com>

abeyad reviewed May 6, 2024

View reviewed changes

repokitteh-read-only bot removed the api label May 6, 2024

RyanTheOptimist approved these changes May 7, 2024

View reviewed changes

ggreenway merged commit c940f24 into envoyproxy:main May 7, 2024
54 checks passed

soulxu mentioned this pull request Jul 29, 2024

QuicHttpIntegrationSPATests/QuicHttpIntegrationSPATest.UsesPreferredAddressDNAT flaky on arm #35467

Closed

quic: support for server-preferred address behind DNAT #33774

quic: support for server-preferred address behind DNAT #33774

Conversation

ggreenway commented Apr 24, 2024

repokitteh-read-only bot commented Apr 24, 2024

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danzh2010 Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abeyad left a comment

Choose a reason for hiding this comment

RyanTheOptimist commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danzh2010 left a comment

Choose a reason for hiding this comment

abeyad left a comment

Choose a reason for hiding this comment

ggreenway commented May 7, 2024

RyanTheOptimist left a comment

Choose a reason for hiding this comment

danzh2010 Apr 30, 2024 •

edited

Loading