feat: [OCISDEV-1032][stable-8.1] LDAP config and retry policy

changelog/unreleased/fix-ldap-retry-send-failure.md

@@ -0,0 +1,19 @@
+Bugfix: Retry LDAP operations that fail while sending the request
+
+An operation on an idle connection that was reaped by a server idle timeout, load balancer or
+firewall could fail without being retried. When the operation reached go-ldap's write loop before
+its reader goroutine noticed the drop, `IsClosing()` was still false and the failure surfaced from
+`processMessages` as `unable to send request: ...`. That is a plain error rather than an
+`*ldap.Error`, so it carries no result code and mapped to `LDAPResultOther`, which neither the read
+nor the write retry policy treated as retryable.
+
+`isSendFailedErr` now matches that failure by message, and both policies retry it. It is safe to
+retry a write: go-ldap adds a message to `messageContexts` only after `conn.Write` succeeds, so an
+operation failing here provably never reached the server and cannot be double-applied. Errors raised
+after the request was transmitted (a drop while reading the response, or a request timeout) are still
+never retried for writes.
+
+`ConnPool.release` also evicts a connection on this error instead of returning it to the idle pool,
+where it would have failed the next checkout that picked it up.
+
+https://github.com/owncloud/reva/pull/678

go.mod

@@ -14,6 +14,7 @@ require (
 	github.com/bluele/gcache v0.0.2
 	github.com/c-bata/go-prompt v0.2.6
 	github.com/cenkalti/backoff v2.2.1+incompatible
+	github.com/cenkalti/backoff/v5 v5.0.3
 	github.com/ceph/go-ceph v0.39.0
 	github.com/cheggaaa/pb v1.0.29
 	github.com/coreos/go-oidc/v3 v3.18.0
@@ -115,7 +116,6 @@ require (
 	github.com/antithesishq/antithesis-sdk-go v0.7.0-default-no-op // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
 	github.com/bitly/go-simplejson v0.5.0 // indirect
-	github.com/cenkalti/backoff/v5 v5.0.3 // indirect
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/cloudflare/circl v1.6.3 // indirect
 	github.com/coreos/go-semver v0.3.1 // indirect

pkg/utils/ldap.go

@@ -22,10 +22,11 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"os"
+	"time"
 
+	"github.com/go-ldap/ldap/v3"
 	"github.com/owncloud/reva/v2/pkg/logger"
 	ldapReconnect "github.com/owncloud/reva/v2/pkg/utils/ldap"
-	"github.com/go-ldap/ldap/v3"
 	"github.com/pkg/errors"
 )
 
@@ -37,65 +38,113 @@ type LDAPConn struct {
 	CACert       string `mapstructure:"cacert"`
 	BindDN       string `mapstructure:"bind_username"`
 	BindPassword string `mapstructure:"bind_password"`
+
+	RetryMaxCount  int           `mapstructure:"retry_max_count"`
+	RetryBaseDelay time.Duration `mapstructure:"retry_base_delay"`
+	RetryMaxDelay  time.Duration `mapstructure:"retry_max_delay"`
+
+	// PoolEnabled switches GetLDAPClientFromConfig 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{
+			MinVersion: tls.VersionTLS12,
 			//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.NewCertPool()
+		if !rpool.AppendCertsFromPEM(pemBytes) {
+			return nil, errors.Errorf("Error adding LDAP CA Cert '%s': no valid certificates found", c.CACert)
+		}
+		return &tls.Config{
+			MinVersion: tls.VersionTLS12,
+			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(
 		ldapReconnect.Config{
-			URI:          c.URI,
-			BindDN:       c.BindDN,
-			BindPassword: c.BindPassword,
-			TLSConfig:    tlsConf,
+			URI:            c.URI,
+			BindDN:         c.BindDN,
+			BindPassword:   c.BindPassword,
+			TLSConfig:      tlsConf,
+			RetryMaxCount:  c.RetryMaxCount,
+			RetryBaseDelay: c.RetryBaseDelay,
+			RetryMaxDelay:  c.RetryMaxDelay,
 		},
 	)
 	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,
+			RetryMaxCount:       c.RetryMaxCount,
+			RetryBaseDelay:      c.RetryBaseDelay,
+			RetryMaxDelay:       c.RetryMaxDelay,
+			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,26 @@
+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
+
+	RetryMaxCount  int
+	RetryBaseDelay time.Duration
+	RetryMaxDelay  time.Duration
+
+	// 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
+}