Copilot commented on code in PR #3525:
URL: https://github.com/apache/brpc/pull/3525#discussion_r3948569644
##########
test/brpc_redis_unittest.cpp:
##########
@@ -1223,6 +1225,83 @@ TEST_F(RedisTest, server_sanity) {
ASSERT_EQ("", response.reply(3).data());
}
+// ServerOptions.internal_port has to be an explicit number, Server::Start()
+// rejects 0 because it stands for an ephemeral port. Let the system hand out a
+// free one instead of hardcoding a port another test may be listening on.
+static int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY,
0)));
+ EXPECT_LE(0, sockfd);
+ butil::EndPoint point;
+ EXPECT_EQ(0, butil::get_local_side(sockfd, &point));
+ return point.port;
+}
Review Comment:
This helper uses `EXPECT_*` but still returns a port unconditionally. If
either expectation fails, the function may return 0 (which `Server::Start()`
rejects for `internal_port`) or an uninitialized value, causing confusing
downstream failures. Prefer making the helper return a sentinel on failure
(e.g., -1) and assert in the caller, or restructure it to avoid `EXPECT_*`
inside the helper (so the test fails at the correct callsite). Also note
there’s still a race between picking the port and binding it; a small retry
loop around `server.Start()` when EADDRINUSE occurs can reduce flakes.
##########
test/brpc_server_unittest.cpp:
##########
@@ -1719,6 +1721,133 @@ TEST_F(ServerTest,
builtin_services_are_gated_by_internal_port) {
ASSERT_EQ(0, server.Join());
}
+// Call the same ordinary service the way a browser would.
+void CallEchoByHttp(const butil::EndPoint& ep, brpc::Controller* cntl) {
+ brpc::ChannelOptions copt;
+ copt.protocol = brpc::PROTOCOL_HTTP;
+ copt.max_retry = 0;
+ brpc::Channel chan;
+ ASSERT_EQ(0, chan.Init(ep, &copt));
+ test::EchoRequest req;
+ test::EchoResponse res;
+ req.set_message(EXP_REQUEST);
+ cntl->http_request().uri() = "/EchoService/Echo";
+ cntl->http_request().set_method(brpc::HTTP_METHOD_POST);
+ cntl->http_request().set_content_type("application/json");
+ chan.CallMethod(nullptr, cntl, &req, &res, nullptr);
+}
+
+TEST_F(ServerTest, ordinary_services_are_not_served_on_internal_port) {
+ const struct {
+ brpc::ProtocolType protocol;
+ const char* name;
+ } cases[] = {
+ { brpc::PROTOCOL_BAIDU_STD, "baidu_std" },
+ { brpc::PROTOCOL_HULU_PBRPC, "hulu_pbrpc" },
+ { brpc::PROTOCOL_SOFA_PBRPC, "sofa_pbrpc" },
+ };
+
+ butil::EndPoint ep;
+ ASSERT_EQ(0, str2endpoint("127.0.0.1:8613", &ep));
+ butil::EndPoint internal_ep;
+ ASSERT_EQ(0, str2endpoint("127.0.0.1:8614", &internal_ep));
+
+ brpc::Server server;
+ EchoServiceImpl echo_svc;
+ ASSERT_EQ(0, server.AddService(&echo_svc,
brpc::SERVER_DOESNT_OWN_SERVICE));
+ brpc::ServerOptions opt;
+ opt.internal_port = internal_ep.port;
+ ASSERT_EQ(0, server.Start(ep, &opt));
Review Comment:
These new tests hardcode ports (8613/8614). This can make the test suite
flaky under parallel CI runs or when ports linger in TIME_WAIT / are already
occupied. Consider binding the public listener to an ephemeral port (e.g.,
`127.0.0.1:0` or a `PortRange`) and selecting a free `internal_port`
dynamically (e.g., via a helper like `PickUnusedPort()` plus
retry-on-EADDRINUSE), so the tests don’t depend on fixed ports.
##########
src/brpc/server.h:
##########
@@ -202,6 +202,16 @@ struct ServerOptions {
// hiding them from public. Setting this option also enables security
// protection code which we may add constantly.
// Update: this option affects Tabbed services as well.
+ // Update: this port carries builtin and Tabbed services only, requests
+ // for ordinary services are rejected with EPERM and must be sent to the
+ // port passed to Start(). Builtin requests are exempted from
+ // ServerOptions.auth here and the exemption authenticates the connection
+ // they arrive on, hence ordinary services would be reachable without
+ // credentials from the same connection. http_master_service and
+ // baidu_master_service answer for every URL or service name, they are
+ // ignored on this port so that the builtin services behind them stay
+ // reachable. redis_service runs its command handlers too early for an
+ // EPERM to be sent back and is not served here at all.
Review Comment:
This header comment is very detailed and includes
protocol-/implementation-specific rationale (auth latching, master service
behavior, redis parser behavior). Consider shortening the header comment to a
concise contract (what is served/rejected on `internal_port`) and moving the
deeper rationale and exceptions to the docs (`docs/*/server.md`). That keeps
the public header easier to scan while still preserving the important
explanation in user-facing documentation.
##########
docs/en/server.md:
##########
@@ -680,9 +680,19 @@ Builtin services are useful, on the other hand include a
lot of internal informa
- Set internal port. Set ServerOptions.internal_port to a port which can
**only be accessible from internal**. You can view builtin services via
internal_port, while accesses from the public port (the one passed to
Server.Start) should see following error:
```
- [a27eda84bcdeef529a76f22872b78305] Not allowed to access builtin services,
try ServerOptions.internal_port=... instead if you're inside internal network
+ Not allowed to access builtin services, try ServerOptions.internal_port=...
instead if you're inside internal network
Review Comment:
The documented error text doesn’t match the new code path:
`Server::RejectBuiltinAccess()` uses “if you're in internal network” (not
“inside”). Either update the documentation to match the exact string, or avoid
quoting the message verbatim (describe it semantically) to prevent docs from
drifting whenever the message changes.
--
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]
