Windows build and test support for 1.3 #2291

peternied · 2022-12-01T22:58:26Z

Description

Updating the github action to support build/test for windows. Note; this change will not include specific backporting. I know this is not desired, but there are some funky changes in our source history that are hard to resolve whereas checkout out specific files is much easier.

Details list of changes

Use most recent format of CI workflows from main
Backport Fix windows encoding issues #2206
Supply workarounds for JDK8 incompatible APIs for Encoding / Pattern matching (Thanks @cwperks!)
Backport only freeport logic from securityadmin: Replace TransportClient by RestHighLevelClient #1638
Backport Add signal/wait model for TestAuditlogImpl #1758
All updates to TestAuditlogImpl.java from main
- Prevent message collection from being updated after message count has been received #2180
- Abstract waitForInit to minimize duplication and improve test reliability #1935
- testComplianceEnable supports variable number of audit messages #1920
- ComplianceAuditlogTest to use signal/wait #1914
- Updates license headers #1829
- And Targeted test updates for ComplianceAuditlogTest and BasicAuditlogTest to keep up with TestAuditlogImpl.java updates

Issues Resolved

Testing

Its all in the CI system!

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.

Signed-off-by: Peter Nied <[email protected]>

.github/workflows/ci.yml

peternied · 2022-12-01T22:59:37Z

.github/workflows/ci.yml

    strategy:
      fail-fast: false
      matrix:
-        jdk: [8, 11, 14]
+        jdk: [8, 11]


JDK14 support? Temurin doesn't have an image for it

@scrawfor99 Could you look into this? My current thinking is that we will merge it after this PR goes in - let me know what you run into

peternied · 2022-12-01T23:13:36Z

Here is a quick report on the test failures, looks like some backports were missed, I'll include those changes directly in this pull request

Test Failures
	JDK11		JDK8
	Linux	Windows	Linux	Windows
TestRun 1
HeapBasedRateTrackerTest. maxTwoTriesTest				1
TenantInfoActionTest. testTenantInfoAPI[1]			1
WebhookAuditLogTest. postGetHttpTest			1
HeapBasedRateTrackerTest. expiryTest		1
AuditApiActionTest. testApi[0]	1
DlsTest. testDlsWithMinDocCountZeroAggregations	1
SlowIntegrationTests. testCustomInterclusterRequestEvaluator				1
HttpIntegrationTests. testTenantInfo	2
BasicAuditlogTest. testRestMethod	2		3
TenantInfoActionTest. testTenantInfoAPI[0]	2	2	1	1
WebhookAuditLogTest. httpsTestPemDefault	7		1
WebhookAuditLogTest. httpsTestPemEndpoint	9		1
IntegrationTests. testSpecialUsernames		6		6
UtilTests. testEnvReplace		6		6
OpenSSLTest. testEnsureOpenSSLAvailability	6	6	6	6

Add CI for Windows and MacOS platforms Due to the increase in the number of platforms, I've separated the newer integration tests into their own workflow. Until retries have been enabled they will automatically pass - but still run and report logs. As soon as we have full confidence we will allow them to start blocking pull requests from merging. opensearch-project#2184 Switch the gradle commands to be platform agnostic via the `gradle/gradle-build-action@v2`, dropping the 'clean' step to the build which allows us to reuse the gradle cache - if we see any problems pulling in more recent snapshots we can disable this setting quickly. Found and fixed an issued with config value replacement via environment variables, long story short Windows and MacOS allow for many more characters that are used in the unix environment variable landscape. Added new tests to cover these interesting scenarios as well. Found an encoding issue with user names from config files, still unclear of the source of this issue, be it test setup specific or a problem in the broader OpenSearch ecosystem, disabling the `testSpecialUsernames` until we can dive deeper. opensearch-project#2194 Disabled the HeapBasedRateTrackerTests - it was depending on system timing and was very brittle if the system under test experienced any undo load, created follow up issue opensearch-project#2193 Fixed a test issue in testDlsWithMinDocCountZeroAggregations where there was a random chance for a test failure, easier to find intermittent tests when they are run so often. OpenSSL has open questions - while it is supported for our Linux JDK11 builds, it seems like a stopgap measure. I've disabled the tests on windows environment while we determine if we should support OpenSSL at all on Windows JDK11. opensearch-project#2195 Signed-off-by: Peter Nied <[email protected]>

Adds spotbugs [1] to detect internalization before they are added to the codebase, also fixed several encoding bugs that impact windows users. [1] https://spotbugs.readthedocs.io/en/stable/index.html Closes opensearch-project#2194 Signed-off-by: Peter Nied <[email protected]>

peternied · 2022-12-01T23:49:56Z

Fixed those bottom 3 tests, they were all windows platform issues, the others might be random failuers. Next step will be looking into the WebhookAuditLogTest failures which seem to be new

Signed-off-by: Peter Nied <[email protected]>

peternied · 2022-12-02T00:03:14Z

@scrawfor99 I know you have a couple PRs out for a couple of backports, you might want to rebase off of this pull request while testing or maybe I should pull some of these changes into this change - what do you think?

