FYI at the beginning of Q2 I think we will be bumping to minor version 3.1.0. We will tag all deprecated fields as deprecated at 3.0.0 prior to that. Then in 2022 Q2, we will bump to 3.2.0 and remove support in Envoy for everything deprecated 3.0.0. This gives 2 years of support for minor version 3.0.0 and any field that is removed in 2022 will have had 1-2 years life as deprecated.

I agree that the first minor version will be 3.1.0, and all deprecated fields will have the deprecated_at annotation set to 3.0.
At the moment it is set to 3.7.0 to "comply" with the api/envoy/config/core/v3/deprecaetd_example.proto example contents.

Also Node will indicate for simple string names. I'd generally lead in with the old way of doing it and then point out the new way.

Added user_agent_latest_api_version and user_agent_oldest_api_version to Node.

Looks good. Suggest also updating the docs here to cover both Node and xdstp://

As discussed IRL, we should probably move this to a separate PR for this feature in general. @markdroth would this help get rid of the "15 second rule"?

Yeah, I think something like this is a good idea. But I do think it should probably be a separate PR.

Do we actually want this to be a separate map? An alternative would be to embed the DiscoveryResponseStatus inside of the Resource message in the normal resources field. That way, clients just need to look at one field for every resource, and some of those resources might be a special type that indicates that the server didn't have the requested resource.

Also, what is the expected semantic here when a subscribed resource does not exist and multiple responses are sent during the time it does not exist? For LDS and CDS, is the server obligated to send every non-existent resource in every response, just like it normally does? For other resource types, can the server send the response status just once, and the client assumes that the status doesn't change until/unless it receives a subsequent response that either populates the resource or changes its response status? (The semantic here might be a bit more obvious if the DiscoveryResponseStatus was inside of the resources field instead of being a separate map.)

I agree that it makes more sense to add the DiscoveryResponseStatus to each specific resource (in Resource).
I'm probably missing something here, but IIUC the resources field in a SotW response is of type google.protobuf.Any, and the response is not required to return the resources in a wrapped Resource. If this implies that the Resource wrapper is required how do we plan to the gradual introduction of this to existing xDS services?

Looks good. Suggest also updating the docs here to cover both Node and xdstp://

within a major version (including all minor versions within that major version), we do not allow ..

Suggest making this a list and enumerating explicitly all things that affect path version, since this is basically the specification doc for versioning.

within a major version (including all minor versions within that major version), we do not allow (seems a repeat of above)

allows for introduction of new capabilities and features that will be supported by Envoy and gradual deprecation of older feature support in Envoy.

Actually, delete this paragraph, I agree it is confusing to keep when reading the context around it.

No API field removal :) These will be human changes to the Envoy where we delete stuff that is not trully unsupported at minor version clock ticks.

Curious if you anticipate any other fields in this message?

I'd phrase this as Latest xDS API patch version supported by the client.

Nit: maybe just absl::StrSplit on .

The name of this file has a typo in it.

Yeah, I think something like this is a good idea. But I do think it should probably be a separate PR.

Do we actually want this to be a separate map? An alternative would be to embed the DiscoveryResponseStatus inside of the Resource message in the normal resources field. That way, clients just need to look at one field for every resource, and some of those resources might be a special type that indicates that the server didn't have the requested resource.

Also, what is the expected semantic here when a subscribed resource does not exist and multiple responses are sent during the time it does not exist? For LDS and CDS, is the server obligated to send every non-existent resource in every response, just like it normally does? For other resource types, can the server send the response status just once, and the client assumes that the status doesn't change until/unless it receives a subsequent response that either populates the resource or changes its response status? (The semantic here might be a bit more obvious if the DiscoveryResponseStatus was inside of the resources field instead of being a separate map.)

I would still like to consider adding a flexible range-based matching semantic for minor versions. Can we do that as part of this effort, or do we want to decouple that from this?

Hmm... Actually, now that we've decided to stick with string representations for new-style resource names, doesn't that mean that we don't actually have a way to add more flexible matching syntax later if we don't do it now? When we were going to be using ResourceLocator on the wire, it would have been easy to add new matching types, because they'd be expressed in different fields. But with the string form, it's basically just "key=value&key=value&...". So if we're interpreting the values literally, then any new special syntax we add there may break existing users.

It seems like we need to either add the more flexible matching syntax now or carve out some sort of escape hatch in the URI syntax to allow for future changes. (Actually, the latter would probably be a good idea even if we do the former, since we may want to add other flexible matcher types in the future.)

I'm on the fence on this one. I would like to future proof with some notion of escape hatch in the URI syntax, that's a no brainer. I think I'd like to understand the full set of flexible match objectives before deciding on a concrete syntax and what do use here instead of min/max version. How do you think is best to go forward on that @markdroth?

