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

Handle "unix" type when custom dialer is set #3990

Closed
menghanl opened this issue Oct 26, 2020 · 1 comment · Fixed by #4021
Closed

Handle "unix" type when custom dialer is set #3990

menghanl opened this issue Oct 26, 2020 · 1 comment · Fixed by #4021
Assignees

Comments

@menghanl
Copy link
Contributor

#3890 changed the way "unix" scheme is handled

  • before: the dialer (default or user-provided) gets the original dial target (unix:///a/b/c)
    • the dialer needs to split and find the network type
  • after: gRPC's default dialer gets network type from address.Attributes

This is a behavior change for user's custom dialers, because they no longer get unix:///a/b/c, and will only get the unix path /a/b/c

The result is, unix:///a/b/c or unix:/a/b/c doesn't work if a custom dialer is specified, because the network type handling in default dialer will be bypassed.

Options

  1. pass network type in context, so WithContextDialer can handle it
  2. new WithDialer API, with resolver.Address as the address type, instead of string
    • add resolver.Address.NetworkType, as type string
    • users will need to handle network type, as net.Dial(addr.NetworkType, addr.Addr)
  3. interceptor style API, which takes gRPC's default dialer as input, and returns a new wrapped dialer
    • Con: doesn't work if user's dialer needs to do TCP level changes thus cannot use gRPC's default dialer
  4. don't call user's if network type is "unix"
    • Con: breaks cases where user wants to install a hook (e.g. wrap net.Conn.Close)
  5. re-attach "unix:///" to addresses before calling custom dialer
    • Con: doesn't play nicely users with their own unix resolver; requires uses to do the extra parsing for the network type
  6. register dialers by network types
@dfawley
Copy link
Member

dfawley commented Oct 28, 2020

interceptor style API

If we're adding a new API, then I don't think we'd want this unless we also did (2). What all does gRPC's dialer do? We have proxy support, and we also detect the network type. Proxies are supposed to be implemented a bit differently, which would make that go away (but cause more backward compatibility problems when we try to make that change). And if the dialer API included the network type, then we wouldn't need to do that anymore, either. So ideally we don't have a "gRPC default dialer" except "net.Dial". Maybe we should more fully flesh out how proxies should work now, though.

register dialers by network types

This would also be a new API. Is this superior to WithDialerV2(func(context.Context, resolver.Address)net.Conn)?

@GarrettGutierrez1 GarrettGutierrez1 self-assigned this Oct 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants