[RAM][SECURITYSOLUTION][ALERTS] - Show warning that custom action intervals cannot be shorter than the rule's check interval #155684

e40pud · 2023-04-25T07:31:36Z

Summary

These changes fixes the issue with the incompatible action frequency selected while creating or updating the rule. The action frequency should never be shorter than the rule's schedule interval. We will show the warning when user selects the action's frequency which is shorter than the rule's schedule interval.

These changes won't apply to Bulk operation, since we don't have access to all rules there. We would need to get the minimum schedule interval among the selected rules in that case and use it in the UI.

Here is how the warning will look like:

…ervals cannot be shorter than the rule's check interval (elastic#155502)

e40pud · 2023-04-25T10:17:04Z

@elasticmachine merge upstream

vitaliidm · 2023-04-25T14:26:37Z

ruled edit form for actions allows to save rule, even if validation fails

Screen.Recording.2023-04-25.at.15.24.16.mov

It is not possible to save rule on other forms. Please, refer to a recorded video.

Same happens on rule creation

vitaliidm · 2023-04-25T14:42:32Z

These changes won't apply to Bulk operation, since we don't have access to all rules there. We would need to get the minimum schedule interval among the selected rules in that case and use it in the UI.

I think we leverage bulk actions dry mode, that was created exactly for this purpose: prevent edit errors by letting know users beforehand the likely outcome of the request.
Details on implementation and usage can be found here

One of the possible ways of implementations:

retrieve the smallest allowed interval through this API and then use it on bulk edit actions form

maximpn

@e40pud it's a nice UX improvement to the the custom action frequency 👍

I've looked through the changes and left some minor comments. On top of that I've run it locally and noticed inconsistencies like 1 minute interval causes the warning to appear while 60 seconds don't. Warning sign appears either in one or two fields (if there is 1 seconds interval) so it can be unclear for users why it works this way.

Screen.Recording.2023-04-25.at.16.41.12.mov

maximpn · 2023-04-25T14:24:54Z

x-pack/plugins/security_solution/common/utils/time_type_value.ts

+    value: 0,
+  };
+  const filterTimeVal = time.match(/\d+/g);
+  const filterTimeType = time.match(/[a-zA-Z]+/g);


Is there some reason to match all letters here and check separately for s, m, h and d below? What if there is time string like 1abch or 20mms?

It could be one regex like

/^(\d+)?(ms|s|m|h|d)?$/

to match an expected format strictly.

Agree with Maxim here, with the additional doubt of why defaulting to 0ms if the only accepted Units are actually ['s', 'm', 'h', 'd']

Same here.

I just moved this method from another file x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_creation/helpers.ts :-)

If I will have time before the release to address the concerns that you put, I will look into this as well.

maximpn · 2023-04-25T14:25:17Z

x-pack/plugins/security_solution/common/utils/time_type_value.ts

+import { isEmpty } from 'lodash/fp';
+import type { Unit } from '@kbn/datemath';
+
+export const getTimeTypeValue = (time: string): { unit: Unit; value: number } => {


It'd be really helpful to have a comment with an expected time format.

+1

I worked last year on creating a TimeDuration io-ts type and added there some explanation of how the type works, that could be of inspiration:
https://github.com/elastic/kibana/blob/main/packages/kbn-securitysolution-io-ts-types/src/time_duration/index.ts

I just moved this method from another file x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_creation/helpers.ts :-)

If I will have time before the release to address the concerns that you put, I will look into this as well.

maximpn · 2023-04-25T14:33:01Z

x-pack/plugins/security_solution/common/utils/time_type_value.test.ts

