From 32e084d46751acd6e2969617a92cb089e3750700 Mon Sep 17 00:00:00 2001 From: Aaron Prindle Date: Wed, 29 Jan 2025 19:17:54 +0000 Subject: [PATCH] resolving KEP comments round #3 --- .../README.md | 130 ++++++++++-------- 1 file changed, 69 insertions(+), 61 deletions(-) diff --git a/keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md b/keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md index b191e059fa8f..badb16b6f54b 100644 --- a/keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md +++ b/keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md @@ -42,9 +42,9 @@ - [validation-gen Implementation Plan](#validation-gen-implementation-plan) - [Catalog of Supported Validation Rules & Associated IDL Tags](#catalog-of-supported-validation-rules--associated-idl-tags) - [Supporting Declarative Validation IDL tags On Shared Struct Fields](#supporting-declarative-validation-idl-tags-on-shared-struct-fields) - - [validation-gen One-deep typedef support](#validation-gen-one-deep-typedef-support) - - [Proposed Options](#proposed-options) - [subfield IDL Tag](#subfield-idl-tag) + - [validation-gen One-deep typedef support](#validation-gen-one-deep-typedef-support) + - [Proposed Solutions](#proposed-solutions) - [Migration Plan](#migration-plan) - [Phase1: Initialization (Responsibility of Contributors Implementing the KEP)](#phase1-initialization-responsibility-of-contributors-implementing-the-kep) - [Phase2: Scaling the Migration (Responsibility of Contributors Implementing the KEP and broader community)](#phase2-scaling-the-migration-responsibility-of-contributors-implementing-the-kep-and-broader-community) @@ -52,7 +52,7 @@ - [Tagging and Validating Against Internal vs Versioned Types](#tagging-and-validating-against-internal-vs-versioned-types) - [Handling Zero Values in Declarative Validation](#handling-zero-values-in-declarative-validation) - [Difficulties with +required and +default](#difficulties-with-required-and-default) - - [Proposed Solutions](#proposed-solutions) + - [Proposed Solutions](#proposed-solutions-1) - [Addressing the Problem with Valid Zero Values Using the Linter](#addressing-the-problem-with-valid-zero-values-using-the-linter) - [Ratcheting](#ratcheting) - [Test Plan](#test-plan) @@ -145,7 +145,7 @@ Declarative validation will also benefit Kubernetes users: * Adding new fields and associated validation rules becomes a simpler process of adding IDL tags to the field definition, rather than writing and maintaining validation functions. This reduces potential inconsistencies and bugs. Testing is also streamlined as centralized rules and frameworks enable the use of test fixtures across k8s types making creating tests simpler and faster to implement for contributors. * Creating k8s APIs becomes faster as developers can focus on the API’s structure and behavior, leaving the validation boilerplate to be generated automatically. This speeds up development and encourages experimentation with new APIs. -* Validation is performed on versioned types (assuming recommended approach is from design below), so error message and attributed field path is more likely to be relevant to the user. (Though no known cases of misattributed field errors have yet occurred) +* Validation is performed on versioned types (assuming recommended approach is used from design below), so error message and attributed field path is more likely to be relevant to the user. (ex: some fields in autoscaling/v2 will be mis-identified with current approach but resolved w/ Declarative Validation + versioned types) Additionally Declarative Validation can be built upon to support features such as: @@ -160,7 +160,7 @@ Please feel free to try out the [prototype](https://github.com/jpbetz/kubernetes * Eliminate 90% of net-new hand-written validation within 5 kube releases (target start: v1.33) * Convert 50% of existing hand-written validation within 5 kube releases (target start: v1.33) -* If we lose steam and abandon this, we can roll it back relatively easily. +* Migrate in such a way that if contributors lose steam and abandon this, we can roll it back relatively easily. * `types.go` files become the de-facto IDL of Kubernetes for native types. It is worth noting that `+enum` support, `+default` support and similar enhancements all moved our API development forward in this direction. This enhancement is an attempt to continue that story arc. * API Validations are readable and manageable. The IDL should be a joy to use for API authors, reviewers and maintainers. Common complex relationships have simple-to-express idioms in the IDL. * Reduce API development costs for API authors and reviewers. Reduce long term maintainer costs. Reclaiming time from core project contributors. @@ -169,7 +169,6 @@ Please feel free to try out the [prototype](https://github.com/jpbetz/kubernetes * Enable development of linters and other API tool chains that use API validation rules and metadata. Further reducing development effort and risk of incorrect validation rules. * Retain native (or nearly native) performance. * Improve testing rigor by being vastly easier to test. -* Migrate in such a way that if contributors lose steam and abandon this, we can roll it back relatively easily. ### Non-Goals @@ -217,14 +216,14 @@ The previous Declarative Validation proposal ([KEP-4153](https://github.com/kube In order to properly support the User Stories for “Kubernetes developer wishes to add a field to an existing API version” and “Kubernetes developer adds a new version an API” it is important that `validation-gen` and the associated tooling for IDL tags have robust UX that immediately notifies users when tags are not used properly. To support this `validation-gen` will have options for validators subject to when they register so that validation authors can express how their associated IDL tag should be used and the framework will error if a user uses an IDL tag incorrectly. See this related WIP PR [here](https://github.com/jpbetz/kubernetes/pull/89) adding such functionality to the prototype for an example of what this might look like. -The goal is that when a user makes a mistake in authoring IDL tags we give a meaning error. Users are not expected to know the underlying system or be an insider on the project to successfully use Declarative Validation. +The goal is that when a user makes a mistake in authoring IDL tags we give a meaningful error. Users are not expected to know the underlying system or be an insider on the project to successfully use Declarative Validation. ### Introduce new validation tests and test framework New validation tests will be created as part of the migration process for contributors migrating fields using `validation-gen`. Additionally, a test framework and associated migration test utilities will be created as part of `validation-gen` to leverage the new centralized validation rules and to ensure validation consistency across hand-written and declarative validation rules. See the [Test Plan](?tab=t.0#bookmark=id.a86sekfgbuen) section for more details. #### New Validations Vs Migrating Validations -`validation-gen` +FIXME... #### New Validation Tests As part of the process for migrating fields, contributors will scrutinize and improve as needed the current validation tests. No field can go thru migration without a robust test for the field being migrated, which proves that it is validated correctly before the change and after. Many existing tests are not sufficient to verify 100% equivalency and need retooling. This allows us to de-risk migration problems by scrutinizing the current tests and enhancing them. @@ -501,10 +500,10 @@ The below rules are currently implemented or are very similar to an existing val string format - +k8s:format={format name} + `+k8s:format={format name}` - format + `format` @@ -512,10 +511,10 @@ The below rules are currently implemented or are very similar to an existing val size limits - +k8s:min{Length,Items}, +k8s:max{Length,Items} + `+k8s:min{Length,Items}`, `+k8s:max{Length,Items}` - min{Length,Items}, max{Length,Items} + `min{Length,Items}`, `max{Length,Items}` @@ -523,10 +522,10 @@ The below rules are currently implemented or are very similar to an existing val numeric limits - +k8s:minimum, +k8s:maximum, +k8s:exclusiveMinimum, +k8s:exclusiveMaximum + `+k8s:minimum`, `+k8s:maximum`, `+k8s:exclusiveMinimum`, `+k8s:exclusiveMaximum` - minimum, maximum, exclusiveMinimum, exclusiveMaximum + `minimum`, `maximum`, `exclusiveMinimum`, `exclusiveMaximum` @@ -534,13 +533,12 @@ The below rules are currently implemented or are very similar to an existing val required fields - +k8s:optional + `+k8s:optional`

- - +k8s:required + `+k8s:required` - required + `required` @@ -548,10 +546,10 @@ The below rules are currently implemented or are very similar to an existing val enum values - +k8s:enum + `+k8s:enum` - enum + `enum` @@ -559,11 +557,11 @@ The below rules are currently implemented or are very similar to an existing val Union values - +k8s:unionMember \ -+k8s:unionDiscriminator + `+k8s:unionMember` \ +`+k8s:unionDiscriminator` - oneOf,anyOf,allOf + `oneOf,anyOf,allOf` @@ -571,9 +569,9 @@ The below rules are currently implemented or are very similar to an existing val forbidden values - +k8s:forbidden + `+k8s:forbidden` - + @@ -581,7 +579,7 @@ The below rules are currently implemented or are very similar to an existing val feature gate is enabled - +k8s:ifOptionEnabled(FeatureX)=<if-enabled-validator-tag> + `+k8s:ifOptionEnabled(FeatureX)=<if-enabled-validator-tag>` N/A @@ -592,7 +590,7 @@ The below rules are currently implemented or are very similar to an existing val feature gate is disabled - +k8s:ifOptionDisabled(FeatureX)=<if-disabled-validator-tag> + `+k8s:ifOptionDisabled(FeatureX)=<if-disabled-validator-tag>` N/A @@ -603,7 +601,7 @@ The below rules are currently implemented or are very similar to an existing val validate each key - +k8s:eachKey=<eachKey-validator-tag> + `+k8s:eachKey=<eachKey-validator-tag>` N/A @@ -614,7 +612,7 @@ The below rules are currently implemented or are very similar to an existing val validate each value - +k8s:eachVal=<eachVal-validator-tag> + `+k8s:eachVal=<eachVal-validator-tag>` N/A @@ -625,10 +623,10 @@ The below rules are currently implemented or are very similar to an existing val uniqueness - +k8s:listType=<type> + `+k8s:listType=<type>` - x-kubernetes-list-type + `x-kubernetes-list-type` @@ -636,7 +634,7 @@ The below rules are currently implemented or are very similar to an existing val shared struct fields (subfield) - +k8s:subfield(subField-json-name)=<subfield-validator-tag> + `+k8s:subfield(subField-json-name)=<subfield-validator-tag>` N/A @@ -663,10 +661,10 @@ The below rules are not currently implemented in the [validation-gen prototype]( regex matches - +k8s:pattern + `+k8s:pattern` - pattern + `pattern` @@ -674,10 +672,10 @@ The below rules are not currently implemented in the [validation-gen prototype]( cross field validation - TBD + `TBD - x-kubernetes-validations + `x-kubernetes-validations` @@ -685,10 +683,10 @@ The below rules are not currently implemented in the [validation-gen prototype]( transition rules - TBD + `TBD - x-kubernetes-validations + `x-kubernetes-validations` @@ -722,6 +720,24 @@ type Bar struct { Shared types present a challenge. For example, different Kubernetes resources have different validation rules for `metadata.name` and `metadata.generateName`. But all resources share the `ObjectMeta` type. +#### `subfield` IDL Tag + +To handle this case, we provide an IDL tag - `k8s:subfield(<field-json-name>)` which can be used to specify a `subfield` validation to add to parent which validates against the the `subfield` value: + +```go +type Struct struct { + // +k8s:subfield(name)=+k8s:format=dns-label + metav1.ObjectMeta +} +``` + +This will also support chaining of subfield calls with other validators (including subfield) which allows for setting subfield validations on arbitrarily deep nested fields of shared structs. An exaggerated example showcasing this is below: \ + +```go +// +k8s:subfield(sliceField)=+k8s:eachVal=+k8s:subfield(stringField)=+k8s: +``` + + #### `validation-gen` One-deep typedef support <<[UNRESOLVED is it ok that `validation-gen`'s currently design only supports one-deep typedef support? There are potential solutions to extend this, see below]>> @@ -747,7 +763,7 @@ type Struct struct { In the above example, FooField would be validated as a DNS label and have at least 4 characters which is expected. What might also be expected though is that BarField would be validated as a DNS label, have at least 4 characters, and have no more than 16 characters. INCORRECT! Due to Go's type system, the relationship of type Foo -> string is represented, but type Bar -> type Foo -> string is flattened to type Bar -> string. This leads to a currently open question around the severity of this potential UX issue as well potential solutions on how this could be mitigated if needed. -##### Proposed Options +##### Proposed Solutions 1. **Have it well documented that `validation-gen`only support one-deep typedef: * Pro: Doesn't require any additional architectural changes to `validation-gen`. @@ -763,22 +779,7 @@ In the above example, FooField would be validated as a DNS label and have at lea * Pro: Better UX for users as they are notified not to use IDL tags that might lead to unintended outcomes when adding IDL tags * Con: Requires implementing the chain of typedefs logic. -#### `subfield` IDL Tag - -To handle this case, we provide an IDL tag - `k8s:subfield(<field-json-name>)` which can be used to specify a `subfield` validation to add to parent which validates against the the `subfield` value: - -```go -type Struct struct { - // +k8s:subfield(name)=+k8s:format=dns-label - metav1.ObjectMeta -} -``` - -This will also support chaining of subfield calls with other validators (including subfield) which allows for setting subfield validations on arbitrarily deep nested fields of shared structs. An exaggerated example showcasing this is below: \ - -```go -// +k8s:subfield(sliceField)=+k8s:eachVal=+k8s:subfield(stringField)=+k8s: -``` +<<[/UNRESOLVED]>> ### Migration Plan @@ -885,20 +886,23 @@ The straightforward approach of using the `+required` tag to enforce the presenc #### Proposed Solutions -1. **Tri-State mutually exclusive options (Recommended): +optional, +default, +required: - * Treat +optional, +default, and +required as mutually exclusive options. - * Fields that allow valid zero values and have defaults would be explicitly tagged with neither +optional nor +required. +1. Tri-State mutually exclusive options (Recommended): `+optional`, `+default`, `+required`: + * Treat `+optional`, `+default`, and `+required` as mutually exclusive options. + * Fields that allow valid zero values and have defaults would be explicitly tagged with neither `+optional` nor `+required`. * Validation logic would need to be aware of this and handle zero values appropriately for such fields. * Drawback: This approach requires a linter to enforce the tri-state rule and prevent invalid combinations. * Benefit: Simplifies the mental model by making the relationship between optionality, defaults, and requiredness explicit. -2. optional-default: zero-allowed Tag: +2. `optional-default: zero-allowed` Tag: * A new tag could be introduced to signify that a zero value is permissible, even with a default. * Drawback: Adds complexity by introducing another tag and complicates the mental model. 3. Compile-Time or Runtime Default Value Check: - * Compile-Time Check: During code generation, the +default tag could be parsed, and if it refers to a zero value, validation logic could be adjusted accordingly. + * Compile-Time Check: During code generation, the `+default` tag could be parsed, and if it refers to a zero value, validation logic could be adjusted accordingly. * Drawback: Complex implementation, requires more information to be available during code generation. * Runtime Check: Validation logic could check if the provided default value is a zero value and skip certain checks. * Drawback: Considered overly-complicated ("gross") and potentially impacts performance. + * Benefit: Closest to correct. +4. Make `+optional` on non-pointer fields be advisory: + * If we find an optional string field, the optional tag can be used as documentation, but the actual validation will rely on the format-checking (e.g. dns-label). To an end user this means that what used to be a "field is required" error now becomes a "not a dns-label" error. Only slightly worse. #### Addressing the Problem with Valid Zero Values Using the Linter @@ -1019,12 +1023,14 @@ func TestVersionedValidationByFuzzing(t *testing.T) { ``` ###### strategy_test.go vs validation_test.go +<<[UNRESOLVED should we move current validation_test.go logic to strategy_test.go as it makes more sense generally and aids this project? ]>> + Currently, validation logic and associated tests are logically split across `validation.go` and `strategy.go`. The hand-written validation functions and associated tests reside in `validation.go` and `validation_test.go` while `strategy.go` determines when to invoke these functions during API object creation and updates. With the introduction of declarative validation (controlled by the `DeclarativeValidation` feature gate) the current logic split is worth considering moving from `validation_test.go` to `strategy_test.go` as the current logic split in the test is unfavorable for the migration as it requires additional plumbing work. The current approach in the prototype experiment to migrate ReplicationController involves directly injecting declarative validation and equivalency tests into `validation_test.go`. This is achieved by conditionally appending calls to `rest.ValidateDeclaratively` within the existing test cases, based on the `DeclarativeValidation` feature gate's status. This allows for a direct comparison of the outputs between hand-written and declarative validation within the same test framework. -**Moving the feature gate check and declarative validation logic to strategy_test.go could offer several advantages: +**Moving the feature gate check and declarative validation logic to `strategy_test.go` could offer several advantages: * **Reduced Test Duplication:** `validation_test.go` could be simplified, as it would no longer need to handle both hand-written and declarative validation paths. * **Clearer Separation of Concerns:** `strategy.go` would be responsible for determining _when_ to validate, while `validation.go` would handle _how_ to validate. @@ -1036,6 +1042,8 @@ However, this approach also has potential drawbacks: Given the low complexity of moving this code prior to the changes, the enhanced logic split of moving the code, and the reduced work for the migration that moving this code would have currently the plan for Declarative Validation is that the current `validation_test.go` tests are moved to `strategy_test.go` in a PR prior to the migration PR. +<<[/UNRESOLVED]>> + ###### Error Message Equivalence Some error messages will be phrased differently but preserve the same semantic meaning when converted to the declarative validation system. For our testing to check if errors are changed we have two options: