Move E/C/R/LDS service definitions to prevent breaking existing management servers

[kyessenov]

This change moves definitions of EDS, CDS, RDS, LDS back to package envoy.api.v2 to prevent incompatible changes with the existing management services for these discovery APIs. New discovery APIs and extensions should be added to the correct location in envoy.service.discovery.v2.

Top-level request/response messages are also placed next to xDS in the original package envoy.api.v2. This is due to the fact that protobuf reflection using protobuf.Any fixes the proto package for these messages. These messages are Cluster, RouteConfiguration, ClusterLoadAssignment, Listener.

Signed-off-by: Kuat Yessenov kuat@google.com

[mattklein123]

This seems like a great compromise to me. @junr03 can you confirm this will work for us?

[kyessenov]

Envoy update envoyproxy/envoy#2468

[mattklein123]

@kyessenov I was just talking to @junr03 and I think we were wrong. I think all APIs do use the Any, which means that I think we may need a translator Envoy side? (Or we have to move all the top level objects) Let's discuss Monday.

[mattklein123]

I just looked at this in a little more detail and I'm pretty sure that we use Any conversion in all cases (https://github.com/envoyproxy/envoy/blob/master/source/common/config/utility.h#L58). This code is fairly confusing so I'm not 100% positive that proto checks the type name but I think it does: https://github.com/google/protobuf/blob/master/src/google/protobuf/any.cc#L72

Assuming this is true I guess the options are:

1. Put type name conversion code inside Envoy for a few cases (inside the config helper above).
2. Move the top level stuff back also.

@htuch since you actually know how all this works WDYT?

[htuch]

@mattklein123 The Any conversion is only an issue where we're embedding things inside a DiscoveryResponse (see https://github.com/envoyproxy/data-plane-api/search?utf8=%E2%9C%93&q=%22google.protobuf.Any%22&type= to show we're not using Any elsewhere).

The compatibility issue only exists with the top-level resource object, e.g. Listener or Cluster. So, as long as those are stable, we're good.

[htuch]

We should probably leave a note explaining why only ADS/HDS/SDS live here for posterity.

[htuch]

Yeah. I think for the JSON/YAML v2 story, since we're the only ones ingesting it, we could do an Envoy-side translation of the type annotation. This is technically a JSON/YAML wire format breaking change, since if we had someone else making use of JSON/YAML ingest of these APIs via the data-plane-api definition, it wouldn't be great, but I think we're the only major consumer.

I think it comes down to how comfortable we are that we won't breaking any other consumers vs. the win of not having to undo the neat namespace moves in the earlier PR and providing a good framework for future growth here.

[htuch]

Thinking about this a bit more, I think translation in Envoy is not viable. This is because with the Any type, we are also breaking proto wire compatibility. Any messages have the type URL embedded in the message, not just in the JSON/YAML textual representation, see https://github.com/google/protobuf/blob/c236896ec5a7f9c0b021f14be61ee2e0a2c7a91b/src/google/protobuf/any.proto#L150.

I think we need to maintain true backwards compatibility here and move the top-level objects (just Cluster for example, OutlierDetection can live whereevs).

[htuch]

I had a look through the protobuf source. I think we're actually safe from breaking Envoy ingest with type URLs (needs verifying though), with both JSON/YAML And protobuf binary formats, since we always know what type we are unpacking to. The type URL is used in some places, for example message differencing, and might technically be used by other consumers of the Envoy API.

Technically we've broken wire compatibility here, but I don't think this will have real world consequences. Hence, we can leave it as is. The easiest way to verify is to run the blaze test //test/integration/xds_integration_test with the new data-plane-api but no changes to the input YAML files used in that test (i.e. preserve the old type URLs and see what happens).

[htuch]

Actually, I know this won't work everywhere from some code inspection; https://github.com/envoyproxy/envoy/blob/899ed8af8904ea61f723e95a7d89c17cadc00c69/source/common/config/grpc_mux_subscription_impl.h#L27. We're making use of type URLs when demuxing ADS streams.

