http: fix issues in CONNECT-UDP forwarding mode. #36174
Conversation
When Envoy operates as a CONNECT-UDP forwarding proxy, it was resetting the upstream stream because it received HTTP Datagrams before receiving the SETTINGS frame. A new enum has been added in QUICHE to distinguish this case, so I added handling logic for this and made Envoy drop the datagrams instead of resetting the stream. Also, Envoy was dropping Datagrams because the default maximum packet length for QUIC connections in QUICHE is not large enough for tunneling use cases such as CONNECT-UDP. I made the cluster manager override the maximum packet length for upstream QUIC connections to fix this issue, but only when the downstream request is CONNECT-UDP. Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
|
/wait |
|
If this is a work-in-progress, please set the status to draft until you're ready for review. |
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
|
/retest |
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
|
Hi Ryan, could you review the update on the HTTP Datagram Handler? Hi Dan, I made some changes in quic_info to set different max packet lengths for QUIC connections if the downstream is CONNECT-UDP. Could you review the changes I made under the QUIC component? |
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
|
||
| // Maximum packet length for QUIC connections. It refers to the largest size of a QUIC packet that can be transmitted over the connection. | ||
| // If not specified, the default value defined in `QUICHE <https://github.com/google/quiche/blob/main/quiche/quic/core/quic_constants.h>` is used. | ||
| google.protobuf.UInt64Value max_packet_length = 9; |
There was a problem hiding this comment.
Thanks for the comments, @RyanTheOptimist and @DavidSchinazi. I will send more commits soon but want to quickly check if we should set a reasonable bound for the max_packet_length option like other options in this config. One range I can think of is 1 <= max_packet_length <= 65,535 (max IP packet size), but I'm wondering if we should just not set the bound here. I used UInt64Value since the corresponding QUICHE variable and constants (e.g. https://github.com/google/quiche/blob/4249f8025caed1e3d71d04d9cadf42251acb7cac/quiche/quic/core/quic_constants.h#L32) use QuicByteCount, which is uint64_t internally. Let me know if you have any thoughts on these.
There was a problem hiding this comment.
I don't think Envoy needs to put a cap on this value. Internally, QUICHE will cap it at 1452, but we shouldn't duplicate that constant or logic here
There was a problem hiding this comment.
SGTM. Let me know if you have any other comments on the new code.
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
|
/wait |
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
Locally ran the doc CI this time. Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
|
/wait on CI |
|
@RyanTheOptimist, @DavidSchinazi Let me know if you have more comments, @abeyad I made your approval obsolete so will appreciate it if you can LGTM again, thank you. |
|
@abeyad, over to you, I think! |
Commit Message:
When Envoy operates as a CONNECT-UDP forwarding proxy, it was resetting the upstream stream because it received HTTP Datagrams before receiving the SETTINGS frame. A new enum has been added in QUICHE to distinguish this case, so I added handling logic for this and made Envoy drop the datagrams instead of resetting the stream.
Also, Envoy was dropping Datagrams because the default maximum packet length for QUIC connections in QUICHE is not large enough for tunneling use cases such as CONNECT-UDP. I added a new QUIC protocol option called
max_packet_lengthto allow users to adjust the maximum packet length for upstream QUIC connections to fix this issue.Additional Description:
Risk Level: Low, this change is only relevant if CONNECT-UDP is enabled with the forwarding mode.
Testing: Added more unit tests.
Docs Changes: Added the
max_packet_lengthQUIC protocol option and its explanation.Release Notes: Added notes about fixing the CONNECT-UDP forwarding mode and adding the new QUIC protocol option.
Platform Specific Features: N/A
[Optional Fixes #Issue]: #34836