net: add (*Resolver).ResolveIPAddr

[iangudger]

In the net package, there are two classes of DNS functions: LookupXxx and ResolveXxx. The net.Resolver type implements the LookupXxx functions as methods, but not the ResolveXxx functions. The ResolveXxx functions are similar to LookupHost, except they allow specifying a network. Adding the ResolveXxx functions as methods will allow a net.Resolver to fully replace the global DNS functions.

[bcmills]

CC @mikioh @bradfitz @ianlancetaylor for net

[bradfitz]

I'm fine with that on principle. I'd never noticed you couldn't do IPv4- vs IPv6-specific lookups on Resolver before. That is, you can't do the equivalent of this today on Resolver:

func main() {
        fmt.Println(net.ResolveIPAddr("ip4", "google.com"))
        fmt.Println(net.ResolveIPAddr("ip6", "google.com"))
}

The name Resolver.ResolveIPAddr is a bit stuttery but probably best for consistency. But I'd also be fine with Resolver.LookupNetIPAddr or something.

[rsc]

It's unclear why we should add net.Resolver.ResolveIPAddr; why not just use LookupIPAddr and filter out what you want? ResolveIPAddr is just LookupIPAddr plus client-side filtering anyway. It's not any fewer bits on the wire.

[iangudger]

Sure, it can be done, but it would be nice to not make everyone who wants it re-implement it.

[bradfitz]

I'm inclined to add this. @rsc, it is fewer bytes on the wire, in both directions: only asking for A or AAAA instead of both, and only getting answers for what we asked for.

[rsc]

Today we have:

func (r *Resolver) LookupIPAddr(ctx context.Context, host string) ([]IPAddr, error)

We need a new name for the function that takes a network. It would be good to keep the Lookup prefix common to all the other methods.

Maybe it would be enough to shorten to "LookupIP":

func (r *Resolver) LookupIP(ctx context.Context, net, host string) ([]IPAddr, error)

where net is either "ip4" or "ip6".

Do we really need IPAddr in the return type vs plain IP? Can DNS return a zone?

[rsc]

@iangudger, any thoughts on the questions in the previous comment? If that API is OK then I think we're close to accepting the proposal. Thanks for your help and your patience.

[iangudger]

Sounds good to me.

[odeke-em]

Howdy @iangudger, kindly pinging you as this proposal was approved almost a year ago. The Go tree closes in 3 weeks so if we’d like to see this for Go1.15, we’ve got a window of opportunity. Thank you!

[gopherbot]

Change https://golang.org/cl/228641 mentions this issue: net: add (*Resolver).LookupIP

[odeke-em]

@rsc, @iangudger @bradfitz @andybons and @golang/proposal-review, I just gave @iangudger a code review in https://go-review.googlesource.com/c/go/+/228641/ and I realized that perhaps the name that we
considered has a couple of problems:
a) There already exists a package level LookuIP; by convention we previously had package level functions that invoked the respectively named methods on the DefaultResolveer
b) The package level ResolveIPAddr(network, address string) (*IPAddr, error) has a signature that returns only a single IPAddr pointer so even a Resolver based ResolveIPAddr of signature (r *Resolver) LookupIP(ctx context.Context, network, host string) ([]IPAddr, error) would be inconsistent, and the name is also stuttery as @bradfitz pointed out

I think @bradfitz's suggestion in #30452 (comment) of

Resolver.LookupNetIPAddr(ctx context.Context, network, host string) ([]IPAddr, error)

might be the best suited, or

Resolver.LookupIPAddrByNetwork(ctx context.Context, network, host string) ([]IPAddr, error)

would suffice. The code otherwise LGTM, but I think that the name will make or break us and inconsistencies might make it harder later on for users to navigate through the net package.

Thank you.

[iangudger]

Ping @rsc, @bradfitz, @andybons and @golang/proposal-review.

[bradfitz]

We never got an answer on @rsc's question:

Do we really need IPAddr in the return type vs plain IP? Can DNS return a zone?

If not, then we could just return []IP instead of []IPAddr and then its name LookupIP matches func LookupIP(host string) ([]IP, error) except for the addition of context + network which seems fine.

[iangudger]

AAAA records cannot contain a zone, so I think []IP should be fine.

[bradfitz]

Phew. That's what I thought & hoped. (Was worried I missed some RFC.)

Let's go with []IP then.

[tmthrgd]

Should CL 228641 have closed this issue?

[ianlancetaylor]

As far as I can tell, this is done. Thanks.