I agree that range-based matching semantics parameters should be supported, but I'm not certain I understand many of the use-cases that it will be used for.
I think that eventually we can replace this with a parameter similar to this: xds.api.range.minor_version={"start":5,"end":6}.

Is the concern here that we don't want magic params that have prior agree to semantics? Meaning, if we have an oldest value and a newest value it seems not hard to just do the range matching manually.

IIUC the issue of exact-matching is that the returned resource must have the same parameters.
Range-parameters enable explicit semantics. For example, a cache (or xds-relay) will be able to return the same resource with minor version 5 for a request with the range [4,5] and for a different request with the range [5,6].

Right, the issue is that the current exact-match semantic means that the context params in the result must exactly match the ones in the request. So if you have one client requesting xds.api.range.minor_version=[0, 1] and another requesting xds.api.range.minor_version=[1, 2], you can provide the contents at version 1 to both of them, but you need to return each one with a different context parameter. This will basically lead to cache pollution, where any caching proxy will need an increasing number of copies of each identical resource under different names, and the pollution will increase combinatorically.

What I am proposing is a range-based matching mechanism where the clients can ask for something like xds.api.range.minor_version=SPECIAL_RANGE_SYNTAX[0, 1] and xds.api.range.minor_version=SPECIAL_RANGE_SYNTAX[1, 2], and both can get back the same resource with context parameter xds.api.range.minor_version=1. In order to make that work, we need to define the syntax for range-based matching, make sure it doesn't conflict with any "flat" context parameter values (e.g., by defining some sort of escaping mechanism), and make sure that xDS servers, clients, and proxies understand it.

This is a bit more work, but I think it's also a cleaner solution. And I think this same mechanism will be able to be used for other use-cases in the future. (We already have one use-case internally for which we've been discussing something like this.)

@htuch, assuming that we have a reasonable way to future-proof the syntax in a way that would allow us to add more flexible matching types if we need to do so in the future, I think I'd advocate for restricting our initial changes to just the cases that we need immediately. For minor version, I think that's range-based matching. And if we want to handle the patch version the same way, we might also want a simple wildcard match for that. I think anything else can wait for the future.

I would be happy to write up a proposal for how this would work, but I need something to build off of. Where did we leave things on plans for the initial xRFC for the new naming scheme? Are you still planning to do that at some point?

OK thanks this makes sense to me. I agree we should do the work to implement range syntax. It doesn't seem too terribly difficult.

Yes, a proposal here for a syntax which is future proof and can initially accommodate simple range would help unblock work here I think. Thanks Mark.

Why is this here instead of being in the Node message? As currently designed, it's by definition something that works only when directly talking to an authoritative server, because it's not part of the resource name, so caching proxies can't handle it.

Note that if we do add a mechanism for flexible range-based context param matching, then we can make this part of the resource name, so it would work basically the same way as the minor version. I think that would actually make this mechanism easier for people to understand.

If it's not federated, it can live in the Node message agreed.

I'm probably missing something but why isn't it effectively part of the resource name? Meaning, if we assume the patch level for a specific message is a recursive walk of all sub-messages, don't we know it on a per message/resource basis and then it might be different and should be set here? I guess maybe I'm then also confused why we don't need major/minor here also? Or we assume that those are sent in context params?

I think it's related to the semantics of exact-match and of an optional patch support.
If this is part of the context-params, caches should only return resources with the patch, and caches will need to cache the same resource with different patch versions.

OK thanks, this makes sense. Both this convo and the range convo does make me wonder a bit if we are thinking about context params wrong and the server should actually be able to return a result with a subset of matching params? Does that make sense?

Yes, let's do range and move things to patch, thanks.

What if there was a special wildcard param value? (Just thinking out loud here). This would then allow intermediate caches to be quite intelligent about mappings. Taking the patch value this would allow the server to respond with patch_value=* or whatever.

I agree with moving patch into Node. Actually Node already has a field ApiVersionNumber user_agent_latest_api_version where a client can add the patch version it supports.
Just wanted to note that at the moment there's no support for patch version in the xdstp naming scheme version negotiation, and that should be fine because patch version support is optional.

What if there was a special wildcard param value? (Just thinking out loud here). This would then allow intermediate caches to be quite intelligent about mappings. Taking the patch value this would allow the server to respond with patch_value=* or whatever.

IMO the patch field should actually be understood as maximal supported patch version, so if the server responds with patch_value=*, the intermediate servers will not be able to know if the resource is supported in max_patch_version=3 or in max_patch_version=6, and they'll need that to process future requests.

, the intermediate servers will not be able to know if the resource is supported in max_patch_version=3 or in max_patch_version=6, and they'll need that to process future requests.

But isn't that effectively by design? The management server shouldn't send * unless it really means it. I think what I'm getting at is it would give a way for the management server to effectively ignore patch that is cache compatible.

I think it's possible to introduce a notion of cache-ignored context params. This is analogous to the ability in CDNs to ignore a list of query params. We've discussed adding this a few times to the spec.. I think the point becomes moot if we have range params though, since instead of max supported version N we can just say [0..N] with the same semantics and caches basically ignore it.

Note that unlike the SotW protocol, which currently doesn't have a way to explicitly indicate that a subscribed resource does not exist, the delta protocol does have such a mechanism (the removed_resources field). So in this case, the only value of this field is to support the version-mismatch case. And if we handle the version via flexible range-based context parameter matching, then we wouldn't need this field at all.

And if we handle the version via flexible range-based context parameter matching, then we wouldn't need this field at all.

How does this relate? The management server can still be missing a version in the specified range.

Right, but if the version was just a context parameter, then it would effectively be part of the name, so NO_MATCHING_RESOURCE and NO_MATCHING_VERSION_RESOURCE would be the same thing. And at that point, we would not need this field, because the existing removed_resources field would do what we need.

In this case, we've actually strictly lost information, but I agree it's a more consistent and logical way of viewing things. I think @mattklein123 was interested in a specific "no matching version" feedback signal, but maybe this would be sufficient.

I think the requirement here is that we have a clear way of providing status to the client about why a resource was not returned. The DiscoveryResponseStatus message seems to do that nicely. I don't have a strong preference on how we do it exactly but I agree with @htuch that we should not lose information on why something happened and should probably have it look similar for both SoTW and delta, whatever the best way of accomplishing that is.

I'm fine with whatever solution you all agree to on this as long as we maintain the data fidelity.

Right, but if the version was just a context parameter, then it would effectively be part of the name, so NO_MATCHING_RESOURCE and NO_MATCHING_VERSION_RESOURCE would be the same thing. And at that point, we would not need this field, because the existing removed_resources field would do what we need.

Maybe I got it wrong, but I thought of the response status as a complimentary field to the removed resources, providing the reason why were they removed. It essentially allows the following differentiation: a requested resource name (no matter what version) doesn't exist in the management server; and a requested resource in a specific version range doesn't exist in the management server (the server only handles older vs newer versions).
At the moment, the client doesn't use this information (other than logging) but it does allow the flexibility to request the resource from other servers, or to notify the operator that there's an issue that needs to be addressed (the client needs to be updated).

Also, could we consider using google.rpc.Status instead of a custom DiscoveryResponseStatus enum? That seems a bit more flexible in the long run.

I agree it should be used, and stating that the code field in it will be one of the DiscoveryResponseStatusTypes that we define.

I don't feel strongly about it, but I tend to agree with Mark that resources_status could be a superset of removed_resources and we can just build it in a way that would allow differentiating the two cases?

At the moment, the client doesn't use this information (other than logging) but it does allow the flexibility to request the resource from other servers, or to notify the operator that there's an issue that needs to be addressed (the client needs to be updated).

I suspect that there are no cases where the client will need to programmatically differentiate between these two cases. Knowing the difference is useful for humans but not really helpful for client implementations. Most clients will simply treat the resource as non-existing in both cases; I could imagine a client deciding to try another management server instead, but it seems unlikely that a client would choose to do so in one of these two cases but not the other.

That's why I favor simply deprecating the existing removed_resources field and replacing it with map<string, google.rpc.Status> resources_not_available, where the map value would be an error message indicating why the specified resource was not available. This approach would allow us to handle any arbitrary reason why the resource is not available, in case we run into cases other than the two we are currently considering.

I agree it should be used, and stating that the code field in it will be one of the DiscoveryResponseStatusTypes that we define.

I think the code in that proto needs to be one of the standard status code enum values. It would be very surprising to users for the code to be a new error space.

Yeah, the primary requirement is debuggability; as long as we can include the version failure in the google.rpc.Status string it's all good.

Is the concern here that we don't want magic params that have prior agree to semantics? Meaning, if we have an oldest value and a newest value it seems not hard to just do the range matching manually.

I'm probably missing something but why isn't it effectively part of the resource name? Meaning, if we assume the patch level for a specific message is a recursive walk of all sub-messages, don't we know it on a per message/resource basis and then it might be different and should be set here? I guess maybe I'm then also confused why we don't need major/minor here also? Or we assume that those are sent in context params?

I think the requirement here is that we have a clear way of providing status to the client about why a resource was not returned. The DiscoveryResponseStatus message seems to do that nicely. I don't have a strong preference on how we do it exactly but I agree with @htuch that we should not lose information on why something happened and should probably have it look similar for both SoTW and delta, whatever the best way of accomplishing that is.