[wora]

This is a breaking change per proto3 spec. The question is whether we can fix all our code to work around the breaking change. If Envoy is used by more than Lyft and Google, there is no chance we can make it work. Companies have to update their running services in proper order to accept the breaking change, which is too difficult. Do we know if any code written by other companies using these protos, especially via Any?

[htuch]

Even within Envoy, to fix this we would need to introduce the ability to alias type URLs in various places to cope with management servers that produce both old and new type URLs. It's doable, but it's pretty ugly.

[wora]

In that case, we can only move unused packages and leave everything else as is.

[htuch]

I don't think this is true. Only things that can be directly embedded in the DiscoveryResponse Any, which are 4 message types {Listener, RouteConfiguration, Cluster, ClusterLoadAssignment} can't be moved; everything else is fair game.

[mattklein123]

I don't think this is true. Only things that can be directly embedded in the DiscoveryResponse Any, which are 4 message types {Listener, RouteConfiguration, Cluster, ClusterLoadAssignment} can't be moved; everything else is fair game.

That's my assessment also, but since this is ridiculously confusing perhaps we could test? My preference at this point is to just move the APIs and top level message types back, and leave everything else. I agree we could modify Envoy to do translation of the type name, but I'm not sure it's worth it. I don't feel that strongly about it though.

This namespacing move is a huge improvement which we are going to live with for years. I think we should push forward as best we can.

[wora]

I agree we should move top-level elements back to un-break things, then we can iterate from a working state. I need a few days to make sure any production system is still working well, including things like logging, monitoring, analytics. Such secondary dependencies may also depend on certain names. In general, we should make a pilot change for large scale refactoring, so we don't cause too much churns at once.

[htuch]

There is no Google production use yet @wora, so we mostly need to consider Lyft and other 3rd parties.

…

On Sun, Jan 28, 2018 at 4:14 PM Hong Zhang ***@***.***> wrote: I agree we should move top-level elements back to un-break things, then we can iterate from a working state. I need a few days to make sure any production system is still working well, including things like logging, monitoring, analytics. Such secondary dependencies may also depend on certain names. In general, we should make a pilot change for large scale refactoring, so we don't cause too much churns at once. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#438 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKaLv93pnnYDK_hoDuXS7upRruFGpDIDks5tPOMbgaJpZM4RvCUw> .

[kyessenov]

I think type aliasing might ultimately end up causing more harm than benefit, so let's move top-level Cluster/Route/Listener/ClusterLoadAssignment back to the old package, since that's the simplest and intuitive way things should operate. I will leave a note explaining why xDS is split in half, and we can fix in later revisions.

In terms of impact, Istio is not affected since it uses v1 APIs. go-control-plane can be transitioned smoothly since the library hides the types, assuming the proxy is updated in lock-step.

[mattklein123]

The only other project I know for sure is using v2 APIs is Contour. There are probably others. @kyessenov I like that plan. It's not optimal, but it's still way better than what we have today.

[htuch]

While we're still hanging out on this thread, I wonder if envoy.api.v2.filter should really be envoy.config.filter.v2; this came up while writing a README.md for envoy/config, it seems that filter configuration has greater affinity for envoy.config than for envoy.api. Sorry to throw this hand grenade, I don't think it's a move we need to make, only nice to have.

[kyessenov]

@htuch I do want to organize filters, just not in the same PR that touches the core APIs.
I can do a follow up specifically for filters. They should be less of concern in terms of breakage since we use structs and internal names for them.

[htuch]

@kyessenov SGTM, makes sense.

[kyessenov]

PTAL: I moved the top-level types for E/C/R/LDS back to the envoy.api.v2 package.

[mattklein123]

LGTM, thanks for slogging through this.

[kyessenov]

Submitted envoyproxy/envoy#2472 to align the API change.

[mattklein123]

@htuch @wora are we good with this? LGTM.