Copilot commented on code in PR #16285:
URL: https://github.com/apache/dubbo/pull/16285#discussion_r3377451948
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyClient.java:
##########
@@ -59,11 +59,11 @@
*/
public class NettyClient extends AbstractClient {
- private static final String SOCKS_PROXY_HOST = "socksProxyHost";
+ public static final String SOCKS_PROXY_HOST = "socksProxyHost";
- private static final String SOCKS_PROXY_PORT = "socksProxyPort";
+ public static final String SOCKS_PROXY_PORT = "socksProxyPort";
- private static final String DEFAULT_SOCKS_PROXY_PORT = "1080";
+ public static final String DEFAULT_SOCKS_PROXY_PORT = "1080";
Review Comment:
These SOCKS proxy property keys are only used within the
`org.apache.dubbo.remoting.transport.netty4` package (currently only by
`NettyConnectionClient`). Making them `public` unnecessarily expands the public
API surface; package-private constants would be sufficient and avoids external
coupling.
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -124,7 +132,16 @@ protected void initChannel(SocketChannel ch) {
// set null but do not close this client, it will be
reconnecting in the future
ch.closeFuture().addListener(channelFuture ->
clearNettyChannel());
- // TODO support Socks5
+ // support Socks5
+ String socksProxyHost =
+
ConfigurationUtils.getProperty(getUrl().getOrDefaultApplicationModel(),
SOCKS_PROXY_HOST);
+ if (StringUtils.isNotBlank(socksProxyHost) &&
!isFilteredAddress(getUrl().getHost())) {
+ int socksProxyPort =
Integer.parseInt(ConfigurationUtils.getProperty(
+ getUrl().getOrDefaultApplicationModel(),
SOCKS_PROXY_PORT, DEFAULT_SOCKS_PROXY_PORT));
Review Comment:
This change introduces SOCKS5 proxy behavior for tri connections, but there
doesn't appear to be any unit test coverage verifying that the
`Socks5ProxyHandler` is (or is not) added to the pipeline based on
configuration and target host. Adding a focused test would help prevent
regressions (e.g., ensuring proxy is skipped for local targets and enabled
otherwise).
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -227,4 +244,9 @@ private void waitConnectionPreface(long start) throws
RemotingException {
throw remotingException;
}
}
+
+ private boolean isFilteredAddress(String host) {
+ // filter local address
+ return StringUtils.isEquals(NetUtils.getLocalHost(), host) ||
NetUtils.isLocalHost(host);
+ }
Review Comment:
`isFilteredAddress` duplicates the same logic already present in
`NettyClient`. Keeping two copies increases the chance they drift over time;
consider extracting to a shared package-private utility (or making a shared
helper method) so both clients use the same filtering rules.
--
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]
