[Security Solution] Handle negative lookback in rule upgrade flyout #204317
+1,961
−518
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maximpn
added
bug
maximpn
force-pushed
the
handle-negative-lookback-in-rule-upgrade-flyout
branch
4 times, most recently
from
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: |
maximpn
force-pushed
the
handle-negative-lookback-in-rule-upgrade-flyout
branch
from
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? |
rylnd
reviewed
Dec 19, 2024
There was a problem hiding this comment.
@maximpn I tested this and verified that it works as described. However, I'm not sure that this is the best solution for users. There are two main issues with the implementation:
- By disallowing negative lookback values, it forces the user to change the rule's logic on upgrade
- The upstream rule will continue to conflict, necessitate resolution, and (again) have its in-situ schedule changed on each subsequent release/upgrade workflow.
If I'm understanding correctly, these rules cannot be edited/upgraded without (significantly) changing their schedule. I would like to discuss this with both rule authors and the rest of the DE team to get their thoughts before approving this.
I appreciate the move toward consistency here, and the addressing of some of the UI/form bugs, but I really think we should strive to preserve the existing values, if possible. Can we not simply use a form component that allows negative values?
| 'xpack.securitySolution.detectionEngine.rules.upgradeRules.ruleSchedule.lookbackInconsistencyWarning', | ||
| { | ||
| defaultMessage: | ||
| 'There is an inconsistency in rule schedule configuration. Rule may run with gaps. Default value "{defaultValue}" is suggested for upgrade.', |
There was a problem hiding this comment.
We might want some additional articles here for legibility:
Suggested change
| 'There is an inconsistency in rule schedule configuration. Rule may run with gaps. Default value "{defaultValue}" is suggested for upgrade.', | |
| 'There is an inconsistency in rule schedule configuration, and the rule may run with gaps. The default value "{defaultValue}" is suggested.', |
| 'xpack.securitySolution.detectionEngine.ruleDetails.lookbackInconsistencyWarning', | ||
| { | ||
| defaultMessage: | ||
| 'There is an inconsistency in rule schedule configuration. Rule may run with gaps. Please edit the rule to resolve it.', |
There was a problem hiding this comment.
Again, I'd make this one sentence to make the relationship clearer between the two statements:
Suggested change
| 'There is an inconsistency in rule schedule configuration. Rule may run with gaps. Please edit the rule to resolve it.', | |
| 'There is an inconsistency in rule schedule configuration, and the rule may run with gaps. Please edit the rule to resolve this.', |
| @@ -577,7 +577,7 @@ export const getCatchupTuples = ({ | |||
| */ | |||
| export const calculateFromValue = (interval: string, lookback: string) => { | |||
| const parsedInterval = parseInterval(interval) ?? moment.duration(0); | |||
| const parsedFrom = parseInterval(lookback) ?? moment.duration(0); | |||
| const parsedFrom = parseInterval(lookback) ?? moment.duration(1, 'm'); | |||
There was a problem hiding this comment.
Should this be more closely tied to DEFAULT_RULE_EXECUTION_LOOKBACK ? It seems like we're adding this default in two places instead of just one.
|
@rylnd thanks for your review 🙏 Generally speaking the problem won't be solved by allowing editing negative look-back. Rule can also have
It's possible but requires some amount of work justification to users.
Sure. Let's discuss it. |
5 tasks
banderror
requested changes
Dec 19, 2024
There was a problem hiding this comment.
@maximpn I agree with:
- @rylnd's points from [Security Solution] Handle negative lookback in rule upgrade flyout #204317 (review)
- @xcrzx's points from [Security Solution] Negative lookback is not handled properly during upgrade #204714 (comment)
Whatever comes or can possibly come from prebuilt rules or rules CRUD API (as long as the rule is valid and can be created/updated/installed) should not be reset to any default values by the rule upgrade workflow, and should stay as is, unless the user explicitly changes it. UI elements should not reset / change the values on their own.
I'm blocking this PR until an agreement is reached in #204714.
We should have a single PR that resolves both #202715 and #204714.
maximpn
force-pushed
the
handle-negative-lookback-in-rule-upgrade-flyout
branch
9 times, most recently
from
9f7223f to
5ebec94
Compare
elastic-vault-github-plugin-prod
bot
requested a review
from a team
as a code owner
maximpn
force-pushed
the
handle-negative-lookback-in-rule-upgrade-flyout
branch
from
09b35af to
e63a9e5
Compare
maximpn
force-pushed
the
handle-negative-lookback-in-rule-upgrade-flyout
branch
from
e63a9e5 to
85aa6da
Compare
maximpn
force-pushed
the
handle-negative-lookback-in-rule-upgrade-flyout
branch
from
b4bf2a1 to
95bbf57
Compare
💔 Build Failed
Failed CI StepsMetrics [docs]Module Count
Async chunks
History
cc @maximpn |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: #202715
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
lookbackproblemThere is a difference between rule schedule value in a saved object and value represented to users
interval,fromandtofields representing rule schedule.intervalshows how often a rule runs in task runner.fromandtostored in date math format likenow-10mrepresent a date time range used to fetch source events. Task manager strives to run rules exactly everyintervalbut it's not always possible due to multiple reasons like system load and various delays. To avoid any gaps to appearfrompoint in time usually stands earlier than current time minusinterval, for exampleintervalis10 minutesandfromisnow-12mmeaning rule will analyze events starting from 12 minutes old.torepresents the latest point in time source events will be analyzed.intervalandlookback. Whereintervalis the same as above andlookbackand a time duration before current time minusinterval. For exampleintervalis10 minutesand lookback is2 minutesit means a rule will analyzing events starting with 12 minutes old until the current moment in time.Literally
interval,fromandtomean a rule runs everyintervaland analyzes events starting fromfromuntilto. Technicallyfromandtomay 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/toandinterval/lookbackworks only whentois equal the current moment in time i.e.now.Rule management APIs allow to set any
fromandtovalues resulting in inconsistent rule schedule. Transformedinterval/lookbackvalue won't represent real time interval used to fetch source events for analysis. On top of that negativelookbackvalue may puzzle users on the meaning of the negative sign.Prebuilt rules with
interval/from/toresulting in negativelookbackSome prebuilt rules have such
interval,fromandtofield values thatnegativelookbackis expected, for exampleMultiple Okta Sessions Detected for a Single User. It runs every60 minutesbut hasfromfield set tonow-30mandtoequalsnow. In the end we havelookbackequalsto-from-interval=30 minutes-60 minutes=-30 minutes.Our UI doesn't handle negative
lookbackvalues. It simply discards a negative sign and substitutes the rest for editing. In the case above30 minuteswill be suggested for editing. Saving the form will result in changingfromtonow-90mChanges in this PR
This PR mitigates rule schedule inconsistencies caused by
tofields not using the current point in time i.e.now. The following was done_performrule upgrade API endpoint was changed to use default 1 minute look back in case provided look back interval can't be parsed. Parsing error happens for negativelookback. Previously it resulted in settinglookbackto0supon rule upgrade.toisn't equalnow.toisn't equalnow.fromvalue whentoisn't equalnow. Transformed values are incorrect in that case since transformation functionality assumestois equalnow.toisn't equalnow.Screenshots
Screen.Recording.2024-12-16.at.15.20.56.mov
How to test?
prebuiltRulesCustomizationEnabledfeature flag is enabledserver.restrictInternalApis: falsetokibana.dev.yamlsecurity_detection_engineFleet packageSuspicious File Creation via Kworkerrule by running a query belowSuspicious File Creation via Kworkerrule