[Search] Fixes EQL search strategy #83064

rylnd · 2020-11-10T14:55:46Z

Summary

#82900
Between search types being too permissive and a lack of test coverage, a recent refactor altered the EQL search strategy responses such that some features in Security Solution were broken. This PR fixes those issues, and adds unit & functional tests to prevent regressions in this area. I also added some integration tests around EQL signal generation to generally bump coverage there as well.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

The shared search utilities expect that response data exists in the response's body field. However, in an EQL response this information also exists as a sibling to the body field, and so we must normalize this data into the body before we can leverage these utilities with EQL queries.

These were previously needed to work around an index resolution but, but this has since been resolved upstream in elasticsearch via elastic/elasticsearch#63573.

Previously, custom preview histograms were passing a data-test-subj prop to our general histogram, but the general histogram did not know/care about this prop and it did not become a data property on the underlying DOM element. While most of our tests leveraged enzyme, they could still query by this react prop and everything worked as expected. However, now that we want to exercise this behavior in cypress, we need something to propagate to the DOM so that we can determine which histogram has rendered, so the prop has been updated to be `dataTestSubj`, which then becomes a data-test-subj on the histogram's panel. Tests have been updated accordingly.

* Asserts that the preview displays a histogram * Asserts that no error toast is displayed

elasticmachine · 2020-11-10T17:02:41Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

rylnd · 2020-11-10T17:22:38Z

@elasticmachine merge upstream

rylnd · 2020-11-10T19:37:17Z

x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts

+  cy.get(EQL_QUERY_VALIDATION_SPINNER).should('not.exist');
+  cy.get(QUERY_PREVIEW_BUTTON).should('not.be.disabled').click({ force: true });
+  cy.get(EQL_QUERY_PREVIEW_HISTOGRAM).should('contain.text', 'Hits');
+  cy.get(NOTIFICATION_TOASTS).children().should('not.have.class', TOAST_ERROR_CLASS); // asserts no error toast on page


@MadameSheema I wanted to call out this addition as it might be a nice sanity check throughout the suite

rylnd · 2020-11-10T19:39:29Z

...security_solution/public/detections/components/rules/query_preview/custom_histogram.test.tsx

