-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
http: add CONNECT-UDP support #27714
Conversation
I also added a basic integration test of sending and receiving data between a client and UDP server through Envoy proxy using CONNECT-UDP. 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>
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>
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>
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
/assign @DavidSchinazi David will do a first pass review. Thank you! |
What's the right way to add api/envoy/extensions/upstreams/http/udp/v3/udp_connection_pool.proto? The check_and_fix_proto_format CI complains about it. |
Drive-by comment.... The proto BUILD file is generated by tool. Once you have the correct proto change (e.g., import, namespace, etc), you can run |
I tried it and it deleted both the proto and the BUILD file. Tried it with the proto file alone then it complained about missing the BUILD file. I guess I should have written the proto in a different way? I don't see any Envoy documentation about this. |
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.
Thanks, this is overall in great shape!
envoy/router/router.h
Outdated
~GenericConnPoolFactory() override = default; | ||
|
||
/* | ||
* @param options for creating the transport socket | ||
* @return may be null | ||
*/ | ||
virtual GenericConnPoolPtr | ||
createGenericConnPool(Upstream::ThreadLocalCluster& thread_local_cluster, bool is_connect, | ||
createGenericConnPool(Upstream::ThreadLocalCluster& thread_local_cluster, | ||
GenericConnPoolFactory::UpstreamProtocol is_connect, |
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.
should we rename is_connect
to upstream_protocol
?
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.
RIght, done.
// Rewrites the host of CONNECT-UDP requests. | ||
if (HeaderUtility::isConnectUdp(*request_headers_) && | ||
!HeaderUtility::rewriteAuthorityForConnectUdp(*request_headers_)) { | ||
sendLocalReply(Code::BadRequest, "Invalid URI Template in the path", nullptr, absl::nullopt, |
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 think this makes more sense to be Code::NotFound
similar to CONNECT above – conceptually, the path the client used isn't invalid, we just don't expect CONNECT-UDP there.
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.
Makes sense. Done.
source/common/http/header_utility.cc
Outdated
@@ -20,6 +20,15 @@ | |||
namespace Envoy { | |||
namespace Http { | |||
|
|||
namespace { | |||
|
|||
const Regex::CompiledGoogleReMatcher& connectUdpPathMatcher() { |
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.
A regex seems pretty heavyweight for something this simple. I'd assume we'd get noticeably better performance by checking whether the path is prefixed by /.well-known/masque/udp/
, removing the prefix and then splitting on /
.
That said, this could be a premature optimization. Is there precedent for regex vs split in the codebase currently?
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.
Regex is used in Envoy mainly for rewriting headers based on the user configuration. I agree that in this case, regex could be an overkill. I think this could make more sense in a later stage if we want to support more custom URI Templates. I will remove regex as you suggested.
// TODO(https://github.com/envoyproxy/envoy/issues/23564): Http3 Datagram support should be | ||
// turned on by returning quic::HttpDatagramSupport::kRfc once the CONNECT-UDP support work is | ||
// completed. | ||
#ifdef ENVOY_ENABLE_HTTP_DATAGRAMS |
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.
Should this be a runtime guard instead of compile time? Ideally the presence of CONNECT-UDP in the config should enable this. Same comment for all other uses of this macro in this PR
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.
So this macro is for excluding HTTP Capsule support from the Envoy Mobile builds. I think the right way would be adding the runtime guard as well as this macro guard. I will add the runtime guard accordingly.
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.
Added the runtime guard.
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.
What's the purpose of removing this from Envoy Mobile at compile time? They're tight on binary size but this is something that will be needed there eventually and a macro adds complexity
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.
It was added in the previous PR as Alyssa and Ryan wanted to have an option to include or exclude Http Datagram support in the Envoy Mobile build. Here's the relevant discussion in the previous PR: #25925 (comment)
If we all agree that it's no longer necessary, I would like to remove it likewise. Perhaps it would be a separate PR to make this one concise.
bool UdpUpstream::OnCapsule(const quiche::Capsule& capsule) { | ||
quiche::CapsuleType capsule_type = capsule.capsule_type(); | ||
if (capsule_type != quiche::CapsuleType::DATAGRAM) { | ||
// Silently drops Datagram Capsules with an unknown type. |
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.
nit: these aren't datagram capsules
// Silently drops Datagram Capsules with an unknown type. | |
// Silently drops capsules with an unknown type. |
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.
Good point. Thanks.
|
||
Network::SocketPtr createSocket(const Upstream::HostConstSharedPtr& host) { | ||
return std::make_unique<Network::SocketImpl>(Network::Socket::Type::Datagram, host->address(), | ||
nullptr, Network::SocketCreationOptions{}); |
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.
nit: /*variable_name=*/nullptr
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.
Done.
std::string received_capsule_fragment = | ||
absl::HexStringToBytes("00" // DATAGRAM capsule type | ||
"08" // capsule length | ||
"a1a2a3a4a5a6a7a8" // HTTP Datagram payload |
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.
Why doesn't this have the 00
byte of the contextID=0?
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 think I misunderstood the specification. Fixed the error in the test cases as well as the UdpUpstream.
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
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.
Thanks a lot for reviewing the PR, David. I will send another commit to complete the fix.
// Rewrites the host of CONNECT-UDP requests. | ||
if (HeaderUtility::isConnectUdp(*request_headers_) && | ||
!HeaderUtility::rewriteAuthorityForConnectUdp(*request_headers_)) { | ||
sendLocalReply(Code::BadRequest, "Invalid URI Template in the path", nullptr, absl::nullopt, |
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.
Makes sense. Done.
// TODO(https://github.com/envoyproxy/envoy/issues/23564): Http3 Datagram support should be | ||
// turned on by returning quic::HttpDatagramSupport::kRfc once the CONNECT-UDP support work is | ||
// completed. | ||
#ifdef ENVOY_ENABLE_HTTP_DATAGRAMS |
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.
So this macro is for excluding HTTP Capsule support from the Envoy Mobile builds. I think the right way would be adding the runtime guard as well as this macro guard. I will add the runtime guard accordingly.
Event::FileReadyType::Read); | ||
} | ||
|
||
void UdpUpstream::encodeData(Buffer::Instance& data, bool /*end_stream*/) { |
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.
Your assumption is correct. Thanks for catching this. I thought Capsules could not span across multiple DATA frames. Done.
} | ||
|
||
Envoy::Http::Status UdpUpstream::encodeHeaders(const Envoy::Http::RequestHeaderMap&, | ||
bool /*end_stream*/) { |
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.
If end_stream
is set, it means the request is header only. The existing CONNECT (TCP) implementation in Envoy uses it only for supporting PROXY protocol, which I assume not relevant to CONNECT-UDP, so I didn't find the use of the end_stream
in this method. Let me know if you think otherwise.
|
||
Network::SocketPtr createSocket(const Upstream::HostConstSharedPtr& host) { | ||
return std::make_unique<Network::SocketImpl>(Network::Socket::Type::Datagram, host->address(), | ||
nullptr, Network::SocketCreationOptions{}); |
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.
Done.
std::string received_capsule_fragment = | ||
absl::HexStringToBytes("00" // DATAGRAM capsule type | ||
"08" // capsule length | ||
"a1a2a3a4a5a6a7a8" // HTTP Datagram payload |
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 think I misunderstood the specification. Fixed the error in the test cases as well as the UdpUpstream.
source/common/http/header_utility.cc
Outdated
@@ -20,6 +20,15 @@ | |||
namespace Envoy { | |||
namespace Http { | |||
|
|||
namespace { | |||
|
|||
const Regex::CompiledGoogleReMatcher& connectUdpPathMatcher() { |
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.
Regex is used in Envoy mainly for rewriting headers based on the user configuration. I agree that in this case, regex could be an overkill. I think this could make more sense in a later stage if we want to support more custom URI Templates. I will remove regex as you suggested.
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
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.
Resolved the rest of the issues such as IPv6 parsing. Can you please review my new changes, David? Thank you.
// TODO(https://github.com/envoyproxy/envoy/issues/23564): Http3 Datagram support should be | ||
// turned on by returning quic::HttpDatagramSupport::kRfc once the CONNECT-UDP support work is | ||
// completed. | ||
#ifdef ENVOY_ENABLE_HTTP_DATAGRAMS |
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.
Added the runtime guard.
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.
Overall this looks great! Please send to wider review after addressing these last comments
source/common/http/header_utility.cc
Outdated
ASSERT(!ip_address.empty()); | ||
// Removes the Zone ID in the IPv6 address. | ||
std::vector<absl::string_view> ip_address_split = absl::StrSplit(ip_address, '%'); | ||
if (ip_address_split.empty()) { |
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.
Should this be an assert? The array should have at least one element when the string isn't empty
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.
Ah, yes this is not necessary because ip_address_split will be empty anyway if the ip_address is empty and the function will return false. Done.
source/common/http/header_utility.cc
Outdated
if (ip_address_split.empty()) { | ||
return false; | ||
} | ||
sockaddr_in6 addr_in; |
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.
nit: seems like you don't need a full sockaddr_in6
here, just an in6_addr
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.
Done.
// TODO(https://github.com/envoyproxy/envoy/issues/23564): Http3 Datagram support should be | ||
// turned on by returning quic::HttpDatagramSupport::kRfc once the CONNECT-UDP support work is | ||
// completed. | ||
#ifdef ENVOY_ENABLE_HTTP_DATAGRAMS |
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.
What's the purpose of removing this from Envoy Mobile at compile time? They're tight on binary size but this is something that will be needed there eventually and a macro adds complexity
} | ||
|
||
Envoy::Http::Status UdpUpstream::encodeHeaders(const Envoy::Http::RequestHeaderMap&, | ||
bool /*end_stream*/) { |
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.
Per the spec The lifetime of the socket is tied to the request stream
so if we get a FIN on the stream then we should tear everything down. It would be silly for the client to FIN the stream right after the headers but if it happens we should tear everything down and not bother opening the socket at all. Maybe reply with status code 400?
TEST(HeaderIsValidTest, IsConnectUdp) { | ||
EXPECT_TRUE( | ||
HeaderUtility::isConnectUdp(Http::TestRequestHeaderMapImpl{{"upgrade", "connect-udp"}})); | ||
// Extended CONNECT requests should be normalied to HTTP/1.1. |
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.
// Extended CONNECT requests should be normalied to HTTP/1.1. | |
// Extended CONNECT requests should be normalized to HTTP/1.1. |
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.
Done.
EXPECT_TRUE(HeaderUtility::rewriteAuthorityForConnectUdp(connect_udp_request)); | ||
EXPECT_EQ(connect_udp_request.getHostValue(), "foo.lyft.com:80"); | ||
TestRequestHeaderMapImpl connect_udp_request_ipv6{ | ||
{":path", "/.well-known/masque/udp/2001:0db8:85a3::8a2e:0370:7334/80/"}, |
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.
Per the spec if "target_host" contains an IPv6 literal, the colons (":") MUST be percent-encoded. For example, if the target host is "2001:db8::42", it will be encoded in the URI as "2001%3Adb8%3A%3A42".
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.
Done.
const std::string sent_capsule_fragment = | ||
absl::HexStringToBytes("00" // DATAGRAM capsule type | ||
"08" // capsule length | ||
"00a1a2a3a4a5a6a7" // UDP Proxying HTTP Datagram payload |
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.
"00a1a2a3a4a5a6a7" // UDP Proxying HTTP Datagram payload | |
"00" // context ID | |
"a1a2a3a4a5a6a7" // UDP Proxying Payload |
Same below
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.
Neat! Done.
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
To fix the changelog you'll need to do a main merge. |
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>
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
/retest |
Looks like some tidy failures here: Also coverage: Code coverage for source/extensions/upstreams is lower than limit of 96.6 (93.8) Detailed coverage results here: |
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
/wait |
I also added the extensions/upstreams/http/generic to the exception list for the coverage test due to the coverage bug of excluding brackets of case statements. Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
CC @envoyproxy/coverage-shephards: FYI only for changes made to |
I added more test cases to increase the test coverage. Because of the coverage bug of excluding nested brackets of case statements, it's not possible to reach the coverage threshold in extensions/upstreams/http/generic/config.cc (https://storage.googleapis.com/envoy-pr/e9f2ada/coverage/source/extensions/upstreams/http/generic/config.cc.gcov.html). I added an additional test case to cover "return nullptr;" but still the maximum coverage I can achieve is 88.5 because of the bug of the coverage CI. I added it to the exception list to make it pass the coverage test. I set the threshold as 85.0 to leave some headroom. Let me know if you have any comments or questions. Thank you! |
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
/retest |
Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com>
Commit Message: This commit adds CONNECT-UDP (RFC 9298) support. UdpConnPool is added to create a UDP socket for a new CONNECT-UDP request, and UDPUpstream is added to maintain the socket and other relevant data associated with UDP upstreams. We added an integration test for the terminating CONNECT-UDP proxy, but not the forwarding proxy in this commit. We are going to add test cases to cover the forwarding proxy scenario in a subsequent commit. Additional Description: Risk Level: Medium, the feature can only be enabled by the new configuration added in this commit. Testing: Integration test Runtime guard: envoy.reloadable_features.enable_connect_udp_support Release Notes: added support for CONNECT-UDP (RFC 9298). Can be disabled by setting runtime feature envoy.reloadable_features.enable_connect_udp_support to false. Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com> Co-authored-by: asingh-g <abhisinghx@google.com> Signed-off-by: asheryer <asheryer@amazon.com>
Commit Message: This commit adds CONNECT-UDP (RFC 9298) support. UdpConnPool is added to create a UDP socket for a new CONNECT-UDP request, and UDPUpstream is added to maintain the socket and other relevant data associated with UDP upstreams. We added an integration test for the terminating CONNECT-UDP proxy, but not the forwarding proxy in this commit. We are going to add test cases to cover the forwarding proxy scenario in a subsequent commit. Additional Description: Risk Level: Medium, the feature can only be enabled by the new configuration added in this commit. Testing: Integration test Runtime guard: envoy.reloadable_features.enable_connect_udp_support Release Notes: added support for CONNECT-UDP (RFC 9298). Can be disabled by setting runtime feature envoy.reloadable_features.enable_connect_udp_support to false. Signed-off-by: Jeongseok Son <jeongseok.son@gmail.com> Co-authored-by: asingh-g <abhisinghx@google.com> Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
Commit Message:
This commit adds CONNECT-UDP (RFC 9298) support. UdpConnPool is added to create a UDP socket for a new CONNECT-UDP request, and UDPUpstream is added to maintain the socket and other relevant data associated with UDP upstreams.
We added an integration test for the terminating CONNECT-UDP proxy, but not the forwarding proxy in this commit. We are going to add test cases to cover the forwarding proxy scenario in a subsequent commit.
Additional Description:
Risk Level: Medium, the feature can only be enabled by the new configuration added in this commit.
Testing: Integration test
Runtime guard: envoy.reloadable_features.enable_connect_udp_support
Release Notes: added support for CONNECT-UDP (RFC 9298). Can be disabled by setting runtime feature
envoy.reloadable_features.enable_connect_udp_support
to false.