[Security Solution] Adds Cypress test for index action in detection rules #141069

MadameSheema · 2022-09-20T12:18:04Z

Summary

In this PR we are adding a test to validate that an index action can be added during the rule creation and that the action is properly triggered.

In order to do that, we have extended the object/connectors.ts file creating a new interface, called IndexConnector, which contains all the information needed to create an index connector. We have also created the Connectors type that would be used when creating a rule with actions.

In this test we are creating the rule from the UI to include also the creation of the action. The main focus of the test is to check that the action works correctly, that is why we don’t need a rule with all the fields, we just need a rule with the mandatory field needed on the UI to make also the test execution faster.

To be able to have a rule with just the UI mandatory fields, we have refactored the CustomRule interface to make optional some of the current fields, forcing us to refactor also the methods where some of those fields are used. A part from the refactor, we have extended the interface, with the Actions field.

The new test performs the following actions:  

Clean any possible data that may impact our test on the before hooks. 
Creates an index we will need to have in elastic search to be able to create the index action.
Creates a custom rule with an index action
Once the rule is executed checks that the document with the expected JSON has been correctly indexed.

MadameSheema · 2022-09-21T13:35:54Z

@elasticmachine merge upstream

MadameSheema · 2022-09-26T09:02:37Z

@elasticmachine merge upstream

banderror

Alright, I ended up posting a ton of comments, sorry about that :)

One general comment: my expectation was that it should be enough to cover action execution at the integration test level - just like it's done for the alerts generation logic for every rule type.

Scenario: Rule executes successfully with added action (1 integration)

But I'm not going to ask to remove this new e2e test - I think it's also good to have one at the e2e level!

x-pack/plugins/security_solution/cypress/e2e/detection_rules/rule_actions.cy.ts

banderror · 2022-09-27T20:56:47Z

x-pack/plugins/security_solution/cypress/objects/rule.ts

+  runsEvery: getRunsEvery(),
+  lookBack: getLookBack(),


I noticed that runsEvery equals 1 second by default.

// Default interval is 1m, our tests config overwrite this to 1s // See https://github.com/elastic/kibana/pull/125396 for details const getRunsEvery = (): Interval => ({ interval: '1', timeType: 'Seconds', type: 's', });

What's the reason for having such a short interval?

I think for most of the tests where we need to run a rule, we need to run it only once in order to verify something. This means we could set the interval to a large value like 24 hours.

If we use a very short interval in e2e tests, this can:

create unnecessary load (Alerting Framework needs to schedule rules all the time while tests are running)

introduce unexpected race conditions between a test and Kibana running and re-running rules in the background

ultimately make the tests more flaky

I don't remember why we did it like that, originally for sure that was a reason behind. Good to have revisited this then :)

banderror · 2022-09-27T21:01:12Z

x-pack/plugins/security_solution/cypress/tasks/api_calls/rules.ts

        description: rule.description,
-        interval: `${rule.runsEvery.interval}${rule.runsEvery.type}`,
-        from: `now-${rule.lookBack.interval}${rule.lookBack.type}`,
+        interval: `${rule.runsEvery?.interval}${rule.runsEvery?.type}`,


This will render it to undefinedundefined if rule.runsEvery is nullable. This is why I'm skeptical about making most of the rule fields optional in the CustomRule interface.

banderror · 2022-09-27T21:02:09Z

x-pack/plugins/security_solution/cypress/tasks/api_calls/rules.ts

+    const riskScore = rule.riskScore != null ? parseInt(rule.riskScore, 10) : undefined;
+    const severity = rule.severity != null ? rule.severity.toLocaleLowerCase() : undefined;


And we also have to check those fields for null all the time...

banderror · 2022-09-27T21:05:21Z

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

+export const deleteConnectors = () => {
+  const kibanaIndexUrl = `${Cypress.env('ELASTICSEARCH_URL')}/.kibana_\*`;
+  cy.request('POST', `${kibanaIndexUrl}/_delete_by_query?conflicts=proceed`, {
+    query: {
+      bool: {
+        filter: [
+          {
+            match: {
+              type: 'action',
+            },
+          },
+        ],
+      },
+    },
+  });
+};


It would be great to extract all API calls from this common file to the corresponding files under x-pack/plugins/security_solution/cypress/tasks/api_calls

…ule_actions.cy.ts Co-authored-by: Dmitrii Shevchenko <dmshevch@gmail.com>

…-ref HEAD~1..HEAD --fix'

MadameSheema · 2022-09-28T05:51:12Z

@xcrzx @banderror lots of thanks for your thorough revision!!! :)

I believe it has helped a lot to put the focus on some pain points we have in general that need to be tackled in order to have a more robust and maintainable test suite. I'll write a ticket to collect all this information and I'll share it with you to gather more ideas.

Thanks!

…ule_actions.cy.ts Co-authored-by: Dmitrii Shevchenko <dmshevch@gmail.com>

MadameSheema · 2022-09-28T06:56:03Z

@elasticmachine merge upstream

MadameSheema · 2022-09-28T18:13:21Z

@elasticmachine merge upstream

kibanamachine · 2022-09-28T18:13:22Z

merge conflict between base and head

kibana-ci · 2022-09-29T07:51:08Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 2495d7c

Failed CI Steps

Security Solution Tests #2

Test Failures

[job] [logs] Security Solution Tests #2 / Alerts detection rules table auto-refresh should disable auto refresh when any rule selected and enable it after rules unselected

Metrics [docs]

Page load bundle

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

id	before	after	diff
`triggersActionsUi`	98.9KB	98.9KB	+36.0B

History

💔 Build #76621 failed 0c739b5
💚 Build #76318 succeeded da751f5
💔 Build #76303 failed 4cbb487
💚 Build #75942 succeeded c448dd4
💚 Build #75531 succeeded 054632e

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

cc @MadameSheema

banderror

The test now looks perfect, and thank you @MadameSheema for addressing so many comments!

I'll write a ticket to collect all this information and I'll share it with you to gather more ideas.

Please do, that would be awesome!

I'd like us to have ticket(s) for the following ideas:

Reusing existing rule schema in Cypress tests (comment)
Making tests driven by code instead of data; getting rid of optional properties in data (comment, comment, comment)
Cleaning up duplicate properties in data (comment, comment)
Setting a large default rule interval (comment)
Extracting some code from tasks/common into separate files (comment)

Let me know if I can help with writing them.