Signed-off-by: Peter Nied <[email protected]>

stephen-crawford · 2022-12-02T14:10:12Z

I dumped the CI backport because I saw @cwperks already had one. I rebased the script changes onto your branch and PR'd against it.

peternied · 2022-12-02T17:28:31Z

Looking much cleaner, Going to dig in on the WebhookAuditLogTest failures

Test Failures
	JDK11		JDK8
	Linux	Windows	Linux	Windows
TestRun 2
HttpIntegrationTests. testTenantInfo				1
SecurityRestFilterTest. checkWhitelistedApisAreAccessible	1
BasicAuditlogTest. testRestMethod	2
SlowIntegrationTests. testDelayInSecurityIndexInitialization		6
WebhookAuditLogTest. httpsTestPemEndpoint	1		8
WebhookAuditLogTest. httpsTestPemDefault	1		8
WebhookAuditLogTest. postGetHttpTest	9		10

…#1638 Signed-off-by: Peter Nied <[email protected]>

* Add signal/wait model for TestAuditlogImpl I have been tracking test failures with testRestMethod very often showing failures. My theory is that the execution environment can impact the order of operations sometimes causing the audit log not to contain messages before it is checked. Adding a new method `doThenWaitForMessages(...)` this ensures the log queue is fresh, the triggering action completes, and the expected number of messages were received. There is a second long time window that allows for the messages to be flushed, this is likely more than enough - if the messages are received the count down latch immediately continues execution so the tests will not wait if they are ready to proceed. While this new method is much more reliable not all tests were encountering such issues, so I've keep the original convention. This can be migrated in one-offs or all at once if we see more troublesome behavior. The previous methods/fields are depreciated to push future tests to follow the new pattern. Modifications to the rest helper not to throw exceptions were needed to keep the Runnable declaration clean and small. Signed-off-by: Peter Nied <[email protected]>

…lize Signed-off-by: Peter Nied <[email protected]>

Signed-off-by: Peter Nied <[email protected]>

peternied · 2022-12-02T21:16:09Z

Audit log tests are flaky - pulling in targeted fixes for those test...

Signed-off-by: Peter Nied <[email protected]>

peternied

Added callouts from when non-trivial changes were made. Stylecop, Checkstyle, and license checks were applied from where changes were pulled, sorry I know that it is messy to review.

peternied · 2022-12-02T22:52:14Z

build.gradle

@@ -164,6 +165,7 @@ publishing {

 tasks.withType(JavaCompile) {
    options.encoding = 'UTF-8'
+    options.warnings = false


New: We are never going to fix the warnings emitted during compile in 1.3 and they were slowing me down

peternied · 2022-12-02T22:57:17Z

src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java

-        }
+            HttpResponse response = rh.executeGetRequest("_search?pretty", encodeBasicHeader("admin", "admin"));
+            assertThat(response.getStatusCode(), equalTo(HttpStatus.SC_OK));
+        }, 7);


New: 1.3 still uses the transport client, those interacts are not logged, so this number was altered from being 14 -> 7 because the 'read' actions were not included in the audit log (which is expected).

Also updated the verification on 261 to only expect the 'write' message types

peternied · 2022-12-02T22:59:30Z

src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java

-        }
+            final HttpResponse response = rh.executeGetRequest("_search?pretty", encodeBasicHeader("admin", "admin"));
+            Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
+        }, 18);


New: Way more audit messages this time because transport messages are included, there were 14 new messages, 7 transport writes, 7 transport reads, and the original 4 expected messages via REST

Ended up filtering out the transport messages on line 302 to keep the rest of the function identical

peternied · 2022-12-02T23:02:30Z

src/test/java/org/opensearch/security/test/AbstractSecurityUnitTest.java

