LookupSession doesn't seem to cache CNAMEs

[dropofwill]

For example, given a cache like:

{Integer@2917} 1 -> {Cache@2918} "{ _dmarc.pa.gov. 86337 IN CNAME [pa.gov.hosted.dmarc-report.com.] } cl = 3
{ pa.gov.hosted.dmarc-report.com. 237 IN TXT ["v=DMARC1; p=quarantine; sp=quarantine; rua=mailto:f8debbc3@mxtoolbox.dmarc-report.com; ruf=mailto:f8debbc3@forensics.dmarc-report.com; fo=0"] } cl = 3

And a call like this with a LookupSession from the default builder (which has a Cache):

lookupSession.lookupAsync(Name.fromString("_dmarc.pa.gov."), Type.TXT)
...

When it hits lookupWithRecords here:

dnsjava/src/main/java/org/xbill/DNS/lookup/LookupSession.java

Lines 470 to 475 in 6204b0e

	private CompletionStage<LookupResult> lookupWithCache(Record queryRecord, List<Name> aliases) {
	return Optional.ofNullable(caches.get(queryRecord.getDClass()))
	.map(c -> c.lookupRecords(queryRecord.getName(), queryRecord.getType(), Credibility.NORMAL))
	.map(setResponse -> setResponseToMessageFuture(setResponse, queryRecord, aliases))
	.orElseGet(() -> lookupWithResolver(queryRecord, aliases));
	}

c.lookupRecords finds the entry, but setResponseToMessageFuture converts it to null, because the response wasn't 'successful' as per this:

dnsjava/src/main/java/org/xbill/DNS/lookup/LookupSession.java

Lines 495 to 513 in 6204b0e

	private CompletionStage<LookupResult> setResponseToMessageFuture(
	SetResponse setResponse, Record queryRecord, List<Name> aliases) {
	if (setResponse.isNXDOMAIN()) {
	return completeExceptionally(
	new NoSuchDomainException(queryRecord.getName(), queryRecord.getType()));
	}
	if (setResponse.isNXRRSET()) {
	return completeExceptionally(
	new NoSuchRRSetException(queryRecord.getName(), queryRecord.getType()));
	}
	if (setResponse.isSuccessful()) {
	List<Record> records =
	setResponse.answers().stream()
	.flatMap(rrset -> rrset.rrs(cycleResults).stream())
	.collect(Collectors.toList());
	return CompletableFuture.completedFuture(new LookupResult(records, aliases));
	}
	return null;
	}

I've tried this with a handful of CNAMEs and they all seem to fail regardless of TTL.

Honestly, I'm fuzzy on how caching in the DNS spec works with CNAMEs, so maybe this is working as intended. It seems like maybe the native JVM dns resolver work similarly (still investigating that, but just based on how long subsequent queries take it might not be?).

[ibauersachs]

Thanks, I'll need to investigate that and read up on TTLs too. At first glance though, line 505 should probably just check for .isCNAME() / .isDNAME() as well.

[dropofwill]

Made a POC PR with your suggestion and it does seem to roughly work from my manual testing.

For TTLs it seems that each record is supposed to have its own TTL. I'm not sure what that means for a client, maybe it should just take the min TTL, I'm not sure. Haven't looked into the Cache internals to see how it handles TTLs yet.