lhotari commented on code in PR #23110:
URL: https://github.com/apache/pulsar/pull/23110#discussion_r1716353523
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -144,34 +150,20 @@ public WebService(PulsarService pulsar) throws
PulsarServerException {
Optional<Integer> tlsPort = config.getWebServicePortTls();
if (tlsPort.isPresent()) {
try {
- SslContextFactory sslCtxFactory;
- if (config.isTlsEnabledWithKeyStore()) {
- sslCtxFactory =
JettySslContextFactory.createServerSslContextWithKeystore(
- config.getWebServiceTlsProvider(),
- config.getTlsKeyStoreType(),
- config.getTlsKeyStore(),
- config.getTlsKeyStorePassword(),
- config.isTlsAllowInsecureConnection(),
- config.getTlsTrustStoreType(),
- config.getTlsTrustStore(),
- config.getTlsTrustStorePassword(),
- config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
- config.getTlsCertRefreshCheckDurationSec()
- );
- } else {
- sslCtxFactory =
JettySslContextFactory.createServerSslContext(
- config.getWebServiceTlsProvider(),
- config.isTlsAllowInsecureConnection(),
- config.getTlsTrustCertsFilePath(),
- config.getTlsCertificateFilePath(),
- config.getTlsKeyFilePath(),
- config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
- config.getTlsCertRefreshCheckDurationSec());
- }
+ PulsarSslConfiguration sslConfiguration =
buildSslConfiguration(config);
+ this.sslFactory = (PulsarSslFactory)
Class.forName(config.getSslFactoryPlugin())
+ .getConstructor().newInstance();
+ this.sslFactory.initialize(sslConfiguration);
+ this.sslFactory.createInternalSslContext();
+ this.scheduledExecutorService = this.pulsar.getExecutor();
+
this.scheduledExecutorService.scheduleWithFixedDelay(this::refreshSslContext,
Review Comment:
a reference to this scheduled task should be kept in a field and cancelled
in the close method. Otherwise we will end up with a lot of resource leaks in
tests.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -144,34 +150,20 @@ public WebService(PulsarService pulsar) throws
PulsarServerException {
Optional<Integer> tlsPort = config.getWebServicePortTls();
if (tlsPort.isPresent()) {
try {
- SslContextFactory sslCtxFactory;
- if (config.isTlsEnabledWithKeyStore()) {
- sslCtxFactory =
JettySslContextFactory.createServerSslContextWithKeystore(
- config.getWebServiceTlsProvider(),
- config.getTlsKeyStoreType(),
- config.getTlsKeyStore(),
- config.getTlsKeyStorePassword(),
- config.isTlsAllowInsecureConnection(),
- config.getTlsTrustStoreType(),
- config.getTlsTrustStore(),
- config.getTlsTrustStorePassword(),
- config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
- config.getTlsCertRefreshCheckDurationSec()
- );
- } else {
- sslCtxFactory =
JettySslContextFactory.createServerSslContext(
- config.getWebServiceTlsProvider(),
- config.isTlsAllowInsecureConnection(),
- config.getTlsTrustCertsFilePath(),
- config.getTlsCertificateFilePath(),
- config.getTlsKeyFilePath(),
- config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
- config.getTlsCertRefreshCheckDurationSec());
- }
+ PulsarSslConfiguration sslConfiguration =
buildSslConfiguration(config);
+ this.sslFactory = (PulsarSslFactory)
Class.forName(config.getSslFactoryPlugin())
+ .getConstructor().newInstance();
+ this.sslFactory.initialize(sslConfiguration);
+ this.sslFactory.createInternalSslContext();
+ this.scheduledExecutorService = this.pulsar.getExecutor();
Review Comment:
Keeping the reference in a field doesn't seem to be necessary in this case,
the field could be removed.
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConnectionPool.java:
##########
@@ -103,20 +104,25 @@ private static class Key {
}
public ConnectionPool(InstrumentProvider instrumentProvider,
- ClientConfigurationData conf, EventLoopGroup
eventLoopGroup) throws PulsarClientException {
- this(instrumentProvider, conf, eventLoopGroup, () -> new
ClientCnx(instrumentProvider, conf, eventLoopGroup));
+ ClientConfigurationData conf, EventLoopGroup
eventLoopGroup,
+ ScheduledExecutorProvider scheduledExecutorProvider)
throws PulsarClientException {
Review Comment:
It seems that this could simple take a ScheduledExecutorService. The
coupling to ScheduledExecutorProvider seems unnecessary.
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ConnectionPoolTest.java:
##########
@@ -138,15 +146,19 @@ public void testDoubleIpAddress() throws Exception {
client.close();
eventLoop.shutdownGracefully();
+ executorProvider.shutdownNow();
Review Comment:
Instead of this, you could have `@Cleanup("shutdownNow")` above the
`ScheduledExecutorProvider executorProvider = ...` line.
##########
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/WebServer.java:
##########
@@ -121,34 +130,20 @@ public WebServer(ProxyConfiguration config,
AuthenticationService authentication
}
if (config.getWebServicePortTls().isPresent()) {
try {
- SslContextFactory sslCtxFactory;
- if (config.isTlsEnabledWithKeyStore()) {
- sslCtxFactory =
JettySslContextFactory.createServerSslContextWithKeystore(
- config.getWebServiceTlsProvider(),
- config.getTlsKeyStoreType(),
- config.getTlsKeyStore(),
- config.getTlsKeyStorePassword(),
- config.isTlsAllowInsecureConnection(),
- config.getTlsTrustStoreType(),
- config.getTlsTrustStore(),
- config.getTlsTrustStorePassword(),
- config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
- config.getTlsCertRefreshCheckDurationSec()
- );
- } else {
- sslCtxFactory =
JettySslContextFactory.createServerSslContext(
- config.getWebServiceTlsProvider(),
- config.isTlsAllowInsecureConnection(),
- config.getTlsTrustCertsFilePath(),
- config.getTlsCertificateFilePath(),
- config.getTlsKeyFilePath(),
- config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
- config.getTlsCertRefreshCheckDurationSec());
- }
+ this.scheduledExecutorService =
Executors.newSingleThreadScheduledExecutor(
Review Comment:
Cleanup is missing for this executor. It should be handled in the stop
method.
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarChannelInitializer.java:
##########
@@ -55,18 +53,16 @@ public class PulsarChannelInitializer extends
ChannelInitializer<SocketChannel>
@Getter
private final boolean tlsEnabled;
private final boolean tlsHostnameVerificationEnabled;
- private final boolean tlsEnabledWithKeyStore;
private final InetSocketAddress socks5ProxyAddress;
private final String socks5ProxyUsername;
private final String socks5ProxyPassword;
- private final Supplier<SslContext> sslContextSupplier;
- private NettySSLContextAutoRefreshBuilder
nettySSLContextAutoRefreshBuilder;
+ private final PulsarSslFactory pulsarSslFactory;
private static final long TLS_CERTIFICATE_CACHE_MILLIS =
TimeUnit.MINUTES.toMillis(1);
- public PulsarChannelInitializer(ClientConfigurationData conf,
Supplier<ClientCnx> clientCnxSupplier)
- throws Exception {
+ public PulsarChannelInitializer(ClientConfigurationData conf,
Supplier<ClientCnx> clientCnxSupplier,
+ ScheduledExecutorProvider
scheduledExecutorProvider) throws Exception {
Review Comment:
It seems that this could simply take a ScheduledExecutorService. The
coupling to ScheduledExecutorProvider seems unnecessary.
##########
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ServiceChannelInitializer.java:
##########
@@ -59,51 +62,25 @@ public ServiceChannelInitializer(ProxyService proxyService,
ProxyConfiguration s
this.maxMessageSize = serviceConfig.getMaxMessageSize();
if (enableTls) {
- if (tlsEnabledWithKeyStore) {
- serverSSLContextAutoRefreshBuilder = new
NettySSLContextAutoRefreshBuilder(
- serviceConfig.getTlsProvider(),
- serviceConfig.getTlsKeyStoreType(),
- serviceConfig.getTlsKeyStore(),
- serviceConfig.getTlsKeyStorePassword(),
- serviceConfig.isTlsAllowInsecureConnection(),
- serviceConfig.getTlsTrustStoreType(),
- serviceConfig.getTlsTrustStore(),
- serviceConfig.getTlsTrustStorePassword(),
- serviceConfig.isTlsRequireTrustedClientCertOnConnect(),
- serviceConfig.getTlsCiphers(),
- serviceConfig.getTlsProtocols(),
- serviceConfig.getTlsCertRefreshCheckDurationSec());
- } else {
- SslProvider sslProvider = null;
- if (serviceConfig.getTlsProvider() != null) {
- sslProvider =
SslProvider.valueOf(serviceConfig.getTlsProvider());
- }
- serverSslCtxRefresher = new NettyServerSslContextBuilder(
- sslProvider,
- serviceConfig.isTlsAllowInsecureConnection(),
- serviceConfig.getTlsTrustCertsFilePath(),
serviceConfig.getTlsCertificateFilePath(),
- serviceConfig.getTlsKeyFilePath(),
serviceConfig.getTlsCiphers(),
- serviceConfig.getTlsProtocols(),
- serviceConfig.isTlsRequireTrustedClientCertOnConnect(),
- serviceConfig.getTlsCertRefreshCheckDurationSec());
- }
- } else {
- this.serverSslCtxRefresher = null;
+ this.scheduledExecutorService =
Executors.newSingleThreadScheduledExecutor(
Review Comment:
cleanup is missing for this executor. It might be easier to pass a reference
from ProxyService and handle the cleanup there.
##########
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java:
##########
@@ -98,7 +102,14 @@ class AdminProxyHandler extends ProxyServlet {
: config.getBrokerWebServiceURL();
this.functionWorkerWebServiceUrl = config.isTlsEnabledWithBroker() ?
config.getFunctionWorkerWebServiceURLTLS()
: config.getFunctionWorkerWebServiceURL();
-
+ if (config.isTlsEnabledWithBroker()) {
+ this.pulsarSslFactory = createPulsarSslFactory();
+ this.scheduledExecutorService =
Executors.newSingleThreadScheduledExecutor(
Review Comment:
the cleanup for scheduledExecutorService is missing. Since this is a
Servlet, you should be able to override `destroy` method which handles this.
`destroy` should also call `super.destroy()`.
--
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]
