Skip to content

[Security Solution] Use AST utils from @kbn/esql-ast for ES|QL rule type query parsing (#9282)#189780

Merged
e40pud merged 13 commits intoelastic:mainfrom
e40pud:security/feature/esql-ast-validator
Aug 7, 2024
Merged

[Security Solution] Use AST utils from @kbn/esql-ast for ES|QL rule type query parsing (#9282)#189780
e40pud merged 13 commits intoelastic:mainfrom
e40pud:security/feature/esql-ast-validator

Conversation

@e40pud
Copy link
Copy Markdown
Contributor

@e40pud e40pud commented Aug 2, 2024 •
edited
Loading

Summary

Addresses https://github.com/elastic/security-team/issues/9282

With these changes we utilise AST based utils to do ES|QL query validation. This allows us to recognise and display syntax errors. Syntax errors have higher priority than the rest of the validation errors.

Validation errors priorities from top to bottom:

  1. Syntax error
  2. Missing metadata for non-aggregating queries
  3. Missing data source and/or data fields
  4. Missing _id column requested for non-aggregating queries via metadata operator

These priorities define the sequence in which we display errors to the user. If there are several errors detected, that the one with higher priority will be shown.

Screen.Recording.2024-08-02.at.11.02.16.mov

Checklist

Delete any items that are not applicable to this PR.

@e40pud e40pud added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. release_note:feature Makes this part of the condensed release notes ci:cloud-deploy Create or update a Cloud deployment Team:Detection Engine Security Solution Detection Engine Area ci:project-deploy-security Create a Security Serverless Project labels Aug 2, 2024
@e40pud e40pud requested a review from vitaliidm August 2, 2024 09:15
@e40pud e40pud self-assigned this Aug 2, 2024
@e40pud
Copy link
Copy Markdown
Contributor Author

e40pud commented Aug 2, 2024

/ci

@e40pud
Copy link
Copy Markdown
Contributor Author

e40pud commented Aug 2, 2024

/ci

@e40pud
Copy link
Copy Markdown
Contributor Author

e40pud commented Aug 2, 2024

/ci

@e40pud
Copy link
Copy Markdown
Contributor Author

e40pud commented Aug 5, 2024

/ci

@e40pud
Copy link
Copy Markdown
Contributor Author

e40pud commented Aug 5, 2024

/ci

@e40pud e40pud marked this pull request as ready for review August 5, 2024 12:02
@e40pud e40pud requested review from a team as code owners August 5, 2024 12:02
@e40pud e40pud requested a review from rylnd August 5, 2024 12:02
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 5, 2024

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

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 5, 2024

Pinging @elastic/security-detection-engine (Team:Detection Engine)

@vitaliidm vitaliidm self-requested a review August 5, 2024 13:21
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Aug 5, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6681

[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/detection_engine/rule_execution_logic/trial_license_complete_tier/configs/ess.config.ts: 100/100 tests passed.
[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/detection_engine/rule_execution_logic/trial_license_complete_tier/configs/serverless.config.ts: 100/100 tests passed.

see run history

vitaliidm
vitaliidm reviewed Aug 5, 2024
View reviewed changes
x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.ts Outdated
}

// Check whether the `metadata` operator has `_id` argument
const idColumnItem = (metadataOption as ESQLCommandOption).args.find(
Copy link
Copy Markdown
Contributor

@vitaliidm vitaliidm Aug 5, 2024

Choose a reason for hiding this comment

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

why casting is needed here?
Given that one of the types there does not have args property, it could lead to error of accessing property. Something we had already in the past.

Copy link
Copy Markdown
Contributor Author

@e40pud e40pud Aug 6, 2024

Choose a reason for hiding this comment

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

Seems like Array.find cannot infer the type in this case. I re-arranged the code.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Aug 6, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6682

[❌] Security Solution Detection Engine - Cypress: 52/100 tests passed.
[❌] [Serverless] Security Solution Detection Engine - Cypress: 81/100 tests passed.

see run history

vitaliidm
vitaliidm approved these changes Aug 6, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@vitaliidm vitaliidm left a comment

Choose a reason for hiding this comment

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

LGTM
thanks for you work

@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Aug 7, 2024 •
edited
Loading

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #98 / Cloud Security Posture Findings Page - Alerts Create detection rule Creates a detection rule from the Take Action button and navigates to rule page

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5614 5645 +31

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
@kbn/securitysolution-utils 44 46 +2

Async chunks

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

id before after diff
securitySolution 20.5MB 20.7MB ⚠️ +232.2KB
Unknown metric groups

API count

id before after diff
@kbn/securitysolution-utils 49 51 +2

History

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

cc @e40pud

@e40pud e40pud merged commit d3d5a7c into elastic:main Aug 7, 2024
@kibanamachine kibanamachine added v8.16.0 backport:skip This PR does not require backporting labels Aug 7, 2024
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Aug 8, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6712

[❌] Security Solution Detection Engine - Cypress: 60/100 tests passed.
[❌] [Serverless] Security Solution Detection Engine - Cypress: 78/100 tests passed.

see run history

@vitaliidm vitaliidm mentioned this pull request Aug 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vitaliidm vitaliidm vitaliidm approved these changes

@rylnd rylnd Awaiting requested review from rylnd rylnd was automatically assigned from elastic/security-detection-engine

Assignees

@e40pud e40pud

Labels

backport:skip This PR does not require backporting ci:cloud-deploy Create or update a Cloud deployment ci:project-deploy-security Create a Security Serverless Project release_note:feature Makes this part of the condensed release notes Team:Detection Engine Security Solution Detection Engine Area Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.16.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@e40pud @elasticmachine @kibanamachine @kibana-ci @vitaliidm @yctercero