KEP-5073: Declarative Validation: Explain and update document with cross-field validation information #5290

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.

Sign up for GitHub

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

Jump to bottom

Open

aaron-prindle wants to merge 5 commits into kubernetes:master from aaron-prindle:kep-5073-update-cross-field

+73 −31

Contributor

aaron-prindle commented May 8, 2025

One-line PR description: This addresses how we intend to handle cross-field validations with Declarative Validation.
Issue link: Declarative Validation Of Kubernetes Native Types With validation-gen #5073

k8s-ci-robot added the cncf-cla: yes label

k8s-ci-robot requested review from apelisse and deads2k

May 8, 2025 13:28

k8s-ci-robot added kind/kep sig/api-machinery size/M labels

aaron-prindle force-pushed the kep-5073-update-cross-field branch from b8205b9 to 63eae72 Compare

May 8, 2025 13:48


          KEP-5073: Declarative Validation: Explain and update document with cr…

a65ce7c

…oss-field validation information

aaron-prindle force-pushed the kep-5073-update-cross-field branch from 63eae72 to a65ce7c Compare

May 8, 2025 14:37

Contributor Author

aaron-prindle commented May 8, 2025

/assign @deads2k @jpbetz @thockin
/cc @yongruilin

k8s-ci-robot assigned deads2k, jpbetz and thockin

k8s-ci-robot requested a review from yongruilin

May 8, 2025 15:18

aaron-prindle closed this

aaron-prindle reopened this

Contributor

k8s-ci-robot commented May 8, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aaron-prindle
Once this PR has been reviewed and has the lgtm label, please ask for approval from deads2k. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

keps/sig-api-machinery/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yongruilin reviewed

View reviewed changes

keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md Outdated

Comment on lines 1052 to 1057

+              #### Cross-Field Validation and Ratcheting
+              For cross-field validations, the validation logic is evaluated at the common ancestor of the fields involved. This approach is necessary for supporting ratcheting. While validation tags (eg: +k8s:maximum=siblingField, +k8s:unionMember , etc.) may be placed on an individual field for clarity, the tag and its associated validation logic will be "hoisted" to the parent struct during code generation. This "hoisting" means the validation is treated as if it were defined on the common ancestor.  By anchoring the cross-field alidation logic at the common ancestor, regardless of tag placement, the ratcheting design can more reliably determine how to perform equality checks across the relevant type nodes and decide if re-validation is necessary.
+              As noted in the Ratcheting section there is an additional challenge that arises if a cross-field validation rule (e.g. X < Y) is defined on a common ancestor struct/field, and an unrelated field (e.g. Z) within that same ancestor is modified. This change to Z makes the common ancestor “changed” overall, triggering re-validation of the X < Y rule. If this rule was recently evolved, it might now fail even if X and Y themselves are not modified by the user’s update. This could violate the principle “Unchanged fields do not cause update rejections”. In practice this means that the validation rules (or validation-gen generally) might have to be more explicit where each validation rule explains “I only care about these fields for ratcheting”.
+              For the initial implementation, this behavior will be documented, and cross-field validation rules must handle ratcheting themselves.  This means that in the initial implementation of the cross-field dedicated tags referenced in the document (+k8s:unionMember, etc.), they will handle ratcheting of the fields they operate on directly.  See the Ratcheting section for more information on this issue as well as longer term plans on addressing this challenge.

Contributor

yongruilin May 8, 2025

Do you think it is more nature that first paragraph is in cross-field section and the 2nd & 3rd paragraphs are in ratcheting section? Maybe you don't need to put a sub-title for that 1st paragraph and then refer to the ratcheting section of how to handle this challenge case?

Contributor Author

aaron-prindle May 8, 2025 •

edited

Loading

+1, I've combined paragraphs 2 & 3 into a single paragraph and de-duped the majority of text from the Ratcheting and Cross-Field Validation section in #5292. This paragraph now explains:

cross-field tags "hoist" all cross-field tags to the parent struct
cross-field tags implement ratcheting for the fields they operate on

jpbetz reviewed

View reviewed changes

Contributor

jpbetz left a comment

nit only, looks good!

keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md Outdated Show resolved Hide resolved

keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md Outdated Show resolved Hide resolved

aaron-prindle and others added 3 commits

May 8, 2025 12:13


          Update keps/sig-api-machinery/5073-declarative-validation-with-valida…

df7c640

…tion-gen/README.md

Co-authored-by: Joe Betz <[email protected]>


          Update keps/sig-api-machinery/5073-declarative-validation-with-valida…

2573e00

…tion-gen/README.md

Co-authored-by: Joe Betz <[email protected]>


          resolving review feedback - update cross-field + ratcheting section

8d62579

aaron-prindle force-pushed the kep-5073-update-cross-field branch from e67cef8 to 8d62579 Compare

May 8, 2025 19:30

deads2k requested changes

View reviewed changes

Contributor

deads2k left a comment

I wasn't expecting to see CEL in the table. I agree with the rest, though particulars around usage of the tags will need to be worked out. Do you want to do it here, on a followup KEP update, on the implementation PR, or somewhere else?

keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md Outdated Show resolved Hide resolved

keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md

    
                 </td>

                 <td>

                  `x-kubernetes-validations`

                  x-kubernetes-validations (CEL: !has(oldSelf...) || self... == oldSelf...)

Contributor

deads2k May 15, 2025

Accidentally left CEL here? If we're trying to avoid it, we don't need it here right?

Contributor Author

aaron-prindle May 15, 2025 •

edited

Loading

This intention of the text there was to show the OpenAPI equivalent for +k8s:immutable which would be using x-kubernetes-validations with a CEL expression similar to that shown. I have kept the text for now but am happy to remove this text. Just wanted to note the current intent which is aligned with previous discussion around CEL in v1.34. This column is to note how some declarative validation rules map-to/could-be-output as OpenAPI rules.

keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md

+                     // +k8s:subfield({"type":"Denied", "status": "true"})=+k8s:unionMember
+                     // +k8s:subfield({"type":"Failed"})=+k8s:optional
+                     // +k8s:subfield({"type":"Failed"})=+k8s:immutable

Contributor

deads2k May 15, 2025

The impl PR will go into the "immutable after create" versus "immutable once set"?

Contributor Author

aaron-prindle May 15, 2025 •

edited

Loading

Yes, the related impl KEP Update will go into the "immutable after create" versus "immutable once set".

keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md Outdated

+              ```go
+              type CertificateSigningRequestStatus struct {
+                     // +k8s:subfield({"type":"Approved"})=+k8s:optional
+                     // +k8s:subfield({"type":"Approved", "status": "true"})=+k8s:unionMember

Contributor

deads2k May 15, 2025

I thought I remembered that unions were named in one of the previews I saw. The detail will be in the impl PR?

Contributor Author

aaron-prindle May 15, 2025 •

edited

Loading

I have updated this to use named unions now (the +k8s:unionMember{"union": "unionName"}).

For simple cases where there is only one union on a struct, currently there is a default name when no union name value is set (example from dev-branch: https://github.com/jpbetz/validation-gen/blob/main/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/tags/union/undiscriminated/simple/doc.go#L33)

Contributor

deads2k commented May 15, 2025

oh, and a note that breaking on sentences makes it easier to comment on the right lines.


          addressing review comments

40bfe3a

aaron-prindle force-pushed the kep-5073-update-cross-field branch from f63f0af to 40bfe3a Compare

May 15, 2025 21:09

k8s-ci-robot added size/L and removed size/M labels

Contributor Author

aaron-prindle commented May 15, 2025

oh, and a note that breaking on sentences makes it easier to comment on the right lines.

Noted, I've updated the KEP Update changes here to break on sentences now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes kind/kep sig/api-machinery size/L