From 1086406a24b16b75028496cfb05aeebec98995fa Mon Sep 17 00:00:00 2001 From: Lukas Hirt Date: Thu, 16 Jul 2026 11:04:13 +0200 Subject: [PATCH 1/2] 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 --- .../enhancement-ldap-connection-pool.md | 10 + pkg/auth/manager/ldap/ldap.go | 6 +- pkg/group/manager/ldap/ldap.go | 6 +- pkg/user/manager/ldap/ldap.go | 6 +- pkg/user/manager/ldap/ldap_test.go | 7 + pkg/utils/ldap.go | 91 +++-- pkg/utils/ldap/pool.go | 342 ++++++++++++++++++ pkg/utils/ldap/pool_test.go | 162 +++++++++ pkg/utils/ldap/reconnect.go | 4 + 9 files changed, 598 insertions(+), 36 deletions(-) create mode 100644 changelog/unreleased/enhancement-ldap-connection-pool.md create mode 100644 pkg/utils/ldap/pool.go create mode 100644 pkg/utils/ldap/pool_test.go diff --git a/changelog/unreleased/enhancement-ldap-connection-pool.md b/changelog/unreleased/enhancement-ldap-connection-pool.md new file mode 100644 index 00000000000..b856cdd0537 --- /dev/null +++ b/changelog/unreleased/enhancement-ldap-connection-pool.md @@ -0,0 +1,10 @@ +Enhancement: Add an opt-in LDAP connection pool + +Added `GetLDAPClientWithPool`, a bounded pool of authenticated LDAP +connections, as a drop-in alternative to the existing single, long-lived +reconnecting connection. It is disabled by default and can be enabled per +backend via `pool_enabled` (plus `pool_size` and `pool_checkout_timeout`) on +the auth, user and group LDAP managers, so concurrent requests no longer +serialize on a single socket. + +https://github.com/owncloud/reva/pull/658 diff --git a/pkg/auth/manager/ldap/ldap.go b/pkg/auth/manager/ldap/ldap.go index 33edca1ed2e..7a682292dfd 100644 --- a/pkg/auth/manager/ldap/ldap.go +++ b/pkg/auth/manager/ldap/ldap.go @@ -78,7 +78,11 @@ func New(m map[string]interface{}) (auth.Manager, error) { if err != nil { return nil, err } - manager.ldapClient, err = utils.GetLDAPClientWithReconnect(&manager.c.LDAPConn) + if manager.c.LDAPConn.PoolEnabled { + manager.ldapClient, err = utils.GetLDAPClientWithPool(&manager.c.LDAPConn) + } else { + manager.ldapClient, err = utils.GetLDAPClientWithReconnect(&manager.c.LDAPConn) + } if err != nil { return nil, err } diff --git a/pkg/group/manager/ldap/ldap.go b/pkg/group/manager/ldap/ldap.go index 2a6a38cea4a..d8c1059be41 100644 --- a/pkg/group/manager/ldap/ldap.go +++ b/pkg/group/manager/ldap/ldap.go @@ -74,7 +74,11 @@ func New(m map[string]interface{}) (group.Manager, error) { return nil, err } - mgr.ldapClient, err = utils.GetLDAPClientWithReconnect(&mgr.c.LDAPConn) + if mgr.c.LDAPConn.PoolEnabled { + mgr.ldapClient, err = utils.GetLDAPClientWithPool(&mgr.c.LDAPConn) + } else { + mgr.ldapClient, err = utils.GetLDAPClientWithReconnect(&mgr.c.LDAPConn) + } if err != nil { return nil, err } diff --git a/pkg/user/manager/ldap/ldap.go b/pkg/user/manager/ldap/ldap.go index 07f9d510a6a..58e6ee87b04 100644 --- a/pkg/user/manager/ldap/ldap.go +++ b/pkg/user/manager/ldap/ldap.go @@ -73,7 +73,11 @@ func New(m map[string]interface{}) (user.Manager, error) { return nil, err } - mgr.ldapClient, err = utils.GetLDAPClientWithReconnect(&mgr.c.LDAPConn) + if mgr.c.LDAPConn.PoolEnabled { + mgr.ldapClient, err = utils.GetLDAPClientWithPool(&mgr.c.LDAPConn) + } else { + mgr.ldapClient, err = utils.GetLDAPClientWithReconnect(&mgr.c.LDAPConn) + } return mgr, err } diff --git a/pkg/user/manager/ldap/ldap_test.go b/pkg/user/manager/ldap/ldap_test.go index f404338ab56..c349760a45b 100644 --- a/pkg/user/manager/ldap/ldap_test.go +++ b/pkg/user/manager/ldap/ldap_test.go @@ -65,4 +65,11 @@ func TestUserManager(t *testing.T) { if err != nil { t.Fatal(err.Error()) } + + // New must not dial anything eagerly, so pool_enabled must also succeed without a + // reachable LDAP server. + _, err = New(map[string]interface{}{"pool_enabled": true}) + if err != nil { + t.Fatal(err.Error()) + } } diff --git a/pkg/utils/ldap.go b/pkg/utils/ldap.go index 627c9a7f41f..8688fcbfbca 100644 --- a/pkg/utils/ldap.go +++ b/pkg/utils/ldap.go @@ -22,6 +22,7 @@ import ( "crypto/tls" "crypto/x509" "os" + "time" "github.com/owncloud/reva/v2/pkg/logger" ldapReconnect "github.com/owncloud/reva/v2/pkg/utils/ldap" @@ -37,30 +38,47 @@ type LDAPConn struct { CACert string `mapstructure:"cacert"` BindDN string `mapstructure:"bind_username"` BindPassword string `mapstructure:"bind_password"` + + // PoolEnabled switches GetLDAPClientWithPool callers to a bounded connection pool instead of + // the single long-lived reconnecting connection. Off by default. + PoolEnabled bool `mapstructure:"pool_enabled"` + // PoolSize caps the number of concurrently open pooled connections. Defaults to 5 when unset. + PoolSize int `mapstructure:"pool_size"` + // PoolCheckoutTimeout bounds how long a checkout waits for a connection to become available + // once the pool is at PoolSize. Defaults to 30s when unset. + PoolCheckoutTimeout time.Duration `mapstructure:"pool_checkout_timeout"` } -// GetLDAPClientWithReconnect initializes a long-lived LDAP connection that -// automatically reconnects on connection errors. It allows to set TLS options -// e.g. to add trusted Certificates or disable Certificate verification -func GetLDAPClientWithReconnect(c *LDAPConn) (ldap.Client, error) { - var tlsConf *tls.Config +// tlsConfigFromLDAPConn builds the *tls.Config shared by all GetLDAPClient* constructors below. +func tlsConfigFromLDAPConn(c *LDAPConn) (*tls.Config, error) { if c.Insecure { logger.New().Warn().Msg("SSL Certificate verification is disabled. This is strongly discouraged for production environments.") - tlsConf = &tls.Config{ + return &tls.Config{ //nolint:gosec // We need the ability to run with "insecure" (dev/testing) InsecureSkipVerify: true, - } + }, nil } - if !c.Insecure && c.CACert != "" { - if pemBytes, err := os.ReadFile(c.CACert); err == nil { - rpool, _ := x509.SystemCertPool() - rpool.AppendCertsFromPEM(pemBytes) - tlsConf = &tls.Config{ - RootCAs: rpool, - } - } else { + if c.CACert != "" { + pemBytes, err := os.ReadFile(c.CACert) + if err != nil { return nil, errors.Wrapf(err, "Error reading LDAP CA Cert '%s.'", c.CACert) } + rpool, _ := x509.SystemCertPool() + rpool.AppendCertsFromPEM(pemBytes) + return &tls.Config{ + RootCAs: rpool, + }, nil + } + return nil, nil +} + +// GetLDAPClientWithReconnect initializes a long-lived LDAP connection that +// automatically reconnects on connection errors. It allows to set TLS options +// e.g. to add trusted Certificates or disable Certificate verification +func GetLDAPClientWithReconnect(c *LDAPConn) (ldap.Client, error) { + tlsConf, err := tlsConfigFromLDAPConn(c) + if err != nil { + return nil, err } conn := ldapReconnect.NewLDAPWithReconnect( @@ -74,28 +92,35 @@ func GetLDAPClientWithReconnect(c *LDAPConn) (ldap.Client, error) { return conn, nil } +// GetLDAPClientWithPool initializes a bounded pool of authenticated LDAP connections, dialed and +// bound lazily on first use. It is a drop-in alternative to GetLDAPClientWithReconnect intended for +// backends that need to serve concurrent requests without serializing on a single connection. +func GetLDAPClientWithPool(c *LDAPConn) (ldap.Client, error) { + tlsConf, err := tlsConfigFromLDAPConn(c) + if err != nil { + return nil, err + } + + pool := ldapReconnect.NewLDAPPool( + ldapReconnect.Config{ + URI: c.URI, + BindDN: c.BindDN, + BindPassword: c.BindPassword, + TLSConfig: tlsConf, + PoolSize: c.PoolSize, + PoolCheckoutTimeout: c.PoolCheckoutTimeout, + }, + ) + return pool, nil +} + // GetLDAPClientForAuth initializes an LDAP connection. The connection is not authenticated // when returned. The main purpose for GetLDAPClientForAuth is to get and LDAP connection that // can be used to issue a single bind request to authenticate a user. func GetLDAPClientForAuth(c *LDAPConn) (ldap.Client, error) { - var tlsConf *tls.Config - if c.Insecure { - logger.New().Warn().Msg("SSL Certificate verification is disabled. Is is strongly discouraged for production environments.") - tlsConf = &tls.Config{ - //nolint:gosec // We need the ability to run with "insecure" (dev/testing) - InsecureSkipVerify: true, - } - } - if !c.Insecure && c.CACert != "" { - if pemBytes, err := os.ReadFile(c.CACert); err == nil { - rpool, _ := x509.SystemCertPool() - rpool.AppendCertsFromPEM(pemBytes) - tlsConf = &tls.Config{ - RootCAs: rpool, - } - } else { - return nil, errors.Wrapf(err, "Error reading LDAP CA Cert '%s.'", c.CACert) - } + tlsConf, err := tlsConfigFromLDAPConn(c) + if err != nil { + return nil, err } l, err := ldap.DialURL(c.URI, ldap.DialWithTLSConfig(tlsConf)) if err != nil { diff --git a/pkg/utils/ldap/pool.go b/pkg/utils/ldap/pool.go new file mode 100644 index 00000000000..decebb61d85 --- /dev/null +++ b/pkg/utils/ldap/pool.go @@ -0,0 +1,342 @@ +// Copyright 2022 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package ldap + +import ( + "context" + "crypto/tls" + "errors" + "fmt" + "sync" + "time" + + "github.com/go-ldap/ldap/v3" + "github.com/rs/zerolog" +) + +const ( + defaultPoolSize = 5 + defaultPoolCheckoutTimeout = 30 * time.Second +) + +var ( + // ErrPoolExhausted is returned by a checkout that couldn't obtain a connection within the + // configured checkout timeout. + ErrPoolExhausted = errors.New("ldap: connection pool exhausted") + errPoolClosed = errors.New("ldap: connection pool is closed") +) + +// ConnPool is a bounded pool of authenticated LDAP connections. Connections are dialed and bound +// lazily on checkout and reused across requests; connections that fail with a network error are +// discarded and lazily re-dialed on a later checkout, instead of the pool eagerly reconnecting them. +type ConnPool struct { + config Config + timeout time.Duration + dial func(Config) (ldap.Client, error) + logger *zerolog.Logger + + sem chan struct{} + idle chan ldap.Client + + mu sync.Mutex + closed bool +} + +// NewLDAPPool returns a new ConnPool initialized from config. No connection is dialed until the +// first checkout. +func NewLDAPPool(config Config) *ConnPool { + size := config.PoolSize + if size <= 0 { + size = defaultPoolSize + } + timeout := config.PoolCheckoutTimeout + if timeout <= 0 { + timeout = defaultPoolCheckoutTimeout + } + logger := zerolog.Nop() + return &ConnPool{ + config: config, + timeout: timeout, + dial: dialLDAP, + logger: &logger, + sem: make(chan struct{}, size), + idle: make(chan ldap.Client, size), + } +} + +func dialLDAP(config Config) (ldap.Client, error) { + var l *ldap.Conn + var err error + if config.TLSConfig != nil { + l, err = ldap.DialURL(config.URI, ldap.DialWithTLSConfig(config.TLSConfig)) + } else { + l, err = ldap.DialURL(config.URI) + } + if err != nil { + return nil, err + } + if config.BindDN != "" { + if err := l.Bind(config.BindDN, config.BindPassword); err != nil { + l.Close() + return nil, err + } + } + return l, nil +} + +// SetLogger sets the logger for the current instance +func (p *ConnPool) SetLogger(logger *zerolog.Logger) { + p.logger = logger +} + +// checkout reserves a pool slot (blocking up to p.timeout) and returns a connection: an idle one if +// available, otherwise a freshly dialed one. +func (p *ConnPool) checkout() (ldap.Client, error) { + select { + case p.sem <- struct{}{}: + case <-time.After(p.timeout): + return nil, ErrPoolExhausted + } + + p.mu.Lock() + closed := p.closed + p.mu.Unlock() + if closed { + <-p.sem + return nil, errPoolClosed + } + + select { + case conn := <-p.idle: + return conn, nil + default: + } + + p.logger.Debug().Msg("dialing new pooled LDAP connection") + conn, err := p.dial(p.config) + if err != nil { + <-p.sem + return nil, err + } + return conn, nil +} + +// release returns conn to the pool, or closes and discards it if opErr is a network error (or the +// pool has since been closed), and frees the slot reserved by checkout. +func (p *ConnPool) release(conn ldap.Client, opErr error) { + p.mu.Lock() + closed := p.closed + p.mu.Unlock() + + if closed || (opErr != nil && ldap.IsErrorWithCode(opErr, ldap.ErrorNetwork)) { + conn.Close() + } else { + p.idle <- conn + } + <-p.sem +} + +func (p *ConnPool) do(fn func(conn ldap.Client) error) error { + conn, err := p.checkout() + if err != nil { + return err + } + opErr := fn(conn) + p.release(conn, opErr) + return opErr +} + +// Close closes the pool: currently idle connections are closed immediately, further checkouts are +// rejected, and connections already checked out are closed as they're returned. +func (p *ConnPool) Close() error { + p.mu.Lock() + if p.closed { + p.mu.Unlock() + return nil + } + p.closed = true + p.mu.Unlock() + + for { + select { + case conn := <-p.idle: + conn.Close() + default: + return nil + } + } +} + +// Search implements the ldap.Client interface +func (p *ConnPool) Search(sr *ldap.SearchRequest) (*ldap.SearchResult, error) { + var res *ldap.SearchResult + err := p.do(func(conn ldap.Client) error { + var err error + res, err = conn.Search(sr) + return err + }) + return res, err +} + +// Add implements the ldap.Client interface +func (p *ConnPool) Add(a *ldap.AddRequest) error { + return p.do(func(conn ldap.Client) error { + return conn.Add(a) + }) +} + +// Del implements the ldap.Client interface +func (p *ConnPool) Del(d *ldap.DelRequest) error { + return p.do(func(conn ldap.Client) error { + return conn.Del(d) + }) +} + +// Modify implements the ldap.Client interface +func (p *ConnPool) Modify(m *ldap.ModifyRequest) error { + return p.do(func(conn ldap.Client) error { + return conn.Modify(m) + }) +} + +// ModifyDN implements the ldap.Client interface +func (p *ConnPool) ModifyDN(m *ldap.ModifyDNRequest) error { + return p.do(func(conn ldap.Client) error { + return conn.ModifyDN(m) + }) +} + +// Extended implements the ldap.Client interface +func (p *ConnPool) Extended(request *ldap.ExtendedRequest) (*ldap.ExtendedResponse, error) { + var res *ldap.ExtendedResponse + err := p.do(func(conn ldap.Client) error { + var err error + res, err = conn.Extended(request) + return err + }) + return res, err +} + +// GetLastError implements the ldap.Client interface +func (p *ConnPool) GetLastError() error { + var lastErr error + _ = p.do(func(conn ldap.Client) error { + lastErr = conn.GetLastError() + return nil + }) + return lastErr +} + +// IsClosing implements the ldap.Client interface +func (p *ConnPool) IsClosing() bool { + p.mu.Lock() + defer p.mu.Unlock() + return p.closed +} + +// Remaining methods to fulfill ldap.Client interface + +// Start implements the ldap.Client interface +func (p *ConnPool) Start() {} + +// SetTimeout implements the ldap.Client interface +func (p *ConnPool) SetTimeout(time.Duration) {} + +// StartTLS implements the ldap.Client interface +func (p *ConnPool) StartTLS(*tls.Config) error { + return ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// Bind implements the ldap.Client interface +func (p *ConnPool) Bind(username, password string) error { + return ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// UnauthenticatedBind implements the ldap.Client interface +func (p *ConnPool) UnauthenticatedBind(username string) error { + return ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// SimpleBind implements the ldap.Client interface +func (p *ConnPool) SimpleBind(*ldap.SimpleBindRequest) (*ldap.SimpleBindResult, error) { + return nil, ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// ExternalBind implements the ldap.Client interface +func (p *ConnPool) ExternalBind() error { + return ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// NTLMUnauthenticatedBind implements the ldap.Client interface +func (p *ConnPool) NTLMUnauthenticatedBind(domain, username string) error { + return ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// Unbind implements the ldap.Client interface +func (p *ConnPool) Unbind() error { + return ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// ModifyWithResult implements the ldap.Client interface +func (p *ConnPool) ModifyWithResult(m *ldap.ModifyRequest) (*ldap.ModifyResult, error) { + return nil, ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// Compare implements the ldap.Client interface +func (p *ConnPool) Compare(dn, attribute, value string) (bool, error) { + return false, ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// PasswordModify implements the ldap.Client interface +func (p *ConnPool) PasswordModify(*ldap.PasswordModifyRequest) (*ldap.PasswordModifyResult, error) { + return nil, ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// SearchWithPaging implements the ldap.Client interface +func (p *ConnPool) SearchWithPaging(searchRequest *ldap.SearchRequest, pagingSize uint32) (*ldap.SearchResult, error) { + return nil, ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// SearchAsync implements the ldap.Client interface +func (p *ConnPool) SearchAsync(ctx context.Context, searchRequest *ldap.SearchRequest, bufferSize int) ldap.Response { + // unimplemented + return nil +} + +// TLSConnectionState implements the ldap.Client interface +func (p *ConnPool) TLSConnectionState() (tls.ConnectionState, bool) { + return tls.ConnectionState{}, false +} + +// DirSync implements the ldap.Client interface +func (p *ConnPool) DirSync(searchRequest *ldap.SearchRequest, flags, maxAttrCount int64, cookie []byte) (*ldap.SearchResult, error) { + return nil, ldap.NewError(ldap.LDAPResultNotSupported, fmt.Errorf("not implemented")) +} + +// DirSyncAsync implements the ldap.Client interface +func (p *ConnPool) DirSyncAsync(ctx context.Context, searchRequest *ldap.SearchRequest, bufferSize int, flags, maxAttrCount int64, cookie []byte) ldap.Response { + // unimplemented + return nil +} + +// Syncrepl implements the ldap.Client interface +func (p *ConnPool) Syncrepl(ctx context.Context, searchRequest *ldap.SearchRequest, bufferSize int, mode ldap.ControlSyncRequestMode, cookie []byte, reloadHint bool) ldap.Response { + // unimplemented + return nil +} diff --git a/pkg/utils/ldap/pool_test.go b/pkg/utils/ldap/pool_test.go new file mode 100644 index 00000000000..1b2cae3b1ac --- /dev/null +++ b/pkg/utils/ldap/pool_test.go @@ -0,0 +1,162 @@ +// Copyright 2022 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package ldap + +import ( + "errors" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/go-ldap/ldap/v3" +) + +// fakeConn is a minimal ldap.Client double used to test pool bookkeeping without a real LDAP +// server. It embeds a nil ldap.Client so it satisfies the interface; only Close is overridden, +// which is the only method the pool itself calls on connections it manages. +type fakeConn struct { + ldap.Client + closed bool +} + +func (f *fakeConn) Close() error { + f.closed = true + return nil +} + +// newTestPool returns a pool with a fake, network-free dial function and a counter of how many +// times it was called. +func newTestPool(size int, timeout time.Duration) (*ConnPool, *int32) { + var dialCount int32 + p := NewLDAPPool(Config{PoolSize: size, PoolCheckoutTimeout: timeout}) + p.dial = func(Config) (ldap.Client, error) { + atomic.AddInt32(&dialCount, 1) + return &fakeConn{}, nil + } + return p, &dialCount +} + +func TestConnPoolLazyConstruction(t *testing.T) { + _, dialCount := newTestPool(2, time.Second) + if got := atomic.LoadInt32(dialCount); got != 0 { + t.Fatalf("expected no dials on construction, got %d", got) + } +} + +func TestConnPoolCheckoutReusesReturnedConnection(t *testing.T) { + p, dialCount := newTestPool(2, time.Second) + + conn, err := p.checkout() + if err != nil { + t.Fatalf("checkout failed: %v", err) + } + p.release(conn, nil) + + conn2, err := p.checkout() + if err != nil { + t.Fatalf("checkout failed: %v", err) + } + if conn2 != conn { + t.Fatalf("expected the returned connection to be reused") + } + if got := atomic.LoadInt32(dialCount); got != 1 { + t.Fatalf("expected exactly 1 dial, got %d", got) + } +} + +func TestConnPoolEvictsUnhealthyConnection(t *testing.T) { + p, dialCount := newTestPool(2, time.Second) + + conn, err := p.checkout() + if err != nil { + t.Fatalf("checkout failed: %v", err) + } + networkErr := ldap.NewError(ldap.ErrorNetwork, errors.New("boom")) + p.release(conn, networkErr) + + if !conn.(*fakeConn).closed { + t.Fatalf("expected the unhealthy connection to be closed") + } + + if _, err := p.checkout(); err != nil { + t.Fatalf("checkout failed: %v", err) + } + if got := atomic.LoadInt32(dialCount); got != 2 { + t.Fatalf("expected a redial after eviction, got %d dials", got) + } +} + +func TestConnPoolExhaustionTimesOut(t *testing.T) { + p, _ := newTestPool(1, 50*time.Millisecond) + + if _, err := p.checkout(); err != nil { + t.Fatalf("checkout failed: %v", err) + } + + start := time.Now() + _, err := p.checkout() + if !errors.Is(err, ErrPoolExhausted) { + t.Fatalf("expected ErrPoolExhausted, got %v", err) + } + if elapsed := time.Since(start); elapsed < 50*time.Millisecond { + t.Fatalf("expected checkout to wait for the timeout, only waited %v", elapsed) + } +} + +func TestConnPoolCloseDrainsIdleAndRejectsCheckout(t *testing.T) { + p, _ := newTestPool(1, time.Second) + + conn, err := p.checkout() + if err != nil { + t.Fatalf("checkout failed: %v", err) + } + p.release(conn, nil) + + if err := p.Close(); err != nil { + t.Fatalf("close failed: %v", err) + } + if !conn.(*fakeConn).closed { + t.Fatalf("expected the idle connection to be closed by Close") + } + if _, err := p.checkout(); !errors.Is(err, errPoolClosed) { + t.Fatalf("expected errPoolClosed, got %v", err) + } +} + +func TestConnPoolConcurrentCheckoutRelease(t *testing.T) { + p, dialCount := newTestPool(4, time.Second) + + var wg sync.WaitGroup + for range 50 { + wg.Go(func() { + conn, err := p.checkout() + if err != nil { + t.Errorf("checkout failed: %v", err) + return + } + p.release(conn, nil) + }) + } + wg.Wait() + + if got := atomic.LoadInt32(dialCount); got > 4 { + t.Fatalf("expected at most 4 dials (pool size), got %d", got) + } +} diff --git a/pkg/utils/ldap/reconnect.go b/pkg/utils/ldap/reconnect.go index 0c190aeffac..0e34c0a1067 100644 --- a/pkg/utils/ldap/reconnect.go +++ b/pkg/utils/ldap/reconnect.go @@ -56,6 +56,10 @@ type Config struct { BindDN string BindPassword string TLSConfig *tls.Config + + // PoolSize and PoolCheckoutTimeout are only used by NewLDAPPool; NewLDAPWithReconnect ignores them. + PoolSize int + PoolCheckoutTimeout time.Duration } // NewLDAPWithReconnect Returns a new ConnWithReconnect initialized from config From 88c97eb9cfa7ad952e0a2045174ba64f761eabcc Mon Sep 17 00:00:00 2001 From: Lukas Hirt Date: Thu, 16 Jul 2026 11:49:20 +0200 Subject: [PATCH 2/2] 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 --- pkg/utils/ldap.go | 1 + pkg/utils/ldap/pool.go | 38 ++++++++++++-------- pkg/utils/ldap/pool_test.go | 70 +++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 14 deletions(-) diff --git a/pkg/utils/ldap.go b/pkg/utils/ldap.go index 8688fcbfbca..e21aae8a2de 100644 --- a/pkg/utils/ldap.go +++ b/pkg/utils/ldap.go @@ -111,6 +111,7 @@ func GetLDAPClientWithPool(c *LDAPConn) (ldap.Client, error) { PoolCheckoutTimeout: c.PoolCheckoutTimeout, }, ) + pool.SetLogger(logger.New()) return pool, nil } diff --git a/pkg/utils/ldap/pool.go b/pkg/utils/ldap/pool.go index decebb61d85..cbc7155f1d5 100644 --- a/pkg/utils/ldap/pool.go +++ b/pkg/utils/ldap/pool.go @@ -108,9 +108,12 @@ func (p *ConnPool) SetLogger(logger *zerolog.Logger) { // checkout reserves a pool slot (blocking up to p.timeout) and returns a connection: an idle one if // available, otherwise a freshly dialed one. func (p *ConnPool) checkout() (ldap.Client, error) { + timer := time.NewTimer(p.timeout) + defer timer.Stop() + select { case p.sem <- struct{}{}: - case <-time.After(p.timeout): + case <-timer.C: return nil, ErrPoolExhausted } @@ -152,14 +155,25 @@ func (p *ConnPool) release(conn ldap.Client, opErr error) { <-p.sem } +// do checks out a connection, runs fn, and releases the connection. Checked-out idle connections +// are not health-checked before use, so a connection that went stale while idle (server-side idle +// timeout, LB/firewall reaping) is expected to fail the first op after being idle; on a network +// error, do retries once more with a freshly checked-out connection (the failed one was evicted by +// release) instead of surfacing the failure to the caller, mirroring ConnWithReconnect's retry. func (p *ConnPool) do(fn func(conn ldap.Client) error) error { - conn, err := p.checkout() - if err != nil { - return err + var opErr error + for try := 0; try <= defaultRetries; try++ { + conn, err := p.checkout() + if err != nil { + return err + } + opErr = fn(conn) + p.release(conn, opErr) + if opErr == nil || !ldap.IsErrorWithCode(opErr, ldap.ErrorNetwork) { + return opErr + } } - opErr := fn(conn) - p.release(conn, opErr) - return opErr + return ldap.NewError(ldap.ErrorNetwork, errMaxRetries) } // Close closes the pool: currently idle connections are closed immediately, further checkouts are @@ -233,14 +247,10 @@ func (p *ConnPool) Extended(request *ldap.ExtendedRequest) (*ldap.ExtendedRespon return res, err } -// GetLastError implements the ldap.Client interface +// GetLastError implements the ldap.Client interface. A pool has no single "last" connection to +// report on, so this always returns nil; it is unused by any caller of ConnPool today. func (p *ConnPool) GetLastError() error { - var lastErr error - _ = p.do(func(conn ldap.Client) error { - lastErr = conn.GetLastError() - return nil - }) - return lastErr + return nil } // IsClosing implements the ldap.Client interface diff --git a/pkg/utils/ldap/pool_test.go b/pkg/utils/ldap/pool_test.go index 1b2cae3b1ac..9716d97b028 100644 --- a/pkg/utils/ldap/pool_test.go +++ b/pkg/utils/ldap/pool_test.go @@ -103,6 +103,76 @@ func TestConnPoolEvictsUnhealthyConnection(t *testing.T) { } } +func TestConnPoolDoRetriesOnceOnNetworkError(t *testing.T) { + p, dialCount := newTestPool(2, time.Second) + + var calls int + err := p.do(func(conn ldap.Client) error { + calls++ + if calls == 1 { + return ldap.NewError(ldap.ErrorNetwork, errors.New("boom")) + } + return nil + }) + if err != nil { + t.Fatalf("expected do to succeed after one retry, got %v", err) + } + if calls != 2 { + t.Fatalf("expected fn to be called twice (initial + 1 retry), got %d", calls) + } + if got := atomic.LoadInt32(dialCount); got != 2 { + t.Fatalf("expected a redial for the retry attempt, got %d dials", got) + } +} + +func TestConnPoolDoGivesUpAfterMaxRetries(t *testing.T) { + p, _ := newTestPool(2, time.Second) + + networkErr := ldap.NewError(ldap.ErrorNetwork, errors.New("boom")) + var calls int + err := p.do(func(conn ldap.Client) error { + calls++ + return networkErr + }) + if err == nil || !ldap.IsErrorWithCode(err, ldap.ErrorNetwork) { + t.Fatalf("expected a network error after exhausting retries, got %v", err) + } + if want := defaultRetries + 1; calls != want { + t.Fatalf("expected fn to be called %d times, got %d", want, calls) + } +} + +func TestConnPoolDoDoesNotRetryNonNetworkError(t *testing.T) { + p, dialCount := newTestPool(2, time.Second) + + nonNetworkErr := errors.New("ldap: invalid credentials") + var calls int + err := p.do(func(conn ldap.Client) error { + calls++ + return nonNetworkErr + }) + if !errors.Is(err, nonNetworkErr) { + t.Fatalf("expected the non-network error to be returned as-is, got %v", err) + } + if calls != 1 { + t.Fatalf("expected fn to be called exactly once (no retry for non-network errors), got %d", calls) + } + if got := atomic.LoadInt32(dialCount); got != 1 { + t.Fatalf("expected exactly 1 dial (connection reused, no eviction), got %d", got) + } +} + +func TestConnPoolGetLastErrorReturnsNilWithoutCheckout(t *testing.T) { + p, dialCount := newTestPool(2, time.Second) + + if err := p.GetLastError(); err != nil { + t.Fatalf("expected nil, got %v", err) + } + if got := atomic.LoadInt32(dialCount); got != 0 { + t.Fatalf("expected GetLastError not to check out a connection, got %d dials", got) + } +} + func TestConnPoolExhaustionTimesOut(t *testing.T) { p, _ := newTestPool(1, 50*time.Millisecond)