-
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
[2.x] Enable security for bwc tests #3257
Merged
cwperks
merged 29 commits into
opensearch-project:2.x
from
cwperks:enable-security-for-bwc-tests
Aug 30, 2023
Merged
[2.x] Enable security for bwc tests #3257
cwperks
merged 29 commits into
opensearch-project:2.x
from
cwperks:enable-security-for-bwc-tests
Aug 30, 2023
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
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Codecov Report
@@ Coverage Diff @@
## 2.x #3257 +/- ##
============================================
+ Coverage 62.32% 62.39% +0.07%
- Complexity 3293 3297 +4
============================================
Files 252 252
Lines 19467 19457 -10
Branches 3308 3308
============================================
+ Hits 12133 12141 +8
+ Misses 5717 5700 -17
+ Partials 1617 1616 -1 |
peternied
reviewed
Aug 28, 2023
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.
Nicely done, a bunch of questions.
src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Craig Perkins <[email protected]>
…le-security-for-bwc-tests
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
peternied
reviewed
Aug 29, 2023
Signed-off-by: Craig Perkins <[email protected]>
cwperks
requested review from
cliu123,
DarshitChanpura,
davidlago,
RyanL1997,
stephen-crawford,
reta and
willyborankin
as code owners
August 29, 2023 15:25
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
cwperks
commented
Aug 29, 2023
peternied
approved these changes
Aug 29, 2023
cwperks
added a commit
to cwperks/security
that referenced
this pull request
Aug 30, 2023
Opening up a PR to describe the issues faced with BWC tests with the security plugin installed and solicit feedback. I plan to forward port this change to main, but first wanted to show this working for 2.9 -> 2.10 tests (as of the time of this writing). Thanks to the work that @scrawfor99 did in [core](opensearch-project/OpenSearch#8900) to supply security settings to testClusters to be able to run the initial wait for cluster yellow checks with a URL that includes the right protocol (`https` when security is enabled) along with a username and password to authenticate the request. I ran into 4 hurdles to get this to run: 1. Initially the cluster didn't form. After a lot of frustration, I ended up finding that by supplying `network.bind_host` and `network.publish_host` to both 127.0.0.1 it resolved the issue. These could probably be combined into a single `network.host`, but I chose to keep them separated. 2. I had issue testing changes to the gradle build-tools after making changes locally. This was the most frustrating hurdle, but ultimately the solution was to change the [`opensearch.version` setting in `bwc-test/build.gradle`](https://github.com/opensearch-project/security/blob/2.x/bwc-test/build.gradle#L47) to `2.10.0-SNAPSHOT`. This value is specifically used as the version of the gradle build-tools that the [BWC tests use](https://github.com/opensearch-project/security/blob/main/bwc-test/build.gradle#L58). The changes I made locally didn't reflect because I was publishing to maven local from the 2.x branch (currently 2.10) and it was looking for 2.9.0-SNAPSHOT artifacts. After updating the value it found my maven local snapshots. For this artifact you can produce maven local snapshots using `./gradlew :build-tools:publishToMavenLocal` from the respective branch in the core repo. 3. After the waitForYellow checks were able to run successfully, the REST Client in the SecurityBackwardsCompatibilityIT was also having problems connecting to the cluster because it didn't recognize the certificates of the server. I ended up using the overly trustworthy route where there is no SSL verification for the REST Client used in this test. I borrowed this implementation from [k-NN's ODFERestTestCase](https://github.com/opensearch-project/k-NN/blob/2.x/src/testFixtures/java/org/opensearch/knn/ODFERestTestCase.java#L118-L141) which is widely used in the plugin ecosystem. There is an open issue to abstract this class into common-utils. More work can be done here to ensure the rest-high-level-client runs with a truststore with the root certificate. 4. The last hurdle I faced was a WarningFailureException where the REST Client could not deserialize the cluster health response because of a warning that was returned with the response about the request including system indices. According to this [comment](opensearch-project/OpenSearch#1108 (comment)), this may only be enabled in snapshots. To fix this, I set preserve cluster to true which [bypasses the method](https://github.com/opensearch-project/OpenSearch/blob/main/test/framework/src/main/java/org/opensearch/test/rest/OpenSearchRestTestCase.java#L364) where the error was thrown. * Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Enhancement opensearch-project#3056 - [ ] 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: Craig Perkins <[email protected]>
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.
Description
Opening up a PR to describe the issues faced with BWC tests with the security plugin installed and solicit feedback.
I plan to forward port this change to main, but first wanted to show this working for 2.9 -> 2.10 tests (as of the time of this writing).
Thanks to the work that @scrawfor99 did in core to supply security settings to testClusters to be able to run the initial wait for cluster yellow checks with a URL that includes the right protocol (
https
when security is enabled) along with a username and password to authenticate the request.I ran into 4 hurdles to get this to run:
network.bind_host
andnetwork.publish_host
to both 127.0.0.1 it resolved the issue. These could probably be combined into a singlenetwork.host
, but I chose to keep them separated.opensearch.version
setting inbwc-test/build.gradle
to2.10.0-SNAPSHOT
. This value is specifically used as the version of the gradle build-tools that the BWC tests use. The changes I made locally didn't reflect because I was publishing to maven local from the 2.x branch (currently 2.10) and it was looking for 2.9.0-SNAPSHOT artifacts. After updating the value it found my maven local snapshots. For this artifact you can produce maven local snapshots using./gradlew :build-tools:publishToMavenLocal
from the respective branch in the core repo.Enhancement
Issues Resolved
#3056
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.