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

[LukasHirt]

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.

[kw-security]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[LukasHirt]

@kobergj I copied over the stubbed methods from reconnect.go but seems like they are quite chaotic? Some are doing nothing, some are throwing error, some are returning nil... what would you say about throwing the same error in every stubbed method?

[2403905]

ai review

issue — no transparent retry on a dead connection (behavioral regression vs. reconnect)

  ConnWithReconnect.retry transparently recovers from network errors: on a dropped connection the first fn fails with ldap.ErrorNetwork, and it re-dials and retries once
  (defaultRetries = 1) before returning to the caller. The pool does not do this:
  
  func (p *ConnPool) do(fn func(conn ldap.Client) error) error {
      conn, err := p.checkout()
      ...
      opErr := fn(conn)
      p.release(conn, opErr)   // evicts on network error, but returns opErr to caller
      return opErr
  }

  checkout hands back idle connections with no health check. LDAP servers routinely drop idle connections (server-side idle timeouts, load-balancer/firewall reaping). With the pool,
  the first request after an idle period lands on a stale connection, fails with a network error, and that error is propagated to the caller — the connection is evicted, but only after
  the request has already failed. The reconnect path masks exactly this case today.

  Suggested fix: retry once inside do on a network error with a freshly dialed connection, to preserve drop-in semantics:

  func (p *ConnPool) do(fn func(conn ldap.Client) error) error {
      for try := 0; try <= defaultRetries; try++ {
          conn, err := p.checkout()
          if err != nil {
              return err
          }
          opErr := fn(conn)
          p.release(conn, opErr)              // release evicts on network error
          if opErr == nil || !ldap.IsErrorWithCode(opErr, ldap.ErrorNetwork) {
              return opErr
          }
      }
      return ldap.NewError(ldap.ErrorNetwork, errMaxRetries)
  }

  (On the retry, checkout will re-dial since the evicted conn is gone — modulo picking up another stale idle conn, which the second dial-on-miss handles well enough.) This is the
  finding I'd most want resolved.

  Minor / nits

  - time.After in checkout isn't stopped when the sem send wins the select; the timer lives until it fires. Negligible, but time.NewTimer + Stop avoids the transient allocation on the
  hot path.
  - GetLastError does a full checkout/release just to read GetLastError() off an arbitrary pooled connection — semantically meaningless for a pool (there's no single "last"
  connection). It's currently unused by callers, so low impact; consider returning nil or documenting it as best-effort.
  - No idle health-check on checkout. Related to the main issue; once do retries on network errors this is acceptable, since the retry covers the stale-conn case. Worth a code comment
  noting the "evict-on-failure, no active health check" design.
  - SetLogger exists but is never called by GetLDAPClientWithPool (the reconnect constructor doesn't wire a logger either, so this matches existing behavior). If observability of pool
  dials/evictions matters operationally, consider plumbing the manager's logger through — the Debug "dialing new pooled LDAP connection" line is otherwise dead.
  - Copyright header on the new files reads "Copyright 2022 CERN" — cosmetic; matches the repo's boilerplate convention so it's fine.

[jvillafanez]

The Config type definition needs to be moved to its own file. It's a pain to scroll in the file and not find the definition, and it's a bigger pain to search the definition in all the package files.

[jvillafanez]

These default values need to be documented, specially when 0 or negative values could mean "unlimited"

[jvillafanez]

This looks weird. Why not create a func (p *CoonPool) getLdapClient(config Config) (ldap.Client, error) ? What benefits creating and using a private callback has, when you won't be able to replace it from outside?

If it's for the tests, to mock the LDAP clients, you can use a LDAP client factory.

In addition, this needs documentation, specially if what it does is "connect + bind", not just connect as the name suggests.

[jvillafanez]

Any benefit? Why the logger isn't enforced when the pool is created? Do we want to allow changing the logger at any time?

[jvillafanez]

Why not an atomic variable to skip the locks?

[jvillafanez]

These 2 vars need explanation. What is their purpose? What is the expected usage?

[jvillafanez]

It seems better a utils.GetLDAPClientFromConfig(manager.c.LDAPConn) that returns the appropriate client.

func GetLDAPClientFromConfig(cfg Config) (*ldap.Client, error) {
  if cfg.PoolEnabled {
    return GetLDAPClientWithPool(cfg)
  } else {
    return GetLDAPClientWithReconnect(cfg)
  }
}

then you can call that method from here

manager.ldapClient, err = utils.GetLDAPClientFromConfig(&manager.c.LDAPConn)

[jvillafanez]

Some additional things:

Why are we trying to use a LDAP connection pool as a regular LDAP client?. I'm pretty sure this will end up causing problems. There are several unimplemented methods in the pool that will return errors. This is fine, but the problem is that the client that will be used (either the "real" client or the pool) is set in one place and used in another. Whoever will use the client won't know if he's using the "real" client or the pool, and as such won't know what methods aren't implemented.
The expectation is that, as LDAP client, all the methods are available, however, since the pool only implements a few of them, only those will be used. We're reducing the functionality and it won't be documented anywhere.

Why not use the LDAP pool as a pool? Either use a regular LDAP client, or use the LDAP pool, but don't try to use one as another because the behavior is naturally different. You can use a LDAP pool with only one slot if you want (assuming everything works fine)

There is a lack of context.Context in the pool's operations. I guess it's because the LDAP client doesn't have support of it.
I see a potential problem with the do method of the pool: I don't see a way to stop the retries. If I want to shutdown or restart the service, I'm not sure if it will stop the retries cleanly or we'll have to kill the app. The other problem is that the pool will exhaust the retries despite the context / request being done: user makes request -> user cancels request after 2 seconds -> LDAP request will still be made and retried.

The last thing is that blocking calls aren't common, so they should be explicitly documented to prevent possible issues in the future. It's true that those call are private, but they could be used internally in other places if we need to add new functionality. It would also help to document the expected behavior.

[2403905]

The Close() func in a ldap.Client interface returns err.
It will be good to log the error.

[2403905]

The Close() func in a ldap.Client interface returns err.
It will be good to log the error.

[2403905]

The ConnPool already has IsClosing() function
We can call if IsClosing() instead

[2403905]

better to use p.mu.RLock() p.mu.RUnlock()