Rfc6724 resolver ordering semantics #778
Conversation
Define INTERLEAVE as no-bias and align tests and debug output.
arturobernalg
force-pushed
the
Rfc6724A
branch
from
0802ffe to
3a1bd12
Compare
rschmitt
left a comment
rschmitt
left a comment
There was a problem hiding this comment.
Main thing here is that I'd like to see a proper implementation of INTERLEAVE, more unit test coverage, and an example program we can use for manual testing.
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| delegate = Mockito.mock(DnsResolver.class); |
There was a problem hiding this comment.
Seems like InMemoryDnsResolver would also be fine here?
| final InetAddress DA = a.dst; | ||
| final InetAddress DB = b.dst; | ||
| final InetAddress SourceDA = a.src; | ||
| final InetAddress SourceDB = b.src; |
There was a problem hiding this comment.
These should be named something like aDst, bDst, aSrc, and aDst respectively, so it doesn't look like you're calling static methods later on
| final int preferDA = -1; | ||
| final int preferDB = 1; | ||
|
|
||
| // Rule 1: Avoid unusable destinations. |
There was a problem hiding this comment.
This is specifically section 6 (destination address selection); there are separate rules for source address selection
| } | ||
| if (ip.isMulticastAddress()) { | ||
| if (ip instanceof Inet6Address) { | ||
| // RFC 4291: low 4 bits of second byte are scope. |
There was a problem hiding this comment.
Also refer to RFC 6724 §3.1 which explains how scope comparisons work between multicast and unicast addresses
| } | ||
|
|
||
| @Override | ||
| public InetAddress[] resolve(final String host) throws UnknownHostException { |
There was a problem hiding this comment.
I think that debug logging should just log the resolution result. Everything else should be logged at trace level.
| if (ip.isLinkLocalAddress()) { | ||
| return Scope.LINK_LOCAL; | ||
| } | ||
| if (ip.isMulticastAddress()) { |
There was a problem hiding this comment.
We're writing this for use in an HTTP client, not as a general-purpose resolver (e.g. JEP 418), and I'm pretty sure TCP connections can only be established to unicast addresses. Should we just filter out multicast addresses?
| /** | ||
| * Interleave address families (v6, then v4, then v6, …) when multiple | ||
| * addresses are available. When staggered connects are enabled, the first | ||
| * address of the other family is delayed by a small offset. | ||
| */ | ||
| INTERLEAVE |
There was a problem hiding this comment.
This is identical to DEFAULT. Why not just implement interleaving here? It's potentially useful, even without staggered concurrent connection attempts; for example, connection failures can trigger a retry on a different address family, since I believe our default behavior is to retry on the next IP address when more than one is returned by the resolver.
| InetAddress src = null; | ||
| try (final DatagramSocket s = new DatagramSocket()) { | ||
| s.connect(dest); // does not send packets; OS picks source addr/if | ||
| src = s.getLocalAddress(); |
There was a problem hiding this comment.
You probably want to inject a thing here that you can mock for unit testing purposes, e.g. a Function<InetSocketAddress, InetAddress> (or an equivalent that throws IOException).
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.Mockito; | ||
|
|
||
| class Rfc6724AddressSelectingDnsResolverTest { |
There was a problem hiding this comment.
I think we need more unit test coverage here. I'd also like to see an integration test or a runnable example (with debug/trace logging enabled) that resolves a given DNS name. That would afford some non-trivial manual testing using loopback addresses, /etc/hosts, Docker networking, etc.
| final boolean preferV6 = pref == ProtocolFamilyPreference.PREFER_IPV6; | ||
| final List<InetAddress> first = new ArrayList<>(); | ||
| final List<InetAddress> second = new ArrayList<>(); | ||
| for (final InetAddress a : rfcSorted) { | ||
| final boolean isV6 = a instanceof Inet6Address; | ||
| if (preferV6 && isV6 || !preferV6 && !isV6) { | ||
| first.add(a); | ||
| } else { | ||
| second.add(a); | ||
| } | ||
| } | ||
| final List<InetAddress> merged = new ArrayList<>(rfcSorted.size()); | ||
| merged.addAll(first); | ||
| merged.addAll(second); |
There was a problem hiding this comment.
Since we're already relying on the JDK to provide a stable sort, you can write this as:
List<InetAddress> merged =
rfcSorted.stream()
.sorted(Comparator.comparing(
a -> ((a instanceof Inet6Address) != preferV6)
))
.toList();Add manual gated IT to dump DEFAULT vs INTERLEAVE results. Expand unit coverage for scope mapping and core RFC comparison rules.
|
@rschmitt Please do another pass. I think i solve all your remarks |
2 participants
Introduce Rfc6724AddressSelectingDnsResolver and unit tests.
Define INTERLEAVE as “no bias” (preserve RFC6724 order); keep ONLY/PREFER behavior unchanged.