-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Security Solution] Handle negative lookback in rule upgrade flyout #204317
base: main
Are you sure you want to change the base?
[Security Solution] Handle negative lookback in rule upgrade flyout #204317
Conversation
3254df1
to
a8dacbc
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
@maximpn when I follow your instructions and attempt to open the upgrade flyout for the modified rule, I receive the following error, which looks to have been thrown by The above error occurred in ErrorBoundary:
|
a8dacbc
to
1a0d56f
Compare
Hi @rylnd, are you sure you pulled the latest PR changes? I double checked and it works for me locally as described in the PR description. Could you try removing the branch and pull the latest changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maximpn I found another bug that we should fix. Described in the comment below.
...n/public/detection_engine/rule_creation/components/step_rule_actions/notification_action.tsx
Outdated
Show resolved
Hide resolved
...ns/security_solution/public/detection_engine/rule_creation_ui/pages/rule_creation/helpers.ts
Outdated
Show resolved
Hide resolved
from: 'from', | ||
to: 'to', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is crazy obscure and unreasonably complex. We should refactor it after the release in Serverless. No action is needed in this PR.
x-pack/test/security_solution_cypress/cypress/tasks/prebuilt_rules_preview.ts
Outdated
Show resolved
Hide resolved
x-pack/test/security_solution_cypress/cypress/screens/alerts_detection_rules.ts
Show resolved
Hide resolved
...ns/security_solution/public/detection_engine/rule_creation_ui/pages/rule_creation/helpers.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed about 5/6 of the changes. Posting the last pack of comments for today.
...ts/rule_details/three_way_diff/final_edit/fields/rule_schedule/simple_rule_schedule_form.tsx
Outdated
Show resolved
Hide resolved
...omponents/rule_details/three_way_diff/final_edit/fields/rule_schedule/rule_schedule_form.tsx
Show resolved
Hide resolved
...components/rule_details/three_way_diff/final_readonly/fields/rule_schedule/rule_schedule.tsx
Outdated
Show resolved
Hide resolved
...components/rule_details/three_way_diff/final_readonly/fields/rule_schedule/rule_schedule.tsx
Outdated
Show resolved
Hide resolved
...rule_management/components/rule_details/three_way_diff/final_edit/common_rule_field_edit.tsx
Show resolved
Hide resolved
a7862ad
to
7699f14
Compare
@banderror Thanks for your through review 🙏 I addressed your comments. In particular
There is one issue left related to non-normalized rule schedule we use right now. I described it in my answer. I suggest to discuss it and address it separately. Could you have a look? |
d48d61e
to
b9299c1
Compare
ad57b91
to
54ed57a
Compare
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
History
cc @maximpn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked the latest code changes and re-tested the app locally.
LGTM, thanks @maximpn.
Files by Code Ownerelastic/kibana-localization
elastic/security-detection-engine
elastic/security-detection-rule-management
elastic/security-engineering-productivity
elastic/security-solution
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes: #202715
Fixes: #204714
Summary
This PR makes inconsistent/wrong rule's look-back duration prominent for a user. It falls back to a default 1 minute value in rule upgrade workflow.
Details
Negative/wrong
lookback
problemThere is a difference between rule schedule value in a saved object and value represented to users
interval
,from
andto
fields representing rule schedule.interval
shows how often a rule runs in task runner.from
andto
stored in date math format likenow-10m
represent a date time range used to fetch source events. Task manager strives to run rules exactly everyinterval
but it's not always possible due to multiple reasons like system load and various delays. To avoid any gaps to appearfrom
point in time usually stands earlier than current time minusinterval
, for exampleinterval
is10 minutes
andfrom
isnow-12m
meaning rule will analyze events starting from 12 minutes old.to
represents the latest point in time source events will be analyzed.interval
andlookback
. Whereinterval
is the same as above andlookback
and a time duration before current time minusinterval
. For exampleinterval
is10 minutes
and lookback is2 minutes
it means a rule will analyzing events starting with 12 minutes old until the current moment in time.Literally
interval
,from
andto
mean a rule runs everyinterval
and analyzes events starting fromfrom
untilto
. Technicallyfrom
andto
may not have any correlation withinterval
, for example a rule may analyze one year old events. While it's reasonable for manual rule runs and gap remediation the same approach doesn't work well for usual rule schedule. Transformation betweeninterval
/from
/to
andinterval
/lookback
works only whento
is equal the current moment in time i.e.now
.Rule management APIs allow to set any
from
andto
values resulting in inconsistent rule schedule. Transformedinterval
/lookback
value won't represent real time interval used to fetch source events for analysis. On top of that negativelookback
value may puzzle users on the meaning of the negative sign.Prebuilt rules with
interval
/from
/to
resulting in negativelookback
Some prebuilt rules have such
interval
,from
andto
field values thatnegativelookback
is expected, for exampleMultiple Okta Sessions Detected for a Single User
. It runs every60 minutes
but hasfrom
field set tonow-30m
andto
equalsnow
. In the end we havelookback
equalsto
-from
-interval
=30 minutes
-60 minutes
=-30 minutes
.Our UI doesn't handle negative
lookback
values. It simply discards a negative sign and substitutes the rest for editing. In the case above30 minutes
will be suggested for editing. Saving the form will result in changingfrom
tonow-90m
Changes in this PR
This PR mitigates rule schedule inconsistencies caused by
to
fields not using the current point in time i.e.now
. The following was doneDiffableRule
'srule_schedule
was changed to haveinterval
,from
andto
fields instead ofinterval
andlookback
_perform
rule upgrade API endpoint was adapted to the newDIffableRule
'srule_schedule
interval
andlookback
in Diff View, readonly view and field form whenlookback
is non-negative andto
equalsnow
interval
,from
andto
in Diff View, readonly view and field form whento
isn't equalnow
or calculatedlookback
is negativeto
isn't equalnow
or calculatedlookback
is negativeinterval
andlookback
whenlookback
is non-negative andto
equalsnow
and showsinterval
,from
andto
in any other caseinterval
,from
andto
in Diff View, readonly view and field form whento
isn't equalnow
or calculatedlookback
is negativemaxValue
was added toScheduleItemField
to have an ability to restrict input at reasonable valuesScreenshots
How to test?
prebuiltRulesCustomizationEnabled
feature flag is enabledserver.restrictInternalApis: false
tokibana.dev.yaml
security_detection_engine
Fleet packageSuspicious File Creation via Kworker
rule by running a query belowSuspicious File Creation via Kworker
rule