Copilot commented on code in PR #16285:
URL: https://github.com/apache/dubbo/pull/16285#discussion_r3377379153
##########
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:
PR description indicates a unit test was added for this new SOCKS5 behavior,
but this change set only updates production code. Please add/adjust tests to
cover enabling/disabling SOCKS5 proxy via `socksProxyHost/socksProxyPort` and
verifying the proxy handler is installed (or that connections are routed
accordingly).
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -52,6 +56,12 @@
public final class NettyConnectionClient extends AbstractNettyConnectionClient
{
+ private static final String SOCKS_PROXY_HOST =
NettyClient.SOCKS_PROXY_HOST;
+
+ private static final String SOCKS_PROXY_PORT =
NettyClient.SOCKS_PROXY_PORT;
+
+ private static final String DEFAULT_SOCKS_PROXY_PORT =
NettyClient.DEFAULT_SOCKS_PROXY_PORT;
Review Comment:
`NettyConnectionClient` references
`NettyClient.SOCKS_PROXY_HOST/PORT/DEFAULT_SOCKS_PROXY_PORT`, but those fields
are `private` in `NettyClient` (same module), so this will not compile. Either
duplicate the string literals here or move these constants to a shared,
non-private location.
--
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]
