-
Notifications
You must be signed in to change notification settings - Fork 7.1k
testing Revert "[5/n] IPv6 support: Add IPv6 localhost support and eliminate … #60003
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
Conversation
…0.0.0.0 server bindings (ray-project#59852)" This reverts commit 60bb749.
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.
Code Review
This pull request appears to be reverting recently added IPv6 support, falling back to IPv4-only logic in many parts of the codebase. The changes span across Python and C++ files, replacing dynamic IP address resolution with hardcoded IPv4 addresses.
My review focuses on the correctness and consistency of this revert. While the Python changes seem correct, I've identified a recurring critical issue in the C++ changes. The logic to determine if a server should bind to localhost is incomplete, as it only checks for "127.0.0.1" and misses the "localhost" hostname case. This could lead to servers unintentionally binding to all interfaces.
| auto core_worker_server = | ||
| std::make_unique<rpc::GrpcServer>(WorkerTypeString(options.worker_type), | ||
| assigned_port, | ||
| options.node_ip_address == "127.0.0.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.
This check for localhost is incomplete. It only checks for "127.0.0.1" and will fail if options.node_ip_address is "localhost". This would cause the gRPC server to incorrectly bind to all interfaces (0.0.0.0) instead of just localhost. To fix this, the check should also include "localhost".
| auto core_worker_server = | |
| std::make_unique<rpc::GrpcServer>(WorkerTypeString(options.worker_type), | |
| assigned_port, | |
| options.node_ip_address == "127.0.0.1"); | |
| auto core_worker_server = | |
| std::make_unique<rpc::GrpcServer>(WorkerTypeString(options.worker_type), | |
| assigned_port, | |
| options.node_ip_address == "127.0.0.1" || options.node_ip_address == "localhost"); |
| rpc_server_(config.grpc_server_name, | ||
| config.grpc_server_port, | ||
| config.node_ip_address, | ||
| config.node_ip_address == "127.0.0.1", | ||
| config.grpc_server_thread_num, | ||
| /*keepalive_time_ms=*/RayConfig::instance().grpc_keepalive_time_ms()), |
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.
This check for localhost is incomplete. It only checks for "127.0.0.1" and will fail if config.node_ip_address is "localhost". This would cause the gRPC server to incorrectly bind to all interfaces (0.0.0.0) instead of just localhost. To fix this, the check should also include "localhost".
| rpc_server_(config.grpc_server_name, | |
| config.grpc_server_port, | |
| config.node_ip_address, | |
| config.node_ip_address == "127.0.0.1", | |
| config.grpc_server_thread_num, | |
| /*keepalive_time_ms=*/RayConfig::instance().grpc_keepalive_time_ms()), | |
| rpc_server_(config.grpc_server_name, | |
| config.grpc_server_port, | |
| config.node_ip_address == "127.0.0.1" || config.node_ip_address == "localhost", | |
| config.grpc_server_thread_num, | |
| /*keepalive_time_ms=*/RayConfig::instance().grpc_keepalive_time_ms()), |
| object_manager_server_("ObjectManager", | ||
| config_.object_manager_port, | ||
| config_.object_manager_address, | ||
| config_.object_manager_address == "127.0.0.1", | ||
| config_.rpc_service_threads_number), | ||
| client_call_manager_(main_service, |
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.
This check for localhost is incomplete. It only checks for "127.0.0.1" and will fail if config_.object_manager_address is "localhost". This would cause the server to incorrectly bind to all interfaces (0.0.0.0) instead of just localhost. To fix this, the check should also include "localhost".
| object_manager_server_("ObjectManager", | |
| config_.object_manager_port, | |
| config_.object_manager_address, | |
| config_.object_manager_address == "127.0.0.1", | |
| config_.rpc_service_threads_number), | |
| client_call_manager_(main_service, | |
| object_manager_server_("ObjectManager", | |
| config_.object_manager_port, | |
| config_.object_manager_address == "127.0.0.1" || config_.object_manager_address == "localhost", | |
| config_.rpc_service_threads_number), |
| node_manager_server_("NodeManager", | ||
| config.node_manager_port, | ||
| config.node_manager_address == "127.0.0.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.
This check for localhost is incomplete. It only checks for "127.0.0.1" and will fail if config.node_manager_address is "localhost". This would cause the server to incorrectly bind to all interfaces (0.0.0.0) instead of just localhost. To fix this, the check should also include "localhost".
| node_manager_server_("NodeManager", | |
| config.node_manager_port, | |
| config.node_manager_address == "127.0.0.1"), | |
| node_manager_server_("NodeManager", | |
| config.node_manager_port, | |
| config.node_manager_address == "127.0.0.1" || config.node_manager_address == "localhost"), |
…0.0.0.0 server bindings (ray-project#59852)" This reverts commit 60bb749. Signed-off-by: zac <[email protected]>
…0.0.0.0 server bindings (#59852)"
This reverts commit 60bb749.
Description
Related issues
Additional information