Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: net: add QueryType to *DNSError #65288

Open
Skeeve opened this issue Jan 25, 2024 · 17 comments
Open

proposal: net: add QueryType to *DNSError #65288

Skeeve opened this issue Jan 25, 2024 · 17 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal
Milestone

Comments

@Skeeve
Copy link

Skeeve commented Jan 25, 2024

Go version

go version go1.21.6 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/scripts/.cache/go-build'
GOENV='/home/scripts/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.6'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='0'
GOMOD='/home/scripts/src/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1931870782=/tmp/go-build -gno-record-gcc-switches'

What did you do?

https://go.dev/play/p/oN0gVQA4nm2

Please Note: On goplay lookups don't work. You have to run it locally.

What did you see happen?

Host Lookup
[208.73.7.224]

TXT Lookup
[]
lookup mail10476.emails3.rightmove.co.uk on 192.168.65.7:53: no such host

What did you expect to see?

Instead of "no such host" I would hav expected "no TXT record"

See also

https://stackoverflow.com/questions/77880685/golang-net-lookuptxt-returns-no-such-host

@Skeeve Skeeve changed the title net.LookupTXT returns 'no such host'import/path: issue title net.LookupTXT returns 'no such host' Jan 25, 2024
@seankhliao seankhliao changed the title net.LookupTXT returns 'no such host' net: LookupTXT errors should not say "no such host" Jan 25, 2024
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 25, 2024
@callthingsoff
Copy link
Contributor

After a quick look at the code, a simple and ugly fix looks like this.

