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.

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] Use Internal User to Get Value of allow_expensive_queries #155430

Merged

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Apr 20, 2023

Summary

This PR changes the way we access allow_expensive_queries to use the internal user. The internal user will have monitor permissions in almost every case, so this PR also removes the fallback added in #155082.

Additionally, because we're now using the internal user, this PR renames getClusterSettings endpoint for Controls into a getExpensiveQueriesSetting endpoint to be more specific.

I tested this PR by:

  • Creating a new role that has no monitor privilege and assigning a user to that role
  • Logging in as that user and ensuring that the controls are using expensive queries
  • Logging back in as the elastic user and setting
    PUT _cluster/settings
    {
      "transient": {
        "search.allow_expensive_queries": "false"
      }
    }
    
  • Logging in as the user without monitor privilege again and ensuring that the Controls are now not using the expensive queries.

@ThomThomson ThomThomson added release_note:fix Feature:Input Control Input controls visualization Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:small Small Level of Effort impact:critical This issue should be addressed immediately due to a critical level of impact on the product. labels Apr 20, 2023
@ThomThomson ThomThomson marked this pull request as ready for review April 20, 2023 17:57
@ThomThomson ThomThomson requested a review from a team as a code owner April 20, 2023 17:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@nreese nreese requested a review from a team April 20, 2023 18:03
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
controls 32.8KB 32.8KB +8.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 394 397 +3

Total ESLint disabled count

id before after diff
securitySolution 474 477 +3

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@jeramysoucy jeramysoucy left a comment

Choose a reason for hiding this comment

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

Looks good from our perspective.

@ThomThomson ThomThomson requested a review from nreese April 21, 2023 15:52
@nreese
Copy link
Contributor

nreese commented Apr 21, 2023

Would it make sense to add an integration test to ensure kibana user has continued permission access? I could see kibana user roles getting changed and this breaking but no one noticing.

@ThomThomson
Copy link
Contributor Author

ThomThomson commented Apr 21, 2023

Good question! I think we will be okay without functional tests specifically designed to catch changes in permissions on the Kibana Internal User.

One reason for this is that we're now using the internal user for all scenarios, not just for some cases - or certain roles etc.
This means that if the kibana internal user no longer had monitor permissions, we would see that reflected as a failing test in CI because:

  • Many of our tests require allow_expensive_queries to be on.
  • If the internal user has had the monitor permission removed the server would throw a permissions error in this endpoint.
  • The client side catches that error and returns false.
  • All of our tests that require expensive_queries will begin to fail because of missing functionality.

Another reason is because we are far from the only application to require the internal Kibana user to have this permission - from @legrego on slack:

At this point Kibana requires that it’s internal user account be granted the kibana_system role. It’s safe to assume this will always be the case. If customers are running an unsupported setup, then we can help them get the right role assigned. These privileges change often enough that other parts of Kibana would also break if users had this misconfigured.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM
code review only

@ThomThomson ThomThomson merged commit 81b003d into elastic:main Apr 21, 2023
@ThomThomson ThomThomson added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Apr 21, 2023
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 21, 2023
elastic#155430)

changes the way we access `allow_expensive_queries` to use the internal user.

(cherry picked from commit 81b003d)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Apr 21, 2023
…ueries` (#155430) (#155548)

# Backport

This will backport the following commits from `main` to `8.7`:
- [[Controls] Use Internal User to Get Value of
`allow_expensive_queries`
(#155430)](#155430)

<!--- 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-21T18:19:53Z","message":"[Controls]
Use Internal User to Get Value of `allow_expensive_queries`
(#155430)\n\nchanges the way we access `allow_expensive_queries` to use
the internal
user.","sha":"81b003d431bbd0d5d0bc3fdf507a99e50133a386","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Feature:Input
Control","Team:Presentation","loe:hours","impact:critical","backport:prev-minor","v8.8.0"],"number":155430,"url":"https://github.com/elastic/kibana/pull/155430","mergeCommit":{"message":"[Controls]
Use Internal User to Get Value of `allow_expensive_queries`
(#155430)\n\nchanges the way we access `allow_expensive_queries` to use
the internal
user.","sha":"81b003d431bbd0d5d0bc3fdf507a99e50133a386"}},"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/155430","number":155430,"mergeCommit":{"message":"[Controls]
Use Internal User to Get Value of `allow_expensive_queries`
(#155430)\n\nchanges the way we access `allow_expensive_queries` to use
the internal
user.","sha":"81b003d431bbd0d5d0bc3fdf507a99e50133a386"}}]}] BACKPORT-->

Co-authored-by: Devon Thomson <[email protected]>
@Heenawter Heenawter linked an issue Apr 24, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Input Control Input controls visualization impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:small Small Level of Effort release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.7.1 v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Controls] Get Expensive Queries Setting With Internal User
6 participants