[RAM] Add notify badge with API #137430
Closed
[RAM] Add notify badge with API #137430
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
XavierM
added
release_note:enhancement
Team:ResponseOps
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
|
@elasticmachine merge upstream |
matschaffer
approved these changes
Sep 13, 2022
There was a problem hiding this comment.
The @elastic/infra-monitoring-ui change seems fine in isolation so ✅ for that, but I'm guessing @elastic/response-ops should probably weigh-in on this as a feature.
The title says "POC" but I guess the intent is to release this?
Also wondering if more of these stubs should be using createDefaultAlertExecutorOptions or something like it to avoid the widespread addition of the fields to test suites that aren't concerned with snooze behavior. Not saying that should happen in this PR, but maybe a follow up issue.
fkanout
approved these changes
Sep 21, 2022
8 tasks
jpdjere
added a commit
that referenced
this pull request
Sep 27, 2022
…140626) ## Intro This PR modifies the logic of bulk updating rule actions, in preparation for #137430 ## Summary - Removes the mute logic for bulk updating rule actions - Remove option for “Perform no actions” from the bulk update rule actions dropdown options ONLY (option still available when creating or editing rules individually) - Also corrects bulk update rule actions flyout, so that: - available actions are always displayed - copy referring to using "Perform No Actions" to mute all selected rules is no longer displayed. ## Screenshots **Removed unwanted copy and "On each rule execution" selected as default**  **"Perform No Action" option no longer available**  ### Checklist Delete any items that are not applicable to this PR. - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Sep 27, 2022
…lastic#140626) ## Intro This PR modifies the logic of bulk updating rule actions, in preparation for elastic#137430 ## Summary - Removes the mute logic for bulk updating rule actions - Remove option for “Perform no actions” from the bulk update rule actions dropdown options ONLY (option still available when creating or editing rules individually) - Also corrects bulk update rule actions flyout, so that: - available actions are always displayed - copy referring to using "Perform No Actions" to mute all selected rules is no longer displayed. ## Screenshots **Removed unwanted copy and "On each rule execution" selected as default**  **"Perform No Action" option no longer available**  ### Checklist Delete any items that are not applicable to this PR. - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 7aa5428)
kibanamachine
added a commit
that referenced
this pull request
Sep 27, 2022
…140626) (#141977) ## Intro This PR modifies the logic of bulk updating rule actions, in preparation for #137430 ## Summary - Removes the mute logic for bulk updating rule actions - Remove option for “Perform no actions” from the bulk update rule actions dropdown options ONLY (option still available when creating or editing rules individually) - Also corrects bulk update rule actions flyout, so that: - available actions are always displayed - copy referring to using "Perform No Actions" to mute all selected rules is no longer displayed. ## Screenshots **Removed unwanted copy and "On each rule execution" selected as default**  **"Perform No Action" option no longer available**  ### Checklist Delete any items that are not applicable to this PR. - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 7aa5428) Co-authored-by: Juan Pablo Djeredjian <[email protected]>
banderror
reviewed
Sep 29, 2022
There was a problem hiding this comment.
@XavierM finally, I reviewed it!
Checked out, tested locally, and reviewed the changes on our side. The snooze component in the Rules table works like a charm. 🙌 Thank you so much for proving that it can work in Security.
Observations after testing
Imported rules without actions are shown as indefinitely snoozed (mute_all: true). This is expected because right now we have this weird logic in Security that mutes a rule under the hood if it has Perform no actions or doesn't have actions in the actions array.
Newly created rules without actions are shown as indefinitely snoozed (mute_all: true). Newly created rules with actions are shown as not snoozed. This is expected and the reason is the same.
Newly installed prebuilt rules are shown as not snoozed, although they don't have any actions. I didn't expect this. It means that probably we just don't have this mute/unmute logic in the add_prepackaged_rules endpoint 🤷♂️
Comments on implementation
I appreciate that the _snooze and _unsnooze endpoints are internal. This will give us some space and time for stabilizing their interface and it won't be blocking the development.
I left a bunch of comments on the implementation below. My main point is to avoid leakage of the snooze data into Security Solution's code.
Suggestions for this PR
Let's close this PR for now and we will reopen and build on top of it later when we start working on implementing support for snoozing in Security. I asked our leads to create an epic for that. I hope we will have some capacity for working on it in 8.7 (can't promise though).
I'd also support merging the changes made in the Framework (triggers_actions_ui plugin, x-pack/test/alerting_api_integration, etc) if it's easy to do in a separate PR.
Comment on lines
503
to
494
| const responseOptionalFields = { | ||
| execution_summary: RuleExecutionSummary, | ||
| active_snoozes: t.array(t.string), | ||
| mute_all: t.boolean, | ||
| is_snoozed_until: t.union([IsoDateString, t.undefined, t.null]), | ||
| snooze_schedule: t.array(RuleSnoozeSchedule), |
There was a problem hiding this comment.
Let's not expose the full snooze model from the detection_engine API endpoints. It leaks the "implementation" details of this feature from the Alerting Framework to Security:
- As you can see from the diff, you had to add the new fields to a lot of places in the code of
security_solutionplugin. - It couples the Security code with the Framework code I'd say quite a lot. Every time you need to evolve this model in ResponseOps you will have to remember to tweak the corresponding code on the Security side and align those changes with the Rules and potentially Alerts area. Let's try to avoid this dependency - it seems unnecessary.
My suggestion is to read the full snooze model via an Alerting API endpoint.
- It would be some "bulk get snooze information for multiple rules by their ids", and we'd call it on every page refresh of the Rules table. In addition, our existing API endpoints in Security could be returning just basic information about rule snoozing (e.g.
is_snoozed_now,has_snooze_schedule; I'd probably even deprecatemute_allif possible). - The basic information that would be returned with rules could be easily constructed on the BE side based on the full snooze model, probably using some helper methods of the
RulesClient. This info could be helpful to make the initial rendering of the Rules Table, Rule Details page, etc -- this could reduce the number of spinners and show some minimally useful info right away. - The full snoozing information would be fetched via a dedicated Alerting endpoint and used to re-render the Snooze components to show full info and make them interactive.
There was a problem hiding this comment.
Yes that make sense to me too and I even like it better
Comment on lines
+50
to
+57
| export const RuleSnoozeSchedule = t.intersection([ | ||
| t.type({ | ||
| duration: t.number, | ||
| rRule: RRuleRecord, | ||
| }), | ||
| t.partial({ | ||
| id: t.string, | ||
| skipRecurrences: t.array(t.string), |
There was a problem hiding this comment.
Nit: it's common to use snake_case in API endpoints' parameters and responses
Comment on lines
229
to
230
| render: (rule: Rule) => { | ||
| return triggersActionsUi.getRulesListNotifyBadge({ |
There was a problem hiding this comment.
Nice! Just out of curiosity: why it is returned via triggersActionsUi and can't be just imported and used as a React component in JSX?
Comment on lines
231
to
232
| rule: { | ||
| ...rule, |
There was a problem hiding this comment.
Why do you need the whole rule here? Let's limit it to the only properties you actually need, e.g. id.
Comment on lines
238
to
239
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| } as any, |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| } as any, | ||
| isLoading: loadingRuleIds.includes(rule.id) || isLoading, | ||
| onRuleChanged: reFetchRules, |
There was a problem hiding this comment.
Why do we need to refetch the current page of rules when only the snooze info can be changed by this component? I mean, it can't change other rule fields and it can't change other rules. Could it be stateful and maintain its own state when calling API endpoints?
There was a problem hiding this comment.
Yes! we can do that
| @@ -368,7 +369,7 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper = | |||
|
|
|||
| const createdSignalsCount = result.createdSignals.length; | |||
|
|
|||
| if (actions.length) { | |||
| if (actions.length && !isRuleSnoozed(options.rule)) { | |||
There was a problem hiding this comment.
Thank you for taking care of legacy rule actions as well 🙏
x-pack/plugins/security_solution/server/lib/detection_engine/rules/find_rules.ts
Show resolved
Hide resolved
| @@ -60,6 +60,7 @@ describe('get_export_by_object_ids', () => { | |||
| rulesNdjson: { | |||
| author: ['Elastic'], | |||
| actions: [], | |||
| active_snoozes: [], | |||
There was a problem hiding this comment.
By the way, regarding rule export and import, I'm not sure we should support snooze data in export/import. It feels like too dynamic/sporadic data that can become outdated pretty quickly whereas exported data should be static and not bound to time.
This is the reason why we don't export rule monitoring data: the execution_summary object.
XavierM
force-pushed
the
snooze-security-solution
branch
from
9fd4278 to
70fdf7a
Compare
XavierM
changed the title
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
1 task
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
POC to bring snooze functionality in the rules table of security solutions
Checklist
Delete any items that are not applicable to this PR.