fix: try all resolved addresses when connecting a TLS link - #2689
fix: try all resolved addresses when connecting a TLS link#2689nikitatsym wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the TLS unicast connection path to behave like the existing TCP link logic when a hostname resolves to multiple IP addresses, by attempting multiple resolved socket addresses instead of repeatedly retrying only the first resolution result.
Changes:
- Added
get_tls_addrs()to resolve and return all non-multicast socket addresses for a TLS endpoint (mirroring TCP’sget_tcp_addrs()pattern). - Updated
LinkManagerUnicastTls::new_linkto iterate over resolved addresses, attempt connections, and aggregate connection errors when all attempts fail.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| io/zenoh-links/zenoh-link-tls/src/utils.rs | Adds a multi-address resolver helper (get_tls_addrs) for TLS endpoints. |
| io/zenoh-links/zenoh-link-tls/src/unicast.rs | Uses the new resolver helper to attempt TCP connection across all resolved addresses and aggregate failures. |
Comments suppressed due to low confidence (1)
io/zenoh-links/zenoh-link-tls/src/unicast.rs:394
- The address-iteration logic only covers the TCP connect step. If a TCP connect succeeds but the TLS handshake fails,
new_linkwill return that handshake error without trying the remaining resolved addresses, even though another address might work. To fully implement "try all resolved addresses when connecting a TLS link", consider moving the TLS handshake into the per-address loop and continuing on handshake failure while accumulating errors.
// Initialize the TlsStream
let tls_stream = connector
.connect(server_name.to_owned(), tcp_stream)
.await
.map_err(|e| {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2689 +/- ##
==========================================
- Coverage 74.51% 74.50% -0.02%
==========================================
Files 417 417
Lines 62942 62996 +54
==========================================
+ Hits 46901 46933 +32
- Misses 16041 16063 +22 ☔ View full report in Codecov by Harness. |
|
For the label check: this PR is a bug fix (the TLS link only ever dials the first resolved address). Could a maintainer add the On the Copilot comment about retrying the TLS handshake across addresses - that's a slightly different scope: this PR is about bringing the TLS link in line with the existing TCP link behaviour, where the iteration covers the connect phase only. Glad to extend it in a follow-up. |
|
Thanks for adding the |
fbcc6f0 to
46893e4
Compare
Signed-off-by: Nikita Tsymbal <nikita.tsym@gmail.com>
Signed-off-by: Nikita Tsymbal <nikita.tsym@gmail.com>
46893e4 to
d29f945
Compare
|
@milyin @diogomatsubara Rebased on latest main and added unit tests covering the multi-address connect fallback. |
Problem
When connecting a
tls/<hostname>:<port>endpoint, the TLS link resolves the hostname and dials only the first resolved address (get_tls_addr()returnslookup_host().next()). If that address is unreachable, the retry loop keeps redialing the same dead address untilconnect/timeout_msexpires — even when another resolved address is reachable.The plain TCP link already handles this correctly:
get_tcp_addrs()returns all resolved addresses andLinkManagerUnicastTcp::new_linkiterates over them, collecting errors (io/zenoh-links/zenoh-link-tcp/src/unicast.rs).Real-world impact
Client on an LTE hotspot whose resolver synthesizes DNS64 AAAA records (
64:ff9b::/96) without a working NAT64 route — common on mobile networks.getaddrinforeturns the synthesized (dead) IPv6 first and the working IPv4 second. A zenoh 1.9.0 client fails with:ncandopenssl s_clientto the same hostname succeed from the same machine because they iterate over resolved addresses.Fix
Mirror the TCP link pattern in the TLS link: add
get_tls_addrs()(all resolved addresses, multicast filtered, same shape asget_tcp_addrs());LinkManagerUnicastTls::new_linkiterates, uses the first successful TCP connection, collects per-address errors, and reports all attempts on total failure. Listener paths unchanged. QUIC/WS links appear to have the same single-address behavior — happy to extend the same pattern there if desired.Testing
connect_first_reachableinzenoh-link-tls): a dead address falls through to a reachable one; total failure reports every attempted address; an empty address list is an error. Deterministic (loopback only, no DNS).cargo test/check/fmt/clippyclean onzenoh-link-tls.🏷️ Label-Based Checklist
Based on the labels applied to this PR, please complete these additional requirements:
Labels:
bug🐛 Bug Fix Requirements
Since this PR is labeled as a bug fix, please ensure:
Why this matters: Bugs without tests often reoccur.
Instructions:
- [ ]to- [x])This checklist updates automatically when labels change, but preserves your checked boxes.