[Security Solution] Add UI incentivizers to upgrade prebuilt rules #211862
+2,051
−1,062
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
dplumlee
added
release_note:skip
…-version-handling
…-version-handling
…-version-handling
dplumlee
force-pushed
the
update-missing-base-version-handling
branch
from
c33ab5b to
0aed247
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) |
dplumlee
requested review from
xcrzx and
banderror
and removed request for
nikitaindik
dplumlee
changed the title
dplumlee
changed the title
banderror
added
bug
banderror
requested review from
maximpn
and removed request for
xcrzx and
banderror
Files by Code Ownerelastic/security-detection-engine
elastic/security-detection-rule-management
elastic/security-engineering-productivity
elastic/security-solution
|
maximpn
requested changes
Mar 4, 2025
There was a problem hiding this comment.
@dplumlee Thanks for improving UX for prebuilt rules with missing base version 🙏
I reviewed the changes and tested them locally. Testing didn't reveal issues.
I left a number of comments though critical are the following
- Revert rate limiting retry logic
- rename
hasBaseVersiontohas_base_versionin the API contract - double check diff algorithms implementation changes are correct. The task ticket assumes we merge the changes for non-functional fields but the implementation does't do that for fields like
description,note,setup.
On top of that I have some general comments not directly related to that PR.
Changes to diff algorithms might be much simpler if the functionality was implemented via Strategy pattern. We have a lot similar logic for each field with some specific details. Missing base version case isn't exception.
| @@ -172,7 +175,20 @@ const EditRulePageComponent: FC<{ rule: RuleResponse }> = ({ rule }) => { | |||
| newTermsFields: defineStepData.newTermsFields, | |||
| }); | |||
|
|
|||
| const loading = userInfoLoading || listsConfigLoading; | |||
| const { upgradeReviewResponse, isLoading: isRuleUpgradeReviewLoading } = usePrebuiltRulesUpgrade({ | |||
There was a problem hiding this comment.
nit: It could be extracted to usePrebuiltRuleUpgradeById() hook accepting a rule id. Additionally isRuleUpgradeable calculation could be included in that hook.
| @@ -89,4 +89,5 @@ export interface RuleUpgradeInfoForReview { | |||
| target_rule: RuleResponse; | |||
| diff: PartialRuleDiff; | |||
| revision: number; | |||
| hasBaseVersion: boolean; | |||
There was a problem hiding this comment.
We stick to snake case in API contracts
Suggested change
| hasBaseVersion: boolean; | |
| has_base_version: boolean; |
The other comment we could address later regarding has_base_version at this level.
Fields Diff includes has_base_version in each field diff response. It looks like overkill since rule either has a base version or not and it's sufficient to have a single has_base_version at the rule level. It means fields diff level has_base_version should be removed.
...ection_engine/rule_management_ui/components/rule_update_callouts/has_rule_update_callout.tsx
Outdated
Show resolved
Hide resolved
...ne/rule_management/api/hooks/prebuilt_rules/use_fetch_prebuilt_rules_install_review_query.ts
Outdated
Show resolved
Hide resolved
| @@ -48,6 +48,7 @@ export const RuleUpgrade = memo(function RuleUpgrade({ | |||
| <RuleUpgradeCallout | |||
| numOfSolvableConflicts={numOfSolvableConflicts} | |||
| numOfNonSolvableConflicts={numOfNonSolvableConflicts} | |||
| hasBaseVersion={ruleUpgradeState.hasBaseVersion} | |||
There was a problem hiding this comment.
Why should missing base version callout logic is placed in RuleUpgradeCallout? It feels simple to place a component named like RuleHasMissingBaseVersionCallout component on top of RuleUpgradeCallout.
| @@ -421,81 +421,162 @@ describe('dataSourceDiffAlgorithm', () => { | |||
| }); | |||
|
|
|||
| describe('if base_version is missing', () => { | |||
There was a problem hiding this comment.
Suggested change
| describe('if base_version is missing', () => { | |
| describe('when base_version is missing', () => { |
|
|
||
| describe('returns target_version as merged output if current_version and target_version are different - scenario -AB', () => { | ||
| it('if versions are different types', () => { | ||
| describe('if current_version and target_version are the same - scenario -AA', () => { |
There was a problem hiding this comment.
Suggested change
| describe('if current_version and target_version are the same - scenario -AA', () => { | |
| describe('and current and target rule versions are the same - scenario -AA', () => { |
| }) | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('if current_version and target_version are different - scenario -AB', () => { |
There was a problem hiding this comment.
Suggested change
| describe('if current_version and target_version are different - scenario -AB', () => { | |
| describe('and current and target rule versions are different - scenario -AB', () => { |
Comment on lines
+531
to
581
| describe('returns SOLVABLE conflict if rule is customized', () => { | ||
| it('if versions are different types', () => { | ||
| const mockVersions: ThreeVersionsOf<RuleDataSource | undefined> = { | ||
| base_version: MissingVersion, | ||
| current_version: { type: DataSourceType.data_view, data_view_id: '456' }, | ||
| target_version: { | ||
| type: DataSourceType.index_patterns, | ||
| index_patterns: ['one', 'three', 'four'], | ||
| }, | ||
| }; | ||
|
|
||
| const result = dataSourceDiffAlgorithm(mockVersions, true); | ||
|
|
||
| expect(result).toEqual( | ||
| expect.objectContaining({ | ||
| has_base_version: false, | ||
| base_version: undefined, | ||
| merged_version: mockVersions.target_version, | ||
| diff_outcome: ThreeWayDiffOutcome.MissingBaseCanUpdate, | ||
| merge_outcome: ThreeWayMergeOutcome.Target, | ||
| conflict: ThreeWayDiffConflict.SOLVABLE, | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it('if current version is undefined', () => { | ||
| const mockVersions: ThreeVersionsOf<RuleDataSource | undefined> = { | ||
| base_version: MissingVersion, | ||
| current_version: undefined, | ||
| target_version: { | ||
| type: DataSourceType.index_patterns, | ||
| index_patterns: ['one', 'three', 'four'], | ||
| }, | ||
| }; | ||
|
|
||
| const result = dataSourceDiffAlgorithm(mockVersions, true); | ||
|
|
||
| expect(result).toEqual( | ||
| expect.objectContaining({ | ||
| has_base_version: false, | ||
| base_version: undefined, | ||
| merged_version: mockVersions.target_version, | ||
| diff_outcome: ThreeWayDiffOutcome.MissingBaseCanUpdate, | ||
| merge_outcome: ThreeWayMergeOutcome.Target, | ||
| conflict: ThreeWayDiffConflict.SOLVABLE, | ||
| }) | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
nit: Context and test names are reversed here. A general rule of thumb describe defines a context like when rule is customized or with conflicts and it says what's expected like returns true or returns a SOLVABLE conflict.
...etection_engine/prebuilt_rules/logic/diff/calculation/algorithms/rule_type_diff_algorithm.ts
Outdated
Show resolved
Hide resolved
vitaliidm
reviewed
Mar 4, 2025
...gins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_editing/index.tsx
Outdated
Show resolved
Hide resolved
...gins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_editing/index.tsx
Outdated
Show resolved
Hide resolved
...gins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_editing/index.tsx
Outdated
Show resolved
Hide resolved
...gins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_editing/index.tsx
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
cc @dplumlee |
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.
Summary
Partially addresses #210358
Adds all callouts and logic to incentivize users to upgrade their rules asap. These include:
This PR also adds related updates to the rule diff algorithms in order to facilitate an easier upgrade experience when rules have missing base versions. These include:
SOLVABLE_CONFLICT.tags,references,new_terms_fields,threat_index). We typically also consider multi-line string fields as mergeable but without three versions of the string, we are currently unable to merge the strings together, so we just return target version.index) and non-mergeable fields and return them asSOLVABLE_CONFLICT.Screenshots
Callout on Rule details page w/ flyout button
Upgrade flyout now accessible from rule details page
Callout on rule editing page
Dismissible callout on rule management page
Callout in rule upgrade flyout when rule has missing base version
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.