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
Do not allow NameResolverProvider to choose which is default; make it application-explicit #7917
Comments
For now, we solve this by copying DnsNameResolver and DnsNameResolverProvider as new classes with different package name and priority. |
Class name isn't all that special, except that it is consistent between runs. We didn't want things to be non-deterministic or to vary between executions.
I think the classname is a red herring. The core problem is the library is making a decision for the entire application and the users of the library don't get any say. That said, there's issues even without that.
This isn't so much "hard-coded dns" but rather "a way for us to introduce schemes and name resolvers in a backward-compatible way" as well as some convenience. Early versions of gRPC didn't have the plugin mechanism and it was added later. So we process the target string twice: once as-is and if that fails try pre-pending the default scheme and try again. The exception handling was just because hostname:port is very common syntax and hostname will look like a scheme.
It's been argued that libraries that expect something in particular should use the explicit scheme prefix. Note that that PR was directly related to StaticNameResolverProvider. The docs for ManagedChannelBuilder contain "We recommend libraries to specify the schema explicitly if it is known, since libraries cannot know which NameResolver will be default during runtime."
I don't like the fact that a provider gets to choose the default scheme. That should be the application author and not a library author. (I'm fine with priorities, but I think that should only apply if there are multiple providers providing "dns:" or the like.) I'd like to remove that capability but it is unclear what to replace it with. Adding a So in summary, I don't think the ordering is the problem by itself. If we ordered by a different metric the library could just bump its priority to 10. The problem is that a library thinks it is important enough to decide how the entire application should behave and gRPC doesn't offer much alternative. Coupled with other libraries needing to hard-code a scheme lest they may "randomly" break. |
Thanks @ejona86 for the reply. The issue title may not be proper for the exact problem. I agree that "hostname:port is very common syntax", so any provider would handle this format, thus causing the problem. I think the primary issue is that there is some confusion among the definition and design-purpose of priority and scheme. It may be great if all target URIs are required to use a specific scheme, thus every target URI would be handled by a specific NameResolverProvider. GRPC could use a scheme like "grpc:" or "google:" as default scheme for internal compatibility. Since multiple providers may support the same scheme, it should be developer to make the choice. So priority should not be coded in library and GRPC should offer a mechanism for developers to overwrite the priority of NameResolverProvider. |
I find ordering of NameResolverProviders loaded by GRPC is up to the priority and classname. Comments in NameResolverRegistry indicate that GRPC needs to make the ordering stable because it relies on the first one as default.
But, I wonder why does it take use of classname for ordering? The classname of NameResolverProvider from third-party library is out of control of developers and it will cause some problem.
Moreover, comments in ManagedChannelImpl indicate that it expects to use DnsNameResolverProvider as default for compatibility with some googleapis endpoint like "foo.googleapis.com:8080". But in fact it use the first NameResolverProvider as default.
If some third-party library offers a NameResolverProvider with a package name greater than "io.grpc", which uses the same priority as DnsNameResolverProvider and also supports the same format like "foo.googleapis.com:8080", this hard-coded compatibility in ManagedChannelImpl will be broken. And it's hard for developers to solve this by doing some config thing.
Here's an example:
Since grpc is kind of basis library and offers extension functionality for NameResolverProvider, I think it should make both of URI format and ordering of NameResolverProviders more clear. Besides, GRPC should also offer a more specific way to set the default NameResolverProvider.
The text was updated successfully, but these errors were encountered: