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.
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
[RAM] Adds auto-incrementing revision field to rules #147398
[RAM] Adds auto-incrementing revision field to rules #147398
Changes from 53 commits
542bf21
57c17dc
04033c8
6d3aa3d
9b28134
ef64bb1
96f386b
31b143a
bdd5285
f70b505
9371731
396ccc1
82feace
3087e63
d71e752
3120bae
b33d3e4
6d63ae4
7f5d57f
1f5ce9f
98b57a3
daf5099
98098b5
e6fef49
40fa9b0
e122adb
673d1c5
e4279f4
4a80747
bd87b0d
f0f5c32
60abd10
827ecb3
e8f40c6
937b2ed
e1ff58a
61dc2c0
8270e78
f7dbf41
d162cd8
ac244db
a71a309
4f971d6
2a567c1
a1a21e6
88a18f8
cc1926b
9b7e948
440d424
cbd1bed
4a39c4d
65ce48b
2b3ea72
1784b14
c8697d2
1a7da2f
5619b02
ef66c8d
62ca943
f949386
f3b5c4e
152d53c
53a5941
9bd4ffd
e6f5a7b
73ccb27
320c706
24eae01
d894736
640526e
78834e3
8ce5da6
41d32b3
adf18cc
eef57f6
2028524
7896ea9
646c2c2
a575eb8
d2366cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nit:
maybeIncrementRevision
orgetNextRevision
would be more precise since it doesn't always increment itThere 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.
Why is it a generic function? Can we use just the
RuleTypeParams
type?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.
At the first glance, these parameters are obscure and it would be helpful to add JSDoc comments clarifying what they exactly mean.
For instance,
updatedParams: RuleTypeParams
-- is this the same asdata.params
?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.
nit: I'm always curious whether double
if
is more readable than one line condition with&&
? 😅 As far as I know nestedif
's isn't the best approach since it increases visual complexity. I could think also about early exit as well so there are a lot of options to avoid unnecessary nesting.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.
Yeah, I agree and tend to avoid them, but it seemed to read easier in this case so I kept them separate. I've got no qualms either way, so just updated it be a single
if
👍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.
Open question: should a rule clone/duplicate reset
revision
to0
or keep the current value? Historic Security behavior has been to keep the current value, but we should align here for consistency (no strong opinion on the security side, but makes sense to me to reset to0
as if creating a new rule).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.
When creating new rules, it's logical to set the revision number to 0 because revisions are used to track the history of changes, and a new rule doesn't have any previous history. Moreover, suppose we were to display the revision history to the user through a dropdown menu, allowing them to select which historical revision to revert to. In that case, it could be counterintuitive if the revision numbers started from a value other than zero, which might look like a bug. Like there are some historical versions I don't have access 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.
While testing locally, I noticed duplicated rules (through bulk actions) have revision set to 1. So it seems like their revision gets immediately bumped after creation. When I create rules one by one they have revision set to 0.
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.
Really good point -- I'll keep it at
0
and remove theseTODO
's.Hmmm, were these rules being duplicated with additional reference objects like
actions
orexceptions
? I came across this in some of the tests, but thought I had it working for the raw duplicate. Let me debug and see what I can do to keep it at0
when duplicating regardless of if there are references or not.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.
Okie, looks like it's not specific to the reference scenario, although that'll need to be fixed too (though only applicable for exceptions).
There's a call to
update()
aftercreate()
when duplicating that's supposed to handle the adding of the exceptions portion:kibana/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/bulk_actions/route.ts
Lines 538 to 547 in 8733774
However it looks like we're passing in a slightly different
riskScoreMapping
(risk_score: undefined
) here which is resulting in the revision being incremented:I'll get a fix in place for this and be sure to add a test on the Security Solution side for this duplication logic. 👍
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.
Alrighty, fix is in place as of f3b5c4e. I ended up adding a
shouldIncrementRevision
callback for consumers as we discussed in the other thread. I didn't fix thisrisk_score_mapping
issue just yet as I want to check with @banderror to see why it may've been typed this way. For now duplicates/clones will work as expected, but updates without changing anything will still result in an unexpected revision update until we resolve this.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.
Let's use
resetRevision()
here