Decoupled OpenSearchSinkIT from the OpenSearch Core test cases #1325

dlvenable · 2022-04-21T21:10:40Z

Description

The OpenSearchSinkIT file currently inherits from OpenSearchRestTestCase. This class comes from the OpenSearch Core test framework. It brings in a number of requirements and assumptions, including some validations internal to OpenSearch. Our integration tests should treat OpenSearch as a black box, but that inheritance treats it as a white box.

This change copies some methods from OpenSearchRestTestCase into a new class OpenSearchIntegrationHelper. By doing this, OpenSearchSinkIT no longer needs to inherit from OpenSearchRestTestCase and I have removed that inheritance.

There may be future approaches to simplify this even more. But, this was a minimally intrusive solution.

Issues Resolved

Contributes to #593 by moving the opensearch plugin toward being able to remove the OpenSearch build-tools plugins. It also contributes to #1311 by decoupling our tests from internal specifics that are causing problems with OpenSearch 2.0.

Check List

New functionality includes testing.
New functionality has been documented.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

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.

… OpenSearchIntegrationHelper to clean up some of the changes to OpenSearchSinkIT. Also clean out templates and wait for tasks to complete in between tests. Signed-off-by: David Venable <dlv@amazon.com>

sbayer55

Good idea moving part of the test class to a helper class. The tests are more readable now.

sbayer55 · 2022-04-21T21:17:06Z

...rc/test/java/com/amazon/dataprepper/plugins/sink/opensearch/OpenSearchIntegrationHelper.java

@@ -0,0 +1,254 @@
+/*


Why is the long format license needed here?

I copied this code from OpenSearchRestTestCase.java. As copied code it must have the long-form header and the headers from the original file.

"I copied this code" hits me like nails on a chalkboard. :) Would other projects have this same issue and benefit from a reliable test client that doesn't bring in the OpenSearch core? I am wondering if we can standardize this for the community and make something usable rather than copying code.

sbayer55 · 2022-04-21T21:22:49Z

...rc/test/java/com/amazon/dataprepper/plugins/sink/opensearch/OpenSearchIntegrationHelper.java

+                        .setSSLHostnameVerifier(NoopHostnameVerifier.INSTANCE)
+                        .setSSLContext(SSLContextBuilder.create().loadTrustMaterial(null, (chains, authType) -> true).build());
+            } catch (Exception e) {
+                throw new RuntimeException(e);


Why catch an exception only to throw a generic runtime exception?

This was already part of OpenSearchSinkIT and I moved this method. There is little value here since these are all tests.

sbayer55 · 2022-04-21T21:26:18Z

...pensearch/src/test/java/com/amazon/dataprepper/plugins/sink/opensearch/OpenSearchSinkIT.java

    MetricsTestUtil.initMetrics();
+
+    client = createOpenSearchClient();


Thanks for cleaning up all the client() calls!

cmanning09 · 2022-04-25T16:52:26Z

...rc/test/java/com/amazon/dataprepper/plugins/sink/opensearch/OpenSearchIntegrationHelper.java

+        if (keystorePath != null) {
+            final String keystorePass = settings.get(TRUSTSTORE_PASSWORD);
+            if (keystorePass == null) {
+                throw new IllegalStateException(TRUSTSTORE_PATH + " is provided but not " + TRUSTSTORE_PASSWORD);
+            }
+            Path path = PathUtils.get(keystorePath);
+            if (!Files.exists(path)) {
+                throw new IllegalStateException(TRUSTSTORE_PATH + " is set but points to a non-existing file");
+            }
+            try {
+                final String keyStoreType = keystorePath.endsWith(".p12") ? "PKCS12" : "jks";
+                KeyStore keyStore = KeyStore.getInstance(keyStoreType);
+                try (InputStream is = Files.newInputStream(path)) {
+                    keyStore.load(is, keystorePass.toCharArray());
+                }
+                SSLContext sslcontext = SSLContexts.custom().loadTrustMaterial(keyStore, null).build();
+                SSLIOSessionStrategy sessionStrategy = new SSLIOSessionStrategy(sslcontext);
+                builder.setHttpClientConfigCallback(httpClientBuilder -> httpClientBuilder.setSSLStrategy(sessionStrategy));
+            } catch (KeyStoreException | NoSuchAlgorithmException | KeyManagementException | CertificateException e) {
+                throw new RuntimeException("Error setting up ssl", e);
+            }
+        }


I am worried we are copying over dead code that we won't use here. configureHttpsClient and configureClient are poorly named and both functions support HTTPS. The clients themselves really differentiate themselves based on the os system.property being set to true. The protocol used is based on this property as well.

Do we need this code block?

I'm unsure if we need this. It does seem that all of our tests are using HTTPS, but I was unable to verify this.

I validated that even OpenDistro 1.0.0 runs with HTTPS enabled by default.

docker run -p 9200:9200 -p 9600:9600 -e "discovery.type=single-node" -d amazon/opendistro-for-elasticsearch:1.0.0

curl https://localhost:9200 --insecure --user admin:admin [14:26:06] { "name" : "5ac8a4703ed1", "cluster_name" : "docker-cluster", "cluster_uuid" : "2AdDi7uSQ-mpHH1j00u40A", "version" : { "number" : "7.0.1", "build_flavor" : "oss", "build_type" : "tar", "build_hash" : "e4efcb5", "build_date" : "2019-04-29T12:56:03.145736Z", "build_snapshot" : false, "lucene_version" : "8.0.0", "minimum_wire_compatibility_version" : "6.7.0", "minimum_index_compatibility_version" : "6.0.0-beta1" }, "tagline" : "You Know, for Search" }

I'm unsure presently what the minimum supported OpenDistro/Elasticsearch version Data Prepper supports, but I believe it is in the OpenDistro 1.x/Elasticsearch 7.x series. So I removed any support for testing against HTTP plaintext.

I had to revert that change. The current build does run an OpenSearch cluster with HTTP. When we remove the OpenSearch build-tools plugin entirely we can remove this.

cmanning09 · 2022-04-25T16:56:32Z

...rc/test/java/com/amazon/dataprepper/plugins/sink/opensearch/OpenSearchIntegrationHelper.java

@@ -0,0 +1,254 @@
+/*


"I copied this code" hits me like nails on a chalkboard. :) Would other projects have this same issue and benefit from a reliable test client that doesn't bring in the OpenSearch core? I am wondering if we can standardize this for the community and make something usable rather than copying code.

cmanning09 · 2022-04-25T16:59:14Z

...rc/test/java/com/amazon/dataprepper/plugins/sink/opensearch/OpenSearchIntegrationHelper.java

+        }
+    }
+
+    private static void configureClient(RestClientBuilder builder, Settings settings) throws IOException {


Is this the only copied piece of code we are leveraging? Let's call this out for the maintainers.

Yes, I think that the best solution would be to refactor the OpenSearch tests. This would likely be a significant change and will take some time to implement. I'd like to fix these tests in Data Prepper to support OpenSearch 2.0. As a follow-up we can best determine how to best refactor the tests for OpenSearch.

I'd also like to see if we can improve our tests - maybe we will need fewer of these components. But, I'd like to do this in a more phased approach. This phase is currently removing the inheritance model which is imposing OpenSearch internal requirements.

~~I just removed the configureClient in favor of always using HTTPS.~~

…r came from. Include TODO comments for future clean-up. Signed-off-by: David Venable <dlv@amazon.com>

Signed-off-by: David Venable <dlv@amazon.com>

dlvenable · 2022-04-25T20:19:46Z

I made a few comment changes and added some TODO items. I'm hoping for a re-review of the changes.

Decoupled OpenSearchSinkIT from the OpenSearch core test cases. Added…

01371ce

… OpenSearchIntegrationHelper to clean up some of the changes to OpenSearchSinkIT. Also clean out templates and wait for tasks to complete in between tests. Signed-off-by: David Venable <dlv@amazon.com>

dlvenable requested a review from a team as a code owner April 21, 2022 21:10

sbayer55 previously approved these changes Apr 21, 2022

View reviewed changes

cmanning09 reviewed Apr 25, 2022

View reviewed changes

dlvenable dismissed sbayer55’s stale review via 35fae1f April 25, 2022 19:21

dlvenable added 2 commits April 25, 2022 14:51

Added comments noting where each method in OpenSearchIntegrationHelpe…

389dc92

…r came from. Include TODO comments for future clean-up. Signed-off-by: David Venable <dlv@amazon.com>

Updated the comments in OpenSearchIntegrationHelper.

35b7a72

Signed-off-by: David Venable <dlv@amazon.com>

dlvenable force-pushed the decoupling-opensearch-it branch from 88e5d8a to 35b7a72 Compare April 25, 2022 19:53

cmanning09 approved these changes Apr 25, 2022

View reviewed changes

sbayer55 approved these changes Apr 25, 2022

View reviewed changes

dlvenable merged commit 24b0c32 into opensearch-project:main Apr 25, 2022

dlvenable deleted the decoupling-opensearch-it branch April 29, 2022 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decoupled OpenSearchSinkIT from the OpenSearch Core test cases #1325

Decoupled OpenSearchSinkIT from the OpenSearch Core test cases #1325

dlvenable commented Apr 21, 2022 •

edited

Loading

sbayer55 left a comment

sbayer55 Apr 21, 2022

dlvenable Apr 21, 2022

cmanning09 Apr 25, 2022

sbayer55 Apr 21, 2022

dlvenable Apr 21, 2022

sbayer55 Apr 21, 2022

cmanning09 Apr 25, 2022

dlvenable Apr 25, 2022

dlvenable Apr 25, 2022 •

edited

Loading

dlvenable Apr 25, 2022

cmanning09 Apr 25, 2022

cmanning09 Apr 25, 2022

dlvenable Apr 25, 2022

dlvenable Apr 25, 2022

dlvenable Apr 25, 2022 •

edited

Loading

dlvenable commented Apr 25, 2022

		MetricsTestUtil.initMetrics();

		client = createOpenSearchClient();

Decoupled OpenSearchSinkIT from the OpenSearch Core test cases #1325

Decoupled OpenSearchSinkIT from the OpenSearch Core test cases #1325

Conversation

dlvenable commented Apr 21, 2022 • edited Loading

Description

Issues Resolved

Check List

sbayer55 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

dlvenable commented Apr 25, 2022

dlvenable commented Apr 21, 2022 •

edited

Loading

dlvenable Apr 25, 2022 •

edited

Loading

dlvenable Apr 25, 2022 •

edited

Loading