[SIEM][Detection Engine] Backend end-to-end tests #57166

FrankHassanabad · 2020-02-08T09:28:29Z

Summary

Adds end to end integration tests
Fixes a bug with import where on imports it was forcing all rules that were being imported to be set to be "enabled: false" instead of honoring what the original export has set for its enabled.
Adds a few "to be safe" await block so that the front end does not get a race condition within the bulk deletes and other parts of the code.
Fixes statusCode to be status_code and removes most of the Hapi Boomer errors
Changes PUT to be PATCH for partial updates
Adds true updates with PUT
Put some TODO blocks around existing bugs found in the API in the e2e tests that we might have time to get to or might not. This will let others maintaining the tests know that once they fix the bug they should update the end to end test to change the behavior.

Testing this:

Go to the latest CI logs and look for any particular lines from the test executing such as:

should set the response content types to be expected

Also run this manually on your machine through this command:

node scripts/functional_tests --config x-pack/test/detection_engine_api_integration/security_and_spaces/config.ts

Change a test manually and re-run the above command to watch something fail.

Screen shot of what you should see on the CI machine when these are running:

Checklist

Delete any items that are not applicable to this PR.

~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~

~~- [ ] Documentation was added for features that require explanation or tutorials~~

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

~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

~~- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser~~

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-02-08T17:29:00Z

Pinging @elastic/siem (Team:SIEM)

rylnd

Submitting these comments as general feedback. I'll defer to @dhurley14 for approval of the e2e tests as I mainly reviewed the errors changes, and nothing I saw seemed blocking.

rylnd · 2020-02-10T20:04:38Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts

@@ -30,12 +29,6 @@ import { KibanaRequest } from '../../../../../../../../../src/core/server';

 type PromiseFromStreams = ImportRuleAlertRest | Error;

