[SIEM] Prevent undefined behavior in our ML popover #62498
Merged
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
If we accept our dispatch functions, enableDatafeed can be abstracted as a pure function. The version bound to popover's dispatch functions is now named 'handleJobStateChange', as that is the callback it's used for.
We no longer deal with jobs in our local state; that's the responsibility of the useSiemJobs hook
When attempting to run a job from the ML Popover, if the job needs to first be installed, we set the rest of the jobs to be "loading" while installation is performed. Without this change, if users are fast enough they can potentially trigger multiple rule installations, which is undefined behavior and leads to failures and bad state in our component.
rylnd
added
bug
|
Pinging @elastic/siem (Team:SIEM) |
spong
approved these changes
Apr 3, 2020
There was a problem hiding this comment.
Checked out and tested locally and was unable to reproduce the error unless getting really creative (quickly opening and closing the dialog and some other perfectly timed clicks). Code looks good as well -- thanks for for the fix @rylnd! LGTM once green on CI 👍
💚 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
Apr 3, 2020
* Moves enableDataFeed outside of MLPopover If we accept our dispatch functions, enableDatafeed can be abstracted as a pure function. The version bound to popover's dispatch functions is now named 'handleJobStateChange', as that is the callback it's used for. * Remove unused component state We no longer deal with jobs in our local state; that's the responsibility of the useSiemJobs hook * Prevent user from initiating multiple job installations When attempting to run a job from the ML Popover, if the job needs to first be installed, we set the rest of the jobs to be "loading" while installation is performed. Without this change, if users are fast enough they can potentially trigger multiple rule installations, which is undefined behavior and leads to failures and bad state in our component. * Remove unused import
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
Apr 3, 2020
* Moves enableDataFeed outside of MLPopover If we accept our dispatch functions, enableDatafeed can be abstracted as a pure function. The version bound to popover's dispatch functions is now named 'handleJobStateChange', as that is the callback it's used for. * Remove unused component state We no longer deal with jobs in our local state; that's the responsibility of the useSiemJobs hook * Prevent user from initiating multiple job installations When attempting to run a job from the ML Popover, if the job needs to first be installed, we set the rest of the jobs to be "loading" while installation is performed. Without this change, if users are fast enough they can potentially trigger multiple rule installations, which is undefined behavior and leads to failures and bad state in our component. * Remove unused import
rylnd
added a commit
that referenced
this pull request
Apr 3, 2020
* Moves enableDataFeed outside of MLPopover If we accept our dispatch functions, enableDatafeed can be abstracted as a pure function. The version bound to popover's dispatch functions is now named 'handleJobStateChange', as that is the callback it's used for. * Remove unused component state We no longer deal with jobs in our local state; that's the responsibility of the useSiemJobs hook * Prevent user from initiating multiple job installations When attempting to run a job from the ML Popover, if the job needs to first be installed, we set the rest of the jobs to be "loading" while installation is performed. Without this change, if users are fast enough they can potentially trigger multiple rule installations, which is undefined behavior and leads to failures and bad state in our component. * Remove unused import
rylnd
added a commit
that referenced
this pull request
Apr 3, 2020
* Moves enableDataFeed outside of MLPopover If we accept our dispatch functions, enableDatafeed can be abstracted as a pure function. The version bound to popover's dispatch functions is now named 'handleJobStateChange', as that is the callback it's used for. * Remove unused component state We no longer deal with jobs in our local state; that's the responsibility of the useSiemJobs hook * Prevent user from initiating multiple job installations When attempting to run a job from the ML Popover, if the job needs to first be installed, we set the rest of the jobs to be "loading" while installation is performed. Without this change, if users are fast enough they can potentially trigger multiple rule installations, which is undefined behavior and leads to failures and bad state in our component. * Remove unused import
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Apr 6, 2020
…into event-log/query-support * 'event-log/query-support' of github.com:gmmorris/kibana: (41 commits) [jenkins] refer to sizes in most pipeline code (elastic#62082) skip flaky suite (elastic#60470) [Discover] Fix flaky FT in field visualize (elastic#62418) [ML] Data Frame Analytics: Fix feature importance (elastic#61761) [Reporting] Use a shim for server config (elastic#62086) [Reporting] Fix reporting for non-default spaces (elastic#62226) Fix bug that coerced empty scaled float value to 0 (elastic#62251) [SIEM] [Detection Engine] Remove has manage api keys requireme… (elastic#62446) [Maps] Safely handle empty string and invalid strings from EuiColorPicker (elastic#62507) Reporting/bug more blacklisted headers (elastic#62389) [SIEM] Prevent undefined behavior in our ML popover (elastic#62498) [SIEM] [Detection Engine] remove all unknowns from all rules t… (elastic#62327) base changes for active/current node styling (elastic#62007) [kbn/ui-shared-deps] expand and split (elastic#62364) [ML] DF Analytics - ensure destination index pattern created (elastic#62450) Mark rule run as failure if there was an error (elastic#62383) Add docs for metric explorer alerts (elastic#62314) skip flaky suite (elastic#62281) [SIEM][Detection Engine] Fixes export of single rule and the icons fixes flakiness (elastic#62406) ...
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
Addresses #62490 by marking all jobs as "loading" during job installation. An alternate solution would be to disable the switches, but loading accomplishes the same goal (the user cannot interact with the job) while being closer to reality: the job is likely being installed during that period.
A finer-grained approach would be to mark as loading only the jobs in the group currently being installed, but I opted not to implement that now as it would provide little value to the user.
Old behavior is shown on the referenced issue. New behavior:
Checklist
For maintainers