api icon indicating copy to clipboard operation
api copied to clipboard

Investigate if we can remove duplication of protos

Open howardjohn opened this issue 5 months ago • 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

howardjohn avatar Mar 14 '24 15:03 howardjohn

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 avatar Apr 08 '24 15:04 howardjohn

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

howardjohn avatar Apr 26 '24 15:04 howardjohn

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.

ericvn avatar May 01 '24 14:05 ericvn

@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 avatar May 01 '24 14:05 howardjohn

@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 avatar May 13 '24 18:05 whitneygriffith

@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

howardjohn avatar May 13 '24 19:05 howardjohn

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

costinm avatar May 14 '24 14:05 costinm

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/

howardjohn avatar May 14 '24 15:05 howardjohn

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

whitneygriffith avatar May 14 '24 15:05 whitneygriffith

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. … <#m_8511947146594261202_> 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...

costinm avatar May 15 '24 02:05 costinm

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

howardjohn avatar May 15 '24 03:05 howardjohn