Skip to content

add ldappool module #582

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
Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

add ldappool module #582

wants to merge 9 commits into from

Conversation

michaelalang
Copy link

I would like to add ldappooling capability to the library.

LDAP Pooling example

entries as dict

from ldappool import ConnectionPool
pool = ConnectionPool(
        params={"keep": True, "autoBind": True, "retries": 2},
        max=5)
pool.set_uri("ldaps://ldap.example.com:636/dc=example,dc=com?uid,mail?sub?(|(uid=test)(mail=test@example.com))")
pool.set_credentials("binddn", "bindpw")
with pool.get() as conn:
        for entry in conn.search_s(pool.basedn,
                                   pool.scope,
                                   pool.filter,
                                   pool.attributes):
                print(f"{entry[0]}: {entry[1].get('uid')} {entry[1].get('mail')}")
                for member in entry[1].get("memberOf", []):
                        print(member)

changing the connection or credentials for the pool

from ldappool import ConnectionPool
from ldappool import e2c
pool = ConnectionPool(                                                                                                                                           40,0-1        13%
        params={"keep": True, "autoBind": True, "retries": 2},
        max=5)
pool.set_uri("ldaps://ldap.example.com:636/dc=example,dc=com?uid,mail?sub?(|(uid=test)(mail=test@example.com))")
pool.set_credentials("binddn", "bindpw")
with pool.get() as conn:
        for entry in map(e2c, conn.search_s(pool.basedn,
                                            pool.scope,
                                            pool.filter,
                                            pool.attributes)):
                print(f"{entry.dn}: {entry.uid} {entry.mail}")

pool.set_credentials(entry.dn, "changeme")
with pool.get() as conn:
        for entry in map(e2c, conn.search_s(pool.basedn,
                                            pool.scope,
                                            pool.filter,
                                            pool.attributes)):
                print(f"{entry.dn}: {entry.uid} {entry.mail}")

@michaelalang
Copy link
Author

@droideck Hi sorry to bother you. The tests fail with c90-py37: skipped because could not find python interpreter with spec(s): py37

I double checked and I am not referencing any shebang interpreter so I wonder what can I do to get the unittests run, fix them until everything show's successs to get the PR finally discussed and worth being considered ?

All the best and kind regards
Michi

@droideck
Copy link
Contributor

droideck commented Jan 29, 2025 •
edited
Loading

@droideck Hi sorry to bother you. The tests fail with c90-py37: skipped because could not find python interpreter with spec(s): py37

I double checked and I am not referencing any shebang interpreter so I wonder what can I do to get the unittests run, fix them until everything show's successs to get the PR finally discussed and worth being considered ?

All the best and kind regards Michi

Hi!
I think this PR should fix it - #583

Regarding this PR, could you share a bit about the motivation behind adding this new ldappool module? I’m curious about what problem precisely the mechanism for pooling and reusing LDAP connections solves for you (what sparked the need for it)?
Thanks!

@michaelalang
Copy link
Author

michaelalang commented Jan 29, 2025 •
edited
Loading

@droideck Hi sorry to bother you. The tests fail with c90-py37: skipped because could not find python interpreter with spec(s): py37
I double checked and I am not referencing any shebang interpreter so I wonder what can I do to get the unittests run, fix them until everything show's successs to get the PR finally discussed and worth being considered ?
All the best and kind regards Michi

Hi! I think this PR should fix it - #583

Regarding this PR, could you share a bit about the motivation behind adding this new ldappool module? I’m curious about what problem precisely the mechanism for pooling and reusing LDAP connections solves for you (what sparked the need for it)? Thanks!

Sure, I did want to put up that for the discussion but I am sure it's also good to have it in here.

The motivation for the module is coming from an application which utilizes the LDAP connections in an context manner when objects are created.
Those objects spin of through a Flask frontend for each connection coming in and loose context between various steps in the workflow.

With adding an connection pool, the behavior can be changed in a manner of avoiding cost-intensive LDAP(s) connection being created over and over and with the builtin lock-authenticate-unlock method the waste of adding yet another connection to verify user authentication is granted.
I am more a fan of the bind dn directly instead of using a search DN to fetch the user bind dn and authenticate with that on another connection, but that binddn game is a holy-cow and I don't want to break off a discussion where both sides are right (for their use-case).

The ldappool module shall abstract the burdens of connection handling, prewarming, ondemand creation as well as basic authentication of users when search by a generic binddn.

For some numbers, the application I am talking about does provide a docker v2 API and calling 100 failing docker auth requests ends up with ~900+ LDAP connections being created and dropped. The module shows that this can also be done with a fixed set of connections avoiding session/connection limits and improving the duration for such events (not talking about the fail delay response for secrutiy obfuscation) from tcp/tls connection establish point of view.

I also decided to not use threading Queues as they do not give the possibility to choose a random available connection for asubsequent search or an authentication request.

Hope it helps solving other similar issues in the python-ldap world
just in case someone is interested, that's the docker v2 API discussion I started off quay/quay#3539

@droideck droideck requested a review from mistotebe January 30, 2025 01:14
@droideck
Copy link
Contributor

@michaelalang, thanks for the great write-up! It makes sense, and I think it can be useful for many cases!
I'll need to review the code a bit closer, though.

@mistotebe could you please check the idea too, please? I think you worked on connection wrappers at some point (not exactly like this one, but still, we need your point of view here...)

@mistotebe
Copy link
Contributor

From my experience with the OpenLDAP load balancer interacting with various pooling implementations, the following are useful when you need a connection pool:

  • a source of connections that a user can safely send a bind() on (Bind requests destroy most of a session's state), not just a source of pre-bound connections
  • something that gives you a connection for as long as you need to let others know when your requests might be correlated, not for one operation only (Python context managers are a good way to do this, I wouldn't react to a del however, the caller should use a with statement or a try:+finally: to make sure they don't leak it, it's not the pool's job to babysit them)
  • when a connection is returned, the pool checks that it hasn't been closed/Unbound (in async contexts you can also monitor idle connection status with the event loop's select())
  • a minimum/maximum number of open connections for efficiency, opening one on request if none are available but connection limit hasn't been reached yet
  • a way to borrow a connection exclusively but maybe (but that's quite advanced use) also in a shared way, promising to use the asynchronous methods only

Also think the implementation could be greatly simplified if the Connection class inherited from LDAPObject (or #464's Connection).

@michaelalang
Copy link
Author

He @mistotebe

thanks for your input and support, in regards to

Also think the implementation could be greatly simplified if the Connection class inherited from LDAPObject (or #464's Connection).

I am not trying to add yet another LDAPObject. The methods implemented are for the specific use case of utilizing one connection with a search principal and and authentication principal. All the other methods should only relate to the Pool.

The other points you mentioned are a generic concept description and thanks for that (really love the definitions).
I haven't check on any asynchronous code parts as from my perspective (and as you stated with bind on shared connections) the connection cannot live outside of a context (unless people can code that properly on their own).
That said and meaning, if one enters the context implemented and calls async on the connection and leaves the context it should not stay in the state as the Pool (at the moment) would not return you the inUsed connection anymore later for another context (only if returned properly).

I also wouldn't see how this should be working out as a pool retuned connection with server controls set on paging will not reset if someone just does a simple_s on it ...

Anyways, thanks again for the great input. From the code I submitted, what exactly would you change in regards to the mentioned points ? (@droideck I think that question is for you unless @mistotebe wants to answer too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mistotebe mistotebe Awaiting requested review from mistotebe

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michaelalang @droideck @mistotebe