-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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][Detection Engine] adds data tier filters to Kibana advanced settings #186908
[Security Solution][Detection Engine] adds data tier filters to Kibana advanced settings #186908
Conversation
…a advanced settings
…://github.com/vitaliidm/kibana into de_8_16/add_data_tiers_filtering_adv_settings
…://github.com/vitaliidm/kibana into de_8_16/add_data_tiers_filtering_adv_settings
…://github.com/vitaliidm/kibana into de_8_16/add_data_tiers_filtering_adv_settings
…://github.com/vitaliidm/kibana into de_8_16/add_data_tiers_filtering_adv_settings
description: i18n.translate( | ||
'xpack.securitySolution.uiSettings.excludedDataTiersForRuleExecutionDescription', | ||
{ | ||
defaultMessage: ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nastasha-solomon, can you have a look at this advanced settings name and description?
Let us know, if we can improved it
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vitaliidm, what do you think about aligning this advanced setting's name with the advanced setting that excludes cold and frozen data tiers in Analyzer (i.e., change excludedDataTiersForRuleExecution
to excludeColdAndFrozenTiersInRules
or excludeSpecifiedTiersInRules
)?
I also really like that the excludeColdAndFrozenTiersInAnalyzer
description explains what the setting does, rather than when you'd use it. There are n number of use cases for toggling a setting that includes/excludes data in a query, so it might be more practical to inform user about what will happen if they use the setting instead of why it's useful for them. Here's a potential revision to consider:
When enabled, events from the specified data tiers are not searched during rules executions. If you specify multiple data tiers, separate values with commas. For example: data_frozen,data_cold
If you want to keep the bit about the value that the setting's offering, here's an alternative:
When enabled, events from the specified data tiers are not searched during rules executions. This might help to improve rule performance or reduce execution time. If you specify multiple data tiers, separate values with commas. For example: data_frozen,data_cold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about aligning this advanced setting's name with the advanced setting that excludes cold and frozen data tiers in Analyzer (i.e., change excludedDataTiersForRuleExecution to excludeColdAndFrozenTiersInRules or excludeSpecifiedTiersInRules )?
@stash, I think excludedDataTiersForRuleExecution
suits better for the purposes. Since Analyzer is boolean switch and this option is string of values - so its name reflects that data tiers in that config would be excluded during rule execution. Initially, users were able to exclude any tier(hit and warm too), but we decided to limit it just to frozen and cold. This name would also help, if decide to include warm tier there too at some point.
When enabled, events from the specified data tiers are not searched during rules executions. This might help to improve rule performance or reduce execution time. If you specify multiple data tiers, separate values with commas. For example: data_frozen,data_cold
I like this option. Only change I would put there is
When configured, events from the specified data tiers are not searched during rules executions. This might help to improve rule performance or reduce execution time. If you specify multiple data tiers, separate values with commas. For example: data_frozen,data_cold
replacing enabled with configured, since it is not boolean switch that can be enabeld/disabled, but config stringm that can have multiple values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look great (just one nit about adding a helper). I smoke tested as well, and things are filtered as expected! Nice work here.
I do see that you have release_note:feature
but is this something we're going to be adding dedicated docs for? The tooltip/UI guidance looks good, but it feels like we might need more than that?
const searchArgs = | ||
alertServices.scopedClusterClient.asCurrentUser.eql.search.mock.calls[0][0]; | ||
|
||
expect(searchArgs).toHaveProperty( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, this is a nice matcher I wasn't aware of!
? await getDataTierFilter({ uiSettingsClient: services.uiSettingsClient }) | ||
: []; | ||
|
||
const mergedFilters = [...(filters ? filters : []), ...dataTiersFilters]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're doing this transformation in a few places. Might be worth adding a utility function like 'mergeArrays`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all other places it's just simple merge using spread operator
[...array1, ...array2]
No additional logic involved, so I think introducing a separate function that does not do anything apart from spread merge is redundant.
And in fact, would make reading code harder. Since, you'll need to go that function to see what exactly it doing
@ryland, we have a doc request for that elastic/security-docs#5483 |
⏳ Build in-progress
Failed CI StepsHistory
cc @vitaliidm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left the one suggestion!
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @vitaliidm |
…om adv settings (#196390) ## Summary - fixes absent data tier filter for ES|QL rule - followup to #186908 ### Demo https://github.com/user-attachments/assets/a6f1290f-ea77-43bf-8def-42712ca5d1b0 ### How to test Create a deployment with cold and frozen data tiers and use following commands to create index and ILM <details> <summary>Data tiers commands</summary> ```JSON PUT /_cluster/settings { "persistent": { "indices.lifecycle.poll_interval": "1m" } } PUT /_ilm/policy/filtering_data_tiers { "policy": { "phases": { "frozen": { "min_age": "10m", "actions": { "searchable_snapshot": { "snapshot_repository": "found-snapshots", "force_merge_index": true } } }, "cold": { "min_age": "1m", "actions": { "searchable_snapshot": { "snapshot_repository": "found-snapshots", "force_merge_index": true }, "set_priority": { "priority": 0 } } }, "hot": { "min_age": "0ms", "actions": { "set_priority": { "priority": 100 } } } } } } PUT /_index_template/filtering_data_tiers_template { "index_patterns": [ "filtering_data_tiers*" ], "template": { "settings": { "index.lifecycle.name": "filtering_data_tiers", "index.lifecycle.rollover_alias": "test-filtering_data_tiers" }, "mappings": { "_meta": { "version": "1.6.0" }, "properties": { "@timestamp": { "type": "date" }, "host": { "properties": { "name": { "type": "keyword", "ignore_above": 1024 } } } } } } } PUT /filtering_data_tiers-000001 { "aliases": { "filtering_data_tiers": { "is_write_index": true } } } POST filtering_data_tiers/_doc { "@timestamp": "2024-07-08T17:00:01.000Z", "host.name": "test-0" } ``` </details>
…om adv settings (elastic#196390) ## Summary - fixes absent data tier filter for ES|QL rule - followup to elastic#186908 ### Demo https://github.com/user-attachments/assets/a6f1290f-ea77-43bf-8def-42712ca5d1b0 ### How to test Create a deployment with cold and frozen data tiers and use following commands to create index and ILM <details> <summary>Data tiers commands</summary> ```JSON PUT /_cluster/settings { "persistent": { "indices.lifecycle.poll_interval": "1m" } } PUT /_ilm/policy/filtering_data_tiers { "policy": { "phases": { "frozen": { "min_age": "10m", "actions": { "searchable_snapshot": { "snapshot_repository": "found-snapshots", "force_merge_index": true } } }, "cold": { "min_age": "1m", "actions": { "searchable_snapshot": { "snapshot_repository": "found-snapshots", "force_merge_index": true }, "set_priority": { "priority": 0 } } }, "hot": { "min_age": "0ms", "actions": { "set_priority": { "priority": 100 } } } } } } PUT /_index_template/filtering_data_tiers_template { "index_patterns": [ "filtering_data_tiers*" ], "template": { "settings": { "index.lifecycle.name": "filtering_data_tiers", "index.lifecycle.rollover_alias": "test-filtering_data_tiers" }, "mappings": { "_meta": { "version": "1.6.0" }, "properties": { "@timestamp": { "type": "date" }, "host": { "properties": { "name": { "type": "keyword", "ignore_above": 1024 } } } } } } } PUT /filtering_data_tiers-000001 { "aliases": { "filtering_data_tiers": { "is_write_index": true } } } POST filtering_data_tiers/_doc { "@timestamp": "2024-07-08T17:00:01.000Z", "host.name": "test-0" } ``` </details> (cherry picked from commit c79f0ae)
…om adv settings (elastic#196390) ## Summary - fixes absent data tier filter for ES|QL rule - followup to elastic#186908 ### Demo https://github.com/user-attachments/assets/a6f1290f-ea77-43bf-8def-42712ca5d1b0 ### How to test Create a deployment with cold and frozen data tiers and use following commands to create index and ILM <details> <summary>Data tiers commands</summary> ```JSON PUT /_cluster/settings { "persistent": { "indices.lifecycle.poll_interval": "1m" } } PUT /_ilm/policy/filtering_data_tiers { "policy": { "phases": { "frozen": { "min_age": "10m", "actions": { "searchable_snapshot": { "snapshot_repository": "found-snapshots", "force_merge_index": true } } }, "cold": { "min_age": "1m", "actions": { "searchable_snapshot": { "snapshot_repository": "found-snapshots", "force_merge_index": true }, "set_priority": { "priority": 0 } } }, "hot": { "min_age": "0ms", "actions": { "set_priority": { "priority": 100 } } } } } } PUT /_index_template/filtering_data_tiers_template { "index_patterns": [ "filtering_data_tiers*" ], "template": { "settings": { "index.lifecycle.name": "filtering_data_tiers", "index.lifecycle.rollover_alias": "test-filtering_data_tiers" }, "mappings": { "_meta": { "version": "1.6.0" }, "properties": { "@timestamp": { "type": "date" }, "host": { "properties": { "name": { "type": "keyword", "ignore_above": 1024 } } } } } } } PUT /filtering_data_tiers-000001 { "aliases": { "filtering_data_tiers": { "is_write_index": true } } } POST filtering_data_tiers/_doc { "@timestamp": "2024-07-08T17:00:01.000Z", "host.name": "test-0" } ``` </details> (cherry picked from commit c79f0ae)
Summary
securitySolution:excludedDataTiersForRuleExecution
, that allows to exclude cold and frozen data tiers from search during rule executiondata_cold
or/anddata_frozen
tiersUI
Demo
test-frozen
indexrestored-test-frozen-000001
, which confirms alert was created from a document in cold tierdata_cold
tierScreen.Recording.2024-07-04.at.17.30.18.mov
How to test
Create a deployment with cold and frozen data tiers and use following commands to create index and ILM
Data tiers commands
OR
reach out to @vitaliidm to get access to already existing deployment/pR deployment, where
test-frozen
index has cold and frozen nodes and ILM policy that move any data to a tier according to config.Check number of documents in tier by
Create rule of supported type and query that index
Checklist
Functional changes are covered with a test plan and automated tests.
Comprehensive manual testing is done by two engineers: the PR author and one of the PR reviewers. Changes are tested in both ESS and Serverless.
Functional changes are communicated to the Docs team. A ticket or PR is opened in https://github.com/elastic/security-docs. The following information is included: any feature flags used, affected environments (Serverless, ESS, or both).