-
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] [Platform] Migrate legacy actions whenever user interacts with the rule #115101
Conversation
@elasticmachine merge upstream |
9fd0bc5
to
cc67c59
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.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.
I left comments but in order to speed up development with the upcoming feature freeze I am going to give this a LGTM so we can check this in as is to start with and then iterate over the bug pointed and others can write tests agains this if needed.
* and putting that into the actions array of the rule, then set the rules onThrottle property, notifyWhen and throttle from null -> actualy value (1hr etc..) | ||
* Then use the rules client to delete the siem.notification | ||
* Then with the legacy Rule Actions saved object type, just delete it. | ||
*/ |
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.
I'd add these comments to the function as JSDoc now.
}; | ||
|
||
const [validated, errors] = validate(newInternalRule, internalRuleUpdate); | ||
if (errors != null || validated === null) { | ||
throw new UpdateError(`Applying update would create invalid rule: ${errors}`, 400); |
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.
Why do we have this extra error throw here now? I wouldn't want to add another layer of validation. We are already validating the input arguments and then the response on the way out. We typically just validate on the API boundaries to ensure that the 3rd party services and user input work as we expect.
Within the boundaries we usually trust we are writing code well enough and straight forward enough. If this begins causing errors at this point I would want to fix the user input at the REST API boundary or the response boundary rather than fix issues here or worry about this part.
Otherwise future maintainers might have to maintain multiple spots of validation or change error codes in multiple spots, etc...
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.
I actually saw this was missing here. This same piece is in the patch rules function
kibana/x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.ts
Lines 204 to 207 in 512d594
const [validated, errors] = validate(newRule, internalRuleUpdate); | |
if (errors != null || validated === null) { | |
throw new PatchError(`Applying patch would create invalid rule: ${errors}`, 400); | |
} |
And I figured it would be good to add it here to keep it consistent.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
merge conflict between base and head |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Saved Object Tagging Functional Tests.x-pack/test/saved_object_tagging/functional/tests/dashboard_integration·ts.saved objects tagging - functional tests dashboard integration editing allows to select tags for an existing dashboardStandard Out
Stack Trace
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @dhurley14 |
…nteracts with the rule (elastic#115101) Migrate legacy actions whenever user interacts with the rule (elastic#115101) Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…nteracts with the rule (#115101) (#115572) Migrate legacy actions whenever user interacts with the rule (#115101) Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Devin W. Hurley <[email protected]>
…-link-to-kibana-app * 'master' of github.com:elastic/kibana: (30 commits) Fix potential error from undefined (elastic#115562) [App Search, Crawler] Fix validation step panel padding/whitespace (elastic#115542) [Cases][Connectors] ServiceNow ITOM: MVP (elastic#114125) Change default session idle timeout to 8 hours. (elastic#115565) Upgrade EUI to v39.1.1 (elastic#114732) [App Search] Wired up organic results on Curation Suggestions view (elastic#114717) [i18n] remove i18n html extractor (elastic#115004) [Logs/Metrics UI] Add deprecated field configuration to Deprecations API (elastic#115103) [Transform] Add alerting rules management to Transform UI (elastic#115363) Update UI links to Fleet and Agent docs (elastic#115295) [ML] Adding ability to change data view in advanced job wizard (elastic#115191) Change deleteByNamespace to include legacy URL aliases (elastic#115459) [Unified Integrations] Remove and cleanup add data views (elastic#115424) [Discover] Show ignored field values (elastic#115040) [ML] Stop reading the ml.max_open_jobs node attribute (elastic#115524) [Discover] Improve doc viewer code in Discover (elastic#114759) [Security Solutions] Adds security detection rule actions as importable and exportable (elastic#115243) [Security Solution] [Platform] Migrate legacy actions whenever user interacts with the rule (elastic#115101) [Fleet] Add telemetry for integration cards (elastic#115413) 🐛 Fix single percentile case when ES is returning no buckets (elastic#115214) ... # Conflicts: # x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap
Summary
Copy actions related data from legacy actions sidecar into
rule.actions
param on the rule whenever a user performs an update / patch on the rule data.Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers