Properly report DNSLink errors #12
Conversation
gammazero
requested review from
aschmahmann
and removed request for
aschmahmann
Only report that there is no DNSLink for a name when there are no DNSLink TXT records available for that name. For all other errors, such as being offline, report the more general "cannot resolve name" error.
gammazero
force-pushed
the
fix/distinguish-dns-errors
branch
from
42b87bb to
4dedb19
Compare
There was a problem hiding this comment.
Thank you @gammazero!
Limiting this to DNSlink lgtm.
To be extra sure, took this for a spin against today's master of go-ipfs and works as expected, each type gets correct error now:
$ ipfs resolve -r /ipns/k51qzi5uqu5dgutdk6i1ynyzgkqngpha5xpgia3a5qqp4jsh0u4csozksxel2r
Error: could not resolve name
$ ipfs resolve -r /ipns/error.example.com
Error: could not resolve name: "error.example.com" is missing a DNSLink record (https://docs.ipfs.io/concepts/dnslink/)
$ ipfs resolve -r /ipns/remind-me-why-we-support-proquints
Error: not a valid proquint string
dns.go
Outdated
| i-- | ||
| } | ||
| // Wrap error so that it can be tested if it is a ErrResolveFailed | ||
| err := fmt.Errorf("%w: %q is missing a DNSLink record (https://docs.ipfs.io/concepts/dnslink/)", ErrResolveFailed, name[i+1:]) |
There was a problem hiding this comment.
Does this mean "is missing a DNSLink record, or the host could not be found" (or some phrasing like that)?
It looks like we're also covering dnsErr.IsNotFound which is "host could not be found" https://golang.org/pkg/net/#DNSError
There was a problem hiding this comment.
This error covers both. So, r.lookupTXT(name) returns a host not found error when DNS does not return a TXT record, but in either case (1. Has TXT records, but none contain DNSLink. 2. No TXT records returned) it is correct that name is missing a DNSLink record.
There was a problem hiding this comment.
If your machine is offline to you also get a host not found?
There was a problem hiding this comment.
No. You do not. I determine that by checking the dnsErr.IsNotFound flag here. If you are offline, then IsNotFound is false.
There was a problem hiding this comment.
If you are offline, then IsNotFound is false.
The DNSError struct documentation is kind of sparse ("host not found" seems like it could mean a bunch of things). If you've tested that IsNotFound doesn't apply when you cannot reach DNS then 👍
- Give precedence to good results from looking up DNSLinks in TXT records from `"_dnslink."+fqdn` over results from `fqdn`. - Document this as intentional in the code
Only report that there is no DNSLink for a name when there are no DNSLink TXT records available for that name. For all other errors, such as being offline, report the more general "cannot resolve name" error.
This fixes issue #11