[Security Solution] Fixes incorrect from field transform logic in upgrade/_perform route
#202824
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
bug
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
xcrzx
reviewed
Dec 4, 2024
...ib/detection_engine/prebuilt_rules/api/perform_rule_upgrade/diffable_rule_fields_mappings.ts
Outdated
Show resolved
Hide resolved
xcrzx
approved these changes
Dec 4, 2024
There was a problem hiding this comment.
Thanks for the fix, @dplumlee! I tested the changes locally with various upgrade scenarios, including cases with and without conflicts, with or without changes to the rule schedule, and situations where a customization was introduced or rolled back during the upgrade workflow. In all cases, the field was upgraded correctly. So this fix resolves the issue with marking rules as customized.
To address the issue of Kibana crashing (#202715), I’d suggest adding proper error handling in the rule upgrade flyout, but that can be done in a separate PR.
I’ve left a couple of comments regarding code deduplication and moving tests to the integration level. But I don’t consider these critical so I’ll leave it to your discretion.
...tection_engine/prebuilt_rules/api/perform_rule_upgrade/diffable_rule_fields_mappings.test.ts
Show resolved
Hide resolved
vitaliidm
reviewed
Dec 5, 2024
x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/utils/utils.ts
Outdated
Show resolved
Hide resolved
| * @param lookback moment.Moment representing the rule's additional lookback | ||
| * @returns moment.Moment representing the rule's 'from' property | ||
| */ | ||
| export const calculateFromValueWithRuleScheduleFields = (interval: string, lookback: string) => { |
There was a problem hiding this comment.
Suggested change
| export const calculateFromValueWithRuleScheduleFields = (interval: string, lookback: string) => { | |
| export const calculateFromValueWithRuleScheduleFields = (interval: string, lookback: string): number => { |
x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/utils/utils.ts
Outdated
Show resolved
Hide resolved
vitaliidm
reviewed
Dec 5, 2024
| * @param lookback moment.Moment representing the rule's additional lookback | ||
| * @returns moment.Moment representing the rule's 'from' property | ||
| */ | ||
| export const calculateFromValueWithRuleScheduleFields = (interval: string, lookback: string) => { |
There was a problem hiding this comment.
I would propose to call this function something like - calculateRange or so on.
It does not return from property that can be used in rule straightaway, just time range in seconds. now is added as prefix in another function.
There was a problem hiding this comment.
Switched the function to return the now-prefixed string
xcrzx
reviewed
Dec 5, 2024
x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/utils/utils.ts
Outdated
Show resolved
Hide resolved
vitaliidm
approved these changes
Dec 6, 2024
x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/utils/utils.test.ts
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
|
@dplumlee Is there anything left to do here before merge? I see a few unresolved comments. |
dplumlee
force-pushed
the
upgrade-perform-interval-bug
branch
from
016f717 to
1c6e505
Compare
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💔 Build Failed
Failed CI StepsTest Failures
Metrics [docs]
History
cc @dplumlee |
|
@elasticmachine merge upstream |
|
Starting backport for target branches: 8.17, 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 10, 2024
…upgrade/_perform` route (elastic#202824) **Fixes: elastic#202575 **Fixes: elastic#201631 **Partially addresses: elastic#202715 ## Summary All bugs have the same source > [!NOTE] > This bug/related fix is only visible with the `prebuiltRulesCustomizationEnabled` feature flag turned on. Fixes an issue where unedited prebuilt rules were being marked as "Modified" when upgraded due to a bug in the `upgrade/_perform` endpoint where the `from` field was incorrectly calculated via the `lookback` field. Solves multiple bugs where prebuilt rules were marked as "Modified" incorrectly when they were upgraded See reproduce steps in related tickets ([example](elastic#202575 (comment))) ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios (cherry picked from commit 93112b9)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 10, 2024
…upgrade/_perform` route (elastic#202824) **Fixes: elastic#202575 **Fixes: elastic#201631 **Partially addresses: elastic#202715 ## Summary All bugs have the same source > [!NOTE] > This bug/related fix is only visible with the `prebuiltRulesCustomizationEnabled` feature flag turned on. Fixes an issue where unedited prebuilt rules were being marked as "Modified" when upgraded due to a bug in the `upgrade/_perform` endpoint where the `from` field was incorrectly calculated via the `lookback` field. Solves multiple bugs where prebuilt rules were marked as "Modified" incorrectly when they were upgraded See reproduce steps in related tickets ([example](elastic#202575 (comment))) ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios (cherry picked from commit 93112b9)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Dec 10, 2024
…nsform logic in `upgrade/_perform` route (#202824) (#203506) # Backport This will backport the following commits from `main` to `8.17`: - [[Security Solution] Fixes incorrect `from` field transform logic in `upgrade/_perform` route (#202824)](#202824) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Davis Plumlee","email":"56367316+dplumlee@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-12-10T01:06:12Z","message":"[Security Solution] Fixes incorrect `from` field transform logic in `upgrade/_perform` route (#202824)\n\n**Fixes: https://github.com/elastic/kibana/issues/202575**\r\n**Fixes: https://github.com/elastic/kibana/issues/201631**\r\n**Partially addresses: https://github.com/elastic/kibana/issues/202715**\r\n\r\n## Summary\r\n\r\nAll bugs have the same source\r\n\r\n> [!NOTE] \r\n> This bug/related fix is only visible with the\r\n`prebuiltRulesCustomizationEnabled` feature flag turned on.\r\n\r\nFixes an issue where unedited prebuilt rules were being marked as\r\n\"Modified\" when upgraded due to a bug in the `upgrade/_perform` endpoint\r\nwhere the `from` field was incorrectly calculated via the `lookback`\r\nfield. Solves multiple bugs where prebuilt rules were marked as\r\n\"Modified\" incorrectly when they were upgraded\r\n\r\nSee reproduce steps in related tickets\r\n([example](https://github.com/elastic/kibana/issues/202575#issue-2713226478))\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following conditions. \r\n\r\nReviewers should verify this PR satisfies this list as well.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"93112b9d968ab8ed0d19de2250c14cece07dba5e","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","v9.0.0","Team:Detections and Resp","Team: SecuritySolution","Team:Detection Rule Management","Feature:Prebuilt Detection Rules","backport:version","v8.18.0","v8.17.1"],"title":"[Security Solution] Fixes incorrect `from` field transform logic in `upgrade/_perform` route","number":202824,"url":"https://github.com/elastic/kibana/pull/202824","mergeCommit":{"message":"[Security Solution] Fixes incorrect `from` field transform logic in `upgrade/_perform` route (#202824)\n\n**Fixes: https://github.com/elastic/kibana/issues/202575**\r\n**Fixes: https://github.com/elastic/kibana/issues/201631**\r\n**Partially addresses: https://github.com/elastic/kibana/issues/202715**\r\n\r\n## Summary\r\n\r\nAll bugs have the same source\r\n\r\n> [!NOTE] \r\n> This bug/related fix is only visible with the\r\n`prebuiltRulesCustomizationEnabled` feature flag turned on.\r\n\r\nFixes an issue where unedited prebuilt rules were being marked as\r\n\"Modified\" when upgraded due to a bug in the `upgrade/_perform` endpoint\r\nwhere the `from` field was incorrectly calculated via the `lookback`\r\nfield. Solves multiple bugs where prebuilt rules were marked as\r\n\"Modified\" incorrectly when they were upgraded\r\n\r\nSee reproduce steps in related tickets\r\n([example](https://github.com/elastic/kibana/issues/202575#issue-2713226478))\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following conditions. \r\n\r\nReviewers should verify this PR satisfies this list as well.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"93112b9d968ab8ed0d19de2250c14cece07dba5e"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.17"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/202824","number":202824,"mergeCommit":{"message":"[Security Solution] Fixes incorrect `from` field transform logic in `upgrade/_perform` route (#202824)\n\n**Fixes: https://github.com/elastic/kibana/issues/202575**\r\n**Fixes: https://github.com/elastic/kibana/issues/201631**\r\n**Partially addresses: https://github.com/elastic/kibana/issues/202715**\r\n\r\n## Summary\r\n\r\nAll bugs have the same source\r\n\r\n> [!NOTE] \r\n> This bug/related fix is only visible with the\r\n`prebuiltRulesCustomizationEnabled` feature flag turned on.\r\n\r\nFixes an issue where unedited prebuilt rules were being marked as\r\n\"Modified\" when upgraded due to a bug in the `upgrade/_perform` endpoint\r\nwhere the `from` field was incorrectly calculated via the `lookback`\r\nfield. Solves multiple bugs where prebuilt rules were marked as\r\n\"Modified\" incorrectly when they were upgraded\r\n\r\nSee reproduce steps in related tickets\r\n([example](https://github.com/elastic/kibana/issues/202575#issue-2713226478))\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following conditions. \r\n\r\nReviewers should verify this PR satisfies this list as well.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"93112b9d968ab8ed0d19de2250c14cece07dba5e"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.17","label":"v8.17.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>
kibanamachine
added a commit
that referenced
this pull request
Dec 10, 2024
…sform logic in `upgrade/_perform` route (#202824) (#203507) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Fixes incorrect `from` field transform logic in `upgrade/_perform` route (#202824)](#202824) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Davis Plumlee","email":"56367316+dplumlee@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-12-10T01:06:12Z","message":"[Security Solution] Fixes incorrect `from` field transform logic in `upgrade/_perform` route (#202824)\n\n**Fixes: https://github.com/elastic/kibana/issues/202575**\r\n**Fixes: https://github.com/elastic/kibana/issues/201631**\r\n**Partially addresses: https://github.com/elastic/kibana/issues/202715**\r\n\r\n## Summary\r\n\r\nAll bugs have the same source\r\n\r\n> [!NOTE] \r\n> This bug/related fix is only visible with the\r\n`prebuiltRulesCustomizationEnabled` feature flag turned on.\r\n\r\nFixes an issue where unedited prebuilt rules were being marked as\r\n\"Modified\" when upgraded due to a bug in the `upgrade/_perform` endpoint\r\nwhere the `from` field was incorrectly calculated via the `lookback`\r\nfield. Solves multiple bugs where prebuilt rules were marked as\r\n\"Modified\" incorrectly when they were upgraded\r\n\r\nSee reproduce steps in related tickets\r\n([example](https://github.com/elastic/kibana/issues/202575#issue-2713226478))\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following conditions. \r\n\r\nReviewers should verify this PR satisfies this list as well.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"93112b9d968ab8ed0d19de2250c14cece07dba5e","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","v9.0.0","Team:Detections and Resp","Team: SecuritySolution","Team:Detection Rule Management","Feature:Prebuilt Detection Rules","backport:version","v8.18.0","v8.17.1"],"title":"[Security Solution] Fixes incorrect `from` field transform logic in `upgrade/_perform` route","number":202824,"url":"https://github.com/elastic/kibana/pull/202824","mergeCommit":{"message":"[Security Solution] Fixes incorrect `from` field transform logic in `upgrade/_perform` route (#202824)\n\n**Fixes: https://github.com/elastic/kibana/issues/202575**\r\n**Fixes: https://github.com/elastic/kibana/issues/201631**\r\n**Partially addresses: https://github.com/elastic/kibana/issues/202715**\r\n\r\n## Summary\r\n\r\nAll bugs have the same source\r\n\r\n> [!NOTE] \r\n> This bug/related fix is only visible with the\r\n`prebuiltRulesCustomizationEnabled` feature flag turned on.\r\n\r\nFixes an issue where unedited prebuilt rules were being marked as\r\n\"Modified\" when upgraded due to a bug in the `upgrade/_perform` endpoint\r\nwhere the `from` field was incorrectly calculated via the `lookback`\r\nfield. Solves multiple bugs where prebuilt rules were marked as\r\n\"Modified\" incorrectly when they were upgraded\r\n\r\nSee reproduce steps in related tickets\r\n([example](https://github.com/elastic/kibana/issues/202575#issue-2713226478))\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following conditions. \r\n\r\nReviewers should verify this PR satisfies this list as well.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"93112b9d968ab8ed0d19de2250c14cece07dba5e"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.17"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/202824","number":202824,"mergeCommit":{"message":"[Security Solution] Fixes incorrect `from` field transform logic in `upgrade/_perform` route (#202824)\n\n**Fixes: https://github.com/elastic/kibana/issues/202575**\r\n**Fixes: https://github.com/elastic/kibana/issues/201631**\r\n**Partially addresses: https://github.com/elastic/kibana/issues/202715**\r\n\r\n## Summary\r\n\r\nAll bugs have the same source\r\n\r\n> [!NOTE] \r\n> This bug/related fix is only visible with the\r\n`prebuiltRulesCustomizationEnabled` feature flag turned on.\r\n\r\nFixes an issue where unedited prebuilt rules were being marked as\r\n\"Modified\" when upgraded due to a bug in the `upgrade/_perform` endpoint\r\nwhere the `from` field was incorrectly calculated via the `lookback`\r\nfield. Solves multiple bugs where prebuilt rules were marked as\r\n\"Modified\" incorrectly when they were upgraded\r\n\r\nSee reproduce steps in related tickets\r\n([example](https://github.com/elastic/kibana/issues/202575#issue-2713226478))\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following conditions. \r\n\r\nReviewers should verify this PR satisfies this list as well.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"93112b9d968ab8ed0d19de2250c14cece07dba5e"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.17","label":"v8.17.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>
CAWilson94
pushed a commit
to CAWilson94/kibana
that referenced
this pull request
Dec 12, 2024
…upgrade/_perform` route (elastic#202824) **Fixes: elastic#202575 **Fixes: elastic#201631 **Partially addresses: elastic#202715 ## Summary All bugs have the same source > [!NOTE] > This bug/related fix is only visible with the `prebuiltRulesCustomizationEnabled` feature flag turned on. Fixes an issue where unedited prebuilt rules were being marked as "Modified" when upgraded due to a bug in the `upgrade/_perform` endpoint where the `from` field was incorrectly calculated via the `lookback` field. Solves multiple bugs where prebuilt rules were marked as "Modified" incorrectly when they were upgraded See reproduce steps in related tickets ([example](elastic#202575 (comment))) ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
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.
Fixes: #202575
Fixes: #201631
Partially addresses: #202715
Summary
All bugs have the same source
Note
This bug/related fix is only visible with the
prebuiltRulesCustomizationEnabledfeature flag turned on.Fixes an issue where unedited prebuilt rules were being marked as "Modified" when upgraded due to a bug in the
upgrade/_performendpoint where thefromfield was incorrectly calculated via thelookbackfield. Solves multiple bugs where prebuilt rules were marked as "Modified" incorrectly when they were upgradedSee reproduce steps in related tickets (example)
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.