Enable security for bwc tests #3269

cwperks · 2023-08-30T13:33:35Z

Description

Opening up a PR to describe the issues faced with BWC tests with the security plugin installed and solicit feedback.

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:

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

@scrawfor99

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 <cwperx@amazon.com>

Signed-off-by: Craig Perkins <cwperx@amazon.com>

codecov · 2023-08-30T14:01:07Z

Codecov Report

Merging #3269 (8a713ff) into main (0338cdd) will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #3269      +/-   ##
============================================
+ Coverage     63.15%   63.18%   +0.02%     
- Complexity     3448     3451       +3     
============================================
  Files           263      263              
  Lines         20024    20024              
  Branches       3341     3341              
============================================
+ Hits          12647    12652       +5     
+ Misses         5748     5742       -6     
- Partials       1629     1630       +1

see 1 file with indirect coverage changes

RyanL1997

Hi @cwperks , thanks for putting this together. I'm sorry that I'm still trying to understand this entire BWC testing following so I left some questions. But, in generally, this is looking good to me.

RyanL1997 · 2023-08-31T18:27:36Z

bwc-test/src/test/java/SecurityBackwardsCompatibilityIT.java

@@ -35,6 +62,11 @@ private void testSetup() {
        CLUSTER_NAME = System.getProperty("tests.clustername");
    }

+    @Override
+    protected final boolean preserveClusterUponCompletion() {


Just for my knowledge what is this boolean for?

Its to bypass this method which throws a WarningFailureException when using snapshots. It fails because its trying to json parse a response along with a warning that's printed out around the dot syntax for system index deprecation.

bwc-test/src/test/java/SecurityBackwardsCompatibilityIT.java

RyanL1997 · 2023-08-31T18:48:42Z

bwc-test/src/test/java/SecurityBackwardsCompatibilityIT.java

+        Header[] defaultHeaders = new Header[headers.size()];
+        int i = 0;
+        for (Map.Entry<String, String> entry : headers.entrySet()) {
+            defaultHeaders[i++] = new BasicHeader(entry.getKey(), entry.getValue());


I can see that within the loop, a new BasicHeader object is created for each entry, using the key as the header name and the value as the header value. This BasicHeader object is then stored in the defaultHeaders array at the index specified by i, which is then incremented. And my question is more like "why we need these basic header for each entry?"

Apache HttpClient has 2 concrete types of headers: BasicHeader and BufferedHeader.

BasicHeader is a Basic implementation of Header which allows a header key and a header value.

https://www.javadoc.io/static/org.apache.httpcomponents/httpcore/4.4.1/index.html?org/apache/http/message/BasicHeader.html

cwperks added 2 commits August 30, 2023 08:58

3.0.0

39d4923

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks requested review from cliu123, DarshitChanpura, davidlago, peternied, RyanL1997, scrawfor99, reta and willyborankin as code owners August 30, 2023 13:33

Use Apache HTTP Client5 on main

8a713ff

Signed-off-by: Craig Perkins <cwperx@amazon.com>

peternied approved these changes Aug 31, 2023

View reviewed changes

DarshitChanpura approved these changes Aug 31, 2023

View reviewed changes

cwperks merged commit 242a3a2 into opensearch-project:main Aug 31, 2023
57 checks passed

RyanL1997 reviewed Aug 31, 2023

View reviewed changes

peternied mentioned this pull request Sep 12, 2023

Improve serialization speeds #2802

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable security for bwc tests #3269

Enable security for bwc tests #3269

cwperks commented Aug 30, 2023

codecov bot commented Aug 30, 2023 •

edited

RyanL1997 left a comment

RyanL1997 Aug 31, 2023

cwperks Aug 31, 2023

RyanL1997 Aug 31, 2023

cwperks Aug 31, 2023

Enable security for bwc tests #3269

Enable security for bwc tests #3269

Conversation

cwperks commented Aug 30, 2023

Description

Issues Resolved

Check List

codecov bot commented Aug 30, 2023 • edited

Codecov Report

RyanL1997 left a comment

Choose a reason for hiding this comment

RyanL1997 Aug 31, 2023

Choose a reason for hiding this comment

cwperks Aug 31, 2023

Choose a reason for hiding this comment

RyanL1997 Aug 31, 2023

Choose a reason for hiding this comment

cwperks Aug 31, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 30, 2023 •

edited