-/*
- * We were getting some error like that possible EventEmitter memory leak detected
- * So we decide to batch the update by 10 to avoid any complication in the node side


Is this no longer relevant to that constant? If not, do we still need the constant?

I think we do as that acts as a throttle for how many to send at a time.

I just took the comments of this out of the file as I did not feel it was appropriate anymore as that EventEmitter issue is a global warning and does not have anything to do with any particular section of code such as this. (I get it at startup now)

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/utils.ts

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts

rylnd · 2020-02-10T20:42:54Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/index/read_index_route.ts

@@ -42,11 +41,22 @@ export const createReadIndexRoute = (server: ServerFacade): Hapi.ServerRoute =>
          if (request.method.toLowerCase() === 'head') {
            return headers.response().code(404);
          } else {
-            return new Boom('index for this space does not exist', { statusCode: 404 });
+            return headers


In NP, this is going to look like:

return response.notFound({ body: 'message' }); // OR return response.notFound({ body: { message: 'message', attributes: { other: 'stuff', }, }, }); // OR return response.customError({ statusCode: 404, body: { message: 'message', attributes: { other: 'stuff', }, }, });

so these are all going to have to change again. I don't think it's worthwhile to put these in a helper or anything, but just an FYI in case it might inform future decisions.

Is any of that available yet for the back-port 7.6.1 or no?

If not, we can change again after this is merged for 7.x and master to move forward and hopefully not get many other bug fixes for 7.6.1

It's available on NP (we're using it for our graphQL stuff), but detections routes are still blocked until we can access the alerting client from NP.

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/delete_rules_bulk_route.ts

x-pack/test/detection_engine_api_integration/common/config.ts

dhurley14 · 2020-02-11T15:39:23Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts

@@ -166,7 +164,7 @@ export const createImportRulesRoute = (server: ServerFacade): Hapi.ServerRoute =
                      alertsClient,
                      actionsClient,
                      description,
-                      enabled: false,
+                      enabled,


We are allowing rules to be enabled on import now? Just an observation.

Yes, I don't know when this bug was introduced.

If you think about it, it would be a bug and not a feature because when I decide to do an import with "overwrite" set to true and then see all of my current running rules suddenly being disabled I would immediately look at my exported file and notice that the ones I wanted to be turned on are actually turned on in the file I am trying to import. I would begin to wonder why the import feature is turning off my rules when I put into the import file that I wanted to have some of them enabled when I do an import.

Perhaps I am writing automation scripts to import rules between spaces using the API (for example) and need the import to do as it is told exactly. Maybe I am changing the rules before importing through python scripts, etc... At that point I need the import to do exactly as it is told to do.

If we want the user to think about which rules they want to be enabled or disabled or to warn them (at some point) that they have exported rules that are enabled it would be on the export feature that we would warn them and ask them if they want to disable all of the rules before exporting them and add a query parameter to the export to support that. Where the rules are not disabled in their environment runtime but rather have enabled: false when exported as a connivence for them.

Then their ndjson file would be a "source of truth" on import and the import would work ok and if they're versioning their saved ndjson files they have confidence they work as they are see in their github repo's.

If they come back to us with this change and tell us they were expecting that imported rules that have enabled: true to actually all be enabled: false on forums, the fix is for them to just open their imports and change them all to be false or to disable the rules before exporting which I think is the correct path forward.

dhurley14

LGTM - Happy to see boom go 💥and the end-to-end tests are awesome. Those will come in handy long-term. New PATCH endpoint looks great too.

spong · 2020-02-11T21:01:50Z

x-pack/legacy/plugins/siem/public/containers/detection_engine/rules/api.ts

@@ -160,7 +160,7 @@ export const deleteRules = async ({ ids }: DeleteRulesProps): Promise<Array<Rule
  const response = await KibanaServices.get().http.fetch<Rule[]>(
    `${DETECTION_ENGINE_RULES_URL}/_bulk_delete`,
    {
-      method: 'PUT',
+      method: 'DELETE',


Looks like I had forgot to update this verb as part of #54521 -- thanks for this cleanup! 🙂

kibanamachine · 2020-02-11T22:20:21Z

💚 Build Succeeded

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

History

💚 Build #25905 succeeded 46449bf
💚 Build #25802 succeeded f58a4f9
💚 Build #25792 succeeded 023e446
💚 Build #25776 succeeded 506c409
💛 Build #25765 was flaky 1dbc830

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

) * [SIEM][Detection Engine] Backend end-to-end tests * Adds end to end integration tests * Fixes a bug with import where on imports it was forcing all rules that were being imported to be set to be "enabled: false" instead of honoring what the original export has set for its enabled. * Adds a few "to be safe" await block so that the front end does not get a race condition within the bulk deletes and other parts of the code. * Fixes `statusCode` to be `status_code` and removes most of the Hapi Boomer errors * Changes PUT to be PATCH for partial updates * Adds true updates with PUT * Put some TODO blocks around existing bugs found in the API in the e2e tests that we might have time to get to or might not. This will let others maintaining the tests know that once they fix the bug they should update the end to end test to change the behavior. Testing this: Go to the latest CI logs and look for any particular lines from the test executing such as: ```ts should set the response content types to be expected ``` Also run this manually on your machine through this command: ```ts node scripts/functional_tests --config x-pack/test/detection_engine_api_integration/security_and_spaces/config.ts ``` Change a test manually and re-run the above command to watch something fail. Screen shot of what you should see on the CI machine when these are running: <img width="1825" alt="Screen Shot 2020-02-08 at 10 15 21 AM" src="https://user-images.githubusercontent.com/1151048/74089355-ae9a8e80-4a5d-11ea-9050-86e68d7e3bba.png"> Delete any items that are not applicable to this PR. ~~- [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)~~ ~~- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials~~ - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios ~~- [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~~ ~~- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)~~ ~~- [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility)~~ - [x] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process) * Removed key not used for end to end tests for 7.6 Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

FrankHassanabad added 3 commits February 8, 2020 02:27

end to end tests

b1687cd

Merge branch 'master' into end-to-end-tests

1738b54

wire up the detection_engine_api_intgeration to the functional tests

01399f8

FrankHassanabad changed the title ~~[SIEM][Detection Engine] Backend end to end tests~~ [SIEM][Detection Engine] Backend end-to-end tests Feb 8, 2020

FrankHassanabad self-assigned this Feb 8, 2020

FrankHassanabad added v8.0.0 v7.6.1 v7.7.0 release_note:fix labels Feb 8, 2020

FrankHassanabad marked this pull request as ready for review February 8, 2020 17:28

FrankHassanabad added the Team:SIEM label Feb 8, 2020

FrankHassanabad added 3 commits February 8, 2020 13:02

Removed Boom and implemented proper snake_case for error handling API

9f523db

Merge branch 'master' into end-to-end-tests

01fb13d

Merge branch 'master' into end-to-end-tests

5355a7b

rylnd reviewed Feb 10, 2020

View reviewed changes

x-pack/test/detection_engine_api_integration/common/config.ts Outdated Show resolved Hide resolved

FrankHassanabad added 5 commits February 10, 2020 16:02

Updated per code review suggestions

1dbc830

Fixes feedback about the transformOrError by naming them better

506c409

Refactored and renamed everything from update to be patch now

023e446

Added updated_rules with PUT and tests

f58a4f9

Merge branch 'master' into end-to-end-tests

46449bf

dhurley14 reviewed Feb 11, 2020

View reviewed changes

dhurley14 approved these changes Feb 11, 2020

View reviewed changes

Updated frontend

c91fb58

spong reviewed Feb 11, 2020

View reviewed changes

FrankHassanabad merged commit f890776 into elastic:master Feb 11, 2020

FrankHassanabad deleted the end-to-end-tests branch February 11, 2020 22:24

FrankHassanabad mentioned this pull request Feb 11, 2020

[7.x] [SIEM][Detection Engine] Backend end-to-end tests (#57166) #57385

Merged

FrankHassanabad mentioned this pull request Feb 11, 2020

[7.6] [SIEM][Detection Engine] Backend end-to-end tests (#57166) #57392

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIEM][Detection Engine] Backend end-to-end tests #57166

[SIEM][Detection Engine] Backend end-to-end tests #57166

FrankHassanabad commented Feb 8, 2020 •

edited

Loading

elasticmachine commented Feb 8, 2020

rylnd left a comment

rylnd Feb 10, 2020

FrankHassanabad Feb 10, 2020

FrankHassanabad Feb 10, 2020

rylnd Feb 10, 2020

FrankHassanabad Feb 10, 2020

rylnd Feb 10, 2020 •

edited

Loading

dhurley14 Feb 11, 2020

FrankHassanabad Feb 11, 2020 •

edited

Loading

dhurley14 left a comment

spong Feb 11, 2020

kibanamachine commented Feb 11, 2020

[SIEM][Detection Engine] Backend end-to-end tests #57166

[SIEM][Detection Engine] Backend end-to-end tests #57166

Conversation

FrankHassanabad commented Feb 8, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Feb 8, 2020

rylnd left a comment

Choose a reason for hiding this comment

rylnd Feb 10, 2020

Choose a reason for hiding this comment

FrankHassanabad Feb 10, 2020

Choose a reason for hiding this comment

FrankHassanabad Feb 10, 2020

Choose a reason for hiding this comment

rylnd Feb 10, 2020

Choose a reason for hiding this comment

FrankHassanabad Feb 10, 2020

Choose a reason for hiding this comment

rylnd Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

dhurley14 Feb 11, 2020

Choose a reason for hiding this comment

FrankHassanabad Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

dhurley14 left a comment

Choose a reason for hiding this comment

spong Feb 11, 2020

Choose a reason for hiding this comment

kibanamachine commented Feb 11, 2020

💚 Build Succeeded

History

FrankHassanabad commented Feb 8, 2020 •

edited

Loading

rylnd Feb 10, 2020 •

edited

Loading

FrankHassanabad Feb 11, 2020 •

edited

Loading