Skip to content

Fix test LDAP connection with multiple ldap connection urls (#31267) #31289

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

Jump to bottom
Merged
ahus1 merged 2 commits into from
Jul 16, 2024
Merged

Fix test LDAP connection with multiple ldap connection urls (#31267) #31289

ahus1 merged 2 commits into from
Jul 16, 2024

Conversation

@thomasdarimont
Copy link
Contributor

@thomasdarimont thomasdarimont commented Jul 15, 2024

Previously, the given connection string was check with URI.create(..) which failed when multiple space separated LDAP URLs were given.

Fixes #31267

@thomasdarimont thomasdarimont requested a review from a team as a code owner July 15, 2024 10:01
jonkoops
jonkoops previously approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@jonkoops jonkoops left a comment

Choose a reason for hiding this comment

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

LGTM, just some optional feedback

@keycloak-github-bot keycloak-github-bot bot mentioned this pull request Jul 15, 2024
Closed
@thomasdarimont thomasdarimont force-pushed the issue/gh-31267-fix-ldap-connection-url-test-multiple-urls branch from f0281a8 to 1343264 Compare July 15, 2024 10:47
@thomasdarimont
Copy link
Contributor Author

thomasdarimont commented Jul 15, 2024 •
edited
Loading

Thanks for the review @jonkoops! I added your suggestions!
With the changes, we can now test ldap connections with space separated LDAP server urls.
image

jonkoops
jonkoops previously approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@jonkoops jonkoops left a comment

Choose a reason for hiding this comment

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

LGTM. @thomasdarimont would it be possible to write a test to cover this? Looks like we already have a UserFederationLdapConnectionTest test that might bbe extendable.

@thomasdarimont
Copy link
Contributor Author

thomasdarimont commented Jul 15, 2024 •
edited
Loading

For some reason the existing tests in org.keycloak.testsuite.admin.UserFederationLdapConnectionTest#testLdapConnectionMoreServers does not trigger the issue but I can clearly reproduce it through the UI. I'll look into this...

@keycloak-github-bot keycloak-github-bot bot mentioned this pull request Jul 15, 2024
Closed
@thomasdarimont thomasdarimont force-pushed the issue/gh-31267-fix-ldap-connection-url-test-multiple-urls branch from 1343264 to 7e64f38 Compare July 15, 2024 11:47
@thomasdarimont thomasdarimont requested review from a team as code owners July 15, 2024 11:47
@thomasdarimont
Copy link
Contributor Author

thomasdarimont commented Jul 15, 2024 •
edited
Loading

@jonkoops added additional test. The issue was that the problem only occurred when the testLDAPConnection was used in combination with a stored LDAP federation provider model.

@ahus1 ahus1 requested a review from rmartinc July 15, 2024 16:22
@ahus1
Copy link
Contributor

ahus1 commented Jul 15, 2024

@rmartinc - could you please review as well? Thanks!

rmartinc
rmartinc suggested changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@rmartinc rmartinc left a comment

Choose a reason for hiding this comment

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

Just a minor change, I think you just need to return false and not throw an exception following the same idea. And thanks for the PR!

@thomasdarimont
Copy link
Contributor Author

thomasdarimont commented Jul 15, 2024

thanks for the review @rmartinc ! I adapted the code accordingly.

@thomasdarimont
Fix test LDAP connection with multiple ldap connection urls (keycloak…
dd7781a 
…#31267)

Previously, the given connection string was check with URI.create(..) which
failed when multiple space separated LDAP URLs were given.

Fixes keycloak#31267

Signed-off-by: Thomas Darimont <[email protected]>
@thomasdarimont
Return false instead if throwing a ModelException
5045455 
Signed-off-by: Thomas Darimont <[email protected]>
@thomasdarimont thomasdarimont force-pushed the issue/gh-31267-fix-ldap-connection-url-test-multiple-urls branch from 867647f to 5045455 Compare July 15, 2024 20:31
rmartinc
rmartinc approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@rmartinc rmartinc left a comment

Choose a reason for hiding this comment

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

Thanks @thomasdarimont!

ahus1
ahus1 approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

Approving based on @rmartinc's review.

@ahus1 ahus1 merged commit 2140e57 into keycloak:main Jul 16, 2024
This was referenced Jul 16, 2024
Merged
Merged
Merged
@nodje
Copy link

nodje commented Sep 22, 2025

Maybe not the right place, but:

there should be a minimal documentation on this.
I couldn't find anything in the doc about it.

I was actually configuring multiple LDAP provider to implement failover.
Since stumbling upon this issue, I realize I have to use one unique provider and fill in multiple LDAP URLs.
Hovering over ? icon on Connection URL disaplys only Connection URL to your LDAP server even in KC 26.3.4

Only https://www.keycloak.org/docs/latest/server_admin/index.html#dealing-with-provider-failures makes clear that mulitple provider conf isn't going to failover, but doesn't say multiple URL for a provider is possible.

@nodje
Copy link

nodje commented Sep 22, 2025

Food for thoughts:

This is how it's implemented in LDAP itself:

          **uri "ldap://host/ ldap://backup-host/"**

          The URI list is space- or comma-separated.  Whenever the server that responds is not the first one in the list, the list is rearranged and the responsive server  is  moved to the head, so that it will be first contacted the next time a connection needs to be created.

That seems clever to me, is it how it works in KC when multiple URLs are provided?

@jonkoops
Copy link
Contributor

jonkoops commented Sep 22, 2025

@nodje comments left here are likely to fall on deaf ears, I would recommend logging a new issue referencing this one.

@nodje
Copy link

nodje commented Sep 22, 2025

thx, will do

@nodje nodje mentioned this pull request Sep 23, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahus1 ahus1 ahus1 approved these changes

+2 more reviewers

@rmartinc rmartinc rmartinc approved these changes

@jonkoops jonkoops jonkoops left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test team/cloud-native team/core-clients team/core-iam

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

multiple ldap url's not working on one realm

5 participants

@thomasdarimont @ahus1 @nodje @jonkoops @rmartinc