Investigate if we can remove duplication of protos #3127
Comments
|
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: CRDsThis 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 This should give identical output, so its 100% compatible. Go typesTwo 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 compatibilitySending 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. OverallThe setup would look like this, just showing one type (and ignoring v1, its the same as v1beta1) |
|
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 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 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. |
Part of istio#3127. Goes with a corresponding tools change; this will fail until that merges. This just shows DR. The tool will support both the new and old way (we can remove the old way if we want), so we don't have to move everything at once. We will, though. I kept it to one so its easy to review first.
Part of istio#3127. Goes with a corresponding tools change; this will fail until that merges. This just shows DR. The tool will support both the new and old way (we can remove the old way if we want), so we don't have to move everything at once. We will, though. I kept it to one so its easy to review first. (cherry picked from commit 0ea8ea9143500c11efe326bdc50afb64dd46059f)
Test to ensure istio/api#3127 work doesn't cause regressions
Test to ensure istio/api#3127 work doesn't cause regressions
Test to ensure istio/api#3127 work doesn't cause regressions
|
@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/ |
I am a fan of moving the .proto files out of the versioned folders. |
* Allow defining CRDs from a single version Part of #3127. Goes with a corresponding tools change; this will fail until that merges. This just shows DR. The tool will support both the new and old way (we can remove the old way if we want), so we don't have to move everything at once. We will, though. I kept it to one so its easy to review first. * Move all APIs over
|
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)
<#3127 (comment)>>, 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...
|
|
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 |
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
alphapackage types look liketype PeerAuthentication = v1.PeerAuthentication. This is what gateway-api does. However, this doesn't quite work for proto, which doesn't have a type alias conceptThe text was updated successfully, but these errors were encountered: