Dns2, the new OkHttp DNS API#9536
Conversation
Big new features are support for SVCB and being async.
swankjesse
left a comment
There was a problem hiding this comment.
@yschimke I studied DnsOverHttps a lot in putting this together, and I’m curious about why that class makes multiple HTTP requests in order to lookup A and AAAA records, rather than making a single HTTP request with two questions. (Is that also possible?)
|
I think it practice that is basically never supported in Dns. But we should check Dns over https servers in practice |
| * @param last true if no further calls to [onRecords] will be made for this call. | ||
| */ | ||
| fun onRecords(call: DnsCall, last: Boolean, records: List<DnsRecord>) | ||
| fun onFailure(call: DnsCall, e: IOException) |
There was a problem hiding this comment.
Is onFailure terminal? Or can part of the DnsCall fail?
There was a problem hiding this comment.
Oooh good question. I think for the users’ benefit it should be terminal. I’ve added a note as such.
If we have a partial failure like A records failing and AAAA records succeeding, the implementation could hold back the failure until after it sends the success. That gives the application layer the opportunity to work degraded if so desired.
There was a problem hiding this comment.
the implementation could hold back the failure until after it sends the success
Not sure that is ideal, why introduce delays?
There was a problem hiding this comment.
I don’t expect our caller will have much to do with a partial failure. For example, if IPv6 fails after 250ms and IPv4 succeeds after 500ms, this will likely result in an ultimate success after 500 ms.
There was a problem hiding this comment.
I thought you suggested the failure at 250ms was terminal?
There was a problem hiding this comment.
The Dns2 implementation can withhold onFailure() calls. (Alternatively, we have a last boolean in the onFailure function?)
There was a problem hiding this comment.
That's the delay I meant. I think we should signal that non-terminal failure, not hide it. Also is one failure, such as HTTPS actually an overall failure?
Maybe it's HTTPS not being found, and I want to change something when ECH not configured?
Maybe it's IPv6, and I want to use HTTPS address hints?
There was a problem hiding this comment.
I think overall, we can't predict which records will be relevant in future. And I think rather than being Dns for HTTP only, we should model something that could evolve over time.
So first questions that come to mind
a) is the scope purely A, AAAA, SVCB, HTTPS? or should we allow for more in future?
b) how do we make it safe when we add a new record, without breaking existing clients?
A few suggestions
- make the result non exhaustive, I've seen this done with a private
private object Invalid: DnsRecord()
So when clauses need an else
-
maybe support raw queries, or rather raw responses. Should this be part of DnsRecord, so any response is available raw?
-
Consider whether the abstraction is focused around OkHttp usage primarily for ease of use? or focused on modelling Dns correctly first? the service path is one example since not all fit _port._service.domain.
That's a decision we gotta make! I can see a few different things to possibly build... 1. A general purpose DNS API.That's gonna expose stuff like raw queries, TXT records, MX records, and recursive resolution. I think this would be a standalone Kotlin DNS library and it could be useful for all sorts of stuff like implementing ACME. I would like for this to exist, but not in OkHttp! I don't want the scope increase. 2. An API to support OkHttp’s Specific Needs OnlyI think this is A, AAAA, and HTTPS records. OkHttp needs to know about alpn, ech, ports, and IP addresses. It should honor the 3. Split the differenceThis is what I've got in this PR. It's almost all 'what does OkHttp need' plus the I am torn between YAGNI and it coming at a very low cost for anyone else using other protocols. Are there other protocols?! I don't expect there's many people using SVCB for other protocols. So under a tiny bit of reflection I think we lean hard into option 2: This is an API to provide DNS information to OkHttp, and not a general-purpose DNS thing. I will drop the
I think we do This interface is tricky because we're building an abstraction to be potentially implemented by anyone, and also potentially called by anyone. But I'd like to assume OkHttp is the primary caller of this interface, and other callers will be rare. (Why would you call this API? Probably to do a decorator pattern?) I'll make the
I think raw queries makes it more difficult to implement. I'd like developers to be able to implement this interface without parsing DNS records.
|
|
@swankjesse sounds good. I didn't mind which way the answer went, just that it was self consistent. |
|
@swankjesse and you should definitely do the standalone OkDns thing :) |
We can't use data classes in public APIs without making backwards-compatibility more difficult.
swankjesse
left a comment
There was a problem hiding this comment.
Next steps:
Change DnsSystem to implement this, in a very basic way that doesn’t actually add any new ServiceMetadata. It will still implement Dns. Use TaskQueue for callback concurrency.
Build a not-public-API adapter that turns a Dns instance into a Dns2 instance. It should also use TaskQueue for concurrency.
Change OkHttpClient to use Dns2 internally instead of Dns. Use the adapter to make Dns2 the canonical implementation. This will want a breaking change in Address, which expects a Dns. Can we avoid that? This may be difficult!
Implement Dns2 in DnsOverHttps. It will still implement Dns.
Build an implementation of Dns2 on Android’s DnsResolver. Enable that implementation by default on Android where it is available.
OkHttp 6? Dns2 extends Dns, and we keep identifying as Dns. Add an extra dns2 field? |
| /** | ||
| * This is a terminal event and no further calls to this callback will be made for this call. | ||
| */ | ||
| fun onFailure(call: Call, e: IOException) |
There was a problem hiding this comment.
Do we chain these via suppressions? Let's say HTTPS works with hints, but A and AAAA fai?
There was a problem hiding this comment.
Probably.
I don't like situations where I'm digging through suppressed exceptions programmatically. So if there's some useful data exposed that way, I'm inclined to expose it in a different way.
| * | ||
| * Implementations of this interface must be safe for concurrent use. | ||
| */ | ||
| interface Dns2 { |
There was a problem hiding this comment.
Should we use an experimental annotation for a couple of releases?
There was a problem hiding this comment.
I'm unsure. If we make it opt-in that makes it more difficult for us to use it in types like OkHttpClient and Address.
The impossible challenge is "what will we regret?" and I think you've come up with a few possibilities! Partial failure for example.
That's where I intend to start. And if/when we do OkHttp6 we might be able to drop the old |
Big new features are support for SVCB and being async.