[CELEBORN-1348] Update infrastructure for SSL communication #2427
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mridulm
pushed a commit
to mridulm/celeborn
that referenced
this pull request
Mar 27, 2024
|
The test failure looks unrelated to this PR, please let me know if I am missing anything: |
|
+CC @otterc, @pan3793, @waitinfuture, @FMX |
FMX
reviewed
Mar 29, 2024
| * <p>If this method returns a ByteBuf, then that buffer's reference count will be incremented and | ||
| * the caller will be responsible for releasing this new reference. | ||
| */ | ||
| public abstract Object convertToNettyForSsl() throws IOException; |
There was a problem hiding this comment.
The encryption process will happen here in another PR?
There was a problem hiding this comment.
Yes, that is in a subsequent PR, which will use this method.
You can see this getting used in the consolidated PR (I am afk, so unable to point to exact details unfortunately).
This PR is only setting up the base infra for it.
FMX
approved these changes
Apr 1, 2024
RexXiong
approved these changes
Apr 1, 2024
CodingCat
pushed a commit
to CodingCat/incubator-celeborn
that referenced
this pull request
Apr 1, 2024
### What changes were proposed in this pull request? Update infrastructure for SSL support. Please see apache#2416 for the consolidated PR with all the changes for reference. ### Why are the changes needed? At a high level, the changes are: * `ManagedBuffer.convertToNettyForSsl`, to support SSL encryption. * Add `EncryptedMessageWithHeader`, which is used to wrap the message and body, for use with SSL. * `SslMessageEncoder` is an encoder for SSL ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? The overall PR apache#2416 (and this PR as well) passes all tests, and this PR includes relevant subset of tests. Closes apache#2427 from mridulm/update-infra-for-ssl. Authored-by: Mridul Muralidharan <mridulatgmail.com> Signed-off-by: SteNicholas <[email protected]>
|
Thanks for the reviews @FMX, @RexXiong and @SteNicholas ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Update infrastructure for SSL support.
Please see #2416 for the consolidated PR with all the changes for reference.
Why are the changes needed?
At a high level, the changes are:
ManagedBuffer.convertToNettyForSsl, to support SSL encryption.EncryptedMessageWithHeader, which is used to wrap the message and body, for use with SSL.SslMessageEncoderis an encoder for SSLDoes this PR introduce any user-facing change?
No.
How was this patch tested?
The overall PR #2416 (and this PR as well) passes all tests, and this PR includes relevant subset of tests.