Reducing common structs types or https://github.com/kube-rs/gateway-api-rs/issues/38

[dawid-nowak]

This approach aims to simplify the types and eliminate redundant ones. The approach works as follows:

1. We scan generated files and find similar items (structs or enums). Two structs are considered to be similar and processed further if their fields are exactly the same. The same method applies to the enums.
2. Once all similar structs and enums have been found, then we try get the name that will applied to similar structs from a file.
3. Once the new type names are settled we change the generated code and add a new file with the reduced types "common_types.rs"

We follow this process in phases:

1. In Phase 1, we only look at the structs with fields which are built from simple types (String, u32, u64)
2. In Phase 2, we also look at the structs with fields that have been changed in Phase 1
3. and repeat

Problems:

1. The documentation is stripped since the same type could be used in many different locations

[shaneutt]

First of all thank you for the PR. The time you put into this solution is very much appreciated!

I've done a high level review, and I see how the process works and the output, and it's impressive. I'll do a in-depth review soon but first I wanted to start off with some questions and comments regarding this approach from a couple dimensions:

UX

Obviously not having multiple types that do the same thing is a huge improvement, so that's great. In this draft however, users would currently have to use the "processed" feature/module. Ideally the users of this crate would have to do nothing special, nor have to understand any semantics of our crate that relate to how we generate code.

What are your thoughts on the feasibility of having these generators interleave the generated code with the code otherwise generated by kopium? That is to say, can we avoid the new processed module (and feature) and emit common_types.rs and the others right next to the APIs they belong to? 🤔

Maintainability & Kopium Interactions

Overall while I think the type-reducer has its complexities, those complexities are natural given the kind of work we're trying to do here. In my brief pass I didn't notice anything that stood out as an obvious workaround or anything like that. In fact, I was very pleased to see it's all built on top of a solid foundation on the syn crate, which is great.

I am curious though, after you've gone through the exercise of building this out: Do you have any new thoughts on the interaction with kopium here? How much of this work might actually be applicable there? Curious as to your thoughts 🤔

---

cc @clux as you may find this one interesting

[dawid-nowak]

Thanks for the kind words. Let me answer your questions:

UX
I think the "processed" name is not great but that could be easily changed. The Kubvernor Gateway implementation was initially based on "standard" APIs but I wanted to see whether "processed" types would improve the code. I have migrated the existing codebase to "processed" APIs and I think the results are optimistic (at least from my perspective).

Initially, I created a lot of types manually to handle duplication between the HTTP and GRPC Routes. With "processed" types I was able to automate that part somewhat. As a result, I was able to (re)move and refactor some duplicated code.
The one thing I found is that I would never mix the types. I would either rely on the types provided by "standard" APIs or on types provided by "processed" APIs.

It is very doable to emit "common" types and keep them side by side with "standard" types. In this approach I think we would need to provide a whole bunch of converters from "standard" types into "common" types. This would be very much akin to my original approach to implementing Kubvernor.

I think from the developer perspective such an approach would be confusing because the developers would need to keep selecting which type to use at a given moment in time and keep on converting from one type to another.

Maintainability & Kopium Interactions
This is an interesting one and it prompted me to learn more about the Kubernetes APIs.
Ultimately, I think the problem is that the yaml files describing CRDs are not using OpenAPI reference mechanisms to avoid duplication of types.

When one investigates the Go implementation, it is clear that there is a lot of code reuse there. GRPCRoute is dependent on types from HTTPRoute for example and there is shared_types module with type definitions shared among all CRDs.

This reuse disappears when yaml versions of the CRDs are generated. All information on how the types are reused is erased which makes it super hard for Kopium.

I think that if yaml/OpenAPI definitions relied on OpenAPI references to reuse shared definitions then it would be much easier for Kopium to generate Rust code very similar to the one in "processed" APIs. Indeed, this PR probably wouldn't even be necessary.

I don't know enough of the Kubernetes/Gateway API process to speculate whether it would be possible to change how yaml versions are generated.

[shaneutt]

Maintenance & Kopium

Using refs in the specification would be more ideal. We discussed this a bit in #38 and I personally think it's an inevitability in the long run if we're going to have strong support for non-Golang implementations. We can still move forward with something that works for us for now, however. The only conflict I would worry about it whether we try to mint a v1 of this library knowing that this might change significantly down the road. For the purposes of this PR however, I don't think we sweat that, but what GA is going to look like for this library is always in the back of my mind.

UX

I'm not sure I'm clear yet: can we adapt this work to emit less, and keep with the original structure?

[dawid-nowak]

Maintenance & Kopium
Looks like this has been around for a while?
kubernetes/kubernetes#62872

UX
Yeah, in terms of the UX I think I am not clear what the ask is either 😃
I think it would be good if you said exactly what is the desired outcome from your perspective in the context of this PR. Then we can discuss whether it is possible, doable or how it fits with my project. I hope this sounds fair.

[shaneutt]

Maintenance & Kopium Looks like this has been around for a while? kubernetes/kubernetes#62872

Yes. However it doesn't preclude the notion of us doing something that will be complete faster, was my point. I still like what you've done here and think it would be better to try to work this in now because making the changes in upstream Gateway API wont be anywhere near as fast.

UX Yeah, in terms of the UX I think I am not clear what the ask is either 😃 I think it would be good if you said exactly what is the desired outcome from your perspective in the context of this PR. Then we can discuss whether it is possible, doable or how it fits with my project. I hope this sounds fair.

Yes no worries, sorry for the confusion.

Basically I'm advocating for maximum reduction in structure change. Ideally the new generators would emit types in an additive way, avoiding changing/adding modules as much as is feasible.

This is for multiple reasons:

1. I have a strong preference for the current layout. Currently: related things are grouped via a module (e.g. gateway_api::apis::standard::httproutes::HTTPRoute), and I would prefer that these remain in place and for common types you use something like gateway_api::apis::standard::common::BackendRef, etc.
2. As it relates to the above: I know we're not v1 so we're not guaranteeing backwards compatible changes, but we do have users and I want our approach to backwards incompatible changes to be something like "we will avoid them unless they are not possible, or the time cost is far too high".

To be clear, I'm open to being persuaded, but I think it's good to have the discussion. LMKWYT? 🤔

[dawid-nowak]

I think exposing common types as gateway_api::apis::standard::common::* shouldn't be a problem.

I am not clear on the first part.

Using HTTPRoute as an example.

Even if we keep the preferred layout , gateway_api::apis::standard::httproutes::HTTPRoute will be changed to use the types from gateway_api::apis::standard::common::*. This would be ideal but it completely breaks the existing clients.

Otherwise, we either need to create a new module so we can keep the name of the type as is or create a new type in gateway_api::apis::standard::httproutes

Creating a new module is much simpler to implement. Interleaving types will be problematic due to the recursive nature of the algorithm. Also the types will have to have different names so they can co-exist.

[shaneutt]

Your example was helpful. As far as the backwards incompatibility of struct attribute types: I don't think we should try to spend all that time trying to avoid that. What we should do instead is publish an actual (pre)release which documents the backwards incompatible change as a helpful hint to users (it doesn't make sense to have a bunch of type aliases everywhere in the long term).

So to review the plan:

1. change the generators to push new common types to gateway_api::apis::standard::common::*
2. change the generators to include the new common types in their respective parent types (e.g. gateway_api::apis::standard::httproute::HTTPRoute)
3. create a CHANGELOG so we can document the breaking change

If that sounds good to you and we can do it in a way that reduces (not eliminates) changes to the API surface, then that sounds like a plan to me.

[dawid-nowak]

I think this PR is ready for a proper review.

• removed processed APIs
• added type generation for experimenal APIs
• added a way to just rename types
• added a mechanism to ignore certain types ... this was needed for experimental UDPRouteSpec and TCPRouteSpec

It would be good to check the mapped types names here and here.

[shaneutt]

Did a first pass, mostly looking at what is emitted first since it's such a huge diff. I will take another pass to specifically review the type-reducer and changes to update.sh. 👍

Mostly just a few minor things, and some questions.

Also @aharbis @the-wondersmith @kflynn @blinsay @EItanya you're all folks that I think may be using gateway-api-rs in some capacity, so you might find this interesting.