DNS resolution happens before connection pool lookup

[yzfeng2020]

Summary

The current implementation resolves (it may hit cache, though) DNS for every request before checking the connection pool, which can introduce unnecessary DNS queries even when pooled connections are available.

Expected behavior

Ideally, the client should:

1. Check the connection pool for an existing connection, ie using the hostname
2. Only resolve DNS if no pooled connection exists

This would minimize DNS queries, and in other words, only resolving DNS when needing to create new connections.

Actual behavior

Currently in HttpClientDelegate.execute():

1. DNS resolution happens first (if endpoint.hasIpAddr() returns false)
2. Then connection pool lookup occurs with the resolved IP

See:

armeria/core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java

Lines 125 to 139 in a74b849

	if (endpointWithPort.hasIpAddr() ||
	proxyConfig.proxyType().isForwardProxy()) {
	// There is no need to resolve the IP address either because it is already known,
	// or it isn't needed for forward proxies.
	acquireConnectionAndExecute(ctx, endpointWithPort, req, res, timingsBuilder, proxyConfig);
	} else {
	resolveAddress(endpointWithPort, ctx, (resolved, cause) -> {
	timingsBuilder.dnsResolutionEnd();
	if (cause == null) {
	assert resolved != null;
	acquireConnectionAndExecute(ctx, resolved, req, res, timingsBuilder, proxyConfig);
	} else {
	earlyCancelRequest(cause, ctx, timingsBuilder);
	}
	});

Why this is a problem

This means:

• Every request must resolve DNS
• It may create high load to the DNS server, even with DNS cache or refresh

[ikhoon]

If the IP address of a hostname changes, it would be risky to reuse the existing connections in the pool as the servers bound to the old IP address may no longer be valid. For instance, DNS has been updated during a load balancer migration, but it would be embarrassing if traffic still reaches to the old servers even after TTL expires.

I do think refreshing DNS records after TTL expires aligns with common industry implementations.

We may consider a special option to keep using the connections pool even after DNS TTL expires instead of performing a lookup.

[minwoox]

Every request must resolve DNS

IIRC, the DNS record is cached until its Time-to-Live (TTL) expires, which should prevent new DNS queries from being sent for every request.

armeria/core/src/main/java/com/linecorp/armeria/client/DefaultDnsCache.java

Lines 132 to 138 in 3ef8c77

	final long ttl = copied.stream()
	.mapToLong(DnsRecord::timeToLive)
	.min()
	.orElse(minTtl);
	final int effectiveTtl = Math.min(maxTtl, Math.max(minTtl, Ints.saturatedCast(ttl)));
	cache.put(question, new CacheEntry(cache, question, copied, null, executor, effectiveTtl));

If the client is sending a DNS query for each request, it is likely a bug that needs to be fixed.

[yzfeng2020]

@ikhoon Thanks for the explanation — that makes sense. I agree that continuing to reuse connections to old IPs can be risky. I can’t speak for other libraries, but in our previous setup with Jetty 9.4 (which we’re migrating off), the pattern I described is actually how connection reuse works.

My main concern is about avoiding unnecessary DNS lookups when a healthy connection already exists in the pool. In other words, even if the DNS TTL has expired, we might not need to re-resolve for each new request as long as the existing connections are still valid and open.

@minwoox Yes, I understand there’s caching. The situation we’re facing is that we have a very large number of fan out instances with ngrok, each with different hostnames that clients talk to, and in practice the resolved IPs rarely change. A full round of DNS queries therefore generates substantial load on the DNS servers. That’s also why we don’t enable DNS refresh — re-querying too often is undesirable in our environment.

Perhaps a balanced approach could be to introduce a configuration knob for users who prefer to minimize DNS queries.

[ikhoon]

Perhaps a balanced approach could be to introduce a configuration knob for users who prefer to minimize DNS queries.

It sounds better to me.

As a workaround, we may want to increase the minimum TTL if the IP addresses are not frequently changed.

armeria/core/src/main/java/com/linecorp/armeria/client/DnsCacheBuilder.java

Lines 82 to 91 in dd694a3

	* Sets the minimum and maximum TTL of the cached DNS resource records in seconds. If the TTL of the DNS
	* resource record returned by the DNS server is less than the minimum TTL or greater than the maximum TTL,
	* this resolver will ignore the TTL from the DNS server and use the minimum TTL or the maximum TTL instead
	* respectively.
	* The default value is {@code 1} and {@link Integer#MAX_VALUE}, which practically tells this resolver to
	* respect the TTL from the DNS server.
	*
	* <p>Note that if {@code maxTtl} is set to {@code 0}, the resolved DNS records are not cached.
	*/
	public DnsCacheBuilder ttl(int minTtl, int maxTtl) {

[jrhee17]

Ideally, the client should:
Check the connection pool for an existing connection, ie using the hostname
Only resolve DNS if no pooled connection exists
This would minimize DNS queries, and in other words, only resolving DNS when needing to create new connections.

Looks like @ikhoon beat me to a possible workaround - rather than keying by hostname, I'm also curious if adjusting (overriding) TTL would work for your scenario.

[minwoox]

Thanks for explaining.
It seems like increasing the minimum TTL would be a good approach if adjusting the TTL on the DNS server isn't an option.
I believe ignoring the TTL completely (by checking the connection pool first) is dangerous unless you are sure that the IP address never changes.