istio / api

API definitions for the Istio project
Apache License 2.0
455 stars 547 forks source link

Investigate if we can remove duplication of protos #3127

Open howardjohn opened 6 months ago

howardjohn commented 6 months ago

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

howardjohn commented 5 months ago

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.

howardjohn commented 4 months ago

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
ericvn commented 4 months ago

Also see https://github.com/istio/istio.io/issues/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.

howardjohn commented 4 months ago

@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.

whitneygriffith commented 4 months ago

@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?

howardjohn commented 4 months ago

@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

costinm commented 4 months ago

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 https://github.com/istio/api/issues/3127#issuecomment-2108548218, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2Q6ZIFP5BHTMNQO67TZCECANAVCNFSM6AAAAABEWLTGWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBYGU2DQMRRHA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

howardjohn commented 4 months ago

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 commented 4 months ago

@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.

costinm commented 4 months ago

On Tue, May 14, 2024 at 8:27 AM John Howard @.***> wrote:

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. … <#m8511947146594261202> On Mon, May 13, 2024 at 11:37 AM Whitney Griffith @.> wrote: @howardjohn https://github.com/howardjohn https://github.com/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) https://github.com/istio/api/issues/3127#issuecomment-2108548218>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2Q6ZIFP5BHTMNQO67TZCECANAVCNFSM6AAAAABEWLTGWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBYGU2DQMRRHA 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/

Good point, I forgot that. So users who read current (1.22) istio.io docs may have a bad time if they run in a cluster not running 1.22 (which is likely a large chance right now).

It was not a problem for many releases since we had v1beta1 since ~1.6.

I missed this - hopefully the users will just rely on ChatGPT/Gemini which still generates beta1 or alpha3, or know how to go to oldest versions of the site.

Did we test that Istio 1.20 deals correctly with a v1 VirtualService ( or whatever is the oldest Istio without the v1 protos) or the 'skip version' upgrades if users start using v1 CRs ? With K8S Gateway we had quite a few problems if I remember correctly.

Semantic versioning for APIs at work once again. I can't believe I didn't think about testing this. Hopefully others did...

howardjohn commented 4 months ago

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