[SIEM][Detections] Allow synchronous rule actions to be updated via PATCH #67914
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
This method was typed to accept actions, but it was not doing anything with them. This was mainly a "bug by omission" so I'm simply adding unit tests for regression purposes.
Now that patchRules uses this field, we simply need to pass it.
rylnd
added
Team:SIEM
v8.0.0
release_note:skip
|
Pinging @elastic/siem (Team:SIEM) |
spong
approved these changes
Jun 2, 2020
There was a problem hiding this comment.
Checked out and verified fix locally as follows:
- Created new rule with synchronous slack action (and default action message)
- Created
./rules/patches/update_action.jsonpatch with updated action message - Used CLI scripts to update action:
./patch_rule.sh ./rules/patches/update_action.json - Verified UI/API returned action with updated message
- Verified in subsequent executions that the action fired with the updated message
Many thanks for the sleuthing and detailed description in both the issue and fix @rylnd! And thanks also to Mercwri over on the community forums for finding this one, appreciate the time! 🙂
LGTM! 👍 I'd say this fix is low-impact enough to make it into 7.8, and 7.7.2 as well. As for the disparity between synch and asynch actions, let's open an issue for tracking, and continue our discussion from earlier today with the team to see what we can do to improve this area of the codebase.
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
Jun 2, 2020
…ATCH (elastic#67914) * Update synchronous actions in patchRules This method was typed to accept actions, but it was not doing anything with them. This was mainly a "bug by omission" so I'm simply adding unit tests for regression purposes. * Allow synchronous actions to be patched either individually or in bulk Now that patchRules uses this field, we simply need to pass it. Co-authored-by: Garrett Spong <spong@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/siem/server/lib/detection_engine/rules/patch_rules.ts
rylnd
added a commit
that referenced
this pull request
Jun 2, 2020
…ATCH (#67914) (#67990) * Update synchronous actions in patchRules This method was typed to accept actions, but it was not doing anything with them. This was mainly a "bug by omission" so I'm simply adding unit tests for regression purposes. * Allow synchronous actions to be patched either individually or in bulk Now that patchRules uses this field, we simply need to pass it. Co-authored-by: Garrett Spong <spong@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/siem/server/lib/detection_engine/rules/patch_rules.ts
rylnd
added a commit
that referenced
this pull request
Jun 2, 2020
…ATCH (#67914) (#67988) * Update synchronous actions in patchRules This method was typed to accept actions, but it was not doing anything with them. This was mainly a "bug by omission" so I'm simply adding unit tests for regression purposes. * Allow synchronous actions to be patched either individually or in bulk Now that patchRules uses this field, we simply need to pass it. Co-authored-by: Garrett Spong <spong@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Garrett Spong <spong@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
Jun 9, 2020
…ATCH (elastic#67914) * Update synchronous actions in patchRules This method was typed to accept actions, but it was not doing anything with them. This was mainly a "bug by omission" so I'm simply adding unit tests for regression purposes. * Allow synchronous actions to be patched either individually or in bulk Now that patchRules uses this field, we simply need to pass it. Co-authored-by: Garrett Spong <spong@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/siem/server/lib/detection_engine/routes/rules/patch_rules_bulk_route.ts # x-pack/plugins/siem/server/lib/detection_engine/routes/rules/patch_rules_route.ts # x-pack/plugins/siem/server/lib/detection_engine/rules/patch_rules.test.ts # x-pack/plugins/siem/server/lib/detection_engine/rules/patch_rules.ts
rylnd
added a commit
that referenced
this pull request
Jun 9, 2020
… via PATCH (#67914) (#68684) * [SIEM][Detections] Allow synchronous rule actions to be updated via PATCH (#67914) * Update synchronous actions in patchRules This method was typed to accept actions, but it was not doing anything with them. This was mainly a "bug by omission" so I'm simply adding unit tests for regression purposes. * Allow synchronous actions to be patched either individually or in bulk Now that patchRules uses this field, we simply need to pass it. Co-authored-by: Garrett Spong <spong@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/siem/server/lib/detection_engine/routes/rules/patch_rules_bulk_route.ts # x-pack/plugins/siem/server/lib/detection_engine/routes/rules/patch_rules_route.ts # x-pack/plugins/siem/server/lib/detection_engine/rules/patch_rules.test.ts # x-pack/plugins/siem/server/lib/detection_engine/rules/patch_rules.ts * Update tests for backport This API changed since 7.7, so we can't just backport our uses of it (e.g. the tests).
MindyRS
added
the
Team: SecuritySolution
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
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
Fixes #67815.
Notes
patchRulesfunction. Testing this at higher levels isn't currently possible due to the way that we generate rule responses: the actions that exist on the rule SO itself (i.e. synchronous actions executed during rule execution) are not part of the rule output and are masked in those situations by the separate ruleActions SO (which is used for asynchronous actions). However, we always create a separate ruleActions SO for these synchronous actions even though it is never used, which is what made this bug so confusing: everything in the UI and API tell you that the actions have been updated, which is partially true: one version of the actions has been updated, but it's not the version that's actually used.As a visualization:
rule.actionsruleActionsruleActionsruleActionsThis PR updates things so that both
rule.actionsandruleActionsare updated together, but it does not address the above disparity. I'm happy to create an issue to discuss removingrule.actionsor other potential solutions to the above.Checklist
For maintainers