Repository navigation
Fix conversion gen cross package conversions - #143367
yashaggarwal85d wants to merge 2 commits into
Conversation
When a type refers to a struct from another API group, conversion-gen only looked for hand-written Convert_* functions. The functions it generates into the other package live in zz_generated files, which are not parsed, so it emitted compileErrorOnMissingConversion() and authors had to add manual wrappers. Those wrappers count as manual conversions, which stops the wrapped type, and every type that contains it, from using memory-identical unsafe conversions. Record the public conversion functions that will be generated into each input package and call them when a conversion between types from another package is needed. They are not treated as manual conversions, so they do not block memory-identical conversions.
conversion-gen now finds the conversion functions it generates for admission/v1 AdmissionRequest and authorization/v1 ConditionsAwareDecision, so drop the hand-written wrappers and regenerate. SubjectAccessReview, SelfSubjectAccessReview and LocalSubjectAccessReview are memory-identical again, so remove them from the TestMemoryIdenticalConversion exemptions.
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Welcome @yashaggarwal85d! |
|
Hi @yashaggarwal85d. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yashaggarwal85d The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
/sig api-machinery
What this PR does / why we need it:
When a type references a struct from another API group (for example
authorization/v1alpha1.AuthorizationConditionsRequest->admission/v1.AdmissionRequest, orAuthorizationConditionsResponse->authorization/v1.ConditionsAwareDecision), conversion-gen only looked for hand-writtenConvert_*functions. The functions it generates itself into the other package live inzz_generated.conversion.go, which is not parsed, so it emittedcompileErrorOnMissingConversion(). The workaround was a hand-written wrapper that just callsautoConvert_*. Those wrappers count as manual conversions, so every type that contains the wrapped type lost its memory-identical (unsafe cast) conversion.This PR:
Convert_X_To_Yfunctions that will be generated into each input package (types with a peer, convertible, and without a manual conversion). When a conversion is needed between types from another package, conversion-gen calls that function instead of emittingcompileErrorOnMissingConversion(). These functions are kept separate from the manual conversions, so they don't block memory-identical conversions and aren't registered withAddConversionFunc.pkg/apis/admission/v1/conversion.goandpkg/apis/authorization/v1/conversion.goand regenerates.ConditionsAwareDecision,NamedConditionsAwareDecision,SubjectAccessReviewStatusandAuthorizationConditionsResponsenow convert with an unsafe cast, andSubjectAccessReview,SelfSubjectAccessReviewandLocalSubjectAccessRevieware removed from theTestMemoryIdenticalConversionexemptions (part of Conditional Authz [3/n]: Make the webhook authorizer support conditions #137633, as requested in the issue).AuthorizationConditionsReviewstays exempt becauseadmission.AdmissionRequesthasruntime.Objectfields.Which issue(s) this PR is related to:
Fixes #142345
Special notes for your reviewer:
hack/update-codegen.shruns it (all tagged packages in one invocation). Packages outside the run behave as before.Convert_*function instead ofcompileErrorOnMissingConversion(). Both fail to compile, and adding the manual function fixes both.hack/verify-codegen.sh,hack/verify-golangci-lint.sh,hack/verify-gofmt.sh,hack/update-codegen.sh conversionsandhack/update-codegen.sh subprojects(no diff in the subprojects or code-generator examples),go test ./staging/src/k8s.io/code-generator/cmd/conversion-gen/...(newTestCrossPackageGeneratedConversions),go test ./pkg/api/testing/...(includingTestMemoryIdenticalConversionand the round-trip tests), andgo test ./pkg/apis/authorization/... ./pkg/apis/admission/... ./pkg/registry/authorization/....Does this PR introduce a user-facing change?
AI usage disclosure:
YES. This PR was written in part with the assistance of generative AI (exploring the generator code, drafting the change and the unit test). The solution was reviewed and tested by me (Yash), and I am responsible for the submitted changes.