--- a/src/net/lookup.go
+++ b/src/net/lookup.go
@@ -856,6 +856,11 @@ func (r *Resolver) goLookupNS(ctx context.Context, name string) ([]*NS, error) {
 func (r *Resolver) goLookupTXT(ctx context.Context, name string) ([]string, error) {
        p, server, err := r.lookup(ctx, name, dnsmessage.TypeTXT, nil)
        if err != nil {
+               if e, ok := err.(*DNSError); ok {
+                       if e.Err == errNoSuchHost.Error() {
+                               e.Err = "no TXT record"
+                       }
+               }
                return nil, err
        }
        var txts []string

@Skeeve
Copy link
Author

Skeeve commented Jan 26, 2024

That wouldn't work. Please replace the hostname in my example with one that doesn't exist, e.g. put an "x" in front of the name.

Your "fix" would say that there is no such TXT record. While it technically is true, it's not consisternt with the host command:

$ host -t TXT xmail10476.emails3.rightmove.co.uk
Host xmail10476.emails3.rightmove.co.uk not found: 3(NXDOMAIN)

$ host -t TXT mail10476.emails3.rightmove.co.uk
mail10476.emails3.rightmove.co.uk has no TXT record

Is there any way to get the underlying DNS errors?

@mateusz834
Copy link
Member

mateusz834 commented Jan 26, 2024

We currently return "no such host" for NXDOMAIN and NODATA conditions. Maybe we should change that and return "no such host" (or "no such domain") and "no records found"?
We might also introduce a QueryType field into the *net.DNSError, so that we can show the queried record type in the error message.

@Skeeve
Copy link
Author

Skeeve commented Jan 26, 2024

We currently return "no such host" for NXDOMAIN and NODATA conditions. Maybe we should change that and return "no such host" (or "no such domain") and "no records found"?

I think, this is a good idea.

In my current project I'm using Perl for the lookups I have to do. But I want to switch to go. Unfortunately I currently report the DNS errors to my customers and with the net module I will have to change the results. I don't think it's a big issue in my case, but at least an inconvenience.

We might also introduce a QueryType field into the *net.DNSError, so that we can show the queried record type in the error message.

This would be perfect.

@mateusz834
Copy link
Member

Ok, lets make this a proposal then.

I propose adding a QueryType field to DNSError:

package net

type DNSError struct {
    // (...)
    QueryType string
}

func (e *DNSError) Error() string {
	if e == nil {
		return "<nil>"
	}
	s := "lookup: " 
        if e.QueryType != "" {
            s += e.QueryType + ": "
        }
        s += e.Name
	if e.Server != "" {
		s += " on " + e.Server
	}
	s += ": " + e.Err
	return s
}

The only question remains how to handle LookupHost errors, I think in that case it should be set to QueryType = "host" instead of A or AAAA, so that we don't have different behavior between platforms.

The "no such host"/"no records found" change does not require a proposal.

@mateusz834 mateusz834 changed the title net: LookupTXT errors should not say "no such host" proposal: net: add QueryType to *DNSError Jan 26, 2024
@gopherbot gopherbot added this to the Proposal milestone Jan 26, 2024
@kostix
Copy link

kostix commented Jan 26, 2024

I would first make sure what @Skeeve's original wish regarding the stated problem was.
If it is purely about how errors of such class look when printed, — that is one thing; if, instead, they would like to be able to programmatically differentiate between NXDOMAIN and missing records of the queried class, — that's another story.

I have one more concern (maybe misguided): I would not say that having zero TXT records for an existing domain is an error to begin with. In the OP's case I would expect net.LookupTXT to return nil, nil or []string{}, nil which genuinely means "there was no errors, and the authoritative DNS server returned zero records of the requested type".
IOW, I'd say it's a client's issue to present this situation to a user as an error, if desired.

@mateusz834
Copy link
Member

I have one more concern (maybe misguided): I would not say that having zero TXT records for an existing domain is an error to begin with. In the OP's case I would expect net.LookupTXT to return nil, nil or []string{}, nil which genuinely means "there was no errors, and the authoritative DNS server returned zero records of the requested type".
IOW, I'd say it's a client's issue to present this situation to a user as an error, if desired.

I don't think that this kind of change is backwards compatible at this point.

@Skeeve
Copy link
Author

Skeeve commented Jan 26, 2024

I would first make sure what @Skeeve's original wish regarding the stated problem was. If it is purely about how errors of such class look when printed, — that is one thing; if, instead, they would like to be able to programmatically differentiate between NXDOMAIN and missing records of the queried class, — that's another story.

First of all I thought that "no such host" for a missing TXT record is plain wrong. That's where I came from.

So regarding your other statement:

I have one more concern (maybe misguided): I would not say that having zero TXT records for an existing domain is an error to begin with.

I fully agree. A missing TXT record is not an error. An empty slice or a nil for the slice would perfectly indicate the "missingness" of the record

if, instead, they would like to be able to programmatically differentiate between NXDOMAIN and missing records of the queried class, — that's another story.

That would be a nice-to have. So being able to drill down without going to another module which allows "bare-bones" DNS access would be very helpful, at least to me.

I don't think that this kind of change is backwards compatible at this point.

That's for sure. Maybe providing another function for that purpose would help here?

@mateusz834
Copy link
Member

I have one more concern (maybe misguided): I would not say that having zero TXT records for an existing domain is an error to begin with.
That would be a nice-to have. So being able to drill down without going to another module which allows "bare-bones" DNS access would be very helpful, at least to me.

This can be implemented, but i would block this on #63116, so that we don't pollute the DNSError with more fields (we already have IsNotFound). Then we can define a global errors:

var (
    ErrNoSuchDomain = errors.New("no such domain") // DNS NXDOMAIN
    ErrNoRecordsFound = errors.New("no records found") // DNS NODATA
)

But then question remains on how to handle /etc/hosts (LookupHost) (is this NXDOMAIN/NODATA or something else?).

Personally I think that if you need this kind of access you should use a DNS library directly like miekg/dns.

@kostix
Copy link

kostix commented Jan 26, 2024

This can be implemented, but i would block this on #63116, so that we don't pollute the DNSError with more fields (we already have IsNotFound).

Oh, that is super cool: I, for one, was puzzled when I've discovered it's impossible to somehow "fetch" a non-string reason from a *net.DNSError.

Then we can define a global errors:

var (
    ErrNoSuchDomain = errors.New("no such domain") // DNS NXDOMAIN
    ErrNoRecordsFound = errors.New("no records found") // DNS NODATA
)

While I would prefer a nil, nil, or []string{}, nil variation, these look sensible.

But then question remains on how to handle /etc/hosts (LookupHost) (is this NXDOMAIN/NODATA or something else?).

I would treat a missing /etc/hosts or a missing host in it as NXDOMAIN because we failed to find the host.

Also–again, may be my guess is misguided,–but I thought it's only posisble to have equivalents of A and AAAA (and PTR) records in /etc/hosts, so, basically, any kind of request which is not of these two types can be failed right away if it were about to be attempted agains that file, and that would amount to a "hard" error: a failure to resolve the domain/host name, which happens before an attempt to obtain the records of its zone.

@Skeeve
Copy link
Author

Skeeve commented Jan 26, 2024

But then question remains on how to handle /etc/hosts (LookupHost) (is this NXDOMAIN/NODATA or something else?).

I'd go with what digand host would give, which is NXDOMAIN.

Or did I misunderstand?

Personally I think that if you need this kind of access you should use a DNS library directly like miekg/dns.

I thought about switching to it. It's just a question of: Is it worth it? After all: I think, and I might be wrong here, the information is already available when using net.LookupXXX. Why not make it accessible?

P.S. "Is it worth it?" My concern here is: What's the effort switching? What's the effort learning it? I'm not a DNS "expert" and to me it looked complicated. On the other hand net is a standard library whih seemed quite easy to us.

@mateusz834
Copy link
Member

I don't agree that /etc/hosts should return a NXDOMAIN-kind of error, because when receiving a NXDOMAIN we can assume that no other record exists on that domain name. With hosts that might not hold true (and probably with other nss modules (with cgo resolver)).

LookupHost("not-in-hosts.example.com") // might return NXDOMAIN when nsswitch.conf is `dns: files`
LookupTXT("not-in-hosts.example.com") // returns an answer even though LookupHost returned NXDOMAIN.

@Skeeve
Copy link
Author

Skeeve commented Jan 26, 2024

For sure I'm not too deeply involved in this, so what exactly do you mean with /etc/hosts? Looking up ONLY in /etc/hosts? So explicitly limiting lookup to /etc/hosts? In that case a NXDOMAIN would be the perfect fit to my understanding as the requestet domain/hosts is not defined in any of the locations the lookup was allowed to look into. But this is just my uneducated idea about it.

@mateusz834
Copy link
Member

mateusz834 commented Jan 26, 2024

NXDOMAIN response means that the requested domain does not contain any records, but depending on the system configuration that might not hold true, like in the example:

LookupHost("not-in-hosts.example.com") // might return NXDOMAIN when nsswitch.conf is `hosts: files`
LookupTXT("not-in-hosts.example.com") // returns an answer even though LookupHost returned NXDOMAIN

Looking up ONLY in /etc/hosts? So explicitly limiting lookup to /etc/hosts? In that case a NXDOMAIN would be the perfect fit to my understanding as the requestet domain/hosts is not defined in any of the locations the lookup was allowed to look into.

Yes, in case where the system is configured to only use /etc/hosts, but it is not perfect, because every other lookup method might resolve that name, even when LookupHost returned NXDOMAIN (other lookup methods directly do DNS without any file lookup).

This is why I am fine with returning strings "no such domain" "no records found", but don't like the idea of separate exported errors because of cases like that. And who knows what the getaddrinfo (through nss modules) and windows report in similar cases.

@Skeeve
Copy link
Author

Skeeve commented Jan 26, 2024

It makes no sense to me, that, if you explicitly limit yourself to one specific source, why the answer NXDOMAIN shouldn't be appropriate, but I will trust your judgement.

@mateusz834
Copy link
Member

mateusz834 commented Jan 26, 2024

But then, why would you want to distinguish NXDOMAIN/NODATA errors in the first place? Maybe i am missing something, we might be looking at this from a different perspective. I've only seen it used to early abort multiple DNS queries, when the first one returns NXDOMAIN (then we are sure that next lookups would return the same response (NXDOMAIN) for the same name).

@Skeeve
Copy link
Author

Skeeve commented Jan 26, 2024

But then, why would you want to distinguish NXDOMAIN/NODATA errors in the first place?

I'm currently using a Perl script which distinguishes, so my users know these answers.

Now I want to switch to go and, with this library I don't get those answers. Don't get me wrong: I do not require them. It simply would be nice to have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal
Projects
Status: Incoming
Development

No branches or pull requests

6 participants