refactor Resolver to support custom per-TLD resolvers #26
Conversation
resolve.go
Outdated
| parts := strings.Split(domain, ".") | ||
| tld := parts[len(parts)-1] |
There was a problem hiding this comment.
Matching only the last DNS label may be not enough: TLD in the contexts we care about can have more than one label, for example .co.uk. In practice, anything from https://publicsuffix.org/list/public_suffix_list.dat can act as de facto TLD in the browser context and our solution should be flexible enough to support a custom resolver for each.
For foo.bar.example.com we should check com first, but if not found, retry with example.com, bar.example.com and foo.bar.example.com
This will not only make our solution flexible enough to support PublicSuffixList for browsers, but also enable users to set up a custom resolver per domain name (when the full name is a match).
There was a problem hiding this comment.
For foo.bar.example.com we should check com first, but if not found, retry with example.com, bar.example.com and foo.bar.example.com
@lidel shouldn't this go the other way (i.e. check if we have an entry for foo.bar.example.com, then (if not found) bar.example.com then example.com then com)?
There was a problem hiding this comment.
implemented left to right resolution; it will first try the full domain and then all the subdomains, left to right and fall back to the default.
There was a problem hiding this comment.
Ack, left to right works better and enables multiple levels of rules within a single TLD.
| type Resolver struct { | ||
| Backend Backend |
There was a problem hiding this comment.
@vyzo IIRC this is making a choice to break backwards compatibility here a bit for the sake of future extensibility.
Two breakages:
- Rename
BackendtoBasicResolver, both not perfect names butBackendis probably worse.- We're not really gaining much out of the name change, but it also seems low cost for people to update. If we really wanted to we could just alias
type Backend BasicResolverand deprecate it, but idk if it's worth bothering
- We're not really gaining much out of the name change, but it also seems low cost for people to update. If we really wanted to we could just alias
- Constructing
Resolveris now different and we can't doResolver.Backendanymore to get out theBasicResolver- Neither of these seems like a big deal and moving to a construction with
Optionsgives us more flexibility to add more options in the future
- Neither of these seems like a big deal and moving to a construction with
cc @Stebalien in case you have any issues with the breakages.
There was a problem hiding this comment.
Yeah, conscious choice -- having the fields public was just a bad idea.
I think the original emerged in a hacky way when the ability to change the backend was added.
to ensure specific resolver supersedes generic one
|
|
||
| const dnsaddrTXTPrefix = "dnsaddr=" | ||
|
|
||
| type Backend interface { | ||
| // BasicResolver is a low level interface for DNS resolution | ||
| type BasicResolver interface { |
There was a problem hiding this comment.
Not sure if I have enough context here, but doesn't DNS allow us to lookup A, AAAA and TXT records in the same query? This interface suggests that looking up both would cause 2 queries (and potentially 2 roundtrips).
There was a problem hiding this comment.
In theory it does, but in practice it doesn't work as most DNS servers only respond to the first one.
There is a caveat about that in the dns library.
There was a problem hiding this comment.
Also, they have different usage, we don't do concurrent A/AAAA and TXT resolution in our usage patterns.
There was a problem hiding this comment.
To be fair, at some point may have some overlap in our usage patterns.
For example, a request for DNSLink might be accompanied by a lookup for a content routing hint (if we implement some form of ipfs/kubo#6516).
That being said, I don't think we should do any premature optimizations.
When time comes, adding cache for raw DNS records (that respects their TTL) will be enough.
vyzo
force-pushed
the
feat/rework-resolver
branch
from
7f98306 to
45cdfcf
Compare
|
Let's wait for @Stebalien to chime in wrt the interface (breaking) changes before merging. |
|
@Stebalien GoForIt'ed in slack, so merging. |
See protocol/web3-dev-team#42
We will have to implement our own DoH basic resolver, as the search for a viable library proved fruitless. But that should live in its own repo, it doesn't belong to
madns.