You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "lhotari (via GitHub)" <gi...@apache.org> on 2024/05/28 22:58:50 UTC
[PR] Fix TLS stability issues with V2 protocol that caused data corruption [bookkeeper]
lhotari opened a new pull request, #4404:
URL: https://github.com/apache/bookkeeper/pull/4404
### Motivation
In Pulsar, there are multiple reports (for example apache/pulsar#22601, apache/pulsar#21421) about data corruption when TLS is enabled between Pulsar Brokers and Bookkeeper Bookies and when V2 protocol is enabled in broker.conf with the default configuration setting `bookkeeperUseV2WireProtocol=true`.
The reason to remove FlushConsolidationHandler while the TLS handshake is in progress is due to the fact that Netty SslHandler's STARTTLS support uses the flush event. My assumption is that having FlushConsolidationHandler in "front" of the flushes could cause problems.
- See the Netty SslHandler code in: https://github.com/netty/netty/blob/5085cef149134951c94a02a743ed70025c8cdad4/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L782-L806 .
However, it's possible that the root cause is different, but having FlushConsolidationHandler as the first handler (to only impact network socket flushes) after the TLS handshake could have some other side-effect which makes the problem go away. SslHandler will coalesce small buffers into a larger buffer and release them so the lifetime of small buffers is shorter.
It's also a mystery why the problem appears only when V2 protocol is used. That could hint to the direction that it's a release issue on a small buffer that gets merged into larger buffers and it reduces the occurrence of the hidden root cause issue.
Regardless of this concern, this PR contains changes that improve the current state of V2 protocol with TLS and it doesn't contain a negative impact.
### Changes
- remove FlushConsolidationHandler before adding the TLS handler to the channel pipeline
- add the FlushConsolidationHandler back to the pipeline as the first handler after the TLS handshake has completed
- This makes TLS connections from Pulsar Broker to Bookkeeper stable when `bookkeeperUseV2WireProtocol=true` is used
- Fix test TestTLS for V2
- Fix inconsistency in client configuration in BookKeeperClusterTestCase
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TLS stability issues with V2 protocol that caused data corruption [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #4404:
URL: https://github.com/apache/bookkeeper/pull/4404#issuecomment-2140983922
I can confirm that the problem that I was able to reproduce previously is fixed without making any changes in Pulsar or Bookkeeper. The only required fix was to switch to use Netty built from netty/netty#14086 .
(more details in https://github.com/apache/pulsar/pull/22810#issuecomment-2140979898)
This means that this PR apache/bookkeeper#4404 didn't address the root cause although it made the problem go away for some unknown reason.
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TLS stability issues with V2 protocol that caused data corruption [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #4404:
URL: https://github.com/apache/bookkeeper/pull/4404#issuecomment-2140189854
I can now confirm that this PR didn't fix the root cause.
The root cause is explained in this Netty PR: https://github.com/netty/netty/pull/14086 . One of the unit test failures in the `DUPLICATE_DIRECT_BUFFER` scenario verifies that a duplicate input buffer could get mutated. The way to prevent this is to do `.retainedSlice` instead of using `.retainedDuplicate`.
In the Pulsar side, I have created PR https://github.com/apache/pulsar/pull/22810 to mitigate issues and workaround the problems. I'll also create a similar PR to Bookkeeper to workaround the SslHandler behavior.
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TLS stability issues with V2 protocol that caused data corruption [bookkeeper]
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli merged PR #4404:
URL: https://github.com/apache/bookkeeper/pull/4404
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TLS stability issues with V2 protocol that caused data corruption [bookkeeper]
Posted by "shoothzj (via GitHub)" <gi...@apache.org>.
shoothzj commented on PR #4404:
URL: https://github.com/apache/bookkeeper/pull/4404#issuecomment-2136289328
Thank you for the detailed PR. Since Netty is an active project, do you think it would be possible to get confirmation from the Netty project team?
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TLS stability issues with V2 protocol that caused data corruption [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #4404:
URL: https://github.com/apache/bookkeeper/pull/4404#issuecomment-2136317118
@shoothzj I verified that the solution works also without removing and re-adding the handler. I changed the solution to add the TLS handler (SslHandler) after the FlushConsolidationHandler in the pipeline. That fixes the issue with V2 protocol stability. 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.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix TLS stability issues with V2 protocol that caused data corruption [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #4404:
URL: https://github.com/apache/bookkeeper/pull/4404#issuecomment-2136298822
> Thank you for the detailed PR. Since Netty is an active project, do you think it would be possible to get confirmation from the Netty project team?
What would you think that should be confirmed?
The javadoc already provides a recommendation:
> The [FlushConsolidationHandler](https://netty.io/4.1/api/io/netty/handler/flush/FlushConsolidationHandler.html) should be put as first [ChannelHandler](https://netty.io/4.1/api/io/netty/channel/ChannelHandler.html) in the [ChannelPipeline](https://netty.io/4.1/api/io/netty/channel/ChannelPipeline.html) to have the best effect.
I could test another change where I the SslHandler is added after the FlushConsolidationHandler instead of removing the handler during the handshake. As I mentioned in the description, the actual root cause might still be hidden, but this change definitely improves the situation since the reproducer https://github.com/lhotari/pulsar-playground/tree/master/issues/issue22601/standalone_env won't reproduce with this 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.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org