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

net: Make it possible to determine if a lookup error is errNoSuchHost #28635

Open
aermakov-zalando opened this Issue Nov 7, 2018 · 24 comments

Comments

Projects
None yet
@aermakov-zalando
Copy link

aermakov-zalando commented Nov 7, 2018

What version of Go are you using (go version)?

$ go version
go version go1.11.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/aermakov/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/aermakov/work/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.1/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/f1/4tjxswld76n6zmb02rmn18bnr4l_7q/T/go-build082609694=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I'm trying to determine whether a net.LookupHost fails because the DNS record doesn't exist, or because of other errors. Unfortunately, errNoSuchHost isn't exposed from net and there are no helper functions or methods to determine the type of net.DNSError. This leaves me with no choice other than string parsing, which is extremely brittle.

What did you expect to see?

I would expect to be able to tell noSuchHost errors from other errors, either by comparing with an exported error or by using a method on net.DNSError.

What did you see instead?

It's impossible to determine whether net.LookupHost returned an error the DNS record doesn't exist or any other reasons without resorting to string parsing.

@aermakov-zalando aermakov-zalando changed the title net: Make it possible to determine if a lookup error is `errNoSuchHost` net: Make it possible to determine if a lookup error is errNoSuchHost Nov 7, 2018

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Nov 7, 2018

/cc @mikioh

@maksadbek

This comment has been minimized.

Copy link

maksadbek commented Nov 7, 2018

I see there are 6 uses of unexposed errNoSuchHost in the Go codebase.
See: https://github.com/golang/go/search?q=errNoSuchHost&unscoped_q=errNoSuchHost.

Including the test file where errNoSuchHost is checked with string comparison. See:

if !strings.HasSuffix(err.Error(), errNoSuchHost.Error()) {

IMHO exposing this error variable and refactoring the codebase would be enough.

@affanv14

This comment has been minimized.

Copy link

affanv14 commented Nov 17, 2018

Hi, I'd like to take this up if it is free

@maksadbek

This comment has been minimized.

Copy link

maksadbek commented Nov 19, 2018

I've missed that errNoSuchHost is always wrapped into DNSError before it is returned.
Could we add a variable IsNoSuchHost to DNSError struct. Then we could assert returned error to DNSError and check if IsNoSuchHost==true. See https://play.golang.org/p/WyhepLJg2WI

@affanv14

This comment has been minimized.

Copy link

affanv14 commented Nov 21, 2018

@maksadbek I'm assuming you are taking this up?

@maksadbek

This comment has been minimized.

Copy link

maksadbek commented Nov 22, 2018

Hi @affanv14
Yes, I'm ready to send a pr if it's ok to use the option I described above.

@affanv14

This comment has been minimized.

Copy link

affanv14 commented Nov 23, 2018

I'm a new contributor myself, I mistook you for a member, I think you may have done the same! Anyway, I'll leave this up to you. Good luck on the PR!

@szuecs

This comment has been minimized.

Copy link

szuecs commented Nov 29, 2018

You could also implement „methods“ on top of DNSError, then you don’t need an extra variable and expose the data required.

@maksadbek

This comment has been minimized.

Copy link

maksadbek commented Nov 30, 2018

@szuecs Do you mean a method that checks prefix of the err text ?

@szuecs

This comment has been minimized.

Copy link

szuecs commented Nov 30, 2018

Instead of storing it in a variable, you could do this yes.
It was just an idea, no need to do this, but maybe it's not so bad. :)

@gabber12

This comment has been minimized.

Copy link
Contributor

gabber12 commented Mar 12, 2019

I'd like to take this up. Shall I start work on a CL, if nobody's doing it?

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Mar 12, 2019

Go for it.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Mar 12, 2019

@gabber12 First tell us exactly how you plan to change the API. Don't send in new API via a CL. Discuss it first. Thanks.

@gabber12

This comment has been minimized.

Copy link
Contributor

gabber12 commented Mar 12, 2019

I am inclined to add a bool var (IsNoSuchHost)in DNSError itself(instead of doing string comparison).

type DNSError struct {
	Err           string // description of the error
	Name          string // name looked for
	Server        string // server used
	IsTimeout     bool   // if true, timed out; not all timeouts set this
	IsTemporary   bool   // if true, error is temporary; not all errors set this
	IsNoSuchHost bool   // if true, host could not be found
}

The idea would be to make IsNoSuchHost true wherever errNoSuchHost is being wrapped in DnsError.

There is a todo too, to do this in dnsclient_unix.go

@ianlancetaylor

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Mar 12, 2019

Thanks. I would like to hear from @jba and @neild how this might interact with the error values proposal being considered for 1.13, and whether there is a different approach that might be a better fit with that.

@neild

This comment has been minimized.

Copy link
Contributor

neild commented Mar 12, 2019

If ErrNoSuchHost is exported, in 1.13 you will be able to test to see if a *DNSError is a no-such-host error with errors.Is(err, net.ErrNoSuchHost). No need to add an additional field to the DNSError struct.

This requires adding an Unwrap method to DNSError, which I'm planning on doing soon.

@jba

This comment has been minimized.

Copy link
Contributor

jba commented Mar 12, 2019

DNSError doesn't currently wrap the error itself, just its message. Is there a concern with exposing the underlying error?

@neild

This comment has been minimized.

Copy link
Contributor

neild commented Mar 13, 2019

@jba Oh, sorry; an Err field is so conventionally an error that it didn't register for me that this one is a string.

In this case, adding another boolean is probably the way to go since it's consistent with the existing IsTimeout and IsTemporary fields.

@gabber12

This comment has been minimized.

Copy link
Contributor

gabber12 commented Mar 14, 2019

Shall we go with

  • Adding a boolean flag in DNSError
  • Or, Make err field in DNSError an Error instead of string

Personally i see similar tradeoffs in both approaches. And it really depends on the Unwrap method's consistency across std lib

@szuecs

This comment has been minimized.

Copy link

szuecs commented Mar 14, 2019

For me it's quite unclear what is meant by Temporary() and Timeout(), which one are retrieable, so while starting a connection?
IMO it would be much better to have errors as types and let the user decide what to do and coument which types are raised for what cases.

@gabber12

This comment has been minimized.

Copy link
Contributor

gabber12 commented Mar 14, 2019

@szuecs I can understand that Timeout can be an error type, but the Temporary() api will still have to be a method, since different errors may be temporary or permanent.
Leaving on the user to maintain which errors are temporary might not work out.

But overall exposing errors to user seems like a good approach to me too.

@jba @neild would love to hear your take on this.

@neild

This comment has been minimized.

Copy link
Contributor

neild commented Mar 16, 2019

@gabber12 You can't change the type of the DNSError.Err field without violating compatibility. If we were starting from scratch I'd recommend making it an error rather than a string, but it's too late now.

@gabber12

This comment has been minimized.

Copy link
Contributor

gabber12 commented Mar 16, 2019

@neild Make sense, since Err is an exported field.
I would agree with going on with the original approach of adding a boolean flag in DNSError.

Shall i go ahead with the same?

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 21, 2019

Change https://golang.org/cl/168597 mentions this issue: net: add NoSuchHostFound to DNSError

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.