Add RFC 3986 support to DnsNameResolverProvider #12602
Conversation
Add examples to the getPathSegments() javadoc. Introduce RFC 3986 concepts of absolute and rootless paths.
Update javadoc/tests to match reality w.r.t target URI path: path isn't actually the name to resolve, only the first path segment is actually used. This is a side effect of authority parsing later, in DnsNameResolver.java. Any additional path segments are silently ignored. Replace discussion of the leading path / by making explicit the requirement that the input java.net.URI be hierarchical. This requirement isn't new -- checkNotNull(targetUri.getPath()) throws today if the target is opaque. Also fix sloppy language in javadoc wrt service authority port: it isn't actually an input to the DNS layer, rather it's copied forward as a property of our output addresses at the io.grpc.NameResolver layer.
Accept both absolute (e.g. 'dns:///hostname') and rootless (e.g. 'dns:hostname') paths as specified by https://github.com/grpc/grpc/blob/master/doc/naming.md and matching the behavior of grpc core and grpc-go.
jdcormie
changed the title
| * target URI, excluding the leading slash {@code '/'}, is treated as the host name and the optional | ||
| * port to be resolved by DNS. Example target URIs: | ||
| * URI is reserved for the address of alternative DNS server (not implemented yet). The first path | ||
| * segment of the hierarchical target URI is interpreted as an RFC 2396 "server-based" authority and |
There was a problem hiding this comment.
I don't think we want to say the "first path segment" by itself. I would be fine with that if we also say "there must only be one segment." I don't think we want arbitrary stuff added to the end to be ignored dns:/example.com/foo. Allowing dns:example.com is good and fine.
There was a problem hiding this comment.
OK. My commits all got squashed but this first one was actually just documenting the current behavior, as verified by my new newNameResolver_toleratesTrailingPathSegments() test. It's a surprising (to me) side effect of hostport parsing later, in DnsNameResolver.java, where a new temp nameUri is parsed from the concatenation of "//" with getPath() from the original input target URI. The constructor proceeds with nameUri.getHost() and nameUri.getPort() but ignores nameUri.getPath(), where any additional path segments ended up.
I can certainly change DnsNameResolverProvider#newNameResolver(io.grpc.Uri...) to forbid this but this new restriction will only take effect when the RFC 3984 flag is flipped. I do worry that it could break existing clients and make actually flipping the flag more likely to get rolled back. You could also argue that it's inconsistent with how DnsNameResolverProvider ignores other parts of the target URI it doesn't care about, like user info, query string and fragment . LMK what you think.
There was a problem hiding this comment.
To the extent that RFC 4501 matters, its grammar does not seem to permit additional path segments, user info, fragment, or query params other than CLASS or TYPE.
3 participants
Accept both absolute (e.g.
dns:///hostname) and rootless (e.g.dns:hostname) paths as specified by https://github.com/grpc/grpc/blob/master/doc/naming.md and matching the behavior of grpc core and grpc-go.