You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/09/14 22:24:43 UTC
[GitHub] [pulsar] rdhabalia opened a new pull request #8062: [pulsar-client] sni-proxy protocol should pass sni-host address without resolving
rdhabalia opened a new pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062
### Motivation
As described in #8056 , Pulsar-client should pass SNI host without resolving in case of sni-proxy.
+ @racorn
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-696394513
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-694607907
@racorn can you PTAL before merging the change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia merged pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia merged pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-696366201
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-696366201
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-692947506
@racorn
that's a good suggestion
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] racorn commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
racorn commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-694807444
@rdhabalia Thanks for the update. Looks good to me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] racorn commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
racorn commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-693224651
@rdhabalia Agree.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] racorn edited a comment on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
racorn edited a comment on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-692653350
@rdhabalia Thanks for adding thread safety.
I notice the use of a mutex. A lock-free alternative would be more involved, not sure if would be worth the effort. Anyway, a possible solution could be to have the `ConnectionPool` take more responsibility of bootstrapping TLS connections. For example invoking `Bootstrap.register` instead of `Bootstrap.connect`, and adding the `SSLHandler` to the channel pipeline outside of `PulsarChannelInitializer.initChannel()` before connecting the channel to the remote host. The `SSLHandler` could be added in a new method in `PulsarChannelInitializer`, for example `CompletableFuture<Channel> initSSL(Channel channel, String sniHostName, int sniHostPort)`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia merged pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia merged pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] racorn commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
racorn commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-695984056
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] racorn commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
racorn commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-695984056
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-692553930
@merlimat can you also please review the PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-696366201
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-694420898
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] racorn commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
racorn commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-693001175
@rdhabalia Thank you were much! It looks good, my only concern is that now `private CompletableFuture<Channel> connectToAddress(InetAddress ipAddress, int port, InetSocketAddress sniHost)` does synchronous channel initialization.
To do asynchronous channel initialization, this is a possible attempt:
```
private CompletableFuture<Channel> connectToAddress(final InetAddress ipAddress, final int port,
final InetSocketAddress sniHost) {
CompletableFuture<Channel> future = new CompletableFuture<>();
// if proxy is configured in pulsar-client then make it thread-safe while updating
// channelInitializerHandler
if (isSniProxy) {
bootstrap.register().addListener((ChannelFutureListener) cf -> {
if (cf.isSuccess()) {
try {
channelInitializerHandler.initChannel(cf.channel(), sniHost);
cf.channel().connect(new InetSocketAddress(ipAddress, port))
.addListener((ChannelFutureListener) channelFuture -> {
if (channelFuture.isSuccess()) {
future.complete(channelFuture.channel());
} else {
future.completeExceptionally(channelFuture.cause());
}
});
} catch (Exception e) {
log.warn("Failed to initialize channel with {}, {}", ipAddress, sniHost, e);
future.completeExceptionally(e);
}
} else {
future.completeExceptionally(cf.cause());
}
});
} else {
bootstrap.connect(ipAddress, port).addListener((ChannelFutureListener) cf -> {
if (cf.isSuccess()) {
future.complete(cf.channel());
} else {
future.completeExceptionally(cf.cause());
}
});
}
return future;
}
```
Here, the `PulsarChannelInitializer.initChannel(cf.channel(), sniHost)` is run in the callback thread, I assume it will be the Channel event loop thread. If that is not good enough, one could take it a step further and make `PulsarChannelInitializer.initChannel(cf.channel(), sniHost)` private, then add a new method to `PulsarChannelInitializer`:
```
public CompletableFuture<Channel> initSniSsl(final Channel ch, final InetSocketAddress sniHost) {
if (!isSniProxyEnabled) {
throw new IllegalStateException("This method can only be invoked when SniProxy is enabled");
}
CompletableFuture<Channel> initFuture = new CompletableFuture<>();
ch.eventLoop().execute(() -> {
try {
initChannel(ch, sniHost);
initFuture.complete(ch);
} catch (Throwable t) {
initFuture.completeExceptionally(t);
}
});
return initFuture;
}
```
Then update ConnectionPool:
```
private CompletableFuture<Channel> connectToAddress(final InetAddress ipAddress, final int port,
final InetSocketAddress sniHost) {
CompletableFuture<Channel> future = new CompletableFuture<>();
// if proxy is configured in pulsar-client then make it thread-safe while updating
// channelInitializerHandler
if (isSniProxy) {
bootstrap.register().addListener((ChannelFutureListener) cf -> {
if (cf.isSuccess()) {
try {
initSniSsl(cf.channel(), sniHost)
.whenComplete((channel, error) -> {
if (error != null) {
future.completeExceptionally(error);
} else {
channel.connect(new InetSocketAddress(ipAddress, port))
.addListener((ChannelFutureListener) channelFuture -> {
if (channelFuture.isSuccess()) {
future.complete(channelFuture.channel());
} else {
future.completeExceptionally(cf.cause());
}
});
}
});
} catch (Throwable t) {
future.completeExceptionally(t);
}
} else {
future.completeExceptionally(cf.cause());
}
});
} else {
bootstrap.connect(ipAddress, port).addListener((ChannelFutureListener) cf -> {
if (cf.isSuccess()) {
future.complete(cf.channel());
} else {
future.completeExceptionally(cf.cause());
}
});
}
return future;
}
```
A bit complicated when having to deal with both ChannelFutures and CompletableFutures :-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-693055027
`bootstrap.register().addListener((ChannelFutureListener) cf -> {..}));` gets executed into io thread which is used by eventloop so, both solutions create connection on io thread instead caller-thread. but first approach seems cleaner so, we can go with first one and let io thread handle connection creation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] racorn commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
racorn commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-692653350
@rdhabalia Thanks for adding thread safety.
I notice the use of a mutex. A lock-free alternative would be more involved, not sure if would be worth the effort. Anyway, a possible solution could be to have the `ConnectionPool` take more responsibility of bootstrapping TLS connections. For example invoking `Bootstrap.register` instead of `Bootstrap.connect`, and adding the `SSLHandler` to the channel pipeline outside of `PulsarChannelInitializer.initChannel()` before connecting the channel to the remote host. The `SSLHandler` could be added in a new method in `PulsarChannelInitializer`, for example `initSSL(Channel channel, String sniHostName, int sniHostPort)`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia merged pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia merged pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] racorn commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
racorn commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-695016776
@rdhabalia if you have not merged yet, maybe fix cosmetic stuff in ConnectionPool:
```
Channel channel = cf.channel();
try {
channelInitializerHandler.initChannel(cf.channel(), sniHost);
channel.connect(new InetSocketAddress(ipAddress, port)).addListener((ChannelFuture channelFuture) -> {
```
Use `channel` instead of `cf.channel()` in `channelInitializerHandler.initChannel(cf.channel(), sniHost);`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8062: [pulsar-client] Fix: avoid resolving address for sni-host + thread-safe connection creation
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8062:
URL: https://github.com/apache/pulsar/pull/8062#issuecomment-692955264
@racorn
I have optimized it accordingly, PTAL.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org