Investigate if we can remove duplication of protos
Opened this issue · 11 comments
Today, we duplicate protos for each API version. But they must remain identical, so we have automation keep them in sync. Given its automated its not so bad, but its still pretty tedious and leads to bloat (in LOC, reviews, binary sizes, ...).
It would be ideal to avoid this.
Requirements: We must retain compatibility in {wire format, yaml format, Go code, documentation}.
We can maybe relax the "Go code" part if its a minor issues, but we don't want to break all importers of istio/api.
If this was pure go, I would say we can just have the alpha package types look like type PeerAuthentication = v1.PeerAuthentication. This is what gateway-api does. However, this doesn't quite work for proto, which doesn't have a type alias concept
New idea and new problem.
At some point we will have Istio reading alpha,beta,stable. Most APIs are in all 3. This means we have 3 copies of each proto across the code. This bloats the binary and is a source of possible errors.
We could have just 1 proto; perhaps the oldest for maximum compat. This is a breaking change, but only to the wire format which is rarely used.
Ok there are a few areas to tackle:
CRDs
This is the easiest. We can have the crd-gen read a single set of protos. It doesn't matter which one since they are all identical. We can then configure on that proto versions: [v1alpha3, v1beta1, v1] (and served,stored, etc).
This should give identical output, so its 100% compatible.
Go types
Two sets here, istio/api and istio/client-go. Additionally, usage with client-go, informers, controller-runtime, etc
There is one way to maintain compatibility of the Go types -- you can type alias every message. gateway-api does this. Its not technically 100% compatible if you do wonky things like reflection, etc but its close enough IMO. We can make a protoc plugin to generate aliases.
We can also consider a breaking change, and only keep one version of the types around. Note that istio/client-go NEEDS multiple versions. However, we can just have the outer object versioned. That is:
// This object is repeated in /v1alpha3, /v1beta1, /v1
type Sidecar struct {
Spec istio_api_common.Sidecar
}This helps align with the k8s client ecosystem which introspects the types to determine GVK, etc. Again, this is what Gateway API does.
Protobuf compatibility
Sending Istio APIs over the wire on proto is rarely used, but is in some cases done. In Istiod directly, and our integrations (sending over xDS), we always use the same version we use for k8s (which is the oldest version, typically alpha).
Type aliases don't work with proto. Like k8s, proto introspects the types for various things. However, they do this on every message, unlike k8s which just does the top level message.
Our only option here is to consolidate on one version.
To keep compatibility, I suggest we keep only the oldest version as the source of truth.
This will only break users who send Istio APIs over protobuf, where both the client and server are not Istio components, and happen to use the bleeding edge versions. For the rare users that do this, they can either generate their own protos, switch to the version we pick, or use an older istio/api import.
Overall
The setup would look like this, just showing one type (and ignoring v1, its the same as v1beta1)
api
|_crds.generated.yaml - same as today
|_networking
|_v1alpha3
|_sidecar.proto - full proto, as is today (but with crd version declaration)
|_sidecar.pb.gb - full go generation, as is today
|_v1beta1
|_~sidecar.proto~ - REMOVED
|_~sidecar.pb.gb~ - REMOVED
|_sidecar.aliases-gen.go - New file, type aliases for all structs in v1alpha3
client-go
|_...everything is the same
Also see istio/istio.io#14992.
Today we have sync.sh which copies the bottom portion of the file from the older version, and sets mode:none on all the newer versions of the file. The buf generate then builds the html file from the older, sync-from, file which results in the html file containing the older version.
Can we simply change the sync-from to the newest version, and sync-to the older versions? This should set the mode:none on the older versions, and result in the html files being created for the new versions.
@ericvn the doc comments need to be the same across versions I think, so the solution would be to document v1 for all the YAML examples on all the versions I think?
There is top level commentary which could vary, but places like https://preliminary.istio.io/latest/docs/reference/config/networking/destination-rule/#ConnectionPoolSettings are in the 'sync zone' so should be v1 everywhere.
@howardjohn even though we are using the oldest API version as source of truth, can we standardize on using the newest API version in doc examples?
@whitneygriffith yep! And we should.
We could, I think, even move the .proto files out of versioned folders if we want. Though we will need to keep the .go files in the versions
Using the newest API in doc examples will mean any user on older versions will have troubles. We could do it once the oldest supported release of Istio is 1.22, and everything else is out of support - but even then it's going to be problematic for vendors that provide a longer support window.
…
On Mon, May 13, 2024 at 11:37 AM Whitney Griffith @.> wrote: @howardjohn https://github.com/howardjohn even though we are using the oldest API version as source of truth, can we standardize on using the newest API version in doc examples? — Reply to this email directly, view it on GitHub <#3127 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2Q6ZIFP5BHTMNQO67TZCECANAVCNFSM6AAAAABEWLTGWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBYGU2DQMRRHA . You are receiving this because you are subscribed to this thread.Message ID: @.>
Istio docs are point in time snapshots. If you want to use Istio vOld you need to use the older docs: https://istio.io/archive/
@whitneygriffith yep! And we should.
We could, I think, even move the .proto files out of versioned folders if we want. Though we will need to keep the .go files in the versions
I am a fan of moving the .proto files out of the versioned folders.
Istio 1.0 would work with V1 CRDs because of how Kubernetes CRD versioning works. https://blog.howardjohn.info/posts/crd-versioning/#version-conversion
Gateway only breaks things because they remove APIs