pickfirst: Interleave IPv6 and IPv4 addresses for happy eyeballs #7742
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7742 +/- ##
==========================================
+ Coverage 81.71% 81.88% +0.17%
==========================================
Files 374 373 -1
Lines 38166 37735 -431
==========================================
- Hits 31188 30901 -287
+ Misses 5699 5550 -149
- Partials 1279 1284 +5
|
b9b602c to
f68c03d
Compare
f68c03d to
f7dcbc9
Compare
e4a4300 to
7b42222
Compare
|
|
||
| func addressFamily(address string) ipAddrFamily { | ||
| // Try parsing addresses without a port specified. | ||
| ip := net.ParseIP(address) |
There was a problem hiding this comment.
Can we not safely assume a port is always present (if it's any IP address)?
I was pretty sure we require that? The DNS resolver is where we add the default port to addresses if needed. So I think we can always do SplitHostPort and return Unknown on error.
There was a problem hiding this comment.
You're correct, the port must be specified. I didn't check if the default port is added in the DNS resolver or somewhere before the dialer. I tried using passthrough to omit the port and the rpc fails with the following error:
code = Unavailable desc = connection error: desc = "transport: Error while dialing: dial tcp: address 127.0.0.1: missing port in address"
Removed the handling of addresses without ports, updating the test cases accordingly.
| // If using a resolver like passthrough, the hostnames may not be IP | ||
| // addresses but in some format that the dialer can parse. | ||
| switch { | ||
| case ip == nil: |
There was a problem hiding this comment.
This could be simplified to:
switch len(ip) {
case IPv4len:
return ipAddrFamilyV4
case IPv6len:
return ipAddrFamilyV16
default:
return ipAddrFamilyUnknown
}There was a problem hiding this comment.
This will not work as the length of the slice held by net.IP is always 16 bytes, see https://stackoverflow.com/a/22751285.
This did uncover an edge case where ip.To4() returns true for IPv4-mapped IPv6 addresses like ::FFFF:127.0.0.1. I've added the check to classify them as IPv6 for interleaving purposes and updated a test case to verify this.
0152d54 to
26425ef
Compare
| // Check for existence of IPv4-mapped IPv6 addresses, which are | ||
| // considered as IPv4 by net.IP. | ||
| // e.g: "::FFFF:127.0.0.1" | ||
| case ip.To4() != nil && !strings.Contains(host, ":"): |
There was a problem hiding this comment.
IPv4-mapped IPv6 addresses probably weren't considered in the dual stack design at all.
I chatted with @ejona86 about this and we think it's unnecessary to check for the colon here. If the resolver produces an IPv4-mapped IPv6 address, we can treat it as either IPv4 or IPv6, whichever is more convenient.
There was a problem hiding this comment.
Acknowledged, removed the special handling.
This PR adds the address interleaving mentioned in A61
RELEASE NOTES:
pickfirstleafLB policy interleaves IPv4 and IPv6 address as described in RFC-8305 section 4 before starting connection attempts.