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

Add Paging Interface for LDAP Connection #17640

Merged
merged 4 commits into from Oct 26, 2022
Merged

Conversation

ltcarbonell
Copy link
Contributor

@ltcarbonell ltcarbonell commented Oct 24, 2022

Vault was returning the following error when trying to filter on a large number of groups

* LDAP search failed: LDAP Result Code 4 "Size Limit Exceeded": 

This change makes use of paginated ldap searching, through a new connection interface as to maintain backwards compatibility for the SDK.

Fixes #7702

Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

LGTM!

Curious if you were able to test this out with group count > the MaxInt32 page size? So many groups 😅

sdk/helper/ldaputil/client.go Outdated Show resolved Hide resolved
@ltcarbonell
Copy link
Contributor Author

LGTM!

Curious if you were able to test this out with group count > the MaxInt32 page size? So many groups 😅

@austingebauer unfortunately no, my machine wasn't a fan of that many groups 😆. I did set it arbitrarily low to make sure that it would work if groups were larger than the page size.

@ltcarbonell ltcarbonell force-pushed the ldap-paging-search branch 2 times, most recently from b1fdf6a to dbc068a Compare October 25, 2022 17:15
@jasonodonnell jasonodonnell enabled auto-merge (squash) October 25, 2022 18:18
ltcarbonell and others added 3 commits October 25, 2022 14:01
This appears to be due to a CI change that resulted in different user
IDs between the host and the container image.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LDAP search failed: Result Code 4 "Size Limit Exceeded"
5 participants