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

[8.13] [Security Solution] Incorporates EQL options in EQL query validation on both Rule Creation and Timeline (#178468) #178994

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

kibanamachine
Copy link
Contributor

Backport

This will backport the following commits from main to 8.13:

Questions ?

Please refer to the Backport tool documentation

…on both Rule Creation and Timeline (elastic#178468)

## Summary

This PR updates the Detection Rule Creation and Timeline forms to
account for the new(er) EQL options fields (`timestamp_field`,
`event_category_field`, and `tiebreaker_field`) when validating the EQL
query. While the rule query and timeline query (respectively) would
correctly persist and use these options, they were unused during EQL
validation, meaning that in certain situations it was impossible to
produce a "valid" (from the perspective of the form) EQL query if your
data necessitated one of those options (see
elastic#158326 for more details).

### Changes
The above was accomplished by:

1. Adding a hidden form field to the UI for `eqlOptions`
* A lack of a field component meant that `formData` never held any value
for this field
* This meant that the form-based validations (i.e. `eqlValidator`) would
not have access to `eqlOptions`
2. Updating the `onOptionsChange` hook to additionally inform the form
of changes to `eqlOptions`
* This allows the form to have updated values for `eqlOptions` with
minimal changes
* Not ideal from a data-flow perspective, since the field changes are
updating two copies of `eqlOptions`. However, we're only using the "form
copy" for the validation call, and everything else works as it did
before.

### Additional changes
__Update__: I've reverted the described changes to the default timeline
values below, and moved that to a [separate
issue](elastic/security-team#8892).

In the course of performing the above tasks for timeline, I found the
behavior there to be slightly different: we were specifying default
values for these fields, which was redundant in some cases
(`timestamp_field`, `event_category_field`), and invalid in the case of
`tiebreaker_field`. You can see here how the `tiebreaker_field` must be
changed to make the query valid:

https://github.com/elastic/kibana/assets/657252/556bc998-c147-4825-8bde-5d8d03873e75

~~I opted to remove these default values, as well as the defaulting
behavior from the search strategy call (see
10a47fd). While these values may be
persisted in existing saved timelines, this makes the EQL workflow much
cleaner for new users, and those users with persisted EQL options can
now modify and validate those options on the fly.~~

#### Steps to Review

For @elastic/kibana-visualizations and/or @elastic/kibana-data-discovery
: I've added a few tests to the EQL search strategy that document
existing behavior RE EQL option fields. Nothing about the search
strategy itself was changed. If you think the tests aren't useful, I'm
happy to delete them.

1. Create an index without a `@timestamp` field, and insert some data:
    <details>
    <summary>Dev Tools</summary>

       PUT timestamp/
       {
         "mappings": {
           "dynamic": "strict",
           "properties": {
             "timestamp": {
               "type": "date",
               "format": "epoch_second"
             },
             "locale": {
               "type": "keyword"
             },
             "created_at": {
               "type": "date",
               "format": "epoch_second"
             },
             "event": {
               "properties": {
                 "category": {
                   "ignore_above": 1024,
                   "type": "keyword"
                 }
               }
             }
           }
         }
       }

       PUT timestamp/_doc/4
       {
         "timestamp": 1710211630,
         "locale": "es",
         "created_at": 1710211630,
         "event.category": "configuration"
       }
    </details>
2. Create a new EQL detection rule, and choose `timestamp` as the index
pattern
3. Type `any where true` as the query, and observe a validation error
(`Found 1 problem line -1:-1: Unknown column [@timestamp]`)
4. Open EQL Settings popover, and select `timestamp` from the `Timestamp
field` dropdown
5. Observe that the query has been revalidated, and found valid.

### New Behavior:

https://github.com/elastic/kibana/assets/657252/22a63363-d597-4dfd-8f9a-647f4084ec0e

### Related Issues
* Addresses elastic#158326

### Checklist

Delete any items that are not applicable to this PR.

- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

(cherry picked from commit 5be91c9)
…ionality"

This reverts commit 7ac409a.

These changes are still necessary on the 8.13 branch, which does not
contain that cypress task functionality.
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 11.6MB 11.6MB +1.3KB

History

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

cc @rylnd

@kibanamachine kibanamachine merged commit 5814aa9 into elastic:8.13 Mar 19, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants