Skip to content
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
merged 29 commits into from
Aug 30, 2023

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Aug 28, 2023

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:

  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 to 2.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.
  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 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, this may only be enabled in snapshots. To fix this, I set preserve cluster to true which bypasses the method where the error was thrown.
  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Enhancement

Issues Resolved

#3056

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.

cwperks added 19 commits August 25, 2023 14:51
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
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #3257 (7fb2ef2) into 2.x (abeedb1) will increase coverage by 0.07%.
Report is 5 commits behind head on 2.x.
The diff coverage is n/a.

Impacted file tree graph

@@             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     

see 12 files with indirect coverage changes

Copy link
Member

@peternied peternied left a 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.

.github/actions/run-bwc-suite/action.yaml Outdated Show resolved Hide resolved
bwc-test/build.gradle Outdated Show resolved Hide resolved
bwc-test/src/test/resources/security/esnode.pem Outdated Show resolved Hide resolved
bwc-test/build.gradle Outdated Show resolved Hide resolved
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]>
@cwperks cwperks marked this pull request as ready for review 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 cwperks merged commit d1590c6 into opensearch-project:2.x Aug 30, 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants