feat(ldap): [OCISDEV-1031] add opt-in LDAP connection pool - #680
Merged
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
LukasHirt
added a commit
that referenced
this pull request
Jul 31, 2026
LukasHirt
changed the title
2 tasks
deyankiteworks
previously approved these changes
Jul 31, 2026
* feat: [OCISDEV-1031] add opt-in LDAP connection pool Add GetLDAPClientWithPool, a bounded pool of authenticated LDAP connections, as a drop-in alternative to the existing single, long-lived reconnecting connection used by the auth/user/group LDAP managers. Connections are dialed and bound lazily on checkout, unhealthy connections are discarded and lazily re-dialed rather than eagerly reconnected, and checkout blocks with a configurable timeout once the pool is exhausted. Disabled by default; enable per backend via pool_enabled (plus pool_size and pool_checkout_timeout) on the LDAP connection config. Signed-off-by: Lukas Hirt <info@hirt.cz> * fix: [OCISDEV-1031] address PR review on the LDAP connection pool Retry once on a network error in ConnPool.do, mirroring ConnWithReconnect's existing retry behavior: idle pooled connections aren't health-checked before use, so a connection that went stale while idle would otherwise fail the caller's first operation instead of being transparently recovered. Also: stop leaking the checkout() timer when the semaphore wins the select, simplify GetLastError to not check out a connection just to read a value that's meaningless for a pool, and wire a real logger into GetLDAPClientWithPool so pool dial/checkout debug logging isn't silently discarded. Signed-off-by: Lukas Hirt <info@hirt.cz> * fix: [OCISDEV-1031] address remaining PR review comments on the LDAP pool Move Config to its own file, document the pool-size/checkout-timeout defaults, name the dial callback type and document what it does, require the logger at construction instead of via a mutable setter, replace the closed-flag mutex with an atomic bool, log Close() errors on evicted/idle connections, and add GetLDAPClientFromConfig so the auth/user/group LDAP managers no longer duplicate the pool-vs-reconnect branch. Signed-off-by: Lukas Hirt <info@hirt.cz> * fix: [OCISDEV-1031] simplify TLS dial and use a weighted semaphore Collapse the TLS/non-TLS DialURL branch in dialLDAP and ConnWithReconnect.ldapConnect into a single call: DialWithTLSConfig(nil) is equivalent to omitting the option, since crypto/tls treats a nil *tls.Config as the zero configuration. Replace ConnPool's channel-based semaphore with golang.org/x/sync/semaphore.Weighted (already a repo dependency), using context.WithTimeout for the checkout deadline instead of a manual timer. Signed-off-by: Lukas Hirt <info@hirt.cz> * fix: [OCISDEV-1031] fix unit-tests build on go1.24 wg.Go (sync.WaitGroup.Go) needs go1.25; this branch is rebased onto stable-8.0, which pins go1.24.0 in go.mod, so CI's unit-tests job failed to build the test binary. Use wg.Add/Done with a plain goroutine instead. Signed-off-by: Lukas Hirt <info@hirt.cz> --------- Signed-off-by: Lukas Hirt <info@hirt.cz> feat: [OCISDEV-1031] implement PasswordModify and ModifyWithResult on LDAP clients (#665) * feat: [OCISDEV-1031] implement PasswordModify and ModifyWithResult on LDAP clients ConnWithReconnect and ConnPool (pkg/utils/ldap) stubbed out PasswordModify and ModifyWithResult with a "not implemented" error, copied over from the existing set of unimplemented ldap.Client methods. Implement both on each client via their existing retry helpers (retry on ConnWithReconnect, do on ConnPool), mirroring the retry-on-network-error behaviour already used by Search/Add/Modify/etc., so callers relying on the LDAP Password Modify Extended Operation or on ModifyWithResult work with either client. Signed-off-by: Lukas Hirt <info@hirt.cz> * test: guard fakeConn against nil overrides, cover retry on PasswordModify/ModifyWithResult Addresses review feedback on #665: fakeConn.PasswordModify/ModifyWithResult now return an error instead of panicking when their func overrides aren't set, and the pool's network-error retry path is now exercised for both new methods (previously only the happy path was tested). Signed-off-by: Lukas Hirt <info@hirt.cz> --------- Signed-off-by: Lukas Hirt <info@hirt.cz> fix(ldap): [OCISDEV-1031] restore TLS 1.2 floor for LDAP connections (#672) tlsConfigFromLDAPConn (shared by GetLDAPClientWithReconnect, GetLDAPClientWithPool and GetLDAPClientForAuth) built its *tls.Config without a MinVersion, letting the LDAP client negotiate down to whatever the Go runtime's default minimum TLS version is. Callers (oCIS graph service, auth-basic/users/groups managers) previously set MinVersion: tls.VersionTLS12 themselves before this helper was introduced in #658. Restore that floor on both the insecure and CA-cert paths. Signed-off-by: Lukas Hirt <info@hirt.cz> fix(ldap): [OCISDEV-1031] restore pinned-CA-only trust for LDAP TLS (#674) tlsConfigFromLDAPConn's CA-cert branch built its RootCAs pool from x509.SystemCertPool() plus the configured CA, so configuring a CA cert also trusted every system-level CA instead of just the pinned one — a trust-scope widening versus the pre-pool graph service behavior (x509.NewCertPool()). It also ignored AppendCertsFromPEM's return value, silently falling back to an unrestricted trust store on a malformed/empty CA file instead of failing initialization. Use x509.NewCertPool() and fail when the PEM contains no valid certificates, matching the behavior callers relied on before this helper was introduced in #658. Flagged during review of owncloud/ocis#12660. Signed-off-by: Lukas Hirt <info@hirt.cz> docs: point changelog fragments at forward-port PR #680
mklos-kw
force-pushed
the
feat/ocisdev-1031-ldap-pool-stable-8.2
branch
from
August 4, 2026 08:13
a9c3ec9 to
3b46ec2
Compare
mklos-kw
approved these changes
Aug 4, 2026
kobergj
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
stable-8.0) tostable-8.2.pkg/utils/ldap/pool.go) as a drop-in alternative to the existing single reconnecting connection, plusPasswordModify/ModifyWithResultsupport on both clients, and two TLS hardening fixes found during review of the original PRs (TLS 1.2 floor, pinned-CA-only trust).Test plan
go build ./...go test ./pkg/utils/ldap/... ./pkg/auth/manager/ldap/... ./pkg/user/manager/ldap/... ./pkg/group/manager/ldap/...