-
Notifications
You must be signed in to change notification settings - Fork 571
Allow defining CRDs from a single version #3186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I like that you focused on removing the CRD generation from each version for now.
Once this is merged, do we want to wait on anything in particular before we remove the repeated protos and the old way of generation?
Also does it make sense to keep a central source of each API and all its API version? And maybe even template the versions value in the protos based on the central source of APIs and their versions. |
Seems reasonable, or could be the other way and derived from the proto as the source of truth.
I don't think in terms of time. I have it implemented in #3188. I split it out for reviewing sake |
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 |
For my curiosity: why don't we use the client-go ( which I think is generated from the CRD) in the pilot code ? The 'source of truth' is likely the CRD in K8S. +1 on the PR, I can see the tradeoffs and benefits - but I still think all the proto use is too complex and not worth it in this case. |
Maybe I am not parsing this correctly, but I don't think that is how it works. Client-go depends on Go structs which follow a certain shape (they can DeepCopy, have object meta, etc). It will call json.(Un)marshal on these to go from api-server to Go. CRD schema is entirely orthogonal and not used at all on the client side (only internally in api-server). For our types. json.Unmarshal calls protojson.Unmarshal. For K8s core types, they have Go structs that use standard So all of the core of Istio (ignoring MCP), doesn't use protobuf wire format, but does use it for JSON/deep copy/etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I don't see any objections from other reviewers either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM - all of the CRDs are the same anyway, so this better reflects the current state
@@ -129,7 +129,7 @@ option go_package = "istio.io/api/networking/v1alpha3"; | |||
// | |||
// <!-- crd generation tags | |||
// +cue-gen:DestinationRule:groupName:networking.istio.io | |||
// +cue-gen:DestinationRule:version:v1alpha3 | |||
// +cue-gen:DestinationRule:versions:v1beta1,v1alpha3,v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To move v1 as the storage version (maybe in 3 releases or so), do we just change the order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH we might need something more complex, but its fairly straightforward to tweak the tooling if that comes
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.