Generate alias for types instead of copies #3188
Conversation
|
Skipping CI for Draft Pull Request. |
istio-testing
added
do-not-merge/work-in-progress
|
What's the process for when we actually need to have 2 versions of a resource? Or is the plan to not ever do that and to rely solely on admission? |
|
@keithmattix it is not possible in Kubernetes to have 2 distinct schemas across versions without a conversion webhook which we don't (currently?) plan to ever do. That being said, if we legitimately do we can just have those type(s) do things the old way I suppose |
|
It would be more like the v1alpha1 and a v1 upgrade story. Istiod would need to be able to read both to prevent breakage. So the rule would be that you can't make a change (i.e adding a field) while promoting a version. If we don't document that, we should. I think that prevents the issue |
|
@keithmattix you can still read both versions from k8s (see info in the "Go types"). Actually it would make it much easier, since the types are actually the same. In todays world, you would either need to have a slow conversion or duplicate code for each version. |
|
Discussed in TOC today; there were no objections that came up there. Feel free to comment here if you were not there/disagree/changed your mind/etc |
istio-testing
added
the
needs-rebase
howardjohn
force-pushed
the
protoc/gen-alias
branch
from
4ea7acd
to
336c5b5
Compare
istio-testing
removed
do-not-merge/work-in-progress
|
/retest |
1 similar comment
|
/retest |
howardjohn
force-pushed
the
protoc/gen-alias
branch
from
849cbed
to
92a29ad
Compare
There was a problem hiding this comment.
LGTM overall
What is not clear to me (might be good to have it doc-ed) is if I need to introduce a new API change (for example, add a field) in a resource, I update the proto file in the alpha version first if it exists (then beta). Assuming I added to the proto file in the alpha version, what to do if I want to include the change in beta version? Not clear to me how to update the corresponding alias.gen.go file.
Today: find the source of truth proto among 3 that look the same, edit it, and run With this PR: there is only 1 proto per type. Edit it and run |
istio-testing
added
the
needs-rebase
howardjohn
force-pushed
the
protoc/gen-alias
branch
from
92a29ad
to
55c99e9
Compare
istio-testing
removed
the
needs-rebase
I mean how to indicate this type is only for alpha but not for beta for example? |
This is impossible both before and after this change. Before it was impossible because our tests assert you do not do this (and if we didn't, Kubenetes will reject it). Now its impossible because there is only 1 proto |
|
I know that some of the work I had started earlier tried to update the apiVersion in the various snippets. Is that something we want to do outside of this work, or maybe pull into this PR (but agree it doesn't seem to fit this PR as titled)? |
@whitneygriffith already has a PR doing this, built on top of this PR. This PR makes it easier since there is ~1/3 as many places to update |
|
Just found that PR, #3192. And unfortunately, the other merge caused some conflicts. |
howardjohn
force-pushed
the
protoc/gen-alias
branch
from
55c99e9
to
aec227a
Compare
For #3127
Depends on istio/tools#2921.
Currently builds on #3186, so merge that first and I will rebase.
This PR refactors our API strategy. Instead of copies of everything, we just have one single source of truth proto. This has benefits in maintenance in this repo (its really confusing and tedious), and real runtime costs (i.e. we don't need to load multiple MB of copies of protos into our binaries).
There are a few facets of backwards compatibility here. Usage (Via CRD json/yaml, Proto wire, and Go types), and type (first party or third party).
CRD: 馃憤 Absolutely zero changes here.
Go types: 馃憤 Istio currently uses only the oldest types, so it doesn't really matter for first party. However, we will want to use newer ones in the future probably, and we will not want to have to change the versioning here. Third party are already using new versions.
This PR maintains effectively full compatibility with the Go API. While its theoretically possible you are doing crazy reflection stuff that now changes behavior, its not an expected use case. Both the kubernetes client-go and protobuf-go fully handle this change without issues, as does just normal use (e.g. creating a struct). Due to the aliasing, users do not need to change imports at all. Note that Gateway API did the same thing and it was fine.
Protobuf types: 馃敶 this is technically incompatible. Today we have
.protofiles that are checked into our repo, and a user could theoretically take those and send them on the wire. However:Based on this, I believe this is an acceptably small breakage. If a user is somehow impacted, they can take our scripts to replicate the protos and/or move to the canonical version.
This PR has been manually tested in istio.io/istio to ensure all tests pass.