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

Consider deprecating Dial and DialContext and replacing with a NewClient #1786

Closed
dfawley opened this issue Jan 8, 2018 · 12 comments
Closed
Labels
P3 Type: Feature New features or improvements in behavior

Comments

@dfawley
Copy link
Member

dfawley commented Jan 8, 2018

Dial and DialContext have several differences from how gRPC works in other languages. A NewClient function that works more like other languages would differ in at least two ways:

  1. Default to DNS resolver instead of passthrough.
  2. Make target compliant with RFC 3986 (Revise URI scheme parsing to be RFC 3986 compliant #1911).
  3. Start IDLE instead of CONNECTING.

If we create a NewClient, we'd probably also want to make matching ClientOptions instead of DialOptions, as some DialOptions (e.g. WithBlock or WithTimeout) do not make sense when not dialing synchronously.

@dfawley dfawley added Type: API Change Breaking API changes (experimental APIs only!) P2 labels Jan 8, 2018
@nhooyr
Copy link

nhooyr commented Mar 29, 2018

Default to DNS resolver instead of passthrough.

Could you elaborate on what passthrough would be? Wouldn't all Dial's use a DNS resolver?

@dfawley
Copy link
Member Author

dfawley commented Mar 29, 2018

Passthrough sends the entire target string (after "passthrough:///") to the dialer as the address, verbatim.

A DNS resolver would resolve the target (after "dns:///"), turn it into IP addresses & ports, and those would be handed to the dialer.

It just so happens that the default dialer (net.Dial) is able to do DNS name resolution, and so passing "google.com:22" will work just fine with it.

However, this passthrough method means we can't do things like load-balance across all the DNS results for that name.

You can see more about the resolver/balancer design in the gRFC here: https://github.com/grpc/proposal/blob/master/L9-go-resolver-balancer-API.md

@dfawley dfawley self-assigned this Jul 9, 2018
@stale stale bot added the stale label Sep 6, 2019
@dfawley dfawley removed the stale label Sep 6, 2019
@grpc grpc deleted a comment from stale bot Apr 23, 2020
@dfawley dfawley removed their assignment May 3, 2021
@dfawley dfawley added P3 Type: Feature New features or improvements in behavior and removed P2 Type: API Change Breaking API changes (experimental APIs only!) labels May 3, 2021
@raphoester
Copy link

Hello guys, I notice that the DialContext method is now deprecated
I was using it a lot for my tests that included the presentation layer, eg :

conn, err := grpc.DialContext(
		context.Background(), "bufnet",
		grpc.WithContextDialer(
			func(context.Context, string) (net.Conn, error) {
				return s.lis.Dial()
			},
		),
		grpc.WithTransportCredentials(insecure.NewCredentials()),
	)
	// then create the client using the new conn
	client := someservice.NewSomeServiceClient(conn)

I tried replacing DialContext with NewClient, e.g :

conn, err := grpc.NewClient(
		"passthrough", // ?
		grpc.WithContextDialer(
			func(context.Context, string) (net.Conn, error) {
				return s.lis.Dial()
			},
		),
		grpc.WithTransportCredentials(insecure.NewCredentials()),
	)

But now i get a DNS error on RPC call from the client : rpc error: code = Unavailable desc = name resolver error: produced zero addresses

Can you please tell me where there is a doc to properly transition this use case ? thanks

@bcessa
Copy link

bcessa commented May 19, 2024

Hi @raphoester, in case you are still having this issue I solve it by setting
the default resolver scheme in my test files.

// set the default resolver schema to "passthrough" to prevent DNS resolution
// issues while testing; the default scheme used by grpc.NewClient is "dns".
func init() {
    resolver.SetDefaultScheme("passthrough")
}

This way you can prevent the DNS issues when replacing Dial for NewClient. In my tests I'm using a dialer similar to yours:

withDialer := func(l *bufconn.Listener) gRPC.DialOption {
    return gRPC.WithContextDialer(func(context.Context, string) (net.Conn, error) {
        return l.Dial()
    })
}

@ash2k
Copy link
Contributor

ash2k commented May 19, 2024

This should work:

conn, err := grpc.NewClient(
		"passthrough:whatever",
		grpc.WithContextDialer(
			func(context.Context, string) (net.Conn, error) {
				return s.lis.Dial()
			},
		),
		grpc.WithTransportCredentials(insecure.NewCredentials()),
	)

I don't know why, but https://github.com/grpc/grpc/blob/master/doc/naming.md doesn't describe passthrough.

@atollena
Copy link
Collaborator

I don't know why, but https://github.com/grpc/grpc/blob/master/doc/naming.md doesn't describe passthrough.

This is because passthrough is go specific and does not exist in other implementations, including C and Java. This is also the reason for making NewClient use DNS by default. Ideally passthrough wouldn't exist and dns would be the default in Go even for Dial/DialContext, but this would be a breaking change that is not worth the trouble at this point.

@ash2k
Copy link
Contributor

ash2k commented May 21, 2024

This is because passthrough is go specific and does not exist in other implementations, including C and Java.

I wonder how other implementations deal with cases where you don't want dns resolution at all. I.e. when you use a dialer that does something special like opening an in-memory pipe to a server. passthrough is the way to go here and that's what I use.

This is also the reason for making NewClient use DNS by default.

I think this makes sense. I also like that NewClient() doesn't start connecting to the server straight away.

@dfawley
Copy link
Member Author

dfawley commented May 21, 2024

The other languages have a real in-memory transport that they support. I believe C++ uses a separate resolver, and Java uses a completely different channel builder (i.e. they don't pass a target string at all).

For Go, I'd love to one day add support for other transports besides HTTP/2 (#906). An "inmemory" transport would come with an "inmemory" resolver that you'd specify as the scheme for the channel and emit a single "inmemory" address to connect to.

The bufconn was never really intended for end users, BTW. Just for our internal testing. :)

@dfawley dfawley closed this as completed May 21, 2024
@ash2k
Copy link
Contributor

ash2k commented May 22, 2024

For anyone who might need something like this - I have this implementation. It's a net.Listener + dialer (DialContext()) so you can connect a server and a client(s) via net.Pipe(). No buffering is done. Anyway, sorry for off topic.

Acedus added a commit to Acedus/kubevirt that referenced this issue Aug 18, 2024
Following manual [0] updating grpc-go version on all relevant files.
This version bump is required in order to address grpc function
deprecations introduced in grpc-go v1.64.0 [1].

[0] https://github.com/kubevirt/kubevirt/blob/main/docs/updating-dependencies.md
[1] grpc/grpc-go#1786

Signed-off-by: Adi Aloni <[email protected]>
Acedus added a commit to Acedus/kubevirt that referenced this issue Aug 20, 2024
Following manual [0] updating grpc-go version on all relevant files.
This version bump is required in order to address grpc function
deprecations introduced in grpc-go v1.64.0 [1].

[0] https://github.com/kubevirt/kubevirt/blob/main/docs/updating-dependencies.md
[1] grpc/grpc-go#1786

Signed-off-by: Adi Aloni <[email protected]>
@puneet-traceable
Copy link

Is this change compatible with forward-proxies?
I see a difference in cases when forward-proxies allow/block based on hostnames to connect to upstream servers.
When I use NewClient instead on Dial the domain is resolved upfront and only resolved ip is sent as target, so the proxy is not able to correctly apply these policies.
Attach a tcpdump screenshot

Screenshot 2024-08-23 at 7 50 34 PM

What is the best to change the behaviour back?

@dfawley
Copy link
Member Author

dfawley commented Aug 23, 2024

@puneet-traceable can you file a new issue for this please?

@puneet-traceable
Copy link

@puneet-traceable can you file a new issue for this please?

Filed a new one #7556

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Type: Feature New features or improvements in behavior
Projects
None yet
Development

No branches or pull requests

7 participants