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

Expand Group providers to allow for paginated lookup of subgroups #22372

Closed
Tracked by #22261
alice-wondered opened this issue Aug 10, 2023 · 2 comments · Fixed by #22700
Closed
Tracked by #22261

Expand Group providers to allow for paginated lookup of subgroups #22372

alice-wondered opened this issue Aug 10, 2023 · 2 comments · Fixed by #22700
Labels
area/storage Indicates an issue that touches storage (change in data layout or data manipulation) kind/enhancement Categorizes a PR related to an enhancement release-notes
Milestone

Comments

@alice-wondered
Copy link
Contributor

Description

At present, subgroup lookups are not done directly through the storage layer by utilizing the GroupLookupProvider, GroupProvider, and corresponding implementations (e.g. JpaRealmProvider) and are instead supported on the GroupModel object only.

By expanding the storage layer's ability to interact with, lookup, and paginate results for subgroups, the door is opened to make tangible improvements to performance on the backend at scale. Notably, the ability to limit the amount of subgroups that are returned from the database and then loaded into memory

This eliminates the need to discard groups from the stream later as seen here:

public final Stream<GroupRepresentation> subgroups(@QueryParam("id") final String groupId, @QueryParam("search")
@DefaultValue("") final String search, @QueryParam("first") @DefaultValue("0") int first, @QueryParam("max") @DefaultValue("10") int max) {
GroupPermissionEvaluator groupsEvaluator = auth.groups();
groupsEvaluator.requireList();
GroupModel group = realm.getGroupById(groupId);
if (group == null) {
return Stream.empty();
}
return group.getSubGroupsStream().filter(g -> g.getName().contains(search))
.map(g -> GroupUtils.toGroupHierarchy(groupsEvaluator, g, search, false, true)).skip(first).limit(max);
}

as well as the amount of subgroups that are then consequently returned to the client at once.

Discussion

No response

Motivation

Red Hat is running into scenarios where groups operating at scale is becoming necessary (future multi-tenancy efforts being one scenario). Additionally, issues have been popping up from time to time where scalability is becoming an issue. Groups is one place that we have identified but could be used in the future as a template for how to ensure other keycloak resources scale well

Details

Some work has been done here but needs final touches, squashing, and a PR

@alice-wondered alice-wondered added kind/enhancement Categorizes a PR related to an enhancement status/triage labels Aug 10, 2023
alice-wondered added a commit to alice-wondered/keycloak that referenced this issue Aug 10, 2023
@ahus1 ahus1 added the area/storage Indicates an issue that touches storage (change in data layout or data manipulation) label Aug 10, 2023
@ghost ghost added the team/store label Aug 10, 2023
@alice-wondered
Copy link
Contributor Author

As work went along on this issue it made sense to eliminate the loading of thousands of groups into memory (via streams) and to allow for the pagination and searching to be done on the database side rather than as a stream operation.

Additionally, the loading of group hierarchies assumes previously assumed that all subgroups would be loaded in this way so had to be modified to load only relevant sections pertaining to the groups given in the result.

Finally, the UI needed to be changed to operate in a way that fetches subgroups as needed and changes to the REST API were made to reflect that pattern as well

@eatikrh
Copy link
Contributor

eatikrh commented Aug 25, 2023

We have also been working on the UI changes that go along with these changes. Having the group providers work separated from the UI work procedurally makes sense. But without the UI changes Groups UI will break. Would you agree?
Loading the Groups tree in its entirety is due to the current UI expects it to be loaded that way. This has been the source of the performance problems we have come across with the Groups pages.
Expanding the group providers will be necessary, but as long as we don't change the UI, the performance problem will remain because UI will still ask for the whole group tree to be loaded and we would have to keep the functionality that loads the whole group tree in t

@ghost ghost removed the status/triage label Oct 26, 2023
mposolda pushed a commit that referenced this issue Oct 26, 2023
closes #22372 


Co-authored-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Co-authored-by: Pedro Igor <pigor.craveiro@gmail.com>
Co-authored-by: Michal Hajas <mhajas@redhat.com>
@mposolda mposolda added this to the 23.0.0 milestone Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/storage Indicates an issue that touches storage (change in data layout or data manipulation) kind/enhancement Categorizes a PR related to an enhancement release-notes
Projects
None yet
4 participants