-
Notifications
You must be signed in to change notification settings - Fork 282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch to built-in security transports from core #4119
Conversation
0d71bb9
to
d29db0e
Compare
02f1041
to
342dc69
Compare
Signed-off-by: Andriy Redko <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4119 +/- ##
==========================================
- Coverage 66.04% 65.90% -0.14%
==========================================
Files 300 298 -2
Lines 21554 21425 -129
Branches 3488 3475 -13
==========================================
- Hits 14235 14121 -114
+ Misses 5571 5563 -8
+ Partials 1748 1741 -7
|
The work is not done :) but this is a first step towards removing secure transport part from the plugin, there are a few gaps that will be covered by opensearch-project/OpenSearch#12903 shortly |
src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @reta! The changes look good to me, I just left 2 small cosmetic comments.
I would be interested in seeing bwc tests for switching to secure transport in core while configured from the security plugin and eventually switching all configuration to core.
Have you tested this in a mixed cluster where some nodes are using the transport from core and some from the security plugin?
src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/SecurityNonSslHttpServerTransport.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Andriy Redko <[email protected]>
Thanks @cwperks ! No, I have not tested it yet but I test migrations for sure, thank you. The current implementation is cheating a bit since the secure transports are still provided by security plugin (although the implementation moved), but the next iteration (opensearch-project/OpenSearch#12907) should make it 100% fair. |
Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
@cwperks mind please retaking a look? thank you! |
Looks good to me! |
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-4119-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e2a06f001154d558c4c717c2a2bc54b9f57d1654
# Push it to GitHub
git push --set-upstream origin backport/backport-4119-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x Then, create a pull request where the |
…4119) The security plugin does not need to provide the secure transports anymore but SecureSettingsFactory so the core transport modules will be able to configure those. Closes opensearch-project#4118 Is this a backport? If so, please add backport PR # and/or commits # [Please provide details of testing done: unit testing, integration testing and manual testing] - [ ] New functionality includes testing - [ ] New functionality has been documented - [ ] Commits are signed per the DCO using --signoff By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Andriy Redko <[email protected]> (cherry picked from commit e2a06f0)
…4119) ### Description The security plugin does not need to provide the secure transports anymore but SecureSettingsFactory so the core transport modules will be able to configure those. ### Issues Resolved Closes opensearch-project#4118 Is this a backport? If so, please add backport PR # and/or commits # ### Testing [Please provide details of testing done: unit testing, integration testing and manual testing] ### Check List - [ ] New functionality includes testing - [ ] New functionality has been documented - [ ] Commits are signed per the DCO using --signoff By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Andriy Redko <[email protected]>
Description
The security plugin does not need to provide the secure transports anymore but SecureSettingsFactory so the core transport modules will be able to configure those.
Issues Resolved
Closes #4118
Is this a backport? If so, please add backport PR # and/or commits #
Testing
[Please provide details of testing done: unit testing, integration testing and manual testing]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.