DnsNameResolver seems overly strict when validating service config

[ejona86]

DnsNameResolver only allows a pre-determined set of keys used as filters, in the entire list:

grpc-java/core/src/main/java/io/grpc/internal/DnsNameResolver.java

Lines 543 to 545 in c606519

	for (Entry<String, ?> entry : choice.entrySet()) {
	Verify.verify(SERVICE_CONFIG_CHOICE_KEYS.contains(entry.getKey()), "Bad key: %s", entry);
	}

That means we can never add a new filter key. That seems like a very bad idea. I don't know if this is cross-language, but the precise behavior doesn't seem nailed down in https://github.com/grpc/proposal/blob/master/A2-service-configs-in-dns.md .

CC @dfawley, @markdroth

[markdroth]

I agree that this is very wrong. The decision to ignore unknown fields was specified in the service config error handling design (https://github.com/grpc/proposal/blob/master/A21-service-config-error-handling.md#criteria-to-determine-the-validity-of-a-service-config).

C-core does properly allow unknown fields (https://github.com/grpc/grpc/blob/4f0875dd9e38006d89d4a0cd122280472f58f7c5/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc#L235).

[ejona86]

@markdroth, as a language lawyer I'll note that the DNS selection filters stuff isn't technically part of the service config, albeit highly related. I do remember there was a point that failing for unknown filter keys was part of the design and I remember discussing alternatives, but I don't remember which alternative was chosen. For example, one option that I think was discussed is to ignore the entry if there is an unknown filter, and go check other entries.

I'm fine with simply ignoring other fields (especially when considering proto), but at least a quick discussion would be good.

[dfawley]

Like C, Go's implementation currently ignores unknown fields in the list entries.

I can see it being problematic if an unknown filter key is ignored, because that key may have prevented the config from being selected. However, it's equally problematic to skip the entry, for similar reasons, and it's obviously very bad to fail the entire list due to a single entry. So, I'm OK with the C/Go behavior, with the expectation that service owners configuring this need to be aware of this behavior and the limitations of older client versions.

[AntonInglebert]

Hey,
I am trying to get involved, could I try implementing this?
Instead of failing on a bad entry, keep going and warn ?