Prevent message collection from being updated after message count has been received by peternied · Pull Request #2180 · opensearch-project/security

peternied · 2022-10-19T18:27:47Z

Description

Prevent message collection from being updated after message count has been received

Also added mechanism to detect if messages were missed so tests can be updated to appropriate counts

Issues Resolved

Saw a failure for PR #2167 which had no code impact, saw the test testSSLPlainText was failing, looks like it was exiting early because the test was only looking for a smaller number of messages.

Testing

Tested locally, CI should confirm things are working as expected. Also ran the bulk integration test workflow [1], 18 runs with no issues

[1] https://github.com/peternied/security/actions/runs/3291303445/jobs/5425278205

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.

… been recieved Also added mechanism to detect if messages were missed so tests can be updated to appropriate counts Signed-off-by: Peter Nied <petern@amazon.com>

peternied · 2022-10-19T18:29:05Z

.github/workflows/ci.yml

      run: ./gradlew clean build -Dbuild.snapshot=false -x test -x integrationTest

    - name: Test
-      run: OPENDISTRO_SECURITY_TEST_OPENSSL_OPT=true ./gradlew test integrationTest -i


This log is ~27mb which doesn't open in the browser, by disabling info log level it will be easier to look at only failing tests in the GitHub Action UX. Note; the full details can be downloaded by looking for artifacts on the test run

peternied · 2022-10-19T18:29:58Z

src/test/java/org/opensearch/security/auditlog/integration/TestAuditlogImpl.java

+                        .collect(Collectors.joining("\n")))
+                    .toString();
+
+                    throw new RuntimeException(missedMessagesErrorMessage);


This failure should help identify any other tests with the same bug in them, waiting for a full test run before marking this ready for review

Runtime exception new RuntimeException(missedMessagesErrorMessage); will be always catch be the below } catch (final Exception e) { but this is probably ok

cwperks · 2022-10-19T19:10:11Z

Thank you @peternied! I had changed the number in this PR (https://github.com/opensearch-project/security/pull/2166/files#r997685463) because this test was failing.

Signed-off-by: Peter Nied <petern@amazon.com>

peternied · 2022-10-19T20:23:34Z

Fixed the following tests that these changes exposed issued with:

org.opensearch.security.auditlog.compliance.ComplianceAuditlogTest.testExternalConfig
org.opensearch.security.auditlog.compliance.ComplianceAuditlogTest.testInternalConfig
org.opensearch.security.auditlog.compliance.ComplianceAuditlogTest.testSourceFilter
org.opensearch.security.auditlog.compliance.ComplianceAuditlogTest.testSourceFilterMsearch

Signed-off-by: Peter Nied <petern@amazon.com>

peternied · 2022-10-20T17:15:16Z

Restarted the CI as there was a network adapter issue on the GHA runner that caused the failure.

codecov-commenter · 2022-10-20T19:37:51Z

Codecov Report

Merging #2180 (ec03ff4) into main (b836040) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #2180   +/-   ##
=========================================
  Coverage     61.07%   61.08%           
- Complexity     3260     3262    +2     
=========================================
  Files           259      259           
  Lines         18327    18330    +3     
  Branches       3249     3249           
=========================================
+ Hits          11193    11196    +3     
  Misses         5539     5539           
  Partials       1595     1595

Impacted Files	Coverage Δ
...pensearch/security/auditlog/impl/AuditMessage.java	`74.12% <100.00%> (+0.39%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cwperks

Thank you @peternied, I like the addition of the missedMessages section here. It gives more context when running these tests.

cwperks · 2022-10-20T21:32:06Z

src/test/java/org/opensearch/security/auditlog/integration/TestAuditlogImpl.java

-        TestAuditlogImpl.messages = messages; 
+        final CountDownLatch latch = resetAuditStorage(expectedCount, messages);

        try {


Thoughts on surrounding this all with one try block?

Seems like a good nit, if I make any more changes I'll include this in with it - sounds fair?

DarshitChanpura

Ty for this PR @peternied ! Added a couple comments.

DarshitChanpura · 2022-10-21T18:06:20Z

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

+        assertThat(genderMsg.getRequestBody(), containsString("Gender"));
+        assertThat(genderMsg.getRequestBody(), not(containsString("Salary")));
+
+        Assert.assertTrue(validateMsgs(messages));


Should this replaced with Matcher's assertTrue?

Rather than cleanup this single line; we should really rewrite or remove this statement. I've created [FEATURE] AuditMessage validation in tests should be more useful / removed #2188 for tracking.

DarshitChanpura · 2022-10-21T18:06:40Z

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

+            );
+        });
+
+        Assert.assertTrue(validateMsgs(messages));


same as above

Seems like a nitpick, if we want to invest there are better kinds of improvements we can make see #2188

… been received (opensearch-project#2180) Also adds mechanism to detect if messages were missed so tests can be updated to appropriate counts. Signed-off-by: Peter Nied <petern@amazon.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com>

@cwperks

Windows build and test support for 1.3 - Use most recent format of CI workflows from main - Backport #2206 - Supply workarounds for JDK8 incompatible APIs for Encoding / Pattern matching (Thanks @cwperks!) - Backport only freeport logic from #1638 - Backport #1758 - All updates to TestAuditlogImpl.java from main - #2180 - #1935 - #1920 - #1914 - #1829 - And Targeted test updates for ComplianceAuditlogTest and BasicAuditlogTest to keep up with TestAuditlogImpl.java updates Signed-off-by: Peter Nied <petern@amazon.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com>

opensearch-trigger-bot · 2023-07-19T16:17:27Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2180-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ba9d82ef6a2c6da137c19fcaaddce6dabcf7160f
# Push it to GitHub
git push --set-upstream origin backport/backport-2180-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2180-to-2.x.

peternied · 2023-07-19T16:18:15Z

Seeing failures on backport into 2.x - #3032 (review) going to trigger backport

Tests with failures:
 - org.opensearch.security.auditlog.compliance.ComplianceAuditlogTest.testInternalConfig
 - org.opensearch.security.auditlog.compliance.ComplianceAuditlogTest.testComplianceEnable
 - org.opensearch.security.auditlog.compliance.ComplianceAuditlogTest.testUpdate
 - org.opensearch.security.auditlog.compliance.RestApiComplianceAuditlogTest.testRestApiNewUser
 - org.opensearch.security.auditlog.compliance.ComplianceAuditlogTest.testInternalConfig
 - org.opensearch.security.auditlog.compliance.RestApiComplianceAuditlogTest.testAutoInit
 - org.opensearch.security.multitenancy.test.TenancyMultitenancyEnabledTests.testMultitenancyDisabled_endToEndTest
 - org.opensearch.security.auditlog.compliance.ComplianceAuditlogTest.testUpdate
 - org.opensearch.security.auditlog.compliance.RestApiComplianceAuditlogTest.testAutoInit
 - org.opensearch.security.auditlog.compliance.ComplianceAuditlogTest.testComplianceEnable
 - ```

peternied · 2023-07-19T16:19:21Z

Actually my day is looking booked up - @DarshitChanpura could you see able backporting this PR into the 2.x line since you were looking into #3021?

… been received (opensearch-project#2180) Also adds mechanism to detect if messages were missed so tests can be updated to appropriate counts. Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit ba9d82e)

…ssage count has been received (#2180) (#3035) * Prevent message collection from being updated after message count has been received (#2180) Also adds mechanism to detect if messages were missed so tests can be updated to appropriate counts. Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit ba9d82e) * Fixes failing citest task Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes spotlessChecks Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes test assertions to reflect correct number Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Co-authored-by: Peter Nied <petern@amazon.com> Co-authored-by: Craig Perkins <cwperx@amazon.com>

Prevent message collection from being updated after message count has…

f89f43b

… been recieved Also added mechanism to detect if messages were missed so tests can be updated to appropriate counts Signed-off-by: Peter Nied <petern@amazon.com>

peternied commented Oct 19, 2022

View reviewed changes

Fix broken audit tests

7c3cc23

Signed-off-by: Peter Nied <petern@amazon.com>

peternied added 2 commits October 19, 2022 22:20

Fix checkstyle miss from test fixes

bd20a98

Signed-off-by: Peter Nied <petern@amazon.com>

Allow for audit message order to change in testSourceFilterMsearch

ec03ff4

Signed-off-by: Peter Nied <petern@amazon.com>

peternied marked this pull request as ready for review October 20, 2022 17:14

peternied requested a review from a team October 20, 2022 17:14

cwperks approved these changes Oct 20, 2022

View reviewed changes

peternied mentioned this pull request Oct 20, 2022

Test extended to verify audit logs. #2153

Merged

3 tasks

lukasz-soszynski-eliatra approved these changes Oct 21, 2022

View reviewed changes

DarshitChanpura reviewed Oct 21, 2022

View reviewed changes

DarshitChanpura approved these changes Oct 24, 2022

View reviewed changes

DarshitChanpura merged commit ba9d82e into opensearch-project:main Oct 24, 2022

peternied deleted the fixlogsink-wrong-message-count branch October 24, 2022 17:35

peternied mentioned this pull request Dec 2, 2022

Windows build and test support for 1.3 #2291

Merged

3 tasks

cwperks mentioned this pull request Jul 19, 2023

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

Closed

peternied added the backport 2.x label Jul 19, 2023

DarshitChanpura mentioned this pull request Jul 19, 2023

[Backport 2.x] Prevent message collection from being updated after message count has been received (#2180) #3035

Merged

3 tasks

Comments

Conversation

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

Uh oh!

Description

Issues Resolved

Testing

Check List

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!

cwperks commented Oct 19, 2022

Uh oh!

peternied commented Oct 19, 2022

Uh oh!

peternied commented Oct 20, 2022

Uh oh!

codecov-commenter commented Oct 20, 2022

Codecov Report

Uh oh!

cwperks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

DarshitChanpura left a comment

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!

opensearch-trigger-bot bot commented Jul 19, 2023

Uh oh!

peternied commented Jul 19, 2023

Uh oh!

peternied commented Jul 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

peternied commented Oct 19, 2022 •

edited

Loading

cwperks left a comment •

edited

Loading