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

Do not allow NameResolverProvider to choose which is default; make it application-explicit #7917

Open
YuanWenqing opened this issue Feb 24, 2021 · 3 comments
Milestone

Comments

@YuanWenqing
Copy link

YuanWenqing commented Feb 24, 2021

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:

  • We use google-cloud-firestore library to integrate firestore service. The hard-coded endpoint is "firestore.googleapis.com:443", which seems to be the case handled by the compatibility of ManagedChannelImpl.
  • We also use another opensource library to extend grpc functionalities. This library offers a StaticNameResolverProvider with priortiy=5, same as DnsNameResolverProvider, and the package name is greater than "io.grpc". It also supports URI in format like "firestore.googleapis.com:443".
  • When grpc wants to find a NameResolverProvider for firestore endpoint, it will find StaticNameResolverProvider instead of DnsNameResolverProvider.

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.

@YuanWenqing
Copy link
Author

For now, we solve this by copying DnsNameResolver and DnsNameResolverProvider as new classes with different package name and priority.
This way is really ugly...

@ejona86
Copy link
Member

ejona86 commented Feb 24, 2021

But, I wonder why does it take use of classname for ordering?

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.

The classname of NameResolverProvider from third-party library is out of control of developers and it will cause some problem.

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.

Moreover, comments in ManagedChannelImpl indicate that it expects to use DnsNameResolverProvider as default for compatibility with some googleapis endpoint like "foo.googleapis.com:8080"

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.

The hard-coded endpoint is "firestore.googleapis.com:443", which seems to be the case handled by the compatibility of ManagedChannelImpl.

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."

This library offers a StaticNameResolverProvider with priortiy=5, same as DnsNameResolverProvider, and the package name is greater than "io.grpc".

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 defaultNameResolverScheme() would help but isn't enough. Maybe a setter that can only be called once. But the "default scheme" was to allow something similar to what is taking place with StaticNameResolverProvider.

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.

@YuanWenqing
Copy link
Author

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.
Scheme seems to be the clear mechanism to find an acceptable NameResolverProvider, but the involvement of priority make it a bit awkward.

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.

@YuanWenqing YuanWenqing changed the title Question about ordering of NameResolverProviders Question about choose of NameResolverProvider Feb 25, 2021
@ejona86 ejona86 changed the title Question about choose of NameResolverProvider Do not allow NameResolverProvider to choose which is default; make it application-explicit Jun 24, 2021
@ejona86 ejona86 added this to the Next milestone Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants