Convert one field to declarative, e2e #61

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

Merged

jpbetz merged 24 commits into jpbetz:validation-gen from thockin:validation-gen-plus-thockin-convert-one-field-e2e

Mar 6, 2025

Collaborator

thockin commented Dec 1, 2024 •

edited

Loading

This PR is WIP and should maybe be a few PRs. It should be reviewed commit-by-commit.

Goal: disable all generation for "real" APIs and prove that a single field of a single struct can be handled. Hopefully lay out a roadmap for future such conversion.

I looked for a field that could be handled by the existing tags, whose validation was trivial (no dependent fields, no other places checking the value, etc). I couldn't find one in core! Even simple enums had complicated validation. So I added +k8s:minimum.

Second: Add declarative validation for ReplicationController.Spec.Replicas which does satisfy the above "trivial" criteria.

Third: Document the testing to prove that this change is "safe". It proved that the test itself was crap, so I had to fix that first.

Fourth: Gate the manual validation of this field.

Fifth: Prove that the union of manual and declarative validation passes tests.

Sixth: Add tests that ensure the equivalence of versioned validation for manual and fuzzed inputs.

STATUS: As of now, the tests pass.

We need to decide if we will keep manually-written tests against generated validation in the long term. We don't, for example, test generated conversions, etc. In the short term, we obviously need to. Perhaps validation testing should move to test against strategy?

We need to figure out how we will do small, obvious conversions when things like enum and union often have complicated manual validation.

We need to figure out the relationship with defaulting, so that generated validation for +k8s:optional is different than +k8s:optional" + +defaultand when+k8s:required` is actually needed.

thockin force-pushed the validation-gen-plus-thockin-convert-one-field-e2e branch 8 times, most recently from ed13360 to 7dd1318 Compare

December 6, 2024 20:47

jpbetz reviewed

View reviewed changes

staging/src/k8s.io/code-generator/cmd/validation-gen/validators/openapi.go Outdated Show resolved Hide resolved

Owner

jpbetz commented Dec 7, 2024 •

edited

Loading

STATUS: As of now, the test fails. We need a way to prove that the "complete" validation, manual and generated, catches this change. @jpbetz I thought you had added some way to call both? I can't find it.

Looks like it accidentally got squashed out of the history. This will add it back: #67

EDIT: The pkg/apis/core/validation/validation_test.go file in c77c9ec has some examples usages

yongruilin reviewed

View reviewed changes

staging/src/k8s.io/code-generator/cmd/validation-gen/validators/openapi.go Outdated Show resolved Hide resolved

staging/src/k8s.io/apimachinery/pkg/api/validate/schema_test.go Outdated Show resolved Hide resolved

thockin force-pushed the validation-gen-plus-thockin-convert-one-field-e2e branch from 20de23e to 68d2246 Compare

December 13, 2024 01:27

Collaborator

aaron-prindle commented Dec 13, 2024

As discussed in the WG meeting, I have reviewed the commits here related to adding the k8s: tag prefix to validators:

Convert all refs and tests to k8s:tag style164e12f
WIP: Change validators to use k8s:<name> tagsb920152

LGTM for the commits there w/ one nit related to openapi.go - b920152#r1884495162 now that kubernetes/kube-openapi#519 has merged

If these commits are split out into a separate PR and merged inner + ObjectMeta.Name validations will work 🎊

thockin force-pushed the validation-gen-plus-thockin-convert-one-field-e2e branch 4 times, most recently from acd4df3 to 74bdacb Compare

December 17, 2024 20:24

Collaborator Author

thockin commented Dec 17, 2024

Tests added as PoC .

jpbetz force-pushed the validation-gen branch from a114b09 to 00237e8 Compare

December 17, 2024 23:10

thockin force-pushed the validation-gen-plus-thockin-convert-one-field-e2e branch 3 times, most recently from 8fb62b1 to 341189e Compare

December 18, 2024 00:13

jpbetz reviewed

View reviewed changes

Owner

jpbetz left a comment

Amazing seeing this all come together end-to-end. Most of my comments are about code organization.

staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go Outdated Show resolved Hide resolved

pkg/apis/core/validation/validation_test.go Outdated Show resolved Hide resolved

pkg/apis/core/validation/validation_test.go

Comment on lines +16975 to +16989

+              	if old == nil {
+              		runtimetest.RunValidationForEachVersion(t, legacyscheme.Scheme, sets.Set[string]{}, obj, accumulate)
+              	} else {
+              		runtimetest.RunUpdateValidationForEachVersion(t, legacyscheme.Scheme, sets.Set[string]{}, obj, old, accumulate)
+              	}

Owner

jpbetz Dec 18, 2024

I've favored having a separate validation and update-validation functions for consistency with how strategy is defined, but it has led to an increase of functions in the scheme, and in these test utilities. Should we reconsider this? The alternative would be to switch to a conversion where a nil oldObject always indicates create validation, and a non-nil oldObject indicates update validation.

Collaborator Author

thockin Dec 19, 2024

If it cuts down on duplication, I think the semantic is plenty obvious. But I am very close to it...

pkg/apis/core/validation/validation_test.go

+              		all[gv] = errs
+              	}
+              	if old == nil {
+              		runtimetest.RunValidationForEachVersion(t, legacyscheme.Scheme, sets.Set[string]{}, obj, accumulate)

Owner

jpbetz Dec 18, 2024

To better support the pattern being established in this PR, I'm okay with replacing RunValidationForEachVersion with a function that returns a map[string]field.ErrorList{} directly.

Collaborator Author

thockin Dec 19, 2024

I think this is OK as it is - it's not terrible, and it is contained in this one func. I will go witth whatever you think gets past deads2k :)

pkg/apis/core/validation/validation_test.go

+              // FIXME: move somewhere generic - pkg/api/testing?
+              func TestVersionedValidationByFuzzing(t *testing.T) {
+              	for i := 0; i < *roundtrip.FuzzIters; i++ {
+              		gv := schema.GroupVersion{Group: "", Version: "v1"}

Owner

jpbetz Dec 18, 2024

Should gv be an arg? (or should a scheme be passed in and all versions of a group be tested?)

Collaborator Author

thockin Dec 19, 2024

If we move this to a more generic place (where?) then yes. As it is, it is embedded in the "core" group.

jpbetz reviewed

View reviewed changes

pkg/apis/core/validation/validation_test.go Show resolved Hide resolved

thockin force-pushed the validation-gen-plus-thockin-convert-one-field-e2e branch 5 times, most recently from eae5842 to 49b7015 Compare

December 23, 2024 06:50

Collaborator Author

thockin commented Dec 23, 2024

Pushed now with declarative defaults (and some open questions) and declarative optional. This still only handles ONE FIELD.

thockin added 5 commits

March 4, 2025 15:25


          WIP: optional+default == required

de2b206

All of our tags are expressed from the perspective of a client of the
API, but the code we generate is for the server. Optional is tricky.

A field which is marked as optional and does not have a default value is
strictly optional. A client is allowed to not set it and the server will
not give it a default value. Code which consumes it must handle that it
might not have any value at all.

A field which is marked as optional but has a default value is optional
to clients, but required to the server. A client is allowed to not set
it but the server will give it a default value. Code which consumes it
can assume that it always has a value.


          Declaratively validate RC...Replicas optionality

3e9c99e

The existing test run both declarative and manual validation and it
still passes.


          Use declarative default for RC...MinReadySeconds

5bdc0da


          Use declarative validation for minval of RC...MinReadySeconds

16a9309


          Add comments to FunctionGen

6ccb3b7

Now we can emit comments which stick to functions instead of coming
before or after the functions when emitting code.

For followup: I think we can simplify FunctionGen and ValidationGen

thockin force-pushed the validation-gen-plus-thockin-convert-one-field-e2e branch from 76bfa14 to ee2385b Compare

March 4, 2025 23:35

Collaborator Author

thockin commented Mar 4, 2025

Rebased and updated this PR.

Aside from gates, there are a few open things. 2 commits marked "WIP" need attention.

This converts optional+default to required. It seemed like everyone was OK with that, and I added comments. I don't love that it involves peeking at the +default tag.
This treats non-pointer fields with defaults whose default is discernibly the zero-value as documentation. Even more than the previous, this has to not just peek at the +default tag but parse the value. This is a low-impact as I could get away with. It does not handle =ref(...) syntax (but could) and it does not handle hand-written validation (but could). I assumed that as we convert fields to declarative we can use declarative defaults, too, when possible. If it is insufficient, I think we can broaden it to all value-fields with defaults or expand on the parsing (even share code with defaulter-gen).

Owner

jpbetz commented Mar 5, 2025

This converts optional+default to required. It seemed like everyone was OK with that, and I added comments. I don't love that it involves peeking at the +default tag.

I'm OK with this. The client-side implications are something I'm still digesting. The way I see it, the tags are fine; the tags convey sane semantics for both client and server. There is the wrinkle that if we support calling generated validation from clients, then not-yet-defaulted fields would fail validation... I can live with this limitation, but it's good to be aware of it.

Peeking at +default... this means we must also migrate defaults to declarative as we migrate field validation to declarative so we don't loose required checks. No objection from me. The idea that validation is aware of defaulting +default is something I've always been OK with. Previous systems I've worked on all did this so maybe that's why I'm fine with it.

This treats non-pointer fields with defaults whose default is discernibly the zero-value as documentation. Even more than the previous, this has to not just peek at the +default tag but parse the value. This is a low-impact as I could get away with. It does not handle =ref(...) syntax (but could) and it does not handle hand-written validation (but could). I assumed that as we convert fields to declarative we can use declarative defaults, too, when possible. If it is insufficient, I think we can broaden it to all value-fields with defaults or expand on the parsing (even share code with defaulter-gen).

Just to make sure I'm understanding this- IF a field is non-pointer, and is also tagged with +default=<zero-value>, then we let the zero-value do the defaulting for us and the +default=<zero-value> is allowed, but considered documentation (since there is no need to generate defaulting code to reaffirm the zero-value)? If so that seems fine. ACK the complexity in checking the value in the general case. @yongruilin any linter opportunities here?

jpbetz reviewed

View reviewed changes

pkg/apis/core/v1/zz_generated.validations.go

    
              		}

              		return field.ErrorList{field.InternalError(nil, fmt.Errorf("no validation found for %T, subresources: %v", obj, subresources))}

              	})

              	scheme.AddValidationFunc((*corev1.ReplicationControllerList)(nil), func(ctx context.Context, op operation.Operation, obj, oldObj interface{}, subresources ...string) field.ErrorList {

Owner

jpbetz Mar 4, 2025

This is benign but unnecessary. @aaron-prindle maybe track that we could elide this somehow?

Collaborator Author

thockin Mar 5, 2025

You mean elide the validation for List? I guess I didn't realize it was being generated, but at least it looks correct, LOL.

Yeah, we probably don't need to generate for those. We could extend the package-tag to be a sort expression, e.g. types which have TypeMeta and not ListMeta ? Today we have:

// +k8s:validation-gen=TypeMeta
// +k8s:validation-gen-input=k8s.io/api/core/v1

It could become:

// +k8s:validation-gen=typesWith(TypeMeta)  // AND...
// +k8s:validation-gen=typesWithout(ListMeta)
// +k8s:validation-gen-input=k8s.io/api/core/v1

It's still a little sloppy but probably OK?

Collaborator Author

thockin Mar 5, 2025

This could be good for @aaron-prindle or @yongruilin to get a little deeper into the code-generator itself. Probably P2, though - not REQUIRED for 1.33?

Owner

jpbetz Mar 5, 2025

Agree, not required. Best to track it and fix when we have cycles. Short term- keep pushing on the critical path!

Collaborator Author

thockin commented Mar 5, 2025

Just to make sure I'm understanding this- IF a field is non-pointer, and is also tagged with +default=, then we let the zero-value do the defaulting for us and the +default= is allowed, but considered documentation (since there is no need to generate defaulting code to reaffirm the zero-value)?

Precisely. I wrote a long comment about it:

+       // A field which is marked as optional and does not have a default is
+       // strictly optional. A client is allowed to not set it and the server will
+       // not give it a default value. Code which consumes it must handle that it
+       // might not have any value at all.
+       //
+       // A field which is marked as optional but has a default is optional to
+       // clients, but required to the server. A client is allowed to not set it
+       // but the server will give it a default value. Code which consumes it can
+       // assume that it always has a value.
+       //
+       // One special case must be handled: optional non-pointer fields with
+       // default values. If the default is not the zero value for the type, then
+       // the zero value is used to decide whether to assign the default value,
+       // and so must be out of bounds; we can proceed as above.
+       //
+       // But if the default is the zero value, then the zero value is obviously
+       // valid, and the fact that the field is optional is meaningless - there is
+       // no way to tell the difference between a client not setting it (yielding
+       // the zero value) and a client setting it to the zero value.

jpbetz reviewed

View reviewed changes

staging/src/k8s.io/code-generator/cmd/validation-gen/validators/required.go Outdated Show resolved Hide resolved

staging/src/k8s.io/code-generator/cmd/validation-gen/validators/required.go

    
              	// But if the default is the zero value, then the zero value is obviously

              	// valid, and the fact that the field is optional is meaningless - there is

              	// no way to tell the difference between a client not setting it (yielding

              	// the zero value) and a client setting it to the zero value.

Owner

jpbetz Mar 5, 2025

Thanks for the detailed write up.

thockin added 7 commits

March 4, 2025 16:57


          Handle optional value-types with defaults

82c1f47


          Declaratively validate MinReadySeconds optionality

b1c7343


          Refactor FunctionGen - no interface needed

463038e


          Non-pointer FunctionGen

b795bb3


          Refactor VariableGen - no interface needed

f9fa698


          Non-pointer VariableGen

f4a4132


          WIP:TODO

76f6175

thockin force-pushed the validation-gen-plus-thockin-convert-one-field-e2e branch from ee2385b to 76f6175 Compare

March 5, 2025 00:58

Collaborator Author

thockin commented Mar 5, 2025

Fix deprecated tag call.

Still considering to move the zero-val logic back to gengo.

thockin changed the title ~~DNM: WIP: Convert one field to declarative, e2e~~ Convert one field to declarative, e2e

Collaborator Author

thockin commented Mar 5, 2025

I'm OK to merge this to dev branch if it passes reviews. When we go to k/k I think we want a distinct PR with a subset of the commits here + followups with gate changes, to illustrate how conversion is done.

Or we can sit on this, get the gate and verification changes in, and then rebase this so the eventual k/k PR is easier?

I am fine either way.

Owner

jpbetz commented Mar 5, 2025

LGTM. I'm fine with this merging.

I'm don't foresee any major snags caused by merging this before gates or verification, but if anyone else does, I'll defer to them.

Collaborator

aaron-prindle commented Mar 5, 2025 •

edited

Loading

Currently in this PR the feature gate - DeclarativeValidation is added and implemented

From recent conversatios we mentioned changing the gate logic and names - would updating the logic here to use EnableDeclarativeValidatoin and DisableImperativeDeclarativeOverlap be another PR that we should track or is this planned as part of the PR here?

Collaborator Author

thockin commented Mar 5, 2025

@aaron-prindle I can change the semantics here or we can merge it and you or I can change it along with the verification. Your call.

Collaborator

aaron-prindle commented Mar 5, 2025 •

edited

Loading

@aaron-prindle I can change the semantics here or we can merge it and you or I can change it along with the verification. Your call.

I'm fine with merging as is and then in a follow-up PR changing the semantics 👍

jpbetz merged commit fda305d into jpbetz:validation-gen

1 check passed

jpbetz mentioned this pull request

Declarative validation: Add default + optional handling kubernetes/kubernetes#130706

Merged

thockin deleted the validation-gen-plus-thockin-convert-one-field-e2e branch

April 22, 2025 01:02

jpbetz pushed a commit that referenced this pull request


          Merge pull request #61 from yongruilin/validation-gen_direct-equal

12b4ab3

chore: Refactor validation comparison functions to use DirectEqual

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

Labels

None yet