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 ?

[YifeiZhuang]

@ejona86 This statement (might be the reason about java's behaviour?) is not accurate to me:

// If any unexpected field name is present in this object, the entire
// config is considered invalid.

It is from this section

[ejona86]

@markdroth, thoughts about @YifeiZhuang's most recent comment? Context since this is old: We noticed Java didn't allow any other keys in the filter matching for service config in DNS. Based on your and Doug's comments, C and Go allow other fields.

[markdroth]

It looks like that restriction was originally added by Carl in grpc/proposal#50, and then slightly modified by @yashykt after A21 was finalized in grpc/proposal#133. That history would seem to imply that we chose this behavior for a reason, although I don't remember specifically discussing that, and it is possible that we simply failed to consider the details here when we had all of the discussions around A21. And while I agree with your "language lawyer" comment above, I think the important thing is to figure out how we really want this to work.

So, stepping back and considering this from first principles, it seems like the argument in favor of rejecting unknown fields would be to avoid older clients accidentally choosing the wrong entry due to lack of support for a newer selector field. We had basically this same concern when we discussed the xDS route matching selectors, and in that case we decided to basically avoid the problem by adding support for all of the common selectors up-front. But now that we've done that, we are in a state where newly added selectors would simply be ignored by the client, just like any other field. So my inclination is to do the same thing here and simply ignore unknown fields.

(The only other argument I can imagine is to avoid silently ignoring a field if there is a typo in the key name, but that argument would apply everywhere in the service config, not just in the DNS-specific choice criteria, and we've clearly already decided in A21 that it's better to not be that picky to allow forward-compatibility. So I don't think that argument is a justification for different behavior specifically for the DNS-specific choice criteria.)

I'm open to other suggestions here. Thoughts...?

[ejona86]

I think we all agree the current language is just bad, because it limits you forever. The choices were to ignore unknown fields or to skip the choice. So Carl's PR makes a lot of sense to me, in multiple ways. I think Yash's change makes sense for the serviceConfig field, but was a mistake for the unexpected field names.

Given the design and the history, I'd favor reverting the "unexpected field name" change in Yash's PR. If we went this route, we should probably reword it to avoid "invalid," so maybe "Any unexpected fields present in this object match no clients." Given C and Go's current behavior, it does add some weight to ignoring unknown fields, but it seems more like bugs or outdated implementations.

We had basically this same concern when we discussed the xDS route matching selectors

For xDS there was no alternative. There's also a more limited timeline of support for xDS and client versions can be monitored by the control plane.

I do think skipping the choice is much better than ignoring the fields. It adds a very good strategy for adding new fields. Ignoring fields doesn't really have a strategy for adding new fields other than "wait years for all clients to update, and then cross your fingers."

Java's current (mis)behavior doesn't really change the calculus, since the behavior prevents adding any new fields entirely... except that Java and C don't have DNS TXT enabled by default. So Go's current behavior is the only one that would be widely deployed.

[ejona86]

@YifeiZhuang, thank you for noticing that language; it now makes sense how things came to be. I think we're probably going to need Doug for the discussion, which means not talking much more about it this week. I've added an item for the TLs to discuss on the 30th. We'll just let this sit until then.

[dfawley]

I've added an item for the TLs to discuss on the 30th.

We still have not had time to discuss this issue, and may not for some time. Given the lack of user complaints here, I think this is going to be shelved until it becomes a real concern.