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: rename Resolver.Dial to Resolver.DialContext before Go1.9rc1? #19910

Closed
rsc opened this issue Apr 10, 2017 · 8 comments

Comments

Projects
None yet
6 participants
@rsc
Copy link
Contributor

commented Apr 10, 2017

CL 37260 proposes add a new field to net.Resolver:

	// Dial is used by Go's built-in DNS resolver to make TCP and
	// UDP connections to DNS services. The Resolver will never call this
	// with names, only valid IP addresses. The Conn returned must be
	// a *TCPConn or *UDPConn as requested by the network parameter.
	// If nil the default dialer is used.
	Dial func(ctx context.Context, network, hostport string) (Conn, error)

Elevating that CL to a proposal, hopefully a quick one.

This would allow control over the source address for DNS lookups in multi-homed systems (#17404).

Discussion on #19268 suggests that the new Dial field would be an acceptable solution for "do DNS lookups using a custom server", by installing a Dial func that lies about what server it has connected to. Of course that only works if /etc/resolv.conf lists some name server. If there are no resolvers then Dial will never be called. This particular use seems a bit kludgy, but fine if it resolves that need.

/cc @bradfitz

@rsc rsc added the Proposal label Apr 10, 2017

@rsc rsc added this to the Proposal milestone Apr 10, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 11, 2017

I'm fine with this. But should it be named DialContext to be consistent with:

?

/cc @nerdatmath

@rsc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2017

SGTM.

@rsc rsc modified the milestones: Go1.9, Proposal Apr 17, 2017

@rsc rsc changed the title proposal: net: allow Resolver to use a custom dialer net: allow Resolver to use a custom dialer Apr 17, 2017

@benburkert

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2017

CL 37260 added this method as Dial, should it be renamed to DialContext?

@gopherbot

This comment has been minimized.

Copy link

commented Jun 8, 2017

CL https://golang.org/cl/45153 mentions this issue.

gopherbot pushed a commit that referenced this issue Jun 8, 2017

net: support all PacketConn and Conn returned by Resolver.Dial
Allow the Resolver.Dial func to return instances of Conn other than
*TCPConn and *UDPConn. If the Conn is also a PacketConn, assume DNS
messages transmitted over the Conn adhere to section 4.2.1. "UDP usage".
Otherwise, follow section 4.2.2. "TCP usage".

Provides a hook mechanism so that DNS queries generated by the net
package may be answered or modified before being sent to over the
network.

Updates #19910

Change-Id: Ib089a28ad4a1848bbeaf624ae889f1e82d56655b
Reviewed-on: https://go-review.googlesource.com/45153
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@odeke-em

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

@bradfitz and @rsc, @benburkert raises a great point that it seems like @nerdatmath's CL https://go-review.googlesource.com/c/37260/8/src/net/lookup.go#106 already added the proposed field and perhaps we just need to rename it to DialContext?
screen shot 2017-07-17 at 10 34 51 pm
we just forgot to tag this issue in that CL.

Also by superficially skimming through the issue, I think after that rename, we'd have addressed @bradfitz's request in #19910 (comment) and then the issue can be marked as resolved. What do y'all think?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 22, 2017

I think Dial is fine. Our other DialContext methods are named so because there was already a Dial in the way hogging the good name.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 22, 2017

But I could see the consistency argument too. We're days away from an rc1, though, so it's a bit late (but still possible) to change.

@rsc, @ianlancetaylor?

@bradfitz bradfitz changed the title net: allow Resolver to use a custom dialer net: rename Resolver.Dial to Resolver.DialContext before Go1.9rc1? Jul 22, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2017

I think Dial is OK.

@bradfitz bradfitz closed this Jul 24, 2017

@golang golang locked and limited conversation to collaborators Jul 24, 2018

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