feat: [OCISDEV-1031] add opt-in LDAP connection pool

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

pkg/auth/manager/ldap/ldap.go

@@ -78,7 +78,7 @@ func New(m map[string]interface{}) (auth.Manager, error) {
 	if err != nil {
 		return nil, err
 	}
-	manager.ldapClient, err = utils.GetLDAPClientWithReconnect(&manager.c.LDAPConn)
+	manager.ldapClient, err = utils.GetLDAPClientFromConfig(&manager.c.LDAPConn)
 	if err != nil {
 		return nil, err
 	}

pkg/group/manager/ldap/ldap.go

@@ -74,7 +74,7 @@ func New(m map[string]interface{}) (group.Manager, error) {
 		return nil, err
 	}
 
-	mgr.ldapClient, err = utils.GetLDAPClientWithReconnect(&mgr.c.LDAPConn)
+	mgr.ldapClient, err = utils.GetLDAPClientFromConfig(&mgr.c.LDAPConn)
 	if err != nil {
 		return nil, err
 	}

pkg/user/manager/ldap/ldap.go

@@ -73,7 +73,7 @@ func New(m map[string]interface{}) (user.Manager, error) {
 		return nil, err
 	}
 
-	mgr.ldapClient, err = utils.GetLDAPClientWithReconnect(&mgr.c.LDAPConn)
+	mgr.ldapClient, err = utils.GetLDAPClientFromConfig(&mgr.c.LDAPConn)
 	return mgr, err
 }
 

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())
+	}
 }

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,45 @@ 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,
+		},
+		logger.New(),
+	)
+	return pool, nil
+}
+
+// GetLDAPClientFromConfig returns a connected ldap.Client for c: a bounded pool when
+// c.PoolEnabled, otherwise the single long-lived reconnecting connection.
+func GetLDAPClientFromConfig(c *LDAPConn) (ldap.Client, error) {
+	if c.PoolEnabled {
+		return GetLDAPClientWithPool(c)
+	}
+	return GetLDAPClientWithReconnect(c)
+}
+
 // 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 {

pkg/utils/ldap/config.go

@@ -0,0 +1,40 @@
+// 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 (
+	"crypto/tls"
+	"time"
+)
+
+// Config holds the basic configuration of the LDAP Connection
+type Config struct {
+	URI          string
+	BindDN       string
+	BindPassword string
+	TLSConfig    *tls.Config
+
+	// PoolSize caps the number of concurrently open connections in the pool. Only used by
+	// NewLDAPPool; NewLDAPWithReconnect ignores it. Defaults to defaultPoolSize (5) when <= 0.
+	PoolSize int
+	// PoolCheckoutTimeout bounds how long a checkout blocks waiting for a connection once the pool
+	// is at PoolSize. Only used by NewLDAPPool; NewLDAPWithReconnect ignores it. Defaults to
+	// defaultPoolCheckoutTimeout (30s) when <= 0.
+	PoolCheckoutTimeout time.Duration
+}