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

LDAP resolver connection pool does not have a pool size or pool lifetime #3633

Open
kkalev opened this issue May 22, 2023 · 6 comments
Open
Assignees
Labels
Type: Enhancement Not a complete new functional component/feature but an enhancement of an already existing feature.
Milestone

Comments

@kkalev
Copy link

kkalev commented May 22, 2023

Top-level intent

  • What did you try to achieve?
  • LDAP resolver connection pool should be of a maximum size and close connections after some level of inactivity or total time.

Steps to reproduce

  1. Start privacyIDEA with an LDAP backend
  2. In the admin panel move between the Tokens and Users pages
  3. Count the number of open LDAP connections in the LDAP server. They will be increasing constantly.

Expected outcome

The LDAP resolver connection pool should be of a maximum size and close connections after some level of inactivity or total time

Code Problem

The problem is that the create_connection() method in LdapIdResolver.py calls the ldap3 Connection() class constructor method without setting any values for the pool_size and pool_lifetime parameters which, if not set, take the value of None.

The LDAP resolver should allow the administrator to set these variables through the admin interface (or at least as general parameters in the pi.cfg configuration file).

Thank you for a very helpful product.

@kkalev kkalev added the Type: Possible bug Suspected bug by user label May 22, 2023
@kkalev
Copy link
Author

kkalev commented May 22, 2023

I actually made a mistake on pinpointing where the problem is. The ServerPool works correctly in the LDAPIdResolver and get_persistent_serverpool() is persistent

What really happens is that the _bind() function always finds self.i_am_bound to be false and opens a new LDAP connection. The problem has to do with the fact that the get_resolver_object() in lib/resolver.py does not find a resolver instance in the store.

The reason for that is that get_resolver_object() calls get_request_local_store() which accesses a dictionary local to the current request instead of get_app_local_store().

If we add a few debugging prints in the relevant code we get the following results for every access to the Users page:

privacyidea_1  | IdResolver: get_resolver_object(): resolver_objects not in store
privacyidea_1  | IdResolver: get_resolver_object(): Creating a resolver instance
privacyidea_1  | LDAPIdResolver: getUserList() calling _bind()
privacyidea_1  | LDAPIdResolver: _bind() starting
privacyidea_1  | LDAPIdResolver: _bind() i_am_bound=false
privacyidea_1  | LDAPIdResolver: _bind(): Not self.serverpool, calling get_serverpool_instance()
privacyidea_1  | LDAPIdResolver: Calling get_persistent_serverpool
privacyidea_1  | LDAPIdResolver: Return existing ServerPool
privacyidea_1  | LDAPIdResolver: _bind() calling create_connection()

Importing get_app_local_store() in lib/resolver.py and using it instead of get_request_local_store() seems to fix things as expected. The LDAP connections remain constant instead of increasing with every new access to the Users page and the LDAP access log shows all LDAP operations going through the same connection.

@kkalev
Copy link
Author

kkalev commented May 23, 2023

Just to add a few actual statistics to illustrate the problem and the fix.

Using get_request_local_store()

  • Disable caching in the LDAP resolver
  • Create an SPASS token
  • uwsgi has 8 processes and 2 threads per process
  • Run ab with 80 total requests and 8 concurrent requests
  • This results in 80 LDAP connections
ldapsearch on cn=monitor for monitorConnectionRead
[...]
monitorConnectionRead: 8
monitorConnectionRead: 8
monitorConnectionRead: 8
monitorConnectionRead: 8
monitorConnectionRead: 8
monitorConnectionRead: 8
monitorConnectionRead: 8

Using get_app_local_store()

  • Run the same becnhmark multiple times
  • Results in 8 LDAP connections (one per process) with a fairly even distribution of reads:
ldapsearch on cn=monitor for monitorConnectionRead
monitorConnectionRead: 98
monitorConnectionRead: 273
monitorConnectionRead: 393
monitorConnectionRead: 63
monitorConnectionRead: 53
monitorConnectionRead: 398
monitorConnectionRead: 353
monitorConnectionRead: 439

@cornelinux cornelinux added Type: Enhancement Not a complete new functional component/feature but an enhancement of an already existing feature. and removed Type: Possible bug Suspected bug by user labels May 23, 2023
@kkalev
Copy link
Author

kkalev commented May 23, 2023

Fixing the Connection Pool actually exposed another problem: The LDAPIdResolver did not handle lost LDAP connections. Yet since each LDAP connection was bound to a request (and not a process) this issue did not present itself as a serious problem.

With our fix, if we tried restarting the LDAP server, privacyIDEA permanently lost its LDAP connections.

The fix for that is to change the ldap3 client_strategy in create_connection() of LDAPIdResolver from SYNC to RESTARTABLE

@cornelinux
Copy link
Member

@kkalev Are you using nginx or apache?

@cornelinux
Copy link
Member

Evaluate if this is easily possible with ldap3.

@cornelinux cornelinux added this to the 3.10 milestone Sep 19, 2023
@nilsbehlen nilsbehlen assigned plettich and unassigned jona-samuel Apr 29, 2024
@plettich
Copy link
Member

plettich commented May 8, 2024

So i took a closer look at this and we don't want the resolver object in the app_local_store since changes to the resolver won't be available until the server process restarts. We would need some kind of invalidation to fix this.
However there is the Per-process server pool setting in the LDAP resolver config which persists the server pool in the app_local_store.
I have to check if this would fix this issue.

@nilsbehlen nilsbehlen modified the milestones: 3.10, 3.10-backlog May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Not a complete new functional component/feature but an enhancement of an already existing feature.
Projects
None yet
Development

No branches or pull requests

5 participants