[Backport 2.x] Add CI for Windows and MacOS platforms (#2190) by peternied · Pull Request #2205 · opensearch-project/security

peternied · 2022-10-28T16:31:10Z

Description

[Backport 2.x] 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. #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. #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 #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. #2195

Signed-off-by: Peter Nied petern@amazon.com
(cherry picked from commit a57fd0a)

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.

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. #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. #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 #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. #2195 Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit a57fd0a)

codecov-commenter · 2022-10-28T16:54:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.03%. Comparing base (51a2862) to head (385f5bc).
⚠️ Report is 672 commits behind head on 2.x.

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #2205      +/-   ##
============================================
- Coverage     61.06%   61.03%   -0.04%     
- Complexity     3232     3240       +8     
============================================
  Files           257      257              
  Lines         18089    18089              
  Branches       3225     3225              
============================================
- Hits          11046    11040       -6     
- Misses         5467     5480      +13     
+ Partials       1576     1569       -7

Files with missing lines	Coverage Δ
...org/opensearch/security/support/SecurityUtils.java	`72.30% <100.00%> (ø)`

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

stephen-crawford · 2022-10-31T13:28:06Z

.github/workflows/ci.yml

-    runs-on: ubuntu-latest
    strategy:
-      fail-fast: true
+      fail-fast: false


Why do we not want to fail-fast anymore? I believe this will now let the whole matrix operate even if one of the builds fails for a specific OS.

Fast failing helps free resources faster, but you build less confidence per build iteration. By forcing all builds to keep running we are sure that all integration test runs are completed and you can compare different results between different platforms. If we were optimizing for cost we might stick to the fast failures

With the platform differences, bugs can be introduced into two platforms at once impacting different tests, IMO its more useful for troubleshooting to be able to see if only one workflow was impacted (indicating a random failure) vs many workflows (indicating a new bug was caught).

stephen-crawford · 2022-10-31T13:29:15Z

.github/workflows/ci.yml

    - name: check archive for debugging
      if: always()
-      run: echo "Check the artifact ${{ matrix.jdk }}-reports.zip for detailed test results"
+      run: echo "Check the artifact ${{ matrix.platform }}-JDK${{ matrix.jdk }}-reports for detailed test results"


Nice use of all the matrices 👍

stephen-crawford · 2022-10-31T13:30:30Z

src/main/java/org/opensearch/security/support/SecurityUtils.java

-    private static final Pattern ENV_PATTERN = Pattern.compile("\\$\\{env\\.([\\w]+)((\\:\\-)?[\\w]*)\\}");
-    private static final Pattern ENVBC_PATTERN = Pattern.compile("\\$\\{envbc\\.([\\w]+)((\\:\\-)?[\\w]*)\\}");
-    private static final Pattern ENVBASE64_PATTERN = Pattern.compile("\\$\\{envbase64\\.([\\w]+)((\\:\\-)?[\\w]*)\\}");
+    private static final String ENV_PATTERN_SUFFIX = "\\.([\\w=():\\-_]+?)(\\:\\-[\\w=():\\-_]*)?\\}";


I know this was already here but maybe we could add a comment saying what this pattern means for readability?

This is a backport PR, I'll create a separate small PR to update the comments around this file to make it easier to understand

stephen-crawford · 2022-10-31T13:31:01Z

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

    }

    @Test
+    @Ignore // https://github.com/opensearch-project/security/issues/2194


I think you remove this test in your next PR haha.

stephen-crawford · 2022-10-31T13:33:43Z

src/test/java/org/opensearch/security/dlic/dlsfls/DlsTest.java

        // Significant Text Aggregation is not impacted.
        // Non-admin user with setting "min_doc_count=0". Expected to only have access to buckets for dept_manager".
-        String query3 = "{\"aggregations\":{\"significant_termX\":{\"significant_terms\":{\"field\":\"termX.keyword\",\"min_doc_count\":0}}}}";
+        String query3 = "{\"size\":100,\"aggregations\":{\"significant_termX\":{\"significant_terms\":{\"field\":\"termX.keyword\",\"min_doc_count\":0}}}}";


What was the reason for swapping to a hard coded implementation?

This test inserts 17 documents and then does a query for matches on them checking the 'hits'. This hits have a default limit of 10... so this test was failing all the time because it depends if you got the right mix of 10/17 items in the output. By making this size much larger this should prevent this issue from happening again.

Always be careful with defaults!

Originally posted by @peternied in #2190 (comment)

…nsearch-project#2205) 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 <petern@amazon.com> (cherry picked from commit a57fd0a)

…nsearch-project#2205) 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 <petern@amazon.com> (cherry picked from commit a57fd0a) Signed-off-by: Stephen Crawford <steecraw@amazon.com>

…nsearch-project#2205) 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 <petern@amazon.com> (cherry picked from commit a57fd0a)

peternied requested a review from a team October 28, 2022 16:31

peternied force-pushed the backport/backport-2190-to-2.x branch from d0acd61 to 385f5bc Compare October 28, 2022 16:32

peternied mentioned this pull request Oct 28, 2022

Add CI for Windows and MacOS platforms #2190

Merged

3 tasks

stephen-crawford reviewed Oct 31, 2022

View reviewed changes

peternied merged commit 920fdf0 into 2.x Oct 31, 2022

peternied deleted the backport/backport-2190-to-2.x branch October 31, 2022 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Backport 2.x] Add CI for Windows and MacOS platforms (#2190)#2205

[Backport 2.x] Add CI for Windows and MacOS platforms (#2190)#2205
peternied merged 1 commit into2.xfrom
backport/backport-2190-to-2.x

peternied commented Oct 28, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Oct 28, 2022 •

edited by codecov bot

Loading

Uh oh!

stephen-crawford Oct 31, 2022

Uh oh!

peternied Oct 31, 2022

Uh oh!

stephen-crawford Oct 31, 2022

Uh oh!

stephen-crawford Oct 31, 2022

Uh oh!

peternied Oct 31, 2022

Uh oh!

stephen-crawford Oct 31, 2022

Uh oh!

stephen-crawford Oct 31, 2022

Uh oh!

peternied Oct 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

peternied commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Check List

Uh oh!

codecov-commenter commented Oct 28, 2022 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peternied commented Oct 28, 2022 •

edited

Loading

codecov-commenter commented Oct 28, 2022 •

edited by codecov bot

Loading