Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG_FIX] fix check for agg rules in detector trigger condition to create chained findings monitor #992

Merged
merged 7 commits into from
Apr 27, 2024

Conversation

eirsep
Copy link
Member

@eirsep eirsep commented Apr 25, 2024

Description

remove check for agg rules in detector trigger condition to create chained findings monitor.
Currently, chained findings monitor is not created unless ids of aggregation Sigma rules are not mentioned in the detector trigger condition
Right behaviour is to create chained findings monitor irrespective of trigger condition.
Add agg rule tags to chained findings monitor

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • 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.

…cket level monitor

Signed-off-by: Surya Sashank Nistala <[email protected]>
@jowg-amazon
Copy link
Collaborator

Without this additional check in the logic won't we call createDocLevelMonitorMatchAllRequest even for non aggregation rules? Is this the expected behavior?

@eirsep eirsep changed the title [BUG_FIX] remove check for agg rules in detector trigger condition to create chained findings monitor [BUG_FIX] fix check for agg rules in detector trigger condition to create chained findings monitor Apr 25, 2024
@eirsep
Copy link
Member Author

eirsep commented Apr 25, 2024

Without this additional check in the logic won't we call createDocLevelMonitorMatchAllRequest even for non aggregation rules? Is this the expected behavior?

fixed to check for atleast one agg rule

RestRequest.Method restMethod
) {
Method restMethod,
List<Pair<String, Rule>> queries) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are multiple aggregation rules/queries, do the queried passed in here include all of them or only the aggregation rule that created the bucket level monitor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are only adding the tags of each aggregation query to the doc level monitor's DocLevelQuery tags. Btw each agg rule creates 1 bucket level monitor. For all bucket level monitors there is one chained findings doc level monitor

sbcd90
sbcd90 previously approved these changes Apr 27, 2024
Signed-off-by: Surya Sashank Nistala <[email protected]>
@eirsep
Copy link
Member Author

eirsep commented Apr 27, 2024

Flaky tests failed

- org.opensearch.securityanalytics.correlation.CorrelationEngineRestApiIT.testBasicCorrelationEngineWorkflowWithRolloverByMaxDoc
 - org.opensearch.securityanalytics.resthandler.DetectorMonitorRestApiIT.testRemoveDocLevelRuleAddAggregationRules_verifyFindings_success
 - org.opensearch.securityanalytics.resthandler.DetectorMonitorRestApiIT.testRemoveDocLevelRuleAddAggregationRules_verifyFindings_success
 - org.opensearch.securityanalytics.threatIntel.integTests.ThreatIntelJobRunnerIT.testCreateDetector_threatIntelEnabled_testJobRunner

@eirsep eirsep merged commit 31a81aa into main Apr 27, 2024
11 of 30 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security-analytics/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security-analytics/backport-2.x
# Create a new branch
git switch --create backport-992-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 31a81aa95b1075f403fbb4fb5d3f095e039ad060
# Push it to GitHub
git push --set-upstream origin backport-992-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security-analytics/backport-2.x

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

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.13 failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security-analytics/backport-2.13 2.13
# Navigate to the new working tree
pushd ../.worktrees/security-analytics/backport-2.13
# Create a new branch
git switch --create backport-992-to-2.13
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 31a81aa95b1075f403fbb4fb5d3f095e039ad060
# Push it to GitHub
git push --set-upstream origin backport-992-to-2.13
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security-analytics/backport-2.13

Then, create a pull request where the base branch is 2.13 and the compare/head branch is backport-992-to-2.13.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.12 failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security-analytics/backport-2.12 2.12
# Navigate to the new working tree
pushd ../.worktrees/security-analytics/backport-2.12
# Create a new branch
git switch --create backport-992-to-2.12
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 31a81aa95b1075f403fbb4fb5d3f095e039ad060
# Push it to GitHub
git push --set-upstream origin backport-992-to-2.12
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security-analytics/backport-2.12

Then, create a pull request where the base branch is 2.12 and the compare/head branch is backport-992-to-2.12.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.11 failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security-analytics/backport-2.11 2.11
# Navigate to the new working tree
pushd ../.worktrees/security-analytics/backport-2.11
# Create a new branch
git switch --create backport-992-to-2.11
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 31a81aa95b1075f403fbb4fb5d3f095e039ad060
# Push it to GitHub
git push --set-upstream origin backport-992-to-2.11
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security-analytics/backport-2.11

Then, create a pull request where the base branch is 2.11 and the compare/head branch is backport-992-to-2.11.

eirsep added a commit to eirsep/security-analytics that referenced this pull request Apr 27, 2024
…eate chained findings monitor (opensearch-project#992)

* remove chekc for agg rules in detector trigger condition to create bucket level monitor

Signed-off-by: Surya Sashank Nistala <[email protected]>

* add agg rules tags in chained monitor query to match trigger condition of detector

Signed-off-by: Surya Sashank Nistala <[email protected]>

* fix check to evaluate agg rules present when creating chained findings monitor

Signed-off-by: Surya Sashank Nistala <[email protected]>

* fix tests where check on group by trigger existed earlier

Signed-off-by: Surya Sashank Nistala <[email protected]>

* fix race condition while creating first monitor

Signed-off-by: Surya Sashank Nistala <[email protected]>

* add test to verify detector trigger function for aggregation rules

Signed-off-by: Surya Sashank Nistala <[email protected]>

* revert step listener change

Signed-off-by: Surya Sashank Nistala <[email protected]>

---------

Signed-off-by: Surya Sashank Nistala <[email protected]>
@eirsep eirsep mentioned this pull request Apr 27, 2024
5 tasks
eirsep added a commit that referenced this pull request Apr 27, 2024
…eate chained findings monitor (#992) (#1002)

* remove chekc for agg rules in detector trigger condition to create bucket level monitor



* add agg rules tags in chained monitor query to match trigger condition of detector



* fix check to evaluate agg rules present when creating chained findings monitor



* fix tests where check on group by trigger existed earlier



* fix race condition while creating first monitor



* add test to verify detector trigger function for aggregation rules



* revert step listener change



---------

Signed-off-by: Surya Sashank Nistala <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 27, 2024
…eate chained findings monitor (#992) (#1002)

* remove chekc for agg rules in detector trigger condition to create bucket level monitor

* add agg rules tags in chained monitor query to match trigger condition of detector

* fix check to evaluate agg rules present when creating chained findings monitor

* fix tests where check on group by trigger existed earlier

* fix race condition while creating first monitor

* add test to verify detector trigger function for aggregation rules

* revert step listener change

---------

Signed-off-by: Surya Sashank Nistala <[email protected]>
(cherry picked from commit 07bf73f)
eirsep added a commit to eirsep/security-analytics that referenced this pull request Apr 28, 2024
…eate chained findings monitor (opensearch-project#992)

* remove chekc for agg rules in detector trigger condition to create bucket level monitor

Signed-off-by: Surya Sashank Nistala <[email protected]>

* add agg rules tags in chained monitor query to match trigger condition of detector

Signed-off-by: Surya Sashank Nistala <[email protected]>

* fix check to evaluate agg rules present when creating chained findings monitor

Signed-off-by: Surya Sashank Nistala <[email protected]>

* fix tests where check on group by trigger existed earlier

Signed-off-by: Surya Sashank Nistala <[email protected]>

* fix race condition while creating first monitor

Signed-off-by: Surya Sashank Nistala <[email protected]>

* add test to verify detector trigger function for aggregation rules

Signed-off-by: Surya Sashank Nistala <[email protected]>

* revert step listener change

Signed-off-by: Surya Sashank Nistala <[email protected]>

---------

Signed-off-by: Surya Sashank Nistala <[email protected]>
@eirsep eirsep mentioned this pull request Apr 28, 2024
5 tasks
eirsep added a commit that referenced this pull request Apr 29, 2024
* [BUG_FIX] fix check for agg rules in detector trigger condition to create chained findings monitor (#992)

* remove chekc for agg rules in detector trigger condition to create bucket level monitor

Signed-off-by: Surya Sashank Nistala <[email protected]>

* add agg rules tags in chained monitor query to match trigger condition of detector

Signed-off-by: Surya Sashank Nistala <[email protected]>

* fix check to evaluate agg rules present when creating chained findings monitor

Signed-off-by: Surya Sashank Nistala <[email protected]>

* fix tests where check on group by trigger existed earlier

Signed-off-by: Surya Sashank Nistala <[email protected]>

* fix race condition while creating first monitor

Signed-off-by: Surya Sashank Nistala <[email protected]>

* add test to verify detector trigger function for aggregation rules

Signed-off-by: Surya Sashank Nistala <[email protected]>

* revert step listener change

Signed-off-by: Surya Sashank Nistala <[email protected]>

---------

Signed-off-by: Surya Sashank Nistala <[email protected]>

* fix imports

Signed-off-by: Surya Sashank Nistala <[email protected]>

---------

Signed-off-by: Surya Sashank Nistala <[email protected]>
eirsep added a commit that referenced this pull request Apr 29, 2024
…eate chained findings monitor (#992) (#1002) (#1003)

* remove chekc for agg rules in detector trigger condition to create bucket level monitor

* add agg rules tags in chained monitor query to match trigger condition of detector

* fix check to evaluate agg rules present when creating chained findings monitor

* fix tests where check on group by trigger existed earlier

* fix race condition while creating first monitor

* add test to verify detector trigger function for aggregation rules

* revert step listener change

---------

Signed-off-by: Surya Sashank Nistala <[email protected]>
(cherry picked from commit 07bf73f)

Co-authored-by: Surya Sashank Nistala <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants