api/config: deprecate build_version in DiscoveryRequest.#9620
api/config: deprecate build_version in DiscoveryRequest.#9620htuch merged 8 commits intoenvoyproxy:masterfrom
Conversation
This is a followup to envoyproxy#9301, where it was not possible to deprecate a field on DiscoveryRequest as we were previously assuming identical v2/v3 transport protocols. After this deprecation, build_version can't appear in v3 messages, and we need to convert back to a true v2 DiscoveryRequest prior to JSON serializing for REST. There's more work to be done in the future, when we add new v3-only fields, but this should work for 1.13.0. Future work tracked at envoyproxy#9619. Risk level: Low Testing: coverage of the behaviors for both gRPC and REST config sources is provided by api_version_integration_test (failing previously as soon as build_version was deprecated). Fixes envoyproxy#9604 Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
In general LGTM, mostly some questions and small comments.
| // DiscoveryRequest. When they are added, we need to do a full v2 conversion | ||
| // and also discard unknown fields. Tracked at | ||
| // https://github.com/envoyproxy/envoy/issues/9619. | ||
| if (transport_api_version_ == envoy::config::core::v3alpha::ApiVersion::V3ALPHA) { |
There was a problem hiding this comment.
Should this potentially be != v2 instead of == v3alpha for when new versions get added? Possibly pedantic, not sure. Up to you.
There was a problem hiding this comment.
I think we need to come back to this code before too long anyway, per the tracking issue, so I think it's fine for the next short while.
| // TODO(htuch): refactor these message visitor patterns into utility.cc and share with | ||
| // MessageUtil::checkForUnexpectedFields. |
There was a problem hiding this comment.
I had a similar comment also about the redacting stuff that @mergeconflict is working on. Can the utility be used there also if possible?
There was a problem hiding this comment.
Probably. I need to do use this visitor in my next PR, so I reckon we can merge this as is, and I will move this to a more general location there
| * Reinterpret an Envoy internal API message at v3 based on a given API | ||
| * version. This will downgrade() to an earlier version or scrub the shadow | ||
| * deprecated fields in the existing one. |
There was a problem hiding this comment.
sorry because I'm dense: the scrubbing here is for which case? Someone ingested v2, but we want to output v3, so we take the current converted v3, and just remove any of the hidden deprecated fields that got us here? But if we do that won't we lose context as the v3 message won't actually be accurate? I think I'm possibly just not understanding the use case.
There was a problem hiding this comment.
It's simpler than that. This is for an internal v3 proto (no ingestion), DiscoveryRequest. We have a deprecated field in it now, build_version, which has the weird mangling with hidden_envoy_deprecated. When we send it out on the wire, e.g. as a v2 JSON REST message, it needs to still be around, but named with the original name, so we need to downgrade. When we send it out on the wire as v3, the field should be totally scrubbed, as it's not a valid v3 field.
Signed-off-by: Harvey Tuch <htuch@google.com>
| // DiscoveryRequest. When they are added, we need to do a full v2 conversion | ||
| // and also discard unknown fields. Tracked at | ||
| // https://github.com/envoyproxy/envoy/issues/9619. | ||
| return downgrade(message); |
There was a problem hiding this comment.
Sorry for being dense but I'm still a little confused around all of this. Is it possible to add some more comments throughout these functions with possibly some simple examples? If I'm downgrading from v3 -> v2, but I have a v3 message that has a smuggled deprecated field, how does that work with the serialize as string conversion? The old message doesn't know about the deprecated rename, so wouldn't we need code to actually map it back?
There was a problem hiding this comment.
I can add some, but the key intuition is that v2/v3 actually have exactly the same fields from a proto wire perspective if we are using the v3 shadow protos. They are just different names, which proto doesn't care about on the wire.
There was a problem hiding this comment.
Ah OK sorry, I saw SerializeAsString and I was thinking about a debug string which would have field names. We are just going back and forth with bytes. OK got it. I think some more comments would be good but feel free to do as a follow up.
Signed-off-by: Harvey Tuch <htuch@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, but needs master merge.
This is a followup to #9301, where it was not possible to deprecate a
field on DiscoveryRequest as we were previously assuming identical v2/v3
transport protocols. After this deprecation, build_version can't appear
in v3 messages, and we need to convert back to a true v2
DiscoveryRequest prior to JSON serializing for REST.
There's more work to be done in the future, when we add new v3-only
fields, but this should work for 1.13.0. Future work tracked at
#9619.
Risk level: Low
Testing: coverage of the behaviors for both gRPC and REST config sources
is provided by api_version_integration_test (failing previously as soon
as build_version was deprecated).
Fixes #9604
Signed-off-by: Harvey Tuch htuch@google.com