thweetkomputer commented on code in PR #3498:
URL: https://github.com/apache/brpc/pull/3498#discussion_r3975133612
##########
test/brpc_server_unittest.cpp:
##########
@@ -1424,6 +1425,159 @@ TEST_F(ServerTest, close_idle_connections) {
ASSERT_EQ(0ul, stat.connection_count);
}
+TEST_F(ServerTest, redis_connection_limit_requires_dedicated_listener) {
+ brpc::Server server;
+ EchoServiceImpl echo_service;
+ ASSERT_EQ(0, server.AddService(
+ &echo_service, brpc::SERVER_DOESNT_OWN_SERVICE));
+
+ brpc::ServerOptions opt;
+ opt.redis_service = new brpc::RedisService;
+ opt.redis_max_connections = 1;
+ opt.enabled_protocols = "redis";
+ opt.has_builtin_services = false;
+ const int rc = server.Start("127.0.0.1:0", &opt);
+ if (rc != 0) {
+ delete opt.redis_service;
+ opt.redis_service = nullptr;
+ }
+ EXPECT_EQ(-1, rc);
+}
+
+TEST_F(ServerTest, reject_redis_connections_over_limit) {
+ brpc::Server server;
+ brpc::ServerOptions opt;
+ opt.redis_service = new brpc::RedisService;
+ opt.redis_max_connections = 0;
+ opt.enabled_protocols = "redis";
+ opt.has_builtin_services = false;
+ ASSERT_EQ(0, server.Start("127.0.0.1:0", &opt));
+ ASSERT_EQ(0, server.SetRedisMaxConnections(1));
+
+ const butil::EndPoint ep = server.listen_address();
+ butil::fd_guard first_client(tcp_connect(ep, nullptr));
+ ASSERT_GT(first_client, 0);
Review Comment:
Fixed in 2c7f28478c03874a95f492d837afdb4abab9a9be: changed all six
fd-validity assertions added by this PR to `ASSERT_GE(fd, 0)`, so fd 0 is
accepted. Rebuilt `brpc_server_unittest` and reran the dedicated-listener,
plaintext/dynamic-limit, pre-TLS rejection, and idle-connection tests; all four
passed.
##########
src/brpc/server.cpp:
##########
@@ -870,6 +893,18 @@ int Server::StartInternal(const butil::EndPoint& endpoint,
const ServerOptions default_opt;
const ServerOptions& real_opt = opt ? *opt : default_opt;
+ // Admission happens before protocol parsing (and, importantly, before a
+ // TLS handshake), so it is only safe on a listener dedicated to Redis.
+ // Reject ambiguous configurations instead of accidentally limiting RPCs
+ // sharing the public port.
+ if (real_opt.redis_max_connections != 0 &&
+ !is_redis_only_public_listener(real_opt, service_count())) {
+ LOG(ERROR) << "redis_max_connections requires a Redis-only public "
+ "listener (redis_service set, enabled_protocols=redis, "
+ "no RPC or builtin services)";
Review Comment:
Fixed in 2c7f28478c03874a95f492d837afdb4abab9a9be: expanded both startup and
runtime setter validation messages to state that `redis_service` must be set,
`enabled_protocols` must be exactly "redis", builtin services must be disabled,
no RPC services may be registered, and all other protocol service pointers must
be null. The configuration checks themselves are unchanged. Rebuilt
`brpc_server_unittest`; all four focused tests passed, including the startup
and runtime rejection paths.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