…ules (elastic#141069) Co-authored-by: Dmitrii Shevchenko <dmshevch@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit db6a825)

MadameSheema · 2022-09-29T13:46:49Z

💚 All backports created successfully

Status	Branch	Result
✅	8.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…ules (#141069) (#142227) Co-authored-by: Dmitrii Shevchenko <dmshevch@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit db6a825)

MadameSheema added 9 commits September 20, 2022 14:17

adds rule actions spec file

2519735

fixes type issue

ce57a56

fixes failing test

1165877

fixes failures

6d0e8b8

refactor

4bcc912

fixes more type check issues

bc749de

fixes more issues

7fde8bc

removes flakiness

151437c

more fixes

b749804

kibanamachine and others added 3 commits September 21, 2022 07:35

Merge branch 'main' into cypress/adds-index-connector

a1f051a

refactor

f429dd0

Merge branch 'main' into cypress/adds-index-connector

d96dd37

MadameSheema mentioned this pull request Sep 22, 2022

[Security Solution] Removes flakiness from pinned filters and adds code owners #141269

Merged

MadameSheema added 2 commits September 22, 2022 08:43

Merge branch 'main' into cypress/adds-index-connector

157c23c

Merge branch 'main' into cypress/adds-index-connector

65ddbc9

Merge branch 'main' into cypress/adds-index-connector

054632e

MadameSheema changed the title ~~adds rule actions spec file~~ [Security Solution] Adds Cypress test for index action in detection rules Sep 26, 2022

MadameSheema self-assigned this Sep 26, 2022

MadameSheema added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team v8.6.0 release_note:skip Skip the PR/issue when compiling release notes labels Sep 26, 2022

MadameSheema marked this pull request as ready for review September 26, 2022 09:04

MadameSheema requested review from a team as code owners September 26, 2022 09:04

banderror reviewed Sep 27, 2022

View reviewed changes

MadameSheema and others added 3 commits September 28, 2022 06:54

Update x-pack/plugins/security_solution/cypress/e2e/detection_rules/r…

9ed56dd

…ule_actions.cy.ts Co-authored-by: Dmitrii Shevchenko <dmshevch@gmail.com>

Update x-pack/plugins/security_solution/cypress/e2e/detection_rules/r…

b60e3bb

…ule_actions.cy.ts Co-authored-by: Dmitrii Shevchenko <dmshevch@gmail.com>

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

4cbb487

…-ref HEAD~1..HEAD --fix'

MadameSheema and others added 2 commits September 28, 2022 08:54

Merge branch 'main' into cypress/adds-index-connector

a4a48f6

Update x-pack/plugins/security_solution/cypress/e2e/detection_rules/r…

bf11fe9

…ule_actions.cy.ts Co-authored-by: Dmitrii Shevchenko <dmshevch@gmail.com>

Merge branch 'main' into cypress/adds-index-connector

da751f5

MadameSheema added 3 commits September 28, 2022 20:22

review follow up

e8dcf55

Merge branch 'main' into cypress/adds-index-connector

0c739b5

fixes issues generated by the conflicts during merge

2495d7c

banderror mentioned this pull request Sep 29, 2022

[Security Solution] Rule schemas tech debt #138606

Open

10 tasks

banderror approved these changes Sep 29, 2022

View reviewed changes

MadameSheema merged commit db6a825 into elastic:main Sep 29, 2022

MadameSheema deleted the cypress/adds-index-connector branch September 29, 2022 13:44

kibanamachine added the backport:skip This commit does not require backporting label Sep 29, 2022

MadameSheema mentioned this pull request Sep 29, 2022

[8.5] [Security Solution] Adds Cypress test for index action in detection rules (#141069) #142227

Merged

MadameSheema added the v8.5.0 label Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Adds Cypress test for index action in detection rules #141069

[Security Solution] Adds Cypress test for index action in detection rules #141069

MadameSheema commented Sep 20, 2022 •

edited

MadameSheema commented Sep 21, 2022

MadameSheema commented Sep 26, 2022

banderror left a comment

banderror Sep 27, 2022

MadameSheema Sep 28, 2022

banderror Sep 27, 2022 •

edited

banderror Sep 27, 2022

banderror Sep 27, 2022

MadameSheema commented Sep 28, 2022

MadameSheema commented Sep 28, 2022

MadameSheema commented Sep 28, 2022

kibanamachine commented Sep 28, 2022

kibana-ci commented Sep 29, 2022

banderror left a comment

MadameSheema commented Sep 29, 2022

		const riskScore = rule.riskScore != null ? parseInt(rule.riskScore, 10) : undefined;
		const severity = rule.severity != null ? rule.severity.toLocaleLowerCase() : undefined;

[Security Solution] Adds Cypress test for index action in detection rules #141069

[Security Solution] Adds Cypress test for index action in detection rules #141069

Conversation

MadameSheema commented Sep 20, 2022 • edited

Summary

MadameSheema commented Sep 21, 2022

MadameSheema commented Sep 26, 2022

banderror left a comment

Choose a reason for hiding this comment

banderror Sep 27, 2022

Choose a reason for hiding this comment

MadameSheema Sep 28, 2022

Choose a reason for hiding this comment

banderror Sep 27, 2022 • edited

Choose a reason for hiding this comment

banderror Sep 27, 2022

Choose a reason for hiding this comment

banderror Sep 27, 2022

Choose a reason for hiding this comment

MadameSheema commented Sep 28, 2022

MadameSheema commented Sep 28, 2022

MadameSheema commented Sep 28, 2022

kibanamachine commented Sep 28, 2022

kibana-ci commented Sep 29, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Page load bundle

History

banderror left a comment

Choose a reason for hiding this comment

MadameSheema commented Sep 29, 2022

💚 All backports created successfully

Questions ?

MadameSheema commented Sep 20, 2022 •

edited

banderror Sep 27, 2022 •

edited