Copilot commented on code in PR #16285:
URL: https://github.com/apache/dubbo/pull/16285#discussion_r3377434900
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -124,7 +135,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 (socksProxyHost != null &&
!isFilteredAddress(getUrl().getHost())) {
+ int socksProxyPort =
Integer.parseInt(ConfigurationUtils.getProperty(
+ getUrl().getOrDefaultApplicationModel(),
SOCKS_PROXY_PORT, DEFAULT_SOCKS_PROXY_PORT));
+ Socks5ProxyHandler socks5ProxyHandler =
+ new Socks5ProxyHandler(new
InetSocketAddress(socksProxyHost, socksProxyPort));
+ pipeline.addFirst(socks5ProxyHandler);
+ }
Review Comment:
SOCKS5 proxy support is added here, but there is no unit test asserting that
`Socks5ProxyHandler` is (or is not) inserted into the pipeline based on
`socksProxyHost/socksProxyPort` and the filtered-address logic. There are
existing tests for `NettyConnectionClient` in this module; please add coverage
for this new behavior to prevent regressions.
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -124,7 +135,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 (socksProxyHost != null &&
!isFilteredAddress(getUrl().getHost())) {
Review Comment:
`ConfigurationUtils.getProperty(...)` trims values but can still return an
empty string; creating `InetSocketAddress("")` will lead to confusing
connection failures. Treat empty proxy host as "not configured" by using
`StringUtils.isNotEmpty(...)` in the guard.
--
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]
