Skip to content
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.

Sign up for GitHub

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

[Controls] Add Expensive Queries Fallback #155082

Merged
merged 3 commits into from
Apr 18, 2023
Merged

[Controls] Add Expensive Queries Fallback #155082

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4886380
Fallback to true when the user does not have permissions to check for…
ThomThomson Apr 17, 2023
70abdf6
Merge remote-tracking branch 'upstream/main' into controls/expensiveQ…
ThomThomson Apr 18, 2023
1844883
remove string true in favour of boolean
ThomThomson Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/plugins/controls/server/options_list/options_list_cluster_settings_route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import { getKbnServerError, reportServerError } from '@kbn/kibana-utils-plugin/server';
import { CoreSetup } from '@kbn/core/server';
import { errors } from '@elastic/elasticsearch';

export const setupOptionsListClusterSettingsRoute = ({ http }: CoreSetup) => {
const router = http.createRouter();
Expand Down Expand Up @@ -39,6 +40,17 @@ export const setupOptionsListClusterSettingsRoute = ({ http }: CoreSetup) => {
},
});
} catch (e) {
if (e instanceof errors.ResponseError && e.body.error.type === 'security_exception') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a warning on the server-side? Is that a pattern that we use?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/**
* in cases where the user does not have the 'monitor' permission this check will fail. In these cases, we will
* fall back to assume that the allowExpensiveQueries setting is on, because it defaults to true.
*/
return response.ok({
body: {
allowExpensiveQueries: true,
},
});
}
const kbnErr = getKbnServerError(e);
return reportServerError(response, kbnErr);
}
Expand Down