Skip to content
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

Allow defining CRDs from a single version #3186

Merged
merged 2 commits into from
May 14, 2024

Conversation

howardjohn
Copy link
Member

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.

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.
@howardjohn howardjohn added the release-notes-none Indicates a PR that does not require release notes. label May 9, 2024
@istio-testing
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 9, 2024
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 9, 2024
Copy link
Contributor

@whitneygriffith whitneygriffith left a 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?

@whitneygriffith
Copy link
Contributor

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.

@howardjohn
Copy link
Member Author

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.

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?

I don't think in terms of time. I have it implemented in #3188. I split it out for reviewing sake

@howardjohn howardjohn marked this pull request as ready for review May 13, 2024 16:17
@howardjohn howardjohn requested a review from a team as a code owner May 13, 2024 16:17
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label May 13, 2024
@howardjohn
Copy link
Member Author

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

@costinm
Copy link
Contributor

costinm commented May 14, 2024

For my curiosity: why don't we use the client-go ( which I think is generated from the CRD) in the pilot code ?
If I understand the code correctly, for k8s gateway we use their generated client, and most of k8s ecosystem does the same. If there are problems parsing in client-go - anyone else using that package would be impacted, I can't see how our proto translations help. There is the (uncommon) case where we send the proto over XDS - but I suspect it's a far less used code path and if needed we can convert to proto only when that feature is used, before sending over XDS.

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.

@howardjohn
Copy link
Member Author

For my curiosity: why don't we use the client-go ( which I think is generated from the CRD) in the pilot code ?

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 json.Unmarshal. They also generate protos from the go structs, but don't use it for the json path -- only if you use the protobuf encoding on api-server (not available for CRDs, though. We use this for the core types).

So all of the core of Istio (ignoring MCP), doesn't use protobuf wire format, but does use it for JSON/deep copy/etc.

Copy link
Member

@linsun linsun left a 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.

Copy link
Contributor

@keithmattix keithmattix left a 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
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

Copy link
Member Author

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

@istio-testing istio-testing merged commit 9ed092e into istio:master May 14, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes-none Indicates a PR that does not require release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants