grpc/xds: Add feature-gated xDS proto codegen and the generated .rs code#2723
grpc/xds: Add feature-gated xDS proto codegen and the generated .rs code#2723gu0keno0 wants to merge 15 commits into
Conversation
|
I've been poking at this today on my computer, trying to understand what protobuf is doing and trying to understand our options. Some notes that don't impact the heart of the PR too much:
I had thought (2) is what we said would be best during our meeting. @dfawley can confirm. (I'll be on vacation tomorrow.) At this point I think I see why you've made the various choices you did. I think I agree that is at least close to optimal right now. But I think we'd also all agree this is ridiculous. I want to think more about what we can suggest to protobuf to make the situation better. I feel we'll be doing everyone a disservice if we don't figure out a better approach before protobuf stabilizes. (I don't yet see a reason protoc is run per-file instead of per-directory/proto-package. I'm right now assuming both ways would work and you just chose something and went with it.) |
|
Thanks for looking. Answer the questions inline.
In #2728 @dfawley is suggesting to put the generated code into a separate crate grpc-xds. I think build.rs makes sense. For Envoy go-control-plaine and java-control-plane, they have to check in generated code because they don't have build.rs . For Rust it's not a problem.
The per-file approach is because there are naming conflicts. If we feed all of them directly into protoc, the generated code will have naming conflicts under the same flattened namespace (5000+ errors), because in generated code, the proto types are re-exported by a statement like "pub use internal_do_not_use_:: re-export." and there are the name conflicts such as "Policy" and "Filter". The per-file approach creates a module wrapper for each file, and therefore eliminates the name collisions. I think this shenanigan will go away if Protobuf natively support organizing the generated code by protobuf package modules (similar to this PR but natively supported by protoc). |
I was contrasting per-file with per-directory/package. There shouldn't be name conflicts within a proto-package. |
cc @esrauchg 😄 |
|
Protobuf Rust maintainer here: I'm definitely happy to get the "wtf is this" feedback spelled out and we can definitely consider adjustments. I have a very firm grasp of how this does/should work in blazel, but I'm not so clear as to how it should work outside of that context and so am open to feedback. Just to clarify, the intended model of protoc (language agonstic) is that you should be running it once per conceptual library/package granularity, and your library/package granularity shouldn't be so large that you have a bunch of shortname collisions within it: it suggests something is conceptually smelly at best if you have two top-level messages with the same name in the same library/package that are only disambiguated by the package name spelled at the top of the file. Theres an oddity which is that there's 3 different concepts of: file paths, object namespacing, and the library boundaries. In Java for example the file paths and namespaces are coupled, but library boundary is M:N compared to that (*except under JPMS, which has somewhat shallow uptake in the ecosystem). In C++ directories:namespaces are M:N and namespaces:libraries are also M:N. Rust and Go are special here because your imports name the package. In blazel model, this is resolved by the target name as written in the BUILD file being used as the effective namespace. With fine-grained rust_library/proto_library (its one crate per .proto file effective) this namespacing is already very tight, and so also adding in mods of the .proto package would be really bad on ergonomics. Its technically true that you can make a proto_library() that doesn't work in Rust here from name collisions, but it would require you to make a proto_library() which has 2 .proto files with different proto-packages and the same top level name, which is basically a "own breakage" at that point. Go in non-blazel has a solution of go_package, where a single protoc invocation actually can end up emitting N different Go Packages. This is basically a historical oddity and debatably regretted: really a single protoc invocation shouldn't ever be emitting N conceptual 'libraries'. But we're not reopening GoProto design here, its basically fine even if this quirk is a bit outside of "what should a single protoc invocation do" ethos. Crates.io Cargo crates are basically coarse-grained instead of the fine-grained ones in blazel, I fully understand that you wouldn't really want 1-crate-per-.proto-file in that environment. I still did mostly expect that you would likely have 1 crate per conceptual package and conceptual package would map back to the package at the top of the .proto file. But broadly I expect you shouldn't have 1 crate with 100 .proto files which have 10 different packages declared at the top of them either:, middle ground is "at least 1 crate per .proto package". In general you also could run protoc N times and then put post-hoc put those into corresponding mods, as the example crate chooses a mod here: Hope that context helps, and I'm happy to entertain improvements here. Thanks! |
|
@esrauchg : thanks for the context and discussion. I was able to read and understand the rational of crate-per-package design as I was working on this PR. I'd like to suggest that the main issue here is maintenance overhead. The xDS protos contains ~67 proto packages, if we make all of them crates, then potentially we will add the 67 crates to workspace, and will need to manage their publications. While technically this is still doable, I just feel it's probably not the ideal approach because we just need the generated code for grpc-xds to work, the publication of the crates, or even the creation of the 67 crates themselves, are just not the essential bit of gRPC xDS. The xDS proto is also moderately complex, many other projects (primarily, data model libs) has a more complicated protobuf setup. This is why I was considering the alternative approach: codegen the .rs files and wrap each file/package with their own rust module. Therefore, the namespace problem is solved by the Rust modules, and we only need to publish grpc-xds crate (I'll update the PR for doing that), which only uses the protos as pub(crate) dependencies. The proto themselves do not even need to be published because users do not need to see them. Maybe there are better alternatives, glad to learn about them. |
|
@ejona86 @dfawley I've done the following clean-ups:
|
Add xds protos, codegen and generated .rs files
Motivation
The usual places for the generated code of xDS protobuf is https://github.com/envoyproxy, e.g. https://github.com/envoyproxy/go-control-plane. However, currently there's no official https://github.com/envoyproxy/rust-control-plane.
For gRPC, we need to use the Google protobuf codegen to generate the xDS structs, therefore created this PR to do the job.
Solution
Test Plan