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

[Security Solution][Detections] Convert EQL validation to use search strategy #79538

Merged
merged 10 commits into from
Oct 7, 2020

Conversation

rylnd
Copy link
Contributor

@rylnd rylnd commented Oct 5, 2020

Summary

  • Removes validation endpoint in the detection engine
  • Moves response parsing helpers into common, as that's now done on the client
  • Converts client function to use search strategy
  • Fixes issue where validation query becomes async

Checklist

For maintainers

These are the same types with a different name. However, the benefit is
that they exist in a non-restricted path (the top level of the plugin).
Rather than calling our custom EQL validation endpoint, we can instead
leverage the EQL search strategy. The downside is that we have to move
our response parsing logic to the frontend, but the benefit is that
there's no backend to maintain.
We're keeping our io-ts schemas for now since they're still being used
to type the I/O of our client function.
I'm not aware of a way to pass react context to the form lib validator
functions, so for now we have to pass this the ugly way :(
We were keeping these around for the types, but they're so simple that
it's really not worth the overhead. The tests are similarly for
functionality that is no longer used, so no hard feelings there.
We only care about the query's validity, so we can tell the response
handler to do less work here.
Without passing transport options to .get, a query with an `ignore`
would succeed if it completed in the `waitForCompletionTimeout` window,
but fail (with the ignored error) on the subsequent request if it became
async.
@rylnd rylnd added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Oct 5, 2020
@rylnd rylnd requested review from a team as code owners October 5, 2020 18:46
@rylnd rylnd self-assigned this Oct 5, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

Common values cannot be consumed directly by client code (compilation
error), so we need to re-export them from data_enhanced's public module.
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

AppArch changes LGTM

Copy link
Contributor

@peluja1012 peluja1012 left a comment

Choose a reason for hiding this comment

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

Thanks for helping us search strategy support for EQL Rules! Code looks good. Pull down the branch and tested locally, validation continues to work as expected :)

@@ -4,8 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import get from 'lodash/get';
import has from 'lodash/has';
import { get, has } from 'lodash';
Copy link
Member

Choose a reason for hiding this comment

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

For posterity, we're going back to named imports from defaults:

#74539 -> #78156

Copy link
Member

@spong spong 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 @rylnd! 🙂

 Conflicts:
	x-pack/plugins/security_solution/public/common/hooks/eql/api.ts
	x-pack/plugins/security_solution/server/lib/detection_engine/routes/__mocks__/request_responses.ts
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
securitySolution 2040 2043 +3

async chunks size

id before after diff
securitySolution 10.5MB 10.5MB +1.4KB

distributable file count

id before after diff
default 48447 48445 -2

page load bundle size

id before after diff
dataEnhanced 34.4KB 34.6KB +228.0B
securitySolution 593.1KB 593.2KB +70.0B
total +298.0B

History

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

@rylnd rylnd merged commit e60cfa0 into elastic:master Oct 7, 2020
@rylnd rylnd deleted the search_strat_validation branch October 7, 2020 01:46
spong pushed a commit that referenced this pull request Oct 7, 2020
…strategy (#79538) (#79806)

* Rename types from the top-level plugin

These are the same types with a different name. However, the benefit is
that they exist in a non-restricted path (the top level of the plugin).

* Convert our validation function to use the EQL search strategy

Rather than calling our custom EQL validation endpoint, we can instead
leverage the EQL search strategy. The downside is that we have to move
our response parsing logic to the frontend, but the benefit is that
there's no backend to maintain.

* Remove server code related to our EQL validation endpoint

We're keeping our io-ts schemas for now since they're still being used
to type the I/O of our client function.

* Add the data contract to our KibanaServices

I'm not aware of a way to pass react context to the form lib validator
functions, so for now we have to pass this the ugly way :(

* Remove io-ts types corresponding to our defunct validation endpoint

We were keeping these around for the types, but they're so simple that
it's really not worth the overhead. The tests are similarly for
functionality that is no longer used, so no hard feelings there.

* Ensure that our validation does not bother generating hits

We only care about the query's validity, so we can tell the response
handler to do less work here.

* Pass transport options when retrieving an existing search

Without passing transport options to .get, a query with an `ignore`
would succeed if it completed in the `waitForCompletionTimeout` window,
but fail (with the ignored error) on the subsequent request if it became
async.

* Use constant for our strategy key

* Export search strategy constants for client consumption

Common values cannot be consumed directly by client code (compilation
error), so we need to re-export them from data_enhanced's public module.
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants