[Security Solution][Detections] EQL Validation #77493
Merged
+1,075
−29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rylnd
force-pushed
the
eql_validation
branch
from
4c0af4a to
261de35
Compare
This is mostly boilerplate with some rough parameter definitions; the actual implementation of the validation is going to live in our validateEql function. A few tests are failing as the mocks haven't yet been implemented, I need to see the shape of the responses first.
* Performs an EQL search * filters out non-parsing errors, and returns what remains in the response * Adds mocks for empty EQL responses (we don't yet have a need for mocked data, but we will when we unit-test validateEql)
* Adds EQL Validation response schema,mocks,tests * Adds frontend function to call our validation endpoint * Adds hook, useEqlValidation, to call the above function and return state * Adds labels/help text for EQL Query bar * EqlQueryBar consumes useEqlValidation and marks the field as invalid, but does not yet report errors.
This causes a broader error that results in a 400 response; we can (and do) handle the case of a blank query in the form itself.
It doesn't add any information for the user, and it currently looks bad when combined with validation errors.
* Fixes issue where old errors were persisted after the user had made modifications
These include errors related to index fields and mappings.
We're concerned with validation errors; the source of those errors is an implementation detail of these functions.
This more closely resembles the new Eui Markdown editor, which places errors and doc links in a footer.
rylnd
force-pushed
the
eql_validation
branch
from
261de35 to
d3a6c75
Compare
These were broken by our use of useAppToasts and the EUI theme.
Decode doesn't do any additional processing, so we can use t.TypeOf here (the default for buildRouteValidation).
Without `ignore: [400]` the ES client will log errors and then throw them. We can catch the error, but the log is undesirable. This updates the query to use the ignore parameter, along with updating the validation logic to work with the updated response. Adds some mocks and tests around these responses and helpers, since these will exist independent of the validation implementation.
These include errors for inaccessible indexes, which should be useful to the rule writer in writing their EQL query.
rylnd
commented
Sep 22, 2020
| @@ -64,6 +65,12 @@ export interface TotalValue { | |||
| relation: string; | |||
| } | |||
|
|
|||
| export interface BaseHit<T> { | |||
There was a problem hiding this comment.
These types are cribbed from #77419, FYI. It doesn't look like they'll conflict but we'll see 🤞
Since EQL rules actually validate against the relevant indexes, changing said indexes should invalidate/revalidate the query. With the form lib, this is beautifully simple :)
Index corresponds to the value from the index field; now that our EQL validation is performed by the form we have no need for it here.
|
@spong @peluja1012 I've updated the form such that:
RE the ability to submit while validations are pending: this is still possible on this branch, but after talking with @sebelga this is a general bug with the form lib that should be addressed in #78588. I haven't yet verified those fixes, but I will do so tomorrow. I think another round of 👀 is warranted (and appreciated!), please let me know if this baby's good to 🚢 ! |
sebelga
reviewed
Oct 1, 2020
| * | ||
| * @returns A debounced async function that resolves on the next invocation | ||
| */ | ||
| export const debounceAsync = <Args extends unknown[], Result extends Promise<unknown>>( |
spong
reviewed
Oct 1, 2020
...ins/security_solution/public/detections/components/rules/eql_query_bar/eql_overview_link.tsx
Outdated
Show resolved
Hide resolved
Adds an entry in our doclinks service, and uses that.
Sounds good to me @rylnd -- thanks for the added research here! 🙂 Depending on how things test during FF with the 300ms debounce we may have to revert to validating onBlur, but I'm good moving forward with these changes. We have a fall-through for submitting while validating (rule will fail to run with appropriate error), so 👍 for waiting for the proper fix from #78588. Will pull this down later today and give it a second test drive so we can 🚢 🚢 🚢 ! Thanks @rylnd! |
peluja1012
approved these changes
Oct 1, 2020
There was a problem hiding this comment.
Pulled down the branch and played around with EQL validation. Debounced validation works great, @rylnd. Thanks!
With our new async validation, a user can quickly navigate away from the Definition tab before the validation has completed, resulting in the form being invalidated. Any subsequent user actions cause the form to correct itself, but until I can find a better solution here this really just gives the validation time to complete and sidesteps the issue.
Conflicts: x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/schema.tsx x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/translations.tsx
pgayvallet
approved these changes
Oct 2, 2020
💛 Build succeeded, but was flaky
Test FailuresChrome UI Functional Tests.test/functional/apps/discover/_field_data·js.discover app discover tab field data search php should show the correct hit countStandard OutStack TraceMetrics [docs]@kbn/optimizer bundle module count
async chunks size
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
Oct 2, 2020
* WIP: Adding new route for EQL Validation This is mostly boilerplate with some rough parameter definitions; the actual implementation of the validation is going to live in our validateEql function. A few tests are failing as the mocks haven't yet been implemented, I need to see the shape of the responses first. * Cherry-pick Marshall's EQL types * Implements actual EQL validation * Performs an EQL search * filters out non-parsing errors, and returns what remains in the response * Adds mocks for empty EQL responses (we don't yet have a need for mocked data, but we will when we unit-test validateEql) * Adds validation calls to the EQL form input * Adds EQL Validation response schema,mocks,tests * Adds frontend function to call our validation endpoint * Adds hook, useEqlValidation, to call the above function and return state * Adds labels/help text for EQL Query bar * EqlQueryBar consumes useEqlValidation and marks the field as invalid, but does not yet report errors. * Do not call the validation API if query is not present This causes a broader error that results in a 400 response; we can (and do) handle the case of a blank query in the form itself. * Remove EQL Help Text It doesn't add any information for the user, and it currently looks bad when combined with validation errors. * Flesh out and use our popover for displaying validation errors * Fixes issue where old errors were persisted after the user had made modifications * Include verification_exception errors as validation errors These include errors related to index fields and mappings. * Generalize our validation helpers We're concerned with validation errors; the source of those errors is an implementation detail of these functions. * Move error popover and EQL reference link to footer This more closely resembles the new Eui Markdown editor, which places errors and doc links in a footer. * Fix jest tests following additional prop * Add icon for EQL Rule card * Fixes existing EqlQueryBar tests These were broken by our use of useAppToasts and the EUI theme. * Add unit tests around error rendering on EQL Query Bar * Add tests for ErrorPopover * Remove unused schema type Decode doesn't do any additional processing, so we can use t.TypeOf here (the default for buildRouteValidation). * Remove duplicated header * Use ignore parameter to prevent EQL validations from logging errors Without `ignore: [400]` the ES client will log errors and then throw them. We can catch the error, but the log is undesirable. This updates the query to use the ignore parameter, along with updating the validation logic to work with the updated response. Adds some mocks and tests around these responses and helpers, since these will exist independent of the validation implementation. * Include mapping_exceptions during EQL query validation These include errors for inaccessible indexes, which should be useful to the rule writer in writing their EQL query. * Display toast messages for non-validation messages * fix type errors This type was renamed. * Do not request data in our validation request By not having the cluster retrieve/send any data, this should saves us a few CPU cycles. * Move EQL validation to an async form validator Rather than invoking a custom validation hook (useEqlValidation) at custom times (onBlur) in our EqlQueryBar component, we can instead move this functionality to a form validation function and have it be invoked automatically by our form when values change. However, because we still need to handle the validation messages slightly differently (place them in a popover as opposed to an EuiFormRow), we also need custom error retrieval in the form of getValidationResults. After much pain, it was determined that the default behavior of _.debounce does not work with async validator functions, as a debounced call will not "wait" for the eventual invocation but will instead return the most recently resolved value. This leads to stale validation results and terrible UX, so I wrote a custom function (debounceAsync) that behaves like we want/need; see tests for details. * Invalidate our query field when index patterns change Since EQL rules actually validate against the relevant indexes, changing said indexes should invalidate/revalidate the query. With the form lib, this is beautifully simple :) * Set a min-height on our EQL textarea * Remove unused prop from EqlQueryBar Index corresponds to the value from the index field; now that our EQL validation is performed by the form we have no need for it here. * Update EQL overview link to point to elasticsearch docs Adds an entry in our doclinks service, and uses that. * Remove unused prop from stale tests * Update docLinks documentation with new EQL link * Fix bug where saved query rules had no type selected on Edit * Wait for kibana requests to complete before moving between rule tabs With our new async validation, a user can quickly navigate away from the Definition tab before the validation has completed, resulting in the form being invalidated. Any subsequent user actions cause the form to correct itself, but until I can find a better solution here this really just gives the validation time to complete and sidesteps the issue.
rylnd
added a commit
that referenced
this pull request
Oct 2, 2020
* WIP: Adding new route for EQL Validation This is mostly boilerplate with some rough parameter definitions; the actual implementation of the validation is going to live in our validateEql function. A few tests are failing as the mocks haven't yet been implemented, I need to see the shape of the responses first. * Cherry-pick Marshall's EQL types * Implements actual EQL validation * Performs an EQL search * filters out non-parsing errors, and returns what remains in the response * Adds mocks for empty EQL responses (we don't yet have a need for mocked data, but we will when we unit-test validateEql) * Adds validation calls to the EQL form input * Adds EQL Validation response schema,mocks,tests * Adds frontend function to call our validation endpoint * Adds hook, useEqlValidation, to call the above function and return state * Adds labels/help text for EQL Query bar * EqlQueryBar consumes useEqlValidation and marks the field as invalid, but does not yet report errors. * Do not call the validation API if query is not present This causes a broader error that results in a 400 response; we can (and do) handle the case of a blank query in the form itself. * Remove EQL Help Text It doesn't add any information for the user, and it currently looks bad when combined with validation errors. * Flesh out and use our popover for displaying validation errors * Fixes issue where old errors were persisted after the user had made modifications * Include verification_exception errors as validation errors These include errors related to index fields and mappings. * Generalize our validation helpers We're concerned with validation errors; the source of those errors is an implementation detail of these functions. * Move error popover and EQL reference link to footer This more closely resembles the new Eui Markdown editor, which places errors and doc links in a footer. * Fix jest tests following additional prop * Add icon for EQL Rule card * Fixes existing EqlQueryBar tests These were broken by our use of useAppToasts and the EUI theme. * Add unit tests around error rendering on EQL Query Bar * Add tests for ErrorPopover * Remove unused schema type Decode doesn't do any additional processing, so we can use t.TypeOf here (the default for buildRouteValidation). * Remove duplicated header * Use ignore parameter to prevent EQL validations from logging errors Without `ignore: [400]` the ES client will log errors and then throw them. We can catch the error, but the log is undesirable. This updates the query to use the ignore parameter, along with updating the validation logic to work with the updated response. Adds some mocks and tests around these responses and helpers, since these will exist independent of the validation implementation. * Include mapping_exceptions during EQL query validation These include errors for inaccessible indexes, which should be useful to the rule writer in writing their EQL query. * Display toast messages for non-validation messages * fix type errors This type was renamed. * Do not request data in our validation request By not having the cluster retrieve/send any data, this should saves us a few CPU cycles. * Move EQL validation to an async form validator Rather than invoking a custom validation hook (useEqlValidation) at custom times (onBlur) in our EqlQueryBar component, we can instead move this functionality to a form validation function and have it be invoked automatically by our form when values change. However, because we still need to handle the validation messages slightly differently (place them in a popover as opposed to an EuiFormRow), we also need custom error retrieval in the form of getValidationResults. After much pain, it was determined that the default behavior of _.debounce does not work with async validator functions, as a debounced call will not "wait" for the eventual invocation but will instead return the most recently resolved value. This leads to stale validation results and terrible UX, so I wrote a custom function (debounceAsync) that behaves like we want/need; see tests for details. * Invalidate our query field when index patterns change Since EQL rules actually validate against the relevant indexes, changing said indexes should invalidate/revalidate the query. With the form lib, this is beautifully simple :) * Set a min-height on our EQL textarea * Remove unused prop from EqlQueryBar Index corresponds to the value from the index field; now that our EQL validation is performed by the form we have no need for it here. * Update EQL overview link to point to elasticsearch docs Adds an entry in our doclinks service, and uses that. * Remove unused prop from stale tests * Update docLinks documentation with new EQL link * Fix bug where saved query rules had no type selected on Edit * Wait for kibana requests to complete before moving between rule tabs With our new async validation, a user can quickly navigate away from the Definition tab before the validation has completed, resulting in the form being invalidated. Any subsequent user actions cause the form to correct itself, but until I can find a better solution here this really just gives the validation time to complete and sidesteps the issue.
This was referenced Oct 6, 2020
MindyRS
added
the
Team: SecuritySolution
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
Oct 18, 2023
Previously (since elastic#77493), we had been leveraging the following facts: * EQL search endpoint returns syntax errors as a 400 response * ES client can specify an `ignore` option to opt out of the "throw unsuccessful responses" behavior in order to perform EQL "validation" in a normal, non-exception control flow. This commit effectively replaces that with a try/catch, where we no longer pass (nor _can_ pass) `ignore` to the search strategy, and instead indicate we'd like the "validation" behavior via a new `validate` param. When set to true, we will capture the 400 response and, if valid, return it as before; in all other situations that syntax error will result in an error response from the search strategy.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This adds two main pieces for EQL Rules:
While the endpoint may soon be replaced by an EQL search strategy (discussion ongoing), the UI and its behaviors should not change, so I'm hoping to get this merged in the interest of earlier feedback from product.
Outstanding Issues:
mapping_exceptionerrors (as would be caused by inaccessible indexes) as validation errors. While this information is useful to the rule writer, it would be more appropriate to move this validation upstream to the index patterns combobox. Once [Security Solution] Options to select index patterns #77192 is merged, we should have the data necessary to perform that validation on index patterns.Empty input
Input with Errors
Input with Errors Expanded
Example validation errors
Checklist
For maintainers