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

[Security Solution] [Detections] Combine multiple timestamp searches into single request #96078

Merged
merged 13 commits into from
Apr 20, 2021

Conversation

dhurley14
Copy link
Contributor

@dhurley14 dhurley14 commented Apr 1, 2021

Summary

Ref: #93550

We've been sending two separate queries when a timestamp override is provided to a rule definition in order to ensure we are also querying data that might have been sent over but is missing that timestamp override field for some reason.

The proposed change in this PR merges the logic of these two queries into one single search.

Added a few integration tests which should cover all cases but please take a look at those and point out any missing cases that should be covered.

Test data
POST myfakeindex-1/_doc
{
  "message": "hello world 1"
}

POST myfakeindex-2/_doc
{
  "message": "hello world 2",
  "event": {
    "ingested": "2021-04-13T21:17:56.235Z"
  }
}

POST myfakeindex-3/_doc
{
  "message": "hello world 3",
  "@timestamp": "2021-04-13T21:17:56.235Z"
}

POST myfakeindex-4/_doc
{
  "message": "hello world 2",
  "@timestamp": "2021-04-13T21:17:56.235Z",
  "event": {
    "ingested": "2021-04-13T21:17:56.235Z"
  }
}
Rule Query
GET myfa*/_search
{
  "query": {
    "bool": {
      "filter": [
        {
          "bool": {
            "must": [],
            "filter": [
              {
                "match_all": {}
              }
            ],
            "should": [],
            "must_not": []
          }
        },
        {
          "bool": {
            "filter": [
              {
                "bool": {
                  "should": [
                    {
                      "range": {
                        "event.ingested": {
                          "lte": "2021-04-15T13:26:40.428Z",
                          "gte": "2021-04-15T12:26:30.428Z",
                          "format": "strict_date_optional_time"
                        }
                      }
                    },
                    {
                      "bool": {
                        "filter": [
                          {
                            "range": {
                              "@timestamp": {
                                "lte": "2021-04-15T13:26:40.428Z",
                                "gte": "2021-04-15T12:26:30.428Z",
                                "format": "strict_date_optional_time"
                              }
                            }
                          },
                          {
                            "bool": {
                              "must_not": {
                                "exists": {
                                  "field": "event.ingested"
                                }
                              }
                            }
                          }
                        ]
                      }
                    }
                  ],
                  "minimum_should_match": 1
                }
              }
            ]
          }
        },
        {
          "match_all": {}
        }
      ]
    }
  },
  "fields": [
    {
      "field": "*",
      "include_unmapped": true
    }
  ],
  "sort": [
    {
      "event.ingested": {
        "order": "asc",
        "unmapped_type": "date"
      }
    },
    {
      "@timestamp": {
        "order": "asc",
        "unmapped_type": "date"
      }
    }
  ]
}

After posting the above documents and executing the query in the search profiler in dev tools we can see that the query is behaving as we expect.

Screenshot profiled_timestamps_query_screenshot

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Comment on lines +66 to +67
// @ts-expect-error
format: 'strict_date_optional_time',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

typscript complains that the format field is not applicable to the @timestamp field. I think the type is wrong.

@dhurley14 dhurley14 added Feature:Detection Rules Anything related to Security Solution's Detection Rules release_note:enhancement review Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team v7.13.0 v8.0.0 labels Apr 15, 2021
@dhurley14 dhurley14 marked this pull request as ready for review April 15, 2021 17:28
@dhurley14 dhurley14 requested a review from a team as a code owner April 15, 2021 17:28
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@dhurley14 dhurley14 added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 15, 2021
@dhurley14
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@marshallmain marshallmain left a comment

Choose a reason for hiding this comment

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

LGTM! Good integration test coverage 👍

…egration test, fixes logic for possibility of receving a null value in sort ids, removes unused utility function for checking valid sort ids
…as failing because we moved the logic for checking that out of the build search query function and up into the big loop. So I moved that unit test into the search after bulk create test file.
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @dhurley14

@dhurley14 dhurley14 merged commit 4d2414e into elastic:master Apr 20, 2021
@dhurley14 dhurley14 deleted the v2-multiple-sort-fields branch April 20, 2021 19:16
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 20, 2021
…into single request (elastic#96078)

* merge multiple timestamp queries into one single search

* fix types and unit tests

* remove unused code for sending secondary search

* removes unused excludeDocsWithTimestampOverride

* adds integration tests to cover cases that should / should not generate signals when timestamp override is present in rule

* adds integration test to ensure unmapped sort fields do not break search after functionality of detection rules

* Need to figure out why moving the tests around fixed them...

* updates tests with new es archive data and fixes bug where exclusion filter was hardcoded to event.ingested :yikes:

* remove dead commented out code

* fixes typo in test file, removes redundant delete signals call in integration test, fixes logic for possibility of receving a null value in sort ids, removes unused utility function for checking valid sort ids

* a unit test for checking if an empty string of a sort id is present was failing because we moved the logic for checking that out of the build search query function and up into the big loop. So I moved that unit test into the search after bulk create test file.

* fix types

* removes isEmpty since it doesn't check for empty strings
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 20, 2021
…into single request (#96078) (#97722)

* merge multiple timestamp queries into one single search

* fix types and unit tests

* remove unused code for sending secondary search

* removes unused excludeDocsWithTimestampOverride

* adds integration tests to cover cases that should / should not generate signals when timestamp override is present in rule

* adds integration test to ensure unmapped sort fields do not break search after functionality of detection rules

* Need to figure out why moving the tests around fixed them...

* updates tests with new es archive data and fixes bug where exclusion filter was hardcoded to event.ingested :yikes:

* remove dead commented out code

* fixes typo in test file, removes redundant delete signals call in integration test, fixes logic for possibility of receving a null value in sort ids, removes unused utility function for checking valid sort ids

* a unit test for checking if an empty string of a sort id is present was failing because we moved the logic for checking that out of the build search query function and up into the big loop. So I moved that unit test into the search after bulk create test file.

* fix types

* removes isEmpty since it doesn't check for empty strings

Co-authored-by: Devin W. Hurley <devin.hurley@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Detection Rules Anything related to Security Solution's Detection Rules release_note:enhancement review Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants