Copilot commented on code in PR #3308:
URL: https://github.com/apache/brpc/pull/3308#discussion_r3295972897
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -163,6 +164,23 @@ class HttpTest : public ::testing::Test{
EXPECT_EQ(expect, brpc::policy::VerifyHttpRequest(msg));
}
+ void VerifyMessageFromLocalPort(brpc::InputMessageBase* msg,
+ bool expect,
+ int local_port) {
+ brpc::SocketId id;
+ brpc::SocketOptions options;
+ options.fd = dup(_pipe_fds[1]);
+ EXPECT_GE(options.fd, 0);
+ options.local_side = butil::EndPoint(butil::my_ip(), local_port);
+ EXPECT_EQ(0, brpc::Socket::Create(options, &id));
+
+ brpc::SocketUniquePtr socket;
+ EXPECT_EQ(0, brpc::Socket::Address(id, &socket));
+ socket->ReAddress(&msg->_socket);
Review Comment:
VerifyMessageFromLocalPort() dup()s a fd and then continues even if
dup/Create/Address fail (EXPECT_* are non-fatal). This can leak the duplicated
fd on failures and may dereference/operate on an invalid socket. Consider
guarding the dup'ed fd with butil::fd_guard (releasing it only after
Socket::Create succeeds) and using ASSERT_* to stop the helper on setup
failures.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -379,6 +438,97 @@ TEST_F(HttpTest, verify_request) {
}
}
+TEST_F(HttpTest, verify_builtin_request_on_internal_port) {
+ _server._options.internal_port = 9527;
+ {
+ brpc::policy::HttpContext* msg = MakeGetRequestMessage("/status");
+ VerifyMessage(msg, false);
+ msg->Destroy();
+ }
+ {
+ brpc::policy::HttpContext* msg = MakeGetRequestMessage("/status");
+ VerifyMessageFromLocalPort(msg, true, _server._options.internal_port);
+ msg->Destroy();
+ }
+}
+
+TEST_F(HttpTest, builtin_auth_policy_on_public_and_internal_port) {
+ const int saved_max_connection_pool_size =
brpc::FLAGS_max_connection_pool_size;
+ brpc::FLAGS_max_connection_pool_size = 1;
+
+ butil::EndPoint ep;
+ ASSERT_EQ(0, str2endpoint("127.0.0.1:0", &ep));
+
+ brpc::Server server;
+ MyEchoService svc;
+ MyAuthenticator auth;
+ brpc::ServerOptions options;
+ options.auth = &auth;
+ options.internal_port = AllocateFreePortOrDie();
+ ASSERT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE));
+ ASSERT_EQ(0, server.Start(ep, &options));
+ ep = server.listen_address();
+ const butil::EndPoint internal_ep(ep.ip, options.internal_port);
+
+ {
+ brpc::Channel chan;
+ brpc::ChannelOptions copt;
+ copt.protocol = brpc::PROTOCOL_HTTP;
+ copt.max_retry = 0;
+ ASSERT_EQ(0, chan.Init(ep, &copt));
+
+ brpc::Controller cntl;
+ cntl.http_request().uri() = "/status";
+ cntl.http_request().set_method(brpc::HTTP_METHOD_GET);
+ chan.CallMethod(NULL, &cntl, NULL, NULL, NULL);
+ ASSERT_TRUE(cntl.Failed());
+ ASSERT_EQ(brpc::EHTTP, cntl.ErrorCode()) << cntl.ErrorText();
+ ASSERT_EQ(brpc::HTTP_STATUS_FORBIDDEN,
cntl.http_response().status_code());
+ }
+
+ {
+ brpc::Channel chan;
+ brpc::ChannelOptions copt;
+ copt.protocol = brpc::PROTOCOL_HTTP;
+ copt.max_retry = 0;
+ ASSERT_EQ(0, chan.Init(internal_ep, &copt));
+
+ brpc::Controller cntl;
+ cntl.http_request().uri() = "/status";
+ cntl.http_request().set_method(brpc::HTTP_METHOD_GET);
+ chan.CallMethod(NULL, &cntl, NULL, NULL, NULL);
+ ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText();
+ ASSERT_EQ(brpc::HTTP_STATUS_OK, cntl.http_response().status_code());
+ }
+
+ {
+ const std::string connection_group = "builtin-auth-policy";
+ brpc::Channel builtin_channel;
+ brpc::Channel protected_channel;
+ brpc::ChannelOptions copt;
+ copt.protocol = brpc::PROTOCOL_HTTP;
+ copt.connection_type = brpc::CONNECTION_TYPE_POOLED;
+ copt.connection_group = connection_group;
+ copt.max_retry = 0;
+ ASSERT_EQ(0, builtin_channel.Init(ep, &copt));
+ ASSERT_EQ(0, protected_channel.Init(ep, &copt));
+
+ brpc::Controller builtin_cntl;
+ CallVersion(&builtin_channel, &builtin_cntl);
+ ASSERT_TRUE(builtin_cntl.Failed());
+ ASSERT_EQ(brpc::EHTTP, builtin_cntl.ErrorCode()) <<
builtin_cntl.ErrorText();
+ ASSERT_EQ(brpc::HTTP_STATUS_FORBIDDEN,
builtin_cntl.http_response().status_code());
+
+ brpc::Controller protected_cntl;
+ CallHttpEcho(&protected_channel, &protected_cntl);
+ ASSERT_TRUE(protected_cntl.Failed());
Review Comment:
The pooled-channel part of builtin_auth_policy_on_public_and_internal_port
only asserts protected_cntl.Failed(), which can pass for unrelated failures
(e.g. connection reset) and doesn’t actually verify the expected auth
rejection. Consider asserting the specific failure (e.g. ErrorCode == EHTTP and
HTTP status == 403) so the test reliably validates the intended behavior.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -300,6 +345,14 @@ class HttpTest : public ::testing::Test{
MyAuthenticator _auth;
};
+int AllocateFreePortOrDie() {
+ butil::fd_guard fd(tcp_listen(butil::EndPoint(butil::my_ip(), 0)));
+ EXPECT_GE(fd, 0);
+ butil::EndPoint point;
+ EXPECT_EQ(0, butil::get_local_side(fd, &point));
Review Comment:
AllocateFreePortOrDie() uses EXPECT_* and may return an invalid port (e.g.
0) when tcp_listen/get_local_side fail, despite the "OrDie" name implying it
cannot. This can make the subsequent server.Start() failure cascade and also
interact badly with the global flag mutation in this test. Consider making the
helper fail-fast (ASSERT/CHECK) or returning an explicit error and handling it
at the call site (and/or renaming the function).
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -379,6 +438,97 @@ TEST_F(HttpTest, verify_request) {
}
}
+TEST_F(HttpTest, verify_builtin_request_on_internal_port) {
+ _server._options.internal_port = 9527;
+ {
+ brpc::policy::HttpContext* msg = MakeGetRequestMessage("/status");
+ VerifyMessage(msg, false);
+ msg->Destroy();
+ }
+ {
+ brpc::policy::HttpContext* msg = MakeGetRequestMessage("/status");
+ VerifyMessageFromLocalPort(msg, true, _server._options.internal_port);
+ msg->Destroy();
+ }
+}
+
+TEST_F(HttpTest, builtin_auth_policy_on_public_and_internal_port) {
+ const int saved_max_connection_pool_size =
brpc::FLAGS_max_connection_pool_size;
Review Comment:
builtin_auth_policy_on_public_and_internal_port mutates the global
brpc::FLAGS_max_connection_pool_size and restores it only at the end of the
test. If any ASSERT_* above fails, the flag won't be restored, which can break
later tests in the same binary. Please restore the flag via an RAII/scope-exit
guard (e.g. BRPC_SCOPE_EXIT) immediately after saving it.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -225,6 +243,33 @@ class HttpTest : public ::testing::Test{
return msg;
}
+ void InitHttpPooledChannel(brpc::Channel* channel,
+ const butil::EndPoint& ep,
+ const std::string& connection_group) {
+ brpc::ChannelOptions options;
+ options.protocol = brpc::PROTOCOL_HTTP;
+ options.connection_type = brpc::CONNECTION_TYPE_POOLED;
+ options.connection_group = connection_group;
+ options.max_retry = 0;
+ ASSERT_EQ(0, channel->Init(ep, &options));
+ }
+
+ void CallVersion(brpc::Channel* channel, brpc::Controller* cntl) {
+ cntl->http_request().uri() = "/status";
+ cntl->http_request().set_method(brpc::HTTP_METHOD_GET);
+ channel->CallMethod(NULL, cntl, NULL, NULL, NULL);
+ }
Review Comment:
CallVersion() sends a request to the "/status" builtin endpoint, so the name
is misleading and makes the test harder to read/maintain. Consider renaming it
to reflect what it actually calls (e.g. CallStatus/CallBuiltinStatus).
--
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]
