Siem query rule - reduce field_caps usage #184890

mattkime · 2024-06-06T04:13:19Z

Summary

Previously, the siem query rule loaded the full set of fields for an index pattern when running a query. This could load 5k fields or more. Now it only loads the fields necessary for the query.

Changes as part of this PR

The data plugin exports queryToFields which takes a query and returns a list of the fields required to translate the query to ES DSL.
queryToFields properly handles all filter types, previously expected unified search bar provided filters.
createSecurityRuleTypeWrapper has been modified to skip field loading for the siem query rule
getFilter takes an optional loadFields arguments which loads only necessary fields
getQueryFilterLoadFields was created - based on getQueryFilter but also loads necessary fields

mattkime · 2024-06-06T15:38:57Z

src/plugins/data/common/search/search_source/query_to_fields.ts

+  if (filters) {
+    const filtersArr = Array.isArray(filters) ? filters : [filters];
+    for (const f of filtersArr) {
+      fields = fields.concat(f?.meta?.key);


fields = fields.concat(f?.meta?.key);

The optional chaining worries me. It was needed to pass the functional test. I'd rather figure out where the filter is being composed and determine whether this is being handled correctly.

x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/query/create_query_alert_type.test.ts produces [ 'user.name', 'user.name' ] at this line of code

There are a couple of use cases here to be aware of:

Filters coming from the unified search bar
a. "Regular" filters where a field is specified (where meta.key is present)
b. "Custom" filters ("Edit as query DSL", where meta.key is not present but meta is)

Any other Elasticsearch query DSL filter that gets passed in by consumers (not coming from unified search, and these probably won't have a meta key at all)

We should probably also update this logic to filter out filters from unified search bar that are disabled (meta.disabled === true) since those aren't needed in the call to field_caps.

Any other Elasticsearch query DSL filter that gets passed in by consumers (not coming from unified search, and these probably won't have a meta key at all)

Based on this it sounds like the change is necessary.

Additionally, we should only be appending if meta.key exists. And, for completeness, if meta.disabled !== true

…o siem_field_caps_diet

mattkime · 2024-06-06T20:31:50Z

/ci

elasticmachine · 2024-06-06T20:32:09Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

lukasolson

Search changes LGTM, I did a little testing and things seem to function properly but I will lean on the security solution team to do the heavy lifting of the actual testing here.

src/plugins/data/common/search/search_source/query_to_fields.ts

marshallmain

Thanks for this improvement Matt! Looks fine overall. I tested a custom query rule manually to verify that wildcards in field names are still processed. I also verified that we have integration tests for wildcards in field names for custom query rules that still pass.

...ecurity_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts

marshallmain · 2024-06-07T17:12:14Z

x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/utils/get_filter.ts

@@ -59,7 +61,11 @@ export const getFilter = async ({
  query,
  exceptionFilter,
  fields = [],


A note for follow up from security: we should split this function into 3 functions, one that loads fields automatically, one that requires fields to be passed in, and one that does not handle fields at all. Then we should explicitly call the version we want for each use case instead of trying to ensure that the parameters are passed in as expected.

Hey @marshallmain - can you open a ticket to track this a tech debt?

mattkime · 2024-06-07T20:17:38Z

/ci

kibana-ci · 2024-06-07T21:43:52Z

💚 Build Succeeded

Buildkite Build
Commit: 4910dfc

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`data`	497	498	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`data`	2585	2590	+5

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`data`	421.5KB	421.6KB	+129.0B

Unknown metric groups

API count

id	before	after	diff
`data`	3194	3199	+5

History

💚 Build #214574 succeeded 5d783e9
💔 Build #214563 failed 947dc6e
💔 Build #214528 failed 8a55ba3
💔 Build #214395 failed 8e21707

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

cc @mattkime

## Summary Previously, the siem query rule loaded the full set of fields for an index pattern when running a query. This could load 5k fields or more. Now it only loads the fields necessary for the query. Changes as part of this PR - The data plugin exports `queryToFields` which takes a query and returns a list of the fields required to translate the query to ES DSL. - `queryToFields` properly handles all filter types, previously expected unified search bar provided filters. - `createSecurityRuleTypeWrapper` has been modified to skip field loading for the siem query rule - `getFilter` takes an optional `loadFields` arguments which loads only necessary fields - `getQueryFilterLoadFields` was created - based on `getQueryFilter` but also loads necessary fields

This reverts commit 257ef7f.

This reverts commit 257ef7f. ## Summary Summarize your PR. If it involves visual changes include a screenshot or gif. ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] 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)

Reverted 257ef7f from deploy@1718000036 as part of emergency release. This PR is following the emergency release guidelines to: `In a separate PR, the fix should be "frontported" to main by manually cherry-picking the commit.` Deployed PR: #186181

## Summary #184890 was reverted in #186196 because it contained a bug with alerts created using Lucene queries. The bug was fixed in #186217. This PR un-reverts the original changes and preserves the fix. It also adds unit tests to cover the failed cases. ### Checklist - [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

limiting changes to query rule

8e21707

mattkime self-assigned this Jun 6, 2024

mattkime added Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Search Querying infrastructure in Kibana labels Jun 6, 2024

mattkime changed the title ~~limiting changes to query rule~~ Siem query rule - reduce field_caps usage Jun 6, 2024

mattkime added 2 commits June 6, 2024 09:22

remove some comments, redundant code

e35bfa1

improve mocks

8a55ba3

mattkime commented Jun 6, 2024

View reviewed changes

mattkime added 4 commits June 6, 2024 13:53

Merge branch 'main' into siem_field_caps_diet

67acf8c

improved filter handling when building field list to load

838c4c0

Merge branch 'siem_field_caps_diet' of github.com:mattkime/kibana int…

947dc6e

…o siem_field_caps_diet

remove comment

5d783e9

mattkime marked this pull request as ready for review June 6, 2024 20:32

mattkime requested review from a team as code owners June 6, 2024 20:32

mattkime requested a review from dhurley14 June 6, 2024 20:32

lukasolson approved these changes Jun 6, 2024

View reviewed changes

mattkime added the release_note:enhancement label Jun 7, 2024

dej611 reviewed Jun 7, 2024

View reviewed changes

src/plugins/data/common/search/search_source/query_to_fields.ts Outdated Show resolved Hide resolved

src/plugins/data/common/search/search_source/query_to_fields.ts Outdated Show resolved Hide resolved

marshallmain approved these changes Jun 7, 2024

View reviewed changes

dhurley14 removed their request for review June 7, 2024 18:17

mattkime added 2 commits June 7, 2024 15:16

minor cleanup

9fd6711

Merge branch 'main' into siem_field_caps_diet

4910dfc

mattkime merged commit 257ef7f into elastic:main Jun 7, 2024
37 checks passed

kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels Jun 7, 2024

marshallmain mentioned this pull request Jun 10, 2024

[Security Solution][Detection Engine] Refactor getFilter into separate functions #185890

Open

dhurley14 added a commit to dhurley14/kibana that referenced this pull request Jun 13, 2024

Revert "Siem query rule - reduce field_caps usage (elastic#184890)"

e2ce233

This reverts commit 257ef7f.

lukasolson mentioned this pull request Jun 17, 2024

Un-revert "Siem query rule - reduce field_caps usage" #186317

Merged

1 task

mattkime mentioned this pull request Jul 24, 2024

[Security Solution][Detection Engine] Detection rules should use dataViewLazy to reduce field caps data transfer #189113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Siem query rule - reduce field_caps usage #184890

Siem query rule - reduce field_caps usage #184890

mattkime commented Jun 6, 2024 •

edited

Loading

mattkime Jun 6, 2024 •

edited

Loading

lukasolson Jun 6, 2024

mattkime Jun 6, 2024

mattkime commented Jun 6, 2024

elasticmachine commented Jun 6, 2024

lukasolson left a comment

marshallmain left a comment

marshallmain Jun 7, 2024

yctercero Jun 10, 2024

marshallmain Jun 10, 2024

mattkime commented Jun 7, 2024

kibana-ci commented Jun 7, 2024

API count

Siem query rule - reduce field_caps usage #184890

Siem query rule - reduce field_caps usage #184890

Conversation

mattkime commented Jun 6, 2024 • edited Loading

Summary

mattkime Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

lukasolson Jun 6, 2024

Choose a reason for hiding this comment

mattkime Jun 6, 2024

Choose a reason for hiding this comment

mattkime commented Jun 6, 2024

elasticmachine commented Jun 6, 2024

lukasolson left a comment

Choose a reason for hiding this comment

marshallmain left a comment

Choose a reason for hiding this comment

marshallmain Jun 7, 2024

Choose a reason for hiding this comment

yctercero Jun 10, 2024

Choose a reason for hiding this comment

marshallmain Jun 10, 2024

Choose a reason for hiding this comment

mattkime commented Jun 7, 2024

kibana-ci commented Jun 7, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Page load bundle

API count

History

mattkime commented Jun 6, 2024 •

edited

Loading

mattkime Jun 6, 2024 •

edited

Loading