You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "kay-owolabi (via GitHub)" <gi...@apache.org> on 2023/12/05 20:44:22 UTC
[I] Fix Memory Leak in Long-Running JDBC Clients Post-Certificate Rotation [pinot]
kay-owolabi opened a new issue, #12100:
URL: https://github.com/apache/pinot/issues/12100
## Overview
This pull request introduces necessary changes to the `SSLContextHolder` and `TlsUtils` classes to address a memory leak issue in long-running JDBC clients. The memory leak is traced back to the static management of `SSL_CONTEXT`, which, while providing thread safety and initialization guarantees, does not account for scenarios involving SSL certificate rotation in long-running applications.
## Root Cause
- The `SSLContextHolder` class holds a static reference to `SSL_CONTEXT`, which is set via `TlsUtils#SSL_CONTEXT_REF`.
- This design ensures thread safety and at most once initialization, but it becomes problematic during SSL certificate rotations, particularly in long-running JDBC clients, leading to memory leaks.
## Proposed Solution
- **Dynamic SSL Context Management**: Implement a mechanism to allow controlled updates to the `SSL_CONTEXT` reference in a thread-safe manner, especially after certificate rotations.
- **Memory Leak Mitigation**: By enabling controlled updates, the solution aims to resolve the memory leak issues associated with the static SSL context in long-running connections.
## Benefits
- **Resolves Memory Leaks**: Directly addresses the memory leak issues in long-running JDBC clients following SSL certificate rotations.
- **Maintains Original Design Benefits**: Preserves the thread safety, initialization guarantees, and performance optimizations of the original design.
- **Adaptability to Certificate Rotation**: Adds the ability to adapt to certificate rotations, enhancing operational stability in long-running applications.
## Related Issue
Maybe:
https://github.com/AsyncHttpClient/async-http-client/issues/1658
--
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: commits-unsubscribe@pinot.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [I] Fix Memory Leak in Long-Running JDBC Clients Post-Certificate Rotation [pinot]
Posted by "kay-owolabi (via GitHub)" <gi...@apache.org>.
kay-owolabi commented on issue #12100:
URL: https://github.com/apache/pinot/issues/12100#issuecomment-1864397242
Hi @walterddr,
After investigating the SSL/TLS configuration issues, particularly related to `JsonAsyncHttpPinotClientTransport` and `PinotControllerTransport`, I've uncovered some key insights and a potential solution.
### Issue
The primary issue occurs when there's a change in the client certificate or TLS configuration. Subsequent attempts to establish a connection, such as obtaining a new `PinotDriver` connection, result in a handshake failure. This behavior is consistent and can be easily reproduced by rotating the client certificate.
### Findings
The crux of the problem appears to be linked to how the `SSLContext` is handled and updated. Initially, I explored making the `SSL_CONTEXT` in `TlsUtils` mutable. However, this approach did not yield the desired results. The usage of the `JdkSslContext` constructor in `AsyncHttpClient`, which is now deprecated, seems to contribute to what appears to be an SSL session caching issue with JDK-SSL.
### Proposed Solution
To address this, I propose changing how we update the SSL context in both `JsonAsyncHttpPinotClientTransport` and `PinotControllerTransport`. The key change involves setting the `SSLContext` based on the `TlsConfig` like so:
```java
builder.setSslContext(TlsUtils.buildClientContext(tlsConfig));
```
This should ensure the SSLContext is dynamically rebuilt and correctly integrated into the Netty SslContext. This modification should resolve the handshake failures post-certificate rotation or TLS config changes.
### Logs for Reference
Below are the relevant logs that highlight the issue:
##### PinotControllerTransport logs
```Log
Caused by: org.apache.pinot.client.PinotClientException: java.util.concurrent.ExecutionException: java.net.ConnectException: https://pinot-controller-url:PORT
at org.apache.pinot.client.controller.PinotControllerTransport.getBrokersFromController(PinotControllerTransport.java:141)
at org.apache.pinot.client.PinotConnection.getBrokerList(PinotConnection.java:125)
at org.apache.pinot.client.PinotConnection.<init>(PinotConnection.java:73)
at org.apache.pinot.client.PinotDriver.connect(PinotDriver.java:124)
... 12 common frames omitted
Caused by: java.util.concurrent.ExecutionException: java.net.ConnectException: https://pinot-controller-url:PORT
at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2096)
at org.apache.pinot.shaded.org.asynchttpclient.netty.NettyResponseFuture.get(NettyResponseFuture.java:206)
at org.apache.pinot.client.controller.response.ControllerResponseFuture.getStreamResponse(ControllerResponseFuture.java:71)
at org.apache.pinot.client.controller.response.ControllerTenantBrokerResponse$ControllerTenantBrokerResponseFuture.getStreamResponse(ControllerTenantBrokerResponse.java:69)
at org.apache.pinot.client.controller.response.ControllerTenantBrokerResponse$ControllerTenantBrokerResponseFuture.get(ControllerTenantBrokerResponse.java:79)
at org.apache.pinot.client.controller.response.ControllerTenantBrokerResponse$ControllerTenantBrokerResponseFuture.get(ControllerTenantBrokerResponse.java:69)
at org.apache.pinot.client.controller.response.ControllerResponseFuture.get(ControllerResponseFuture.java:60)
at org.apache.pinot.client.controller.response.ControllerTenantBrokerResponse$ControllerTenantBrokerResponseFuture.get(ControllerTenantBrokerResponse.java:69)
at org.apache.pinot.client.controller.PinotControllerTransport.getBrokersFromController(PinotControllerTransport.java:139)
... 15 common frames omitted
Caused by: java.net.ConnectException: https://pinot-controller-url:PORT
at org.apache.pinot.shaded.org.asynchttpclient.netty.channel.NettyConnectListener.onFailure(NettyConnectListener.java:179)
at org.apache.pinot.shaded.org.asynchttpclient.netty.channel.NettyConnectListener$1.onFailure(NettyConnectListener.java:151)
at org.apache.pinot.shaded.org.asynchttpclient.netty.SimpleFutureListener.operationComplete(SimpleFutureListener.java:26)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.notifyListener0(DefaultPromise.java:500)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.notifyListeners0(DefaultPromise.java:493)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.notifyListenersNow(DefaultPromise.java:472)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.notifyListeners(DefaultPromise.java:413)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.setValue0(DefaultPromise.java:538)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.setFailure0(DefaultPromise.java:531)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.tryFailure(DefaultPromise.java:111)
at nettyfourone.io.netty.handler.ssl.SslHandler.setHandshakeFailure(SslHandler.java:1788)
at nettyfourone.io.netty.handler.ssl.SslHandler.channelInactive(SslHandler.java:1070)
at nettyfourone.io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:257)
at nettyfourone.io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:243)
at nettyfourone.io.netty.channel.AbstractChannelHandlerContext.fireChannelInactive(AbstractChannelHandlerContext.java:236)
at nettyfourone.io.netty.channel.DefaultChannelPipeline$HeadContext.channelInactive(DefaultChannelPipeline.java:1416)
at nettyfourone.io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:257)
at nettyfourone.io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:243)
at nettyfourone.io.netty.channel.DefaultChannelPipeline.fireChannelInactive(DefaultChannelPipeline.java:912)
at nettyfourone.io.netty.channel.AbstractChannel$AbstractUnsafe$8.run(AbstractChannel.java:816)
at nettyfourone.io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:163)
at nettyfourone.io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:416)
at nettyfourone.io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:515)
at nettyfourone.io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:918)
at nettyfourone.io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at nettyfourone.io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
... 1 common frames omitted
Caused by: java.nio.channels.ClosedChannelException: null
at nettyfourone.io.netty.handler.ssl.SslHandler.channelInactive(SslHandler.java:1067)
... 15 common frames omitted
```
##### JsonAsyncHttpPinotClientTransport logs
```Log
Caused by: org.apache.pinot.client.PinotClientException: java.util.concurrent.ExecutionException: java.net.ConnectException: https://pinot-broker-url:PORT
at org.apache.pinot.client.JsonAsyncHttpPinotClientTransport.executeQuery(JsonAsyncHttpPinotClientTransport.java:101)
at org.apache.pinot.client.Connection.execute(Connection.java:124)
at org.apache.pinot.client.Connection.execute(Connection.java:94)
at org.apache.pinot.client.PinotStatement.executeQuery(PinotStatement.java:67)
... 82 common frames omitted
Caused by: java.util.concurrent.ExecutionException: java.net.ConnectException: https://pinot-broker-url:PORT
at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2096)
at org.apache.pinot.client.JsonAsyncHttpPinotClientTransport.executeQuery(JsonAsyncHttpPinotClientTransport.java:99)
... 85 common frames omitted
Caused by: java.net.ConnectException: https://pinot-broker-url:PORT
at org.apache.pinot.shaded.org.asynchttpclient.netty.channel.NettyConnectListener.onFailure(NettyConnectListener.java:179)
at org.apache.pinot.shaded.org.asynchttpclient.netty.channel.NettyConnectListener$1.onFailure(NettyConnectListener.java:151)
at org.apache.pinot.shaded.org.asynchttpclient.netty.SimpleFutureListener.operationComplete(SimpleFutureListener.java:26)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.notifyListener0(DefaultPromise.java:500)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.notifyListeners0(DefaultPromise.java:493)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.notifyListenersNow(DefaultPromise.java:472)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.notifyListeners(DefaultPromise.java:413)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.setValue0(DefaultPromise.java:538)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.setFailure0(DefaultPromise.java:531)
at nettyfourone.io.netty.util.concurrent.DefaultPromise.tryFailure(DefaultPromise.java:111)
at nettyfourone.io.netty.handler.ssl.SslHandler.setHandshakeFailure(SslHandler.java:1788)
at nettyfourone.io.netty.handler.ssl.SslHandler.channelInactive(SslHandler.java:1070)
at nettyfourone.io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:257)
at nettyfourone.io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:243)
at nettyfourone.io.netty.channel.AbstractChannelHandlerContext.fireChannelInactive(AbstractChannelHandlerContext.java:236)
at nettyfourone.io.netty.channel.DefaultChannelPipeline$HeadContext.channelInactive(DefaultChannelPipeline.java:1416)
at nettyfourone.io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:257)
at nettyfourone.io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:243)
at nettyfourone.io.netty.channel.DefaultChannelPipeline.fireChannelInactive(DefaultChannelPipeline.java:912)
at nettyfourone.io.netty.channel.AbstractChannel$AbstractUnsafe$8.run(AbstractChannel.java:816)
at nettyfourone.io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:163)
at nettyfourone.io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:416)
at nettyfourone.io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:515)
at nettyfourone.io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:918)
at nettyfourone.io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at nettyfourone.io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
... 1 common frames omitted
Caused by: java.nio.channels.ClosedChannelException: null
at nettyfourone.io.netty.handler.ssl.SslHandler.channelInactive(SslHandler.java:1067)
... 15 common frames omitted
```
The proposal should enhance the robustness of SSL/TLS handling in Pinot, particularly for scenarios involving certificate rotation. I would greatly appreciate your thoughts on this approach and any further insights you have.
Thank you for your attention to this matter.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [I] Fix Memory Leak in Long-Running JDBC Clients Post-Certificate Rotation [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on issue #12100:
URL: https://github.com/apache/pinot/issues/12100#issuecomment-1843403781
hmm I am not sure i understand the context here so let me clarify:
presumably you are referring to the pinot-java-client module `JsonAsyncHttpPinotClientTransport` --> which is the only client utilize async-http-client and `TlsUtils.SSLContextHolder`. correct?
IIUC there's to connections (clientTransport and controllerTransport) which are currently utilizing the same sslcontext modules. can you elaborate a more concrete example on how the SSLContext rotation is being configured?
one thing i notice is that the clientTransport deduce the sslcontext from property info and the controllerTransport utilizes the `TlsUtils.SSLContextHolder`; that means if the clientTransport is the issue we can potentially deal with it in a much smaller scale (e.g. they don't really have to be coupled)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [I] Fix Memory Leak in Long-Running JDBC Clients Post-Certificate Rotation [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on issue #12100:
URL: https://github.com/apache/pinot/issues/12100#issuecomment-1865430878
thank you for the detail analysis. I definitely agree with the findings. b/c this TlsUtils doesn't really use the static `SSL_CONTEXT_REF`
```
builder.setSslContext(TlsUtils.buildClientContext(tlsConfig));
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [I] Fix Memory Leak in Long-Running JDBC Clients Post-Certificate Rotation [pinot]
Posted by "kay-owolabi (via GitHub)" <gi...@apache.org>.
kay-owolabi commented on issue #12100:
URL: https://github.com/apache/pinot/issues/12100#issuecomment-1877472887
@walterddr and @Jackie-Jiang I think issue [12107](https://github.com/apache/pinot/issues/12107) is related
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [I] Fix Memory Leak in Long-Running JDBC Clients Post-Certificate Rotation [pinot]
Posted by "mayankshriv (via GitHub)" <gi...@apache.org>.
mayankshriv commented on issue #12100:
URL: https://github.com/apache/pinot/issues/12100#issuecomment-1841622694
@Jackie-Jiang @KKcorps
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [I] Fix Memory Leak in Long-Running JDBC Clients Post-Certificate Rotation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on issue #12100:
URL: https://github.com/apache/pinot/issues/12100#issuecomment-1841754027
@kay-owolabi Were you originally planning to send a PR? Currently it is files as an issue without any code attached.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org