Address comments

Signed-off-by: Fredy Wijaya <[email protected]>
envoyproxy · Jul 25, 2024 · 042ee12 · 042ee12
1 parent 15aa112
commit 042ee12
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 17 deletions.
diff --git a/mobile/library/cc/engine_builder.cc b/mobile/library/cc/engine_builder.cc
@@ -240,8 +240,8 @@ EngineBuilder& EngineBuilder::setUseGroIfAvailable(bool use_gro_if_available) {
   return *this;
 }
 
-EngineBuilder& EngineBuilder::setSocketReceiveBufferSize(int32_t size) {
-  socket_receive_buffer_size_ = size;
+EngineBuilder& EngineBuilder::setUdpSocketReceiveBufferSize(int32_t size) {
+  udp_socket_receive_buffer_size_ = size;
   return *this;
 }
 
@@ -726,25 +726,24 @@ std::unique_ptr<envoy::config::bootstrap::v3::Bootstrap> EngineBuilder::generate
         ["envoy.extensions.upstreams.http.v3.HttpProtocolOptions"]
             .PackFrom(alpn_options);
 
-    // Set the upstream connections socket receive buffer size. The operating system defaults are
-    // usually too small for QUIC.
-    // NOTE: An H3 cluster can also establish H2 connections (for example, if the H3 connection is
-    // marked as broken in the ConnectivityGrid). This option would apply to all connections in the
-    // cluster, meaning H2 TCP connections buffer size would also be set to 1MB. On the platforms
-    // we've tested, IPPROTO_UDP cannot be used as a level for the SO_RCVBUF option.
-    envoy::config::core::v3::SocketOption* rcv_buf_sock_opt =
+    // Set the upstream connections UDP socket receive buffer size. The operating system defaults
+    // are usually too small for QUIC.
+    envoy::config::core::v3::SocketOption* udp_rcv_buf_sock_opt =
         base_cluster->mutable_upstream_bind_config()->add_socket_options();
-    rcv_buf_sock_opt->set_name(SO_RCVBUF);
-    rcv_buf_sock_opt->set_level(SOL_SOCKET);
-    rcv_buf_sock_opt->set_int_value(socket_receive_buffer_size_);
-    rcv_buf_sock_opt->set_description(
-        absl::StrCat("SO_RCVBUF = ", socket_receive_buffer_size_, " bytes"));
+    udp_rcv_buf_sock_opt->set_name(SO_RCVBUF);
+    udp_rcv_buf_sock_opt->set_level(SOL_SOCKET);
+    udp_rcv_buf_sock_opt->set_int_value(udp_socket_receive_buffer_size_);
+    // Only apply the socket option to the datagram socket.
+    udp_rcv_buf_sock_opt->mutable_type()->mutable_datagram();
+    udp_rcv_buf_sock_opt->set_description(
+        absl::StrCat("UDP SO_RCVBUF = ", udp_socket_receive_buffer_size_, " bytes"));
 
     envoy::config::core::v3::SocketOption* udp_snd_buf_sock_opt =
         base_cluster->mutable_upstream_bind_config()->add_socket_options();
     udp_snd_buf_sock_opt->set_name(SO_SNDBUF);
     udp_snd_buf_sock_opt->set_level(SOL_SOCKET);
     udp_snd_buf_sock_opt->set_int_value(udp_socket_send_buffer_size_);
+    // Only apply the socket option to the datagram socket.
     udp_snd_buf_sock_opt->mutable_type()->mutable_datagram();
     udp_snd_buf_sock_opt->set_description(
         absl::StrCat("UDP SO_SNDBUF = ", udp_socket_send_buffer_size_, " bytes"));

diff --git a/mobile/library/cc/engine_builder.h b/mobile/library/cc/engine_builder.h
@@ -68,7 +68,7 @@ class EngineBuilder {
   EngineBuilder& enableDrainPostDnsRefresh(bool drain_post_dns_refresh_on);
   // Sets whether to use GRO for upstream UDP sockets (QUIC/HTTP3).
   EngineBuilder& setUseGroIfAvailable(bool use_gro_if_available);
-  EngineBuilder& setSocketReceiveBufferSize(int32_t size);
+  EngineBuilder& setUdpSocketReceiveBufferSize(int32_t size);
   EngineBuilder& setUdpSocketSendBufferSize(int32_t size);
   EngineBuilder& enforceTrustChainVerification(bool trust_chain_verification_on);
   EngineBuilder& setUpstreamTlsSni(std::string sni);
@@ -183,7 +183,7 @@ class EngineBuilder {
 
   // This is the same value Cronet uses for QUIC:
   // https://source.chromium.org/chromium/chromium/src/+/main:net/quic/quic_context.h;drc=ccfe61524368c94b138ddf96ae8121d7eb7096cf;l=87
-  int32_t socket_receive_buffer_size_ = 1024 * 1024; // 1MB
+  int32_t udp_socket_receive_buffer_size_ = 1024 * 1024; // 1MB
   // This is the same value Cronet uses for QUIC:
   // https://source.chromium.org/chromium/chromium/src/+/main:net/quic/quic_session_pool.cc;l=790-793;drc=7f04a8e033c23dede6beae129cd212e6d4473d72
   // https://source.chromium.org/chromium/chromium/src/+/main:net/third_party/quiche/src/quiche/quic/core/quic_constants.h;l=43-47;drc=34ad7f3844f882baf3d31a6bc6e300acaa0e3fc8

diff --git a/mobile/test/cc/unit/envoy_config_test.cc b/mobile/test/cc/unit/envoy_config_test.cc
@@ -331,7 +331,7 @@ TEST(TestConfig, DisableHttp3) {
 }
 
 #ifdef ENVOY_ENABLE_QUIC
-TEST(TestConfig, SocketReceiveBufferSize) {
+TEST(TestConfig, UdpSocketReceiveBufferSize) {
   EngineBuilder engine_builder;
   engine_builder.enableHttp3(true);
 
@@ -358,6 +358,7 @@ TEST(TestConfig, SocketReceiveBufferSize) {
   // When using an H3 cluster, the UDP receive buffer size option should always be set.
   ASSERT_THAT(rcv_buf_option, NotNull());
   EXPECT_EQ(rcv_buf_option->level(), SOL_SOCKET);
+  EXPECT_TRUE(rcv_buf_option->type().has_datagram());
   EXPECT_EQ(rcv_buf_option->int_value(), 1024 * 1024 /* 1 MB */);
 }