@@ -199,23 +199,7 @@ protected void initialize(ClusterInfo info, Settings initTransportClientSettings
            Assert.assertEquals(info.numNodes, cur.getNodes().size());

            SearchResponse sr = tc.search(new SearchRequest(".opendistro_security")).actionGet();


Note; this is inline with main - all of these other requested slowed down how long startup took and these assertions aren't that useful since the plugin just throws exceptions if it couldn't create the security index.

cwperks

Great work! 🎸

cwperks · 2022-12-05T16:26:35Z

src/test/java/org/opensearch/security/auditlog/sink/WebhookAuditLogTest.java

@@ -222,15 +224,16 @@ public void noServerRunningHttpTest() throws Exception {
 	public void postGetHttpTest() throws Exception {
 		TestHttpHandler handler = new TestHttpHandler();

+		int port = findFreePort();


* Add jdk 14 to CI Signed-off-by: Stephen Crawford <[email protected]> Signed-off-by: Stephen Crawford <[email protected]>

peternied · 2022-12-05T19:15:27Z

The latest test failures were from SSLTest.testTransportClientSSLExternalContext which could have been from a network hiccup... tests are rerunning with JDK14 🤞

Signed-off-by: Peter Nied <[email protected]>

peternied added 3 commits December 1, 2022 14:40

Updated CI for Windows

2393d50

Signed-off-by: Peter Nied <[email protected]>

Remove tasks that should not be executed

04de2c6

Signed-off-by: Peter Nied <[email protected]>

Remove mac and JDK14

113ebc8

Signed-off-by: Peter Nied <[email protected]>

peternied self-assigned this Dec 1, 2022

peternied commented Dec 1, 2022

View reviewed changes

peternied added 2 commits December 1, 2022 17:34

Fix Missing Constructors in JDK8

c43498c

Signed-off-by: Peter Nied <[email protected]>

Fix missing Pattern.asMatchPredicate(...) in JDK8

258043a

Signed-off-by: Peter Nied <[email protected]>

peternied mentioned this pull request Dec 2, 2022

Backport Windows support to 1.x branch #2281

Closed

13 tasks

peternied added 4 commits December 2, 2022 11:33

[Backport] WebhookAuditLogTest find free port from opensearch-project…

cb0bd6a

…#1638 Signed-off-by: Peter Nied <[email protected]>

Remove unused queries into cluster health before returning from inita…

63d5424

…lize Signed-off-by: Peter Nied <[email protected]>

Add build artifacts names back into the test workflow

723148a

Signed-off-by: Peter Nied <[email protected]>

Pull in the latest updates to TestAuditlogImpl

6d15e82

Signed-off-by: Peter Nied <[email protected]>

peternied marked this pull request as ready for review December 2, 2022 21:43

peternied requested a review from a team December 2, 2022 21:43

Fix JDK8 missing features

0c5be45

Signed-off-by: Peter Nied <[email protected]>

peternied mentioned this pull request Dec 2, 2022

[Infrastructure] Intermittent 1.3 branch CI failures #2023

Closed

peternied commented Dec 2, 2022

View reviewed changes

cwperks previously approved these changes Dec 5, 2022

View reviewed changes

Jdk14 for OpenSearch 1.3 support (#6)

539730f

* Add jdk 14 to CI Signed-off-by: Stephen Crawford <[email protected]> Signed-off-by: Stephen Crawford <[email protected]>

peternied dismissed cwperks’s stale review via 539730f December 5, 2022 19:02

peternied added 3 commits December 5, 2022 21:27

Fixing test failures

4e197ad

Signed-off-by: Peter Nied <[email protected]>

Remove duplicate test suite

0523b3c

Signed-off-by: Peter Nied <[email protected]>

Make the config file dynamically pick up the port

a9ad24e

Signed-off-by: Peter Nied <[email protected]>

cwperks approved these changes Dec 5, 2022

View reviewed changes

peternied merged commit 8e1adfa into opensearch-project:1.3 Dec 6, 2022

cwperks mentioned this pull request Jul 19, 2023

[BUG] [CI] Investigate Flaky test failure for windows CI tasks #3021

Closed

peternied deleted the 1.3-windows branch July 19, 2023 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows build and test support for 1.3 #2291

Windows build and test support for 1.3 #2291

peternied commented Dec 1, 2022 •

edited

Loading

peternied Dec 1, 2022

peternied Dec 2, 2022

peternied commented Dec 1, 2022

peternied commented Dec 1, 2022

peternied commented Dec 2, 2022

stephen-crawford commented Dec 2, 2022 •

edited

Loading

peternied commented Dec 2, 2022

peternied commented Dec 2, 2022

peternied left a comment

peternied Dec 2, 2022

peternied Dec 2, 2022

peternied Dec 2, 2022

peternied Dec 2, 2022

cwperks left a comment

cwperks Dec 5, 2022

peternied commented Dec 5, 2022

		@@ -199,23 +199,7 @@ protected void initialize(ClusterInfo info, Settings initTransportClientSettings
		Assert.assertEquals(info.numNodes, cur.getNodes().size());

		SearchResponse sr = tc.search(new SearchRequest(".opendistro_security")).actionGet();

Windows build and test support for 1.3 #2291

Windows build and test support for 1.3 #2291

Conversation

peternied commented Dec 1, 2022 • edited Loading

Description

Details list of changes

Issues Resolved

Testing

Check List

peternied Dec 1, 2022

Choose a reason for hiding this comment

peternied Dec 2, 2022

Choose a reason for hiding this comment

peternied commented Dec 1, 2022

peternied commented Dec 1, 2022

peternied commented Dec 2, 2022

stephen-crawford commented Dec 2, 2022 • edited Loading

peternied commented Dec 2, 2022

peternied commented Dec 2, 2022

peternied left a comment

Choose a reason for hiding this comment

peternied Dec 2, 2022

Choose a reason for hiding this comment

peternied Dec 2, 2022

Choose a reason for hiding this comment

peternied Dec 2, 2022

Choose a reason for hiding this comment

peternied Dec 2, 2022

Choose a reason for hiding this comment

cwperks left a comment

Choose a reason for hiding this comment

cwperks Dec 5, 2022

Choose a reason for hiding this comment

peternied commented Dec 5, 2022

peternied commented Dec 1, 2022 •

edited

Loading

stephen-crawford commented Dec 2, 2022 •

edited

Loading