@@ -51,7 +51,7 @@ describe('PreviewCustomQueryHistogram', () => {

    expect(wrapper.find('[data-test-subj="queryPreviewLoading"]').exists()).toBeTruthy();
    expect(
-      wrapper.find('[data-test-subj="queryPreviewCustomHistogram"]').at(0).prop('subtitle')
+      wrapper.find('[dataTestSubj="queryPreviewCustomHistogram"]').at(0).prop('subtitle')


@yctercero I tried to make as few changes to these tests as possible to maintain behavior while allowing a custom data-test-subj on each histogram; see 471595f for details

Looks great!

rylnd · 2020-11-10T19:43:36Z

x-pack/plugins/data_enhanced/server/search/eql_search_strategy.ts

@@ -64,7 +67,7 @@ export const eqlSearchStrategyProvider = (
        (response) => response.body.id,
        request.id,
        options
-      ).pipe(utils.toKibanaSearchResponse());
+      ).pipe(normalizeEqlResponse(), utils.toKibanaSearchResponse());


This is the crux of the fix; details are in 9e5abf4

rylnd · 2020-11-12T19:25:29Z

@elasticmachine merge upstream

FrankHassanabad · 2020-11-12T21:36:04Z

x-pack/plugins/security_solution/cypress/screens/shared.ts

+
+export const NOTIFICATION_TOASTS = '[data-test-subj="globalToastList"]';
+
+export const TOAST_ERROR_CLASS = 'euiToast--danger';


Always a bummer when we have to use a class for part of a selector but I get when we have to do it.

Yeah, this was the best identifier I could find for now. I'll make a note to add a data-test-subj upstream in notifications next time I come through here 👍

FrankHassanabad · 2020-11-12T21:42:02Z

x-pack/plugins/security_solution/public/common/hooks/eql/use_eql_preview.ts

@@ -74,8 +74,6 @@ export const useEqlPreview = (): [
          .search<EqlSearchStrategyRequest, EqlSearchStrategyResponse<EqlSearchResponse<Source>>>(
            {
              params: {
-                // @ts-expect-error allow_no_indices is missing on EqlSearch
-                allow_no_indices: true,


FrankHassanabad

Reviewed code, everything looks great from my end 👍

yctercero

This looks great! Pulled down and tested. The endpoint is intermittently giving a 404, but confirmed that it is due to an unrelated and known issue.

👍

rylnd · 2020-11-13T01:25:38Z

@elasticmachine merge upstream

lukasolson

LGTM

lukasolson · 2020-11-16T14:49:27Z

Just an FYI, we have another refactoring happening here: #82545. I'll update that PR with these changes, and thanks for adding the tests so we can be sure not to break this in the future!

rylnd · 2020-11-16T16:21:54Z

@elasticmachine merge upstream

apmmachine · 2020-11-16T16:28:36Z

💚 Build Succeeded

pipeline
Commit: 34d0d7c

These were updated on an upstream refactor.

kibanamachine · 2020-11-16T19:20:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: e5de2f0

Metrics [docs]

Async chunks

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

id	before	after	diff
`securitySolution`	7.9MB	7.9MB	-141.0B

Distributable file count

id	before	after	diff
`default`	42835	42836	+1

Page load bundle

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

id	before	after	diff
`dataEnhanced`	34.7KB	36.1KB	+1.4KB

History

💔 Build #88050 failed 34d0d7c
💚 Build #87806 succeeded aba3b21
💚 Build #87712 succeeded 9b2b6e5
💚 Build #87156 succeeded 6bb0f63
💛 Build #87092 was flaky fde464e

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

* Ensure that data is not lost when parsing EQL responses The shared search utilities expect that response data exists in the response's body field. However, in an EQL response this information also exists as a sibling to the body field, and so we must normalize this data into the body before we can leverage these utilities with EQL queries. * Remove unused EQL parameters These were previously needed to work around an index resolution but, but this has since been resolved upstream in elasticsearch via elastic/elasticsearch#63573. * Allow custom test subj for Preview Histogram to propagate to DOM Previously, custom preview histograms were passing a data-test-subj prop to our general histogram, but the general histogram did not know/care about this prop and it did not become a data property on the underlying DOM element. While most of our tests leveraged enzyme, they could still query by this react prop and everything worked as expected. However, now that we want to exercise this behavior in cypress, we need something to propagate to the DOM so that we can determine which histogram has rendered, so the prop has been updated to be `dataTestSubj`, which then becomes a data-test-subj on the histogram's panel. Tests have been updated accordingly. * Exercise Query Preview during EQL rule creation * Asserts that the preview displays a histogram * Asserts that no error toast is displayed * Add integration tests around EQL sequence signal generation * Clearer assertion * Simplify test assertion * Fix typings These were updated on an upstream refactor. Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts

rylnd added 6 commits November 9, 2020 16:47

Remove unused EQL parameters

3aa44db

These were previously needed to work around an index resolution but, but this has since been resolved upstream in elasticsearch via elastic/elasticsearch#63573.

Exercise Query Preview during EQL rule creation

447aaf3

* Asserts that the preview displays a histogram * Asserts that no error toast is displayed

Add integration tests around EQL sequence signal generation

1b4188f

Clearer assertion

fde464e

rylnd added v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.11.0 Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Nov 10, 2020

rylnd self-assigned this Nov 10, 2020

rylnd marked this pull request as ready for review November 10, 2020 17:02

rylnd requested review from a team as code owners November 10, 2020 17:02

kibanamachine and others added 2 commits November 10, 2020 12:22

Merge branch 'master' into fix_eql_strategy

52e67a3

Simplify test assertion

6bb0f63

rylnd commented Nov 10, 2020

View reviewed changes

Merge branch 'master' into fix_eql_strategy

9b2b6e5

FrankHassanabad reviewed Nov 12, 2020

View reviewed changes

FrankHassanabad approved these changes Nov 12, 2020

View reviewed changes

yctercero approved these changes Nov 12, 2020

View reviewed changes

Merge branch 'master' into fix_eql_strategy

aba3b21

lukasolson approved these changes Nov 16, 2020

View reviewed changes

Merge branch 'master' into fix_eql_strategy

34d0d7c

Fix typings

e5de2f0

These were updated on an upstream refactor.

rylnd merged commit c6e984d into elastic:master Nov 16, 2020

rylnd deleted the fix_eql_strategy branch November 16, 2020 21:05

rylnd mentioned this pull request Nov 16, 2020

[7.x] [Search] Fixes EQL search strategy (#83064) #83502

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search] Fixes EQL search strategy #83064

[Search] Fixes EQL search strategy #83064

rylnd commented Nov 10, 2020

elasticmachine commented Nov 10, 2020

rylnd commented Nov 10, 2020

rylnd Nov 10, 2020

rylnd Nov 10, 2020

yctercero Nov 12, 2020

rylnd Nov 10, 2020

rylnd commented Nov 12, 2020

FrankHassanabad Nov 12, 2020

rylnd Nov 12, 2020

FrankHassanabad Nov 12, 2020

FrankHassanabad left a comment

yctercero left a comment

rylnd commented Nov 13, 2020

lukasolson left a comment

lukasolson commented Nov 16, 2020

rylnd commented Nov 16, 2020

apmmachine commented Nov 16, 2020

kibanamachine commented Nov 16, 2020


		export const NOTIFICATION_TOASTS = '[data-test-subj="globalToastList"]';

		export const TOAST_ERROR_CLASS = 'euiToast--danger';

[Search] Fixes EQL search strategy #83064

[Search] Fixes EQL search strategy #83064

Conversation

rylnd commented Nov 10, 2020

Summary

Checklist

For maintainers

elasticmachine commented Nov 10, 2020

rylnd commented Nov 10, 2020

rylnd Nov 10, 2020

Choose a reason for hiding this comment

rylnd Nov 10, 2020

Choose a reason for hiding this comment

yctercero Nov 12, 2020

Choose a reason for hiding this comment

rylnd Nov 10, 2020

Choose a reason for hiding this comment

rylnd commented Nov 12, 2020

FrankHassanabad Nov 12, 2020

Choose a reason for hiding this comment

rylnd Nov 12, 2020

Choose a reason for hiding this comment

FrankHassanabad Nov 12, 2020

Choose a reason for hiding this comment

FrankHassanabad left a comment

Choose a reason for hiding this comment

yctercero left a comment

Choose a reason for hiding this comment

rylnd commented Nov 13, 2020

lukasolson left a comment

Choose a reason for hiding this comment

lukasolson commented Nov 16, 2020

rylnd commented Nov 16, 2020

apmmachine commented Nov 16, 2020

💚 Build Succeeded

kibanamachine commented Nov 16, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

Page load bundle

History