KEP-3937: Declarative Validation #3938

jpbetz · 2023-04-03T20:54:40Z

One-line PR description: Declaratively validate Kubernetes built-in types and publish the validation rules in OpenAPI.

Issue link: Declarative Validation #3937

Other comments: I realize this is ambitious, but recent work in CEL and OpenAPI in recent releases has made this significantly more tractable. Please see the working prototype.

k8s-ci-robot · 2023-04-03T20:54:42Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot · 2023-04-03T20:54:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~keps/sig-api-machinery/OWNERS~~ [jpbetz]

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

apelisse

Thanks for the doc, excited to see that happen!

I have a few questions that are mostly all rooting from the same place:

Where is this going to happen in the apiserver, before decoding, after decoding?
Will this use the openapi as the source of truth? Or will the code be generated like we do for defaults?
Will you decode into unstructured and then do the validation and then decode into built-in type?
Or will you run against decoded values? (i.e. null string becomes empty string for non-nil strings).
There may be a few more questions depending on some answers :-)

keps/sig-api-machinery/3937-declarative-validation/README.md

apelisse · 2023-04-04T16:44:36Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+[prototype](https://github.com/jpbetz/kubernetes/blob/cel-for-native/staging/src/k8s.io/sample-apiserver/pkg/apis/wardle/validation/README.md)
+to get hands on experience with this proposed enhancement.
+
+### Goals


I would have expected in the goals section to know if you're going to publish in v2 and/or v3. I'd love to know how you come-up with that decision.

I could use some openapi expertise on this one, should we support both? How long do we expect to keep v2 around?

IMO, v2 is deprecated, I wouldn't care so much about it. We don't plan on using it for any of the new usage. explain uses v3, kube-verify uses v3, everything should use v3.

apelisse · 2023-04-04T16:52:37Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+| string format          | `+format={format name}`                              | `format`                                                                           |
+| size limits            | `+min`, `+max`                                       | `minLength`, `minProperties`, `minItems`, `maxLength`, `maxProperties`, `maxItems` |
+| numeric limits         | `+min`, `+max`, `+min:exclusive`, `+max:exclusive`   | `minimum`, `maximum`, `exclusiveMinimum`, `exclusiveMaximum`                       |
+| required fields        | `+optional`                                          | `required`                                                                         |


Maybe mention in this table those that already exist.

apelisse · 2023-04-04T17:01:46Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+Declarative validation will be performed against versioned APIs. This differs from the hand
+written validation, which is evaluated against internal types.
+
+Go IDL tags will be added to support the following declarative validation rules:


is the list exhaustive?

That is the intent, yes. Let me know if I missed anything.

I did check, I think it has all of it, thanks.

keps/sig-api-machinery/3937-declarative-validation/README.md

apelisse · 2023-04-04T17:17:06Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+- When both `DeclarativeValidationEnabled` and `LegacyValidationEnabled` are
+  enabled, any difference in the validation errors caught by the two validators
+  will be logged in a special way making them easy to find and analyze, but only
+  the validation errors caught by legacy validation will be reported to the
+  client.


Can we do that without requiring the two separate feature gate?

apelisse · 2023-04-04T17:23:54Z

Also, somewhat related: kubernetes/kubernetes#97733

smarterclayton · 2023-04-04T21:42:48Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+  - Potential causes:
+    - Validation versioned types introduces extra conversions.
+      - Why this might be OK: Requests are received as the versioned type, so it
+        should be feasible to avoid extra conversions.


Would also be a minor security win - currently conversion to internal involves use of unsafe operations for performance, and doing validation before conversion reduces the risk of malformed data making it to conversion.

apelisse · 2023-04-06T21:15:32Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+```go
+// staging/src/k8s.io/api/core/v1/types.go
+
+// +valdiation=rule:"!self.hostNetwork || self.containers.all(c, c.containerPort.all(cp, cp.hostPort == cp.containerPort))"


Suggested change

// +valdiation=rule:"!self.hostNetwork || self.containers.all(c, c.containerPort.all(cp, cp.hostPort == cp.containerPort))"

// +validation=rule:"!self.hostNetwork || self.containers.all(c, c.containerPort.all(cp, cp.hostPort == cp.containerPort))"

Not a requirement, but I would point/copy the code that this replace to give both an idea of the order of magnitude code that can possibly remove, and how different it is.

apelisse · 2023-04-06T21:17:03Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+}
+```
+
+In this example, both `+valdiation`, `+minimum` and `+maximum` are IDL tags.


Suggested change

In this example, both `+valdiation`, `+minimum` and `+maximum` are IDL tags.

In this example, both `+validation`, `+minimum` and `+maximum` are IDL tags.

😂

apelisse · 2023-04-06T21:18:57Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+[prototype](https://github.com/jpbetz/kubernetes/blob/cel-for-native/staging/src/k8s.io/sample-apiserver/pkg/apis/wardle/validation/README.md)
+to get hands on experience with this proposed enhancement.
+
+### Goals


IMO, v2 is deprecated, I wouldn't care so much about it. We don't plan on using it for any of the new usage. explain uses v3, kube-verify uses v3, everything should use v3.

apelisse · 2023-04-06T21:19:30Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+Declarative validation will be performed against versioned APIs. This differs from the hand
+written validation, which is evaluated against internal types.
+
+Go IDL tags will be added to support the following declarative validation rules:


I did check, I think it has all of it, thanks.

apelisse · 2023-04-06T21:21:11Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+| Type of valiation      | Go IDL tag                                                       | OpenAPI validation field                                                           |
+| ---------------------- | ---------------------------------------------------------------- | ---------------------------------------------------------------------------------- |
+| string format          | `+format={format name}`                                          | `format`                                                                           |
+| size limits            | `+min{Length,Properties,Items}`, `+max{Length,Properties,Items}` | `min{Length,Properties,Items}`, `max{Length,Properties,Items}` |


I think mapping directly between the IDL and the OpenAPI is the least confusing for people familiar with openapi. For people who are not, the difference between these can be hard to get initially, we should definitely have some linting if maxLength is used on non-string, maxProperties is used on non-maps, and maxItems is used on non-lists.

+1. Also, Kubebuilder has set significant precedent here, and Kuberbuilder uses a direct mapping (it's prefixed with "kubebuilder:validations:", but it otherwise direct).

apelisse · 2023-04-06T21:24:48Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+We will need to extend our CEL libraries to make it possible to migrate all the
+validation rules that exist in the Kubernetes API today.
+
+- `isFormat() <bool>` and `validateFormat() <list<string>>` will be added to allow formats to be checked in CEL


Wait, is this a CEL rule or is this something that goes in the format part of the openapi, I'm a little confused? It looks like it's not since you have the formats section right below, but still confused :-)

I'd like to almost all format validation to be done just with OpenAPI.

ObjectMeta.name has a problem. We have a single shared type for ObjectMeta, but different types have different format requirements for the name (and generateName), so we have to cope with that somehow.

I probably need to explain my thinking in the KEP more, but my current thinking is that we could do something like:

struct Pod type { // +objectNameFormat="dns1123subdomain" ObjectMeta `json:...` }

And have this generate openAPI like:

"io.k8s.api.core.v1.Pod": { "properties": { "metadata": { "allOf": [ { "$ref": "#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta" } ], "x-kubernetes-validations": [ {"rule": "!has(self.name) || self.name.isFormat('dsn1123subbdomain')"}, {"rule": "!has(self.generateName) || self.generateName.replace('_$', '_a').isFormat('dsn1123subbdomain')"}, ]

If we could avoid this and instead put "format" directly on name, that would be amazing, but I don't know how.

@alexzielenski had the idea of using a second schema in allOf to validate the name directly (no CEL needed).

That's a neat idea.

Somewhat related to that, if we could validate whether namespace can or can not be present, that'd be nice. Also, I've been asked a few times how to know if a kind is namespaced or not, can we re-use that technic to detect it?

Something like this sounds good to me. Today, CRD resources declare scope, but the OpenAPI does not. A fix that can be applied to both CRDs and native types would be amazing. Of the fields in a CRD that do not appear in OpenAPI (scope, names, conversion, deprecated, deprecationWarning, additionalPrinterColumns), the ones that should probably be in OpenAPI (somehow) are scope, deprecated and deprecationWarning.

apelisse · 2023-04-06T22:42:28Z

keps/sig-api-machinery/3937-declarative-validation/README.md

+        wrappers have already been benchmarked and shown to have low overhead.
+- Risk: Migration introduces breaking change to API validation.
+  - Mitigations: See above migration plan, which includes numerous cross checks
+    to prevent mistakes from slipping through.


Today validation for all the versions are done on internal, we might have discrepencies between versions now (since we'll require N validation instead of 1). We have to be sure that all the validations do the exact same thing.

jpbetz · 2023-08-08T16:47:41Z

/close
We're planning to do something along these lines. @alexzielenski will open a replacement KEP

k8s-ci-robot · 2023-08-08T16:47:47Z

@jpbetz: Closed this PR.

In response to this:

/close
We're planning to do something along these lines. @alexzielenski will open a replacement KEP

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/test-infra repository.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 3, 2023

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Apr 3, 2023

k8s-ci-robot requested review from deads2k and fedebongio April 3, 2023 20:54

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 3, 2023

jpbetz mentioned this pull request Apr 3, 2023

Declarative Validation #3937

Closed

4 tasks

jpbetz force-pushed the declarative-validation branch from 99fa271 to 7b5f810 Compare April 4, 2023 02:32

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 4, 2023

jpbetz force-pushed the declarative-validation branch 6 times, most recently from b1553c6 to 4180c5f Compare April 4, 2023 05:15

KEP-3937: Declarative Validation

160d26a

jpbetz force-pushed the declarative-validation branch from 4180c5f to 160d26a Compare April 4, 2023 05:23

apelisse reviewed Apr 4, 2023

View reviewed changes

smarterclayton reviewed Apr 4, 2023

View reviewed changes

jpbetz force-pushed the declarative-validation branch from 5910a1b to 0e3f9c6 Compare April 5, 2023 00:41

Update README.md

6aa3b5b

jpbetz force-pushed the declarative-validation branch from 0e3f9c6 to 6aa3b5b Compare April 5, 2023 01:01

jpbetz added 2 commits April 5, 2023 14:47

Update README.md

bdc9ed5

Apply feedback

8e805e5

Add notes on request handling

03a4be4

apelisse reviewed Apr 6, 2023

View reviewed changes

jpbetz added 6 commits April 7, 2023 13:59

Clarify performance thoughts

61758fe

rename 'validation' to 'cel', add user stories

feb34a8

Clarify request handling and conversions

0f09e12

Clarify formats details

5ade671

Clarify IP/CIDR lib

84b7189

clarify tags and cel libs

c34da8a

apelisse mentioned this pull request May 4, 2023

Namespace scoping of CRDs is inferred kubernetes-sigs/kubectl-validate#16

Closed

jpbetz mentioned this pull request Jun 6, 2023

Lenient validation and merging for non-strict listType=map kubernetes-sigs/structured-merge-diff#234

Closed

k8s-ci-robot closed this Aug 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-3937: Declarative Validation #3938

KEP-3937: Declarative Validation #3938

jpbetz commented Apr 3, 2023

k8s-ci-robot commented Apr 3, 2023

k8s-ci-robot commented Apr 3, 2023

apelisse left a comment

apelisse Apr 4, 2023

jpbetz Apr 5, 2023

apelisse Apr 6, 2023

apelisse Apr 4, 2023

apelisse Apr 4, 2023

jpbetz Apr 5, 2023

apelisse Apr 6, 2023

apelisse Apr 4, 2023

apelisse commented Apr 4, 2023

smarterclayton Apr 4, 2023

apelisse Apr 6, 2023

apelisse Apr 6, 2023

apelisse Apr 6, 2023

apelisse Apr 6, 2023

apelisse Apr 6, 2023

apelisse Apr 6, 2023

jpbetz May 4, 2023

apelisse Apr 6, 2023

jpbetz May 4, 2023

jpbetz May 4, 2023

apelisse May 4, 2023

apelisse May 4, 2023

jpbetz May 4, 2023

apelisse Apr 6, 2023

jpbetz commented Aug 8, 2023

k8s-ci-robot commented Aug 8, 2023

	// +valdiation=rule:"!self.hostNetwork \|\| self.containers.all(c, c.containerPort.all(cp, cp.hostPort == cp.containerPort))"
	// +validation=rule:"!self.hostNetwork \|\| self.containers.all(c, c.containerPort.all(cp, cp.hostPort == cp.containerPort))"

	In this example, both `+valdiation`, `+minimum` and `+maximum` are IDL tags.
	In this example, both `+validation`, `+minimum` and `+maximum` are IDL tags.

KEP-3937: Declarative Validation #3938

KEP-3937: Declarative Validation #3938

Conversation

jpbetz commented Apr 3, 2023

k8s-ci-robot commented Apr 3, 2023

k8s-ci-robot commented Apr 3, 2023

apelisse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbetz commented Aug 8, 2023

k8s-ci-robot commented Aug 8, 2023