10000 [WIP] [Don't review] Validation gen by lalitc375 · Pull Request #132629 · kubernetes/kubernetes · GitHub
[go: up one dir, main page]

Skip to content

[WIP] [Don't review] Validation gen #132629

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

Open
wants to merge 162 commits into
base: master
Choose a base branch
from

Conversation

lalitc375
Copy link
Contributor

[WIP] [Don't review] Validation gen - Test PR.

yongruilin and others added 30 commits June 4, 2025 16:45
Also fix boilerplate errors
This crashes during codegen with a pretty useless panic(), which doesn't
make sense any more.

```
$ pushd; ./hack/update-codegen.sh valid; pushd
~/src/kubernetes ~/src/kubernetes/staging/src/k8s.io/code-generator/cmd/validation-gen
+++ [0418 23:56:03] validation: 69 targets
panic: alias type should already have been unwrapped

goroutine 1 [running]:
k8s.io/code-generator/cmd/validation-gen/validators.requirednessTagValidator.GetValidations({{0x86cb10?, 0xc0000c0b60?}}, {{0x86ca5a, 0xd}, 0xc01ff9efc0, 0xc01ff9f0a0, 0xc02a3f0cd0, 0xc02a4092c0}, {0x0, 0x0, ...}, ...)
	/home/thockin/src/kubernetes/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/required.go:69 +0x40f
k8s.io/code-generator/cmd/validation-gen/validators.(*registry).ExtractValidations(0xc0a4e0, {{0x86ca5a, 0xd}, 0xc01ff9efc0, 0xc01ff9f0a0, 0xc02a3f0cd0, 0xc02a4092c0}, {0xc0200640e0, 0x2, 0x2})
```

Subsequent commits will clean this area up more.
Before this, pointer fields would fail at codegen.
Prior to this it would look at the .Kind, see "Alias", and generate
calls to (e.g.) `RequiredValue()`.  Now calls `RequiredSlice()`.
I did an audit of how we are handling pointerness and aliases in
validators.  In a word - inconsistent.

I went through each validator and added test cases where they were
missing (previous commits). This commit changes how Context.Type is
defined for typedefs. Previously Context.Type was the potentially
aliased and potentially pointerful type for struct fields, but the
"real" concrete type for typedefs.  Now it is consistent, and validators
need to unalias and unpointer types (which they had to do anyway because
it was inconsistent.
Previous commits focused on checking for pointers and aliases more appropriately.

What emerged was two functions `realType` which gave us the concrete
native type (non-pointer, non-alias) an `unaliasType` which stripped
aliasing but only until it hit pointerness.  They were sort of
inconsistently applied and in a few places I found the composition of
them to be weird.

I want to reframe those as:

1) `func nativeType(t) t` which removes all aliasing but retains pointerness.  So for example:
```
type T1 string
type T2 *T1
type T3 *T2

type Foo struct {
    Name *T3
}
```

Calling `nativeType()` on the `Name` member would yield `***string`.
This is what we want in most places.  We need to retain pointerness
because (e.g.) we DO NOT support pointer to slice, and we want that to
fail, but we DO support pointer to string and we want that to succeed,
but `optional` needs to handle pointers and non-pointers differently.

We filter totally disallowed things early on, but if a pointer to slice
should sneak in somehow, this will refuse to generate bad code (fail in
an obvious way).

2) `func nonPointer(t) t` which removes all pointerness (if present),
but does not look past aliases. We needed to compose these both and call
`nonPointer(nativeType(t))` in a few places. E.g. when applying
`+k8s:format`, we need to know that the field is a string OR pointer to
string.

These two are more composable, I think.  If you agree I can finish that
off this weekend and ping the PR.  If not, would like to hear your
thoughts.  Mostly the goal is put as many defensive checks in place as
possible.  New validators will copy these ones and I want it to be as
clear as possible.
…-view

chore: update +k8s:neq tests and error text based on review comments
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2025
thockin and others added 20 commits July 8, 2025 15:18
We don't care if the values of the members changed, only which members
are set / unset.
…ich_fields_are_set_not_values

Change union ratcheting to only look at membership
The custom IP address validation format used within Kubernetes was
previously named 'k8s-ip-sloppy'. This name was chosen as a temporary
placeholder to reserve the 'k8s-ip' format name for a future, stricter
IP validation implementation.

With the introduction of validation ratcheting, it is now possible to
evolve validation rules for fields without breaking existing stored
objects. This makes the "sloppy" suffix redundant, as the validation
for 'k8s-ip' can be tightened over time as needed.
refactor(validation): Rename k8s-ip-sloppy format to k8s-ip
…heting

No lookup correlated for non-map/set list in ratcheting
This caused some changes in generated code which brought to light the
fact that we are not considering list-metadata which is defined on
typedefs when emitting code for fields which use those typedefs.

I will fix that in a subsequent commit, and these deltas will be
corrected.
fix: check error isValid() before render interface
chore: fix forward on validation versioned test
Subsequent changes are more obvious with this done first.
When we have a typedef to a list, and we define list metadata (e.g.
listType=map) on the typedef, we were not considering that when we emit
uniqueness checks.

Additionally, we did not check that the user didn't specify incompatible
metadata on both the type and the field.

Now we disallow those tags to be in both places, and we look at
type-attached metadata if there is not field-attached metadata.  We
should never have both.
Field validators come last.  I think we can depend on that, and it is
consistent with main discovery (a field's type before the field itself).
The path is sometimes a type path.
Atomic is the default, but if someone specifies it explicitly, we should
consider it for conflicts like set and map.
…efs_and_fields

Fix HEAD, fix uniqueness checks for typedefs
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lalitc375
Once this PR has been reviewed and has the lgtm label, please assign palnabarun, smarterclayton for approval. 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:

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

@k8s-ci-robot
Copy link
Contributor
k8s-ci-robot commented Jul 11, 2025

@lalitc375: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-conformance-image-test 185fc4b link false /test pull-kubernetes-conformance-image-test
pull-kubernetes-e2e-gce-cos-alpha-features 185fc4b link false /test pull-kubernetes-e2e-gce-cos-alpha-features
pull-kubernetes-e2e-gci-gce-ingress 185fc4b link false /test pull-kubernetes-e2e-gci-gce-ingress
pull-kubernetes-e2e-kind-nftables 185fc4b link false /test pull-kubernetes-e2e-kind-nftables
pull-kubernetes-e2e-gce-storage-slow 185fc4b link false /test pull-kubernetes-e2e-gce-storage-slow
pull-kubernetes-e2e-gce-network-policies 185fc4b link false /test pull-kubernetes-e2e-gce-network-policies
pull-kubernetes-e2e-gce-storage-snapshot 185fc4b link false /test pull-kubernetes-e2e-gce-storage-snapshot
pull-kubernetes-e2e-gce-csi-serial 185fc4b link false /test pull-kubernetes-e2e-gce-csi-serial
pull-kubernetes-e2e-kind-alpha-beta-features 185fc4b link false /test pull-kubernetes-e2e-kind-alpha-beta-features
pull-kubernetes-apidiff-client-go 185fc4b link false /test pull-kubernetes-apidiff-client-go
pull-kubernetes-unit-windows-master 185fc4b link false /test pull-kubernetes-unit-windows-master
pull-kubernetes-e2e-capz-windows-master 185fc4b link false /test pull-kubernetes-e2e-capz-windows-master
pull-kubernetes-e2e-storage-kind-disruptive 185fc4b link false /test pull-kubernetes-e2e-storage-kind-disruptive

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions 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. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions 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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/kube-proxy area/kubectl area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. wg/device-management Categorizes an issue or PR as relevant to WG Device Management.
Projects
Archived in project
Status: Needs Triage
Status: Needs Triage
Status: Needs Triage
Status: Archive-it
Development

Successfully merging this pull request may close these issues.

6 participants
0