+  ].forEach(({ interval, value, unit }) => {
+    it(`should correctly return time duration and time unit when 'interval' is ${interval}`, () => {
+      const { value: actualValue, unit: actualUnit } = getTimeTypeValue(interval);
+      expect(actualValue).toEqual(value);


nit: toBe is stricter and better for asserting primitive values.

jpdjere · 2023-04-25T15:50:30Z

These changes won't apply to Bulk operation, since we don't have access to all rules there. We would need to get the minimum schedule interval among the selected rules in that case and use it in the UI.

What would happen now if a user bulk edited the actions interval to be shorter than the rules' execution interval?

e40pud · 2023-04-25T16:45:21Z

@elasticmachine merge upstream

e40pud · 2023-04-25T18:19:14Z

@elasticmachine merge upstream

banderror · 2023-04-25T18:19:55Z

@e40pud I didn't have time to check this one today, but noticed a lot of valid comments here.

I have a few questions off the top of my head:

Do we restrict action frequency at the API level (request schemas) or only on the FE side in the form? I think we should do this in the API - we know that a lot of our users are direct API users.
Is there anything that would prevent us from doing such validation in the _bulk_action endpoint? When we edit the rule schedule or action frequency, we should be able to reject edits that don't pass validation (such as, when the user sets an action frequency that is below the rule interval or when the user changes the rule interval to become longer than some of the action frequency). Even if we don't use the dry-run approach suggested by @vitaliidm.
Do we handle a similar case in our rule update/patch endpoints? E.g. when a user opens the Rule Editing page and changes the rule interval so it becomes longer than the frequency of any of the actions, which would make this frequency invalid.

e40pud · 2023-04-26T06:54:12Z

@elasticmachine merge upstream

kibanamachine · 2023-04-26T06:54:14Z

merge conflict between base and head

e40pud · 2023-04-27T08:22:53Z

@elasticmachine merge upstream

e40pud · 2023-04-27T15:28:09Z

x-pack/plugins/security_solution/public/detections/components/rules/step_rule_actions/index.tsx

Replying here for 🧵ing...

These changes won't apply to Bulk operation, since we don't have access to all rules there. We would need to get the minimum schedule interval among the selected rules in that case and use it in the UI.

I think we leverage bulk actions dry mode, that was created exactly for this purpose: prevent edit errors by letting know users beforehand the likely outcome of the request. Details on implementation and usage can be found here

One of the possible ways of implementations:

retrieve the smallest allowed interval through this API and then use it on bulk edit actions form

@vitaliidm do you suggest to add another variable to BulkEditActionResponse which will have minimum interval of the rules in bulk operation? And then on dry run of bulk.edit operation and type one of BulkActionEditType.set_rule_actions, BulkActionEditType.add_rule_actions we would calculate minimum interval among those rules and return as part of the response?

This definitely doable, just thinking if that new parameters will fit the current structure of the response. It is all about updated, created, deleted, skipped and number of succeeded and failed rules. Now we will add another attribute which is kind of not related to anything of that.

Right now, on dry_run=true we send the following payload

POST http://localhost:5601/kbn/api/detection_engine/rules/_bulk_action?dry_run=true { "action":"edit", "ids":[ "0483b100-e5c9-11ed-a754-61de71d8c618", "22670e00-e5d4-11ed-a029-c5fa447f7c6e", "a51fa760-e5e5-11ed-8c3b-19bc4e0ce732" ], "edit":[ { "type":"add_rule_actions", "value":{ "actions":[ ] } } ] }

Instead, if we can send here action with the minimal throttle frequency, let's say 1s and perform validation check on BE: we can receive a response with new error code for exceeding action's interval value.

{ "statusCode":500, "error":"Internal Server Error", "message":"Bulk edit partially failed", "attributes":{ "errors":[ { "message":"action interval can not be smaller than interval", "status_code":500, "err_code":"ACTION_INTERVAL_CAN_NOT_BE_SMALLER_THAN_RULE_INTERVAL", "params": { "minimalValidActionInterval": "5s" }, "rules":[ { "id":"22670e00-e5d4-11ed-a029-c5fa447f7c6e", "name":"test ml" } ] } ], "results":{ "updated":[ ], "created":[ ], "deleted":[ ], "skipped":[ ] }, "summary":{ "failed":1, "succeeded":1, "skipped":0, "total":2 } } }

Introducing a new params property that will have minimal valid action interval can help to keep the same format of response.

There is, though, one thing we need to be conscientious of. We do not include actions values for dry run, only action type. As we don't know what user will select. So if we plan to send action with rule actions payload, new ACTION_INTERVAL_CAN_NOT_BE_SMALLER_THAN_RULE_INTERVAL errors should not count when we checking if we want to show dry run warning modal window. That might overcomplicate dry run processing logic

Other solution might be, introducing warnings or conditional_errors property. That woudn't interfere with summary section of the response

e40pud · 2023-04-27T15:40:41Z

@elasticmachine merge upstream

e40pud · 2023-05-02T12:09:58Z

@elasticmachine merge upstream

e40pud · 2023-05-03T11:10:47Z

@elasticmachine merge upstream

e40pud · 2023-05-10T13:14:53Z

@elasticmachine merge upstream

kibana-ci · 2023-07-18T13:27:46Z

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

[job] [logs] Defend Workflows Endpoint Cypress Tests #2 / Automated Response Actions From alerts should have generated endpoint and rule
[job] [logs] FTR Configs #16 / InfraOps App Metrics UI Hosts View #Single host Flyout "after all" hook for "should navigate to APM services after click"
[job] [logs] FTR Configs #16 / InfraOps App Metrics UI Hosts View #Single host Flyout should navigate to APM services after click

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	4313	4314	+1

Async chunks

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

id	before	after	diff
`securitySolution`	9.8MB	9.8MB	+379.0B

History

💚 Build #138450 succeeded 6344ac0
💔 Build #138435 failed a8b5dda
💛 Build #126852 was flaky b1974ff
💚 Build #125107 succeeded 2bf8f5b
💛 Build #124763 was flaky 9e42376

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

cc @e40pud

[RAM][SECURITYSOLUTION][ALERTS] - Show warning that custom action int…

a12b25e

…ervals cannot be shorter than the rule's check interval (elastic#155502)

e40pud added release_note:enhancement ci:cloud-deploy Create or update a Cloud deployment labels Apr 25, 2023

e40pud requested review from a team as code owners April 25, 2023 07:31

e40pud self-assigned this Apr 25, 2023

e40pud requested a review from jpdjere April 25, 2023 07:31

e40pud mentioned this pull request Apr 25, 2023

[RAM][Security Solution][Alerts] Support the ability to trigger a rule action per alert generated (#153611) #155384

Merged

1 task

Merge branch 'main' into security/feature/action-short-interval

7ee3a3f

maximpn reviewed Apr 25, 2023

View reviewed changes

vitaliidm self-requested a review April 25, 2023 16:18

Merge branch 'main' into security/feature/action-short-interval

8903310

Merge branch 'main' into security/feature/action-short-interval

736e2c7

Merge branch 'main' into security/feature/action-short-interval

92f6090

kibanamachine and others added 2 commits April 27, 2023 04:22

Merge branch 'main' into security/feature/action-short-interval

ce925cb

Review feedback

07609ca

e40pud commented Apr 27, 2023

View reviewed changes

Merge branch 'main' into security/feature/action-short-interval

20b53c6

Merge branch 'main' into security/feature/action-short-interval

9e42376

Merge branch 'main' into security/feature/action-short-interval

2bf8f5b

vitaliidm removed their request for review May 4, 2023 10:13

e40pud added the v8.9.0 label May 8, 2023

e40pud marked this pull request as draft May 10, 2023 13:14

kibanamachine and others added 6 commits May 10, 2023 09:14

Merge branch 'main' into security/feature/action-short-interval

b1974ff

Merge branch 'main' into security/feature/action-short-interval

567d278

Resolve conflicts

a8b5dda

Fix types

6344ac0

Merge branch 'main' into security/feature/action-short-interval

fc38954

Update index.tsx

5e886ff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RAM][SECURITYSOLUTION][ALERTS] - Show warning that custom action intervals cannot be shorter than the rule's check interval #155684

[RAM][SECURITYSOLUTION][ALERTS] - Show warning that custom action intervals cannot be shorter than the rule's check interval #155684

e40pud commented Apr 25, 2023

e40pud commented Apr 25, 2023

vitaliidm commented Apr 25, 2023 •

edited

Loading

vitaliidm commented Apr 25, 2023

maximpn left a comment

maximpn Apr 25, 2023 •

edited

Loading

jpdjere Apr 25, 2023

e40pud Apr 27, 2023

maximpn Apr 25, 2023 •

edited

Loading

jpdjere Apr 25, 2023

e40pud Apr 27, 2023 •

edited

Loading

maximpn Apr 25, 2023

jpdjere commented Apr 25, 2023

e40pud commented Apr 25, 2023

e40pud commented Apr 25, 2023

banderror commented Apr 25, 2023

e40pud commented Apr 26, 2023

kibanamachine commented Apr 26, 2023

e40pud commented Apr 27, 2023

e40pud Apr 27, 2023

vitaliidm May 4, 2023

e40pud commented Apr 27, 2023

e40pud commented May 2, 2023

e40pud commented May 3, 2023

e40pud commented May 10, 2023

kibana-ci commented Jul 18, 2023 •

edited

Loading

[RAM][SECURITYSOLUTION][ALERTS] - Show warning that custom action intervals cannot be shorter than the rule's check interval #155684

Are you sure you want to change the base?

[RAM][SECURITYSOLUTION][ALERTS] - Show warning that custom action intervals cannot be shorter than the rule's check interval #155684

Conversation

e40pud commented Apr 25, 2023

Summary

e40pud commented Apr 25, 2023

vitaliidm commented Apr 25, 2023 • edited Loading

vitaliidm commented Apr 25, 2023

maximpn left a comment

Choose a reason for hiding this comment

maximpn Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

jpdjere Apr 25, 2023

Choose a reason for hiding this comment

e40pud Apr 27, 2023

Choose a reason for hiding this comment

maximpn Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

jpdjere Apr 25, 2023

Choose a reason for hiding this comment

e40pud Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

maximpn Apr 25, 2023

Choose a reason for hiding this comment

jpdjere commented Apr 25, 2023

e40pud commented Apr 25, 2023

e40pud commented Apr 25, 2023

banderror commented Apr 25, 2023

e40pud commented Apr 26, 2023

kibanamachine commented Apr 26, 2023

e40pud commented Apr 27, 2023

e40pud Apr 27, 2023

Choose a reason for hiding this comment

vitaliidm May 4, 2023

Choose a reason for hiding this comment

e40pud commented Apr 27, 2023

e40pud commented May 2, 2023

e40pud commented May 3, 2023

e40pud commented May 10, 2023

kibana-ci commented Jul 18, 2023 • edited Loading

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

History

vitaliidm commented Apr 25, 2023 •

edited

Loading

maximpn Apr 25, 2023 •

edited

Loading

maximpn Apr 25, 2023 •

edited

Loading

e40pud Apr 27, 2023 •

edited

Loading

kibana-ci commented Jul 18, 2023 •

edited

Loading