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]

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]

One more point, this epic pointed to 8.0.7. We should work with the fork from stable-8.0
I created the feature branches in both ocis & reva repo feat/OCISDEV-1030-ldap-client

[LukasHirt]

Why are we trying to use a LDAP connection pool as a regular LDAP client?

This is per the acceptance criteria in the ticket:

Connection pooling implemented behind the existing ldap.Client interface (Story 1).

There are several unimplemented methods in the pool that will return errors.

I followed the same approach here as is done with the existing reconnect implementation.

[jvillafanez]

Does it makes more sense that the checkout and release use a counter instead of slots?
I think the current process is difficult to follow: channels are expected to be used to transfer data between goroutines, not to hold data. In addition, to checkout a client you need to write "something" in the slot instead of extracting from the slot; this seems counterintuitive.

You might want to check https://pkg.go.dev/golang.org/x/sync/semaphore as it seems more aligned to what you're trying to do. There is also the context.WithTimeout(....) that can be used with Acquire method of the semaphore.

[LukasHirt]

Does it makes more sense that the checkout and release use a counter instead of slots?

Switched sem to golang.org/x/sync/semaphore.Weighted (already a repo dependency) and use context.WithTimeout + Acquire/Release for the checkout deadline instead of the manual time.NewTimer/select, as suggested. Kept idle as a channel though — it's holding actual ldap.Client values that need to live somewhere, so a mutex-guarded slice/counter combo wouldn't really be simpler than the buffered channel, and release never blocking on it is a property I'd like to keep. See commit 427b8fd.

Separately, re-surfacing your earlier point about retries not respecting request cancellation (2026-07-16): that's intentionally out of scope for OCISDEV-1031 — retry/backoff behavior (including making it context-aware) is covered by the sibling story OCISDEV-1032, so I'd rather not grow this PR's scope to cover it. Flagging it explicitly here so it doesn't read as an oversight.

[jvillafanez]

Code looks good, but need to fix the tests. 👍