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

[Roles] Use Query Roles API for Role Management grid screen #194630

Open
wants to merge 52 commits into
base: main
Choose a base branch
from

Conversation

SiddharthMantri
Copy link
Contributor

@SiddharthMantri SiddharthMantri commented Oct 1, 2024

Closes #186266

Release notes

Enhanced Role management to manage larger number of roles by adding server side filtering, pagination and querying.

Summary

  • Replaced the usage of Get Roles API with Query Role API
  • Added server side pagination and filtering with a maximum limit of 10000 keys (default for max results on index). Added new label to indicate that we show only 10k results.
  • Search box replicates client side implementation by only filtering on Role names.

Run locally

Start ES with the JVM option to enable this feature:

ES_JAVA_OPTS="-Des.queryable_built_in_roles_enabled=true" yarn es snapshot --license=trial

Start Kibana normally

yarn start --no-base-path

Navigate to Stack Management > Roles and verify the same behavior as the screen recording below

Screen recording

Screen.Recording.2024-12-23.at.19.46.42.mov

Technical notes

  • Client side EuiInMemory table has been replaced by EuiSearchBar, EuiBasicTable and Filters
  • One new Kibana endpoint added
    • roles/_query
    • Replicates existing get_role endpoint by being public and added to Open API spec
  • Extra logic to handle previously UI only filter to show/hide reserved roles
    • Parse the query to construct the correct DSL if the filter is present
  • Update Get All Roles by Space internal API to use the Query Role and filter by space id using query DSL.

Checklist

Delete any items that are not applicable to this PR.

@SiddharthMantri
Copy link
Contributor Author

@elasticmachine merge upstream

@SiddharthMantri SiddharthMantri marked this pull request as ready for review December 23, 2024 10:27
@SiddharthMantri SiddharthMantri requested a review from a team as a code owner December 23, 2024 10:27
@SiddharthMantri
Copy link
Contributor Author

@elasticmachine merge upstream

elasticmachine and others added 5 commits December 23, 2024 10:27
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --include-path /api/dashboards --update'
@SiddharthMantri SiddharthMantri added release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Users/Roles/API Keys backport:skip This commit does not require backporting v9.0.0 backport:version Backport to applied version labels v8.18.0 labels Dec 23, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@kc13greiner kc13greiner self-requested a review January 2, 2025 20:43
logger,
buildFlavor,
}: RouteDefinitionParams) {
router.post(
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Should this be switched to use the versioned router? I see many of the other routes in this directory use it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, good point! What makes us choose versioned router vs regular router when building these routes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking for guidance. Ill have some feedback soon

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so I have the following: For public routes, we will always want to use the versioned router. Otherwise, it's still strongly preferred, but not mandatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thank you! Updated to versioned router in 989120f

@SiddharthMantri SiddharthMantri force-pushed the roles-management-use-query-roles branch from 7b84a2b to 1c542ce Compare January 10, 2025 14:15
@SiddharthMantri
Copy link
Contributor Author

@elasticmachine merge upstream

…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --include-path /api/dashboards --update'
@kc13greiner
Copy link
Contributor

Last observations/questions for my final review:

  1. Case sensitivity
    I am not sure if this is intended or just how the ES queries work, but I just wanted to raise it.
Screenshot 2025-01-14 at 11 27 11 PM Screenshot 2025-01-14 at 11 27 03 PM
  1. Status not sorting reserved vs non-reserved. Maybe this is intentional?
Screenshot 2025-01-14 at 11 26 19 PM Screenshot 2025-01-14 at 11 26 24 PM Screenshot 2025-01-14 at 11 26 32 PM

@SiddharthMantri
Copy link
Contributor Author

SiddharthMantri commented Jan 15, 2025

Thank you for the review @kc13greiner! Answered the questions:

  1. Case sensitivity
    I am not sure if this is intended or just how the ES queries work, but I just wanted to raise it.

Since we're using the wildcard query on the name field, i think it defaults to case sensitivity. Happy to change it to Case insensitive search, what do you think?

  1. Status not sorting reserved vs non-reserved. Maybe this is intentional?

I believe the field is not sortable on the API. I've changed the column config in 31cf053

@kc13greiner
Copy link
Contributor

Thank you for the review @kc13greiner! Answered the questions:

  1. Case sensitivity
    I am not sure if this is intended or just how the ES queries work, but I just wanted to raise it.

Since we're using the wildcard query on the name field, i think it defaults to case sensitivity. Happy to change it to Case insensitive search, what do you think?

This could be a good question for product @bitzandeb !

  1. Status not sorting reserved vs non-reserved. Maybe this is intentional?

I believe the field is not sortable on the API. I've changed the column config in 31cf053

++ makes sense - thank you!

Copy link
Contributor

@kc13greiner kc13greiner left a comment

Choose a reason for hiding this comment

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

LGTM - works great, very excited for this 🚀

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/security-plugin-types-common 66 71 +5

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
security 565.2KB 565.9KB +771.0B

Page load bundle

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

id before after diff
security 62.8KB 62.9KB +123.0B
Unknown metric groups

API count

id before after diff
@kbn/security-plugin-types-common 127 132 +5

History

cc @SiddharthMantri

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels Feature:Users/Roles/API Keys release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Role Management screens to use Query Roles API
4 participants