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

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

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Jun 17, 2024

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

@lukasolson lukasolson added Feature:Search Querying infrastructure in Kibana Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Jun 17, 2024
@lukasolson lukasolson self-assigned this Jun 17, 2024
@lukasolson lukasolson requested review from a team as code owners June 17, 2024 16:44
@elasticmachine
Copy link
Contributor

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

@lukasolson lukasolson requested a review from mattkime June 17, 2024 17:37
Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

Changes lgtm, the additional unit tests are a nice touch. I'm relying on @elastic/security-detection-engine to verify the functionality of their tests.

We should also add a functional test for a lucene query in discover but that's best done as a follow up.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 511 512 +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 420.0KB 420.2KB +129.0B
Unknown metric groups

API count

id before after diff
data 3194 3199 +5

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

cc @lukasolson

Comment on lines +23 to +34
let fields = dataView.timeFieldName ? [dataView.timeFieldName] : [];
if (sort) {
const sortArr = Array.isArray(sort) ? sort : [sort];
fields.push(...sortArr.flatMap((s) => Object.keys(s)));
}
for (const query of request.query) {
if (query.query && query.language === 'kuery') {
const nodes = fromKueryExpression(query.query);
const queryFields = getKqlFieldNames(nodes);
fields = fields.concat(queryFields);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if the let usage is only due to the concat call at the end, maybe that can be replaced with push(...queryFields) and make this a const

Suggested change
let fields = dataView.timeFieldName ? [dataView.timeFieldName] : [];
if (sort) {
const sortArr = Array.isArray(sort) ? sort : [sort];
fields.push(...sortArr.flatMap((s) => Object.keys(s)));
}
for (const query of request.query) {
if (query.query && query.language === 'kuery') {
const nodes = fromKueryExpression(query.query);
const queryFields = getKqlFieldNames(nodes);
fields = fields.concat(queryFields);
}
}
const fields = dataView.timeFieldName ? [dataView.timeFieldName] : [];
if (sort) {
const sortArr = Array.isArray(sort) ? sort : [sort];
fields.push(...sortArr.flatMap((s) => Object.keys(s)));
}
for (const query of request.query) {
if (query.query && query.language === 'kuery') {
const nodes = fromKueryExpression(query.query);
const queryFields = getKqlFieldNames(nodes);
fields.push(...queryFields);
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Although this nit makes sense, I prefer to leave this PR as close to its original counterpart (#184890) as possible

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Left a minor code review, not a blocker for the PR.
Approve.

Copy link
Contributor

@dhurley14 dhurley14 left a comment

Choose a reason for hiding this comment

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

Tested locally with one of the original offending pre-built rules Cobalt Strike Command and Control Beacon and that generated an alert. This LGTM! Glad the fix was a simple one-liner.

@lukasolson lukasolson merged commit 0dd4f9b into elastic:main Jun 18, 2024
39 checks passed
@kibanamachine kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants