Skip to content

Retry transient LDAP connection failures in binds and searches - #86

Merged
hosom merged 6 commits into
mainfrom
hosom/retry-ldap-connection
Nov 4, 2026
Merged

hosom merged 6 commits into
mainfrom
hosom/retry-ldap-connection

Conversation

@hosom

@hosom hosom commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor
  • Retry transient LDAP connection refusals, DNS failures, and timeouts during initial binds and read-only searches with bounded backoff.
  • Discard partial search results before retrying; preserve permanent errors and never replay writes.
  • Fix Ruby CI setup by pinning affected workflows to Ubuntu 24.04.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 98c5825a-bd27-4391-a924-14e339aab71e
Copilot AI auto review requested due to automatic review settings November 3, 2026 21:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues block the change adding bounded retries to initial LDAP connection setup.

No open issues 🎉

What changed in this PR

This PR makes initial LDAP connection setup resilient to transient connection refusals without replaying searches or mutations.

Changes:

  • Retry Errno::ECONNREFUSED up to three total attempts, with 1- and 2-second delays and retry/exhaustion logging.
  • Create a fresh client for each attempt and cache only a successfully bound client.
  • Add regression coverage for recovery, exhaustion, caching, and failures that must not be retried.
File Description
spec/​unit/​entitlements/​service/​ldap_spec.rb Adds retry regressions and verifies non-retryable failures and operations remain unchanged.
lib/​entitlements/​service/​ldap.rb Adds bounded connection-refusal retries and logging around initial LDAP setup.

Review effort: Balanced (auto)

hosom and others added 2 commits November 3, 2026 17:03
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 98c5825a-bd27-4391-a924-14e339aab71e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 98c5825a-bd27-4391-a924-14e339aab71e
@hosom hosom changed the title Retry initial LDAP binds after connection refusals Retry transient LDAP connection failures in binds and searches Nov 3, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 98c5825a-bd27-4391-a924-14e339aab71e

@northrup northrup left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry shape looks right to me. The part I'd change is the classifier: exact-message matching depends on libc wording, and it misses a few transient failures. I ran it against real failures (net-ldap 0.19.0, Ruby 3.4.9):

Failure This PR (glibc) This PR (musl) Suggested
Connection refused ✅ ✅ ✅
Connect timeout ✅ ✅ ✅
DNS EAI_AGAIN ✅ ❌ ✅
TLS handshake timeout ❌ ❌ ✅
Accept, then reset (LB with no backend) ❌ ❌ ✅
NXDOMAIN – – ✅
Bad cert – – –

✅ retried · ❌ should have been · – not retried

musl spells EAI_AGAIN as getaddrinfo: Try again, so the DNS retry never fires on Alpine.

The good news is net-ldap does the classifying for us. Every single-host connection-setup failure comes back as a bare Net::LDAP::Error (error.rb), and operation errors are subclasses. So key on the class and deny-list the permanent TLS failures. Getting it wrong that way costs ~3s; getting it wrong the current way fails the run.

Suggestions are inline. With them applied, RuboCop and the unit suite are unchanged, and the new specs fail against the current classifier.

Two smaller things:

  • 1s + 2s covers a blip, not a restart or failover. It depends on what bit you.
  • The runner pin is setup-ruby's documented workaround for ubuntu-26.04. It might merge faster as its own PR.
Repro
# Usage: ruby repro.rb [dns]    (requires: gem install net-ldap -v 0.19.0)
require "net/ldap"
require "socket"
require "openssl"

def pr86?(e) # PR #86 as written
  return false unless [Errno::ECONNREFUSED, Errno::ETIMEDOUT, Net::LDAP::Error].any? { |k| k === e }
  return true if e.is_a?(Errno::ECONNREFUSED) || e.is_a?(Errno::ETIMEDOUT)
  return false unless e.instance_of?(Net::LDAP::Error)
  ["getaddrinfo: Temporary failure in name resolution",
   "Connection timed out - user specified timeout",
   "Operation timed out - user specified timeout"].include?(e.message)
end

def suggested?(e) # suggested rescue + retryable_connection_error?
  return false unless [SystemCallError, Net::LDAP::Error].any? { |k| k === e }
  case e
  when Errno::ECONNREFUSED, Errno::ECONNRESET, Errno::EPIPE, Errno::ETIMEDOUT then true
  when Net::LDAP::Error
    e.instance_of?(Net::LDAP::Error) && !e.message.match?(/certificate verify failed|does not match the server certificate/)
  else false
  end
end

def bind(label, host, port, tls: false)
  opts = { host: host, port: port, connect_timeout: 1, auth: { method: :simple, username: "cn=x", password: "x" } }
  opts[:encryption] = { method: :simple_tls, tls_options: { verify_mode: OpenSSL::SSL::VERIFY_PEER } } if tls
  Net::LDAP.new(opts).bind
rescue => e
  puts format("%-20s pr86=%-5s suggested=%-5s %s: %s", label, pr86?(e), suggested?(e), e.class, e.message[0, 55])
end

def listener(&handler)
  server = TCPServer.new("127.0.0.1", 0)
  Thread.new { loop { handler.call(server.accept) rescue nil } }
  server.addr[1]
end

if ARGV == ["dns"]
  bind("dns-eai-again", "ldap.example.com", 389)
  exit
end

closed = TCPServer.new("127.0.0.1", 0)
refused_port = closed.addr[1]
closed.close
bind("refused", "127.0.0.1", refused_port)

# A full accept queue makes Linux drop SYNs, giving a genuine connect timeout.
backlog = Socket.new(:INET, :STREAM)
backlog.bind(Addrinfo.tcp("127.0.0.1", 0))
backlog.listen(0)
fillers = 3.times.map { Socket.new(:INET, :STREAM).tap { |c| c.connect_nonblock(backlog.local_address, exception: false) } }
sleep 0.3
bind("connect-timeout", "127.0.0.1", backlog.local_address.ip_port)

bind("nxdomain", "nonexistent.invalid", 389)
bind("tls-handshake-stall", "127.0.0.1", listener { |c| Thread.new { sleep 30; c.close } }, tls: true)
bind("accept-then-reset", "127.0.0.1", listener(&:close))

key = OpenSSL::PKey::RSA.new(2048)
cert = OpenSSL::X509::Certificate.new
cert.version = 2
cert.serial = 1
cert.subject = cert.issuer = OpenSSL::X509::Name.parse("/CN=localhost")
cert.public_key = key.public_key
cert.not_before = Time.now - 60
cert.not_after = Time.now + 3600
cert.sign(key, OpenSSL::Digest.new("SHA256"))
ctx = OpenSSL::SSL::SSLContext.new
ctx.cert = cert
ctx.key = key
tls_server = OpenSSL::SSL::SSLServer.new(TCPServer.new("127.0.0.1", 0), ctx)
Thread.new { loop { tls_server.accept rescue nil } }
bind("cert-verify-failed", "127.0.0.1", tls_server.to_io.addr[1], tls: true)
for img in ruby:3.4.9-slim ruby:3.4.9-alpine; do
  docker run --rm -v "$PWD/repro.rb:/repro.rb:ro" "$img" sh -c '
    gem install -q net-ldap -v 0.19.0 >/dev/null &&
    ruby /repro.rb &&
    printf "nameserver 192.0.2.1\noptions timeout:1 attempts:1\n" > /etc/resolv.conf &&
    ruby /repro.rb dns'
done

Comment thread lib/entitlements/service/ldap.rb Resolved
Comment thread lib/entitlements/service/ldap.rb Resolved
Comment thread lib/entitlements/service/ldap.rb Resolved
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 98c5825a-bd27-4391-a924-14e339aab71e
northrup
northrup previously approved these changes Nov 4, 2026

@northrup northrup left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 98c5825a-bd27-4391-a924-14e339aab71e
@hosom
hosom merged commit eca2c47 into main Nov 4, 2026
20 checks passed
@hosom
hosom deleted the hosom/retry-ldap-connection branch November 4, 2026 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants