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

Revise URI scheme parsing to be RFC 3986 compliant #1911

Closed
dfawley opened this issue Mar 9, 2018 · 9 comments
Closed

Revise URI scheme parsing to be RFC 3986 compliant #1911

dfawley opened this issue Mar 9, 2018 · 9 comments
Assignees

Comments

@dfawley
Copy link
Member

dfawley commented Mar 9, 2018

https://tools.ietf.org/html/rfc3986

This includes honoring scheme:endpoint syntax and scheme://authority/endpoint parsing resulting in endpoint = "/endpoint".

Note: scheme:endpoint syntax will break all users dialing with hostname:port unless we apply the default resolver to cases where the scheme isn't registered. This could result in usability problems, however, as the error that occurs if a resolver isn't registered by mistake will be obscured, and would probably lead to RPCs just not working, as opposed to an error being returned from Dial.

@dmcgowan
Copy link

dmcgowan commented Mar 9, 2018

Note: scheme:endpoint syntax will break all users dialing with hostname:port unless we apply the default resolver to cases where the scheme isn't registered.

Based on the syntax from rfc8089, the "endpoint" in scheme:endpoint must be an absolute path. Would the parser be able to distinguish between hostname:port and file:/path based on the second element being an absolute path and not a port name/number?

@dfawley
Copy link
Member Author

dfawley commented Mar 13, 2018

Note that 8089 is specifically about the "file" scheme; we are implementing 3986, which is support for URI schemes generally. I would rather not make assumptions about the format of the target to determine whether to fallback to a default; that just makes things more complicated.

@stevvooe
Copy link
Contributor

I would rather not make assumptions about the format of the target to determine whether to fallback to a default; that just makes things more complicated.

This might be the main problem with the logic. The correct parsing of a URI will pass everything after the scheme:// as completely own by the scheme, which would include that trailing slash in unix:///. There may also need to be more clarity around how a given scheme converts a URI into a (network, address) tuple that is passed into the net.Dialer. If we look at https://godoc.org/google.golang.org/grpc/naming#Update for example, it doesn't really have all the necessary inputs for dialer. Also, if we look at https://godoc.org/google.golang.org/grpc#BalancerConfig, the dialer there doesn't match the dialer from the stdlib, at all.

@dfawley
Copy link
Member Author

dfawley commented Mar 27, 2018

@stevvooe,

If we look at godoc.org/google.golang.org/grpc/naming#Update for example, it doesn't really have all the necessary inputs for dialer. Also, if we look at godoc.org/google.golang.org/grpc#BalancerConfig, the dialer there doesn't match the dialer from the stdlib, at all.

The naming package and grpc.Balancer are part of the old API. The new functionality is in the balancer and resolver packages. Sorry for the confusion; we will update the docstrings for the old API (to mark it as deprecated instead of experimental) soon.

@menghanl
Copy link
Contributor

We will keep the behavior of Dial to avoid possible breakages.

In the planned NewClient API (#1786), we will make it compliant with the RFC.

@stevvooe
Copy link
Contributor

We will keep the behavior of Dial to avoid possible breakages.

Aren't people already broken? Everyone is having to fix this in the leaf projects to make it work for users.

@smola
Copy link

smola commented Jul 10, 2018

@stevvooe By now our projects rely on Dial broken behavior. Either we are feeding non-standard values or we have our own conversion process before passing them to Dial, I've seen both. So these projects are not necessarily broken, if you fix Dial itself you might break projects that had valid workarounds.

@dfawley
Copy link
Member Author

dfawley commented Jul 10, 2018

Exactly. In retrospect, we should have kept this behavior out of Dial entirely, but now that it's done, we shouldn't make further changes to avoid breaking those relying on the current behavior.

@dfawley
Copy link
Member Author

dfawley commented May 3, 2021

If we change the target parsing semantics, we probably only want to do that with a NewClient (#1786), instead of potentially breaking existing users of Dial and DialContext. Closing this.

@dfawley dfawley closed this as completed May 3, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants