[Controls] Add Expensive Queries Fallback #155082
Conversation
… expensive queries
ThomThomson
added
Team:Presentation
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
src/plugins/controls/server/options_list/options_list_cluster_settings_route.ts
Outdated
Show resolved
Hide resolved
| @@ -39,6 +40,17 @@ export const setupOptionsListClusterSettingsRoute = ({ http }: CoreSetup) => { | |||
| }, | |||
| }); | |||
| } catch (e) { | |||
| if (e instanceof errors.ResponseError && e.body.error.type === 'security_exception') { | |||
There was a problem hiding this comment.
Should we also show a toast, notifying users that they are missing permissions? We do something like this in maps https://github.com/elastic/kibana/blob/main/x-pack/plugins/maps/public/classes/sources/es_search_source/util/load_index_settings.ts#L46 when fetching index.max_result_window, where we use a default but notify users when there is a permission problem.
There was a problem hiding this comment.
I'm honestly not sure about this. Yes there is a permissions problem, which should probably bubble up - but we don't really want to tie the usage of Controls to the monitor privilege so closely. If there was a toast, it would basically tell the end-user - not the author in this case because they usually have the right permissions - that in order to use controls they need the monitor privilege on the index they use.
If there was a way to warn only the author when they were setting up the roles that would be okay with me, but I don't think we have that kind of mechanism.
There was a problem hiding this comment.
Maybe a warning on the server-side? Is that a pattern that we use?
There was a problem hiding this comment.
Maybe a warning on the server-side? Is that a pattern that we use?
not sure. For maps, we wanted users to see the warning since not being able to read the value may cause problems
There was a problem hiding this comment.
I think I'm going to merge this as is, but if it does cause that problem we're aware of - where allow_expensive_queries is off and the user doesn't have permissions to check - we can revisit this conversation and show a toast, or do some other less intrusive warning.
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
## Summary If the user has no permission to check for the value of `allow_expensive_queries`, it will now default to true instead of false. (cherry picked from commit 66f68d4)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
# Backport This will backport the following commits from `main` to `8.7`: - [[Controls] Add Expensive Queries Fallback (#155082)](#155082) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Devon Thomson","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-04-18T17:04:26Z","message":"[Controls] Add Expensive Queries Fallback (#155082)\n\n## Summary\r\nIf the user has no permission to check for the value of `allow_expensive_queries`, it will now default to true instead of false.","sha":"66f68d4123a333f4651662f70eb2465ade7e9081","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Feature:Input Control","Team:Presentation","loe:days","impact:high","backport:prev-minor","v8.8.0"],"number":155082,"url":"https://github.com/elastic/kibana/pull/155082","mergeCommit":{"message":"[Controls] Add Expensive Queries Fallback (#155082)\n\n## Summary\r\nIf the user has no permission to check for the value of `allow_expensive_queries`, it will now default to true instead of false.","sha":"66f68d4123a333f4651662f70eb2465ade7e9081"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/155082","number":155082,"mergeCommit":{"message":"[Controls] Add Expensive Queries Fallback (#155082)\n\n## Summary\r\nIf the user has no permission to check for the value of `allow_expensive_queries`, it will now default to true instead of false.","sha":"66f68d4123a333f4651662f70eb2465ade7e9081"}}]}] BACKPORT--> Co-authored-by: Devon Thomson <[email protected]>
* 8.7: (93 commits) [8.7] [Controls] Use EUI Selectable for Field search (#151231) (#155454) [8.7] [Synthetics] Fix performance breakdown link from error details page (#155393) (#155427) [8.7] [DOCS] Remove or move book-scoped attributes (#155210) (#155426) [8.7] [Synthetics] add default email recovery message (#154862) (#155418) [8.7] [Uptime] Add both both ip filters for view host in uptime location for host and monitor (#155382) (#155399) [8.7] Setup Node.js environment before instrumenting Kibana with APM. (#155063) (#155300) [8.7] [Discover] Address react warnings for legacy table (#154579) (#155345) [8.7] [Fleet] Fix logs useless rerender (#155305) (#155310) [8.7] [kbn-failed-test-reporter-cli] truncate report message to fix github api call failure (#155141) (#155286) [8.7][APM] Fleet migration support for bundled APM package (#153159) (#155281) [8.7] [Enterprise Search] Fix Connector scheduling show week information correctly (#155191) (#155227) [8.7] [Synthetics] Fix pending count in case of location filtering (#155200) (#155225) [8.7] [Controls] Add Expensive Queries Fallback (#155082) (#155189) [8.7] [data view field editor] Runtime field code editor - move state out of controller (#155107) (#155150) [8.7] [FullStory] Update snippet (#153570) (#155138) [8.7] [Security Solution][Exceptions] - Fix exception operator logic when mapping conflict (#155071) (#155094) [DOCS] Adds 8.7.1 release notes (#154844) [8.7] Sync bundled packages with Package Storage (#155042) [APM] plugin description (#154811) Update api.asciidoc (#155021) ...
Summary
Fixes #155078.
In main right now if you set up a user with
readonly access to an index controls will default to using the non-expensive queries, and some features will be missing. This is because without themonitorprivilege, it is impossible to check whether expensive queries are on or off.Because the default setting for
allow_expensive_queriesistrue, we should default totruefor cases where a security exception prevents us from checking the value of the setting.How to test
readpermission for the logs web traffic index.Open question
Should we change this default behaviour?
allow_expensive_queriesoff, and the user cannot check the value of the setting due to a missing permission, the Controls will throw errors. This is similar to the way the table list view and solutions work - they don't even check for the value of the setting.monitorpermission.