KEP-4008: CRDValidationRatcheting: Ratchet errors from CEL expressions if `old` DeepEqual `new` #121016

alexzielenski · 2023-10-06T01:52:58Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds support for CEL expressions to CRDValidationRatcheting as part of beta. This is NOT the "Advanced CEL Ratcheting" part of the KEP. This is an implementation of standard ratcheting for CEL rules when old is DeepEqual to new.

Factors out the correlation and comparison bits from the schema ratcheting code into a reusable type so that it can be shared with CEL implementation without much ripple

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

CRDValidationRatcheting: Adds support for ratcheting `x-kubernetes-validations` in schema

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: http://kep.k8s.io/4008

/sig api-machinery
/assign @apelisse
/cc @sttts @cici37

k8s-ci-robot · 2023-10-06T01:52:59Z

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

apelisse

Mostly looks good so far.

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go

staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go

Jefftree · 2023-10-17T19:57:19Z

/triage accepted

apelisse · 2023-10-18T22:47:14Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go

+ schema: mustSchema(`
+ type: object
+ properties:
+ foo:
+ type: string
+ x-kubernetes-validations:
+ - rule: self == "bar"
+ message: "gotta be baz"
+ `),


This looks so beautiful, tabsdiedie is proud of you

Ah, is this a Scala stripMargin equivalent? Nice. I'd really like to have a general purpose one of these in a util package at some point (but this is great for now).

apelisse · 2023-10-18T22:49:15Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go

+ - bar: bar
+ `),
+ warnings: []string{
+ `root[0].bar: Invalid value: "string": gotta be baz`,


Unrelated and not an action, but what is this

invalid value: "string": gotta be baz // instead of invalid value: "bar": gotta be baz

I've always wondered that. It is a curiosity of how our forked openapi validator in kube-openapi generates errors :)

Yeah, I guess it's a string indeed, cool ...

apelisse · 2023-10-18T22:51:25Z

/lgtm

k8s-ci-robot · 2023-10-18T22:51:31Z

LGTM label has been added.

Git tree hash: b57b5ade299652c9ac34f428ef727e31aebb5ef1

jpbetz

minor nit

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go

jpbetz · 2023-10-23T15:32:33Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go

+ schema: mustSchema(`
+ type: object
+ properties:
+ foo:
+ type: string
+ x-kubernetes-validations:
+ - rule: self == "bar"
+ message: "gotta be baz"
+ `),


Ah, is this a Scala stripMargin equivalent? Nice. I'd really like to have a general purpose one of these in a util package at some point (but this is great for now).

jpbetz · 2023-10-23T15:35:13Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go

+ name: "ratchet error triggered by normal CEL expression from an uncorrelatable part of the schema if one of its parent nodes is totally unchanged",
+ schema: mustSchema(`
+ type: array
+ x-kubernetes-list-type: atomic


nit: Can we also get one test using x-kubernetes-list-type: atomic (I'm confident it will pass, I mostly want it for regression prevention)? It can be a copy-paste of an atomic one..

Did you mean x-kubernetes-list-type: set? Either way, I noticed it wasnt included, and added some more cases, as well as some maplist and nested map cases.

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go

alexzielenski · 2023-10-23T18:54:19Z

/retest

jpbetz · 2023-10-23T21:19:17Z

/approve
/lgtm

k8s-ci-robot · 2023-10-23T21:19:25Z

LGTM label has been added.

Git tree hash: 2ef307349d17924e738faa0f405074e8949befb8

k8s-ci-robot · 2023-10-23T21:19:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, 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:

~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [jpbetz]

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

sftim · 2023-10-25T07:49:54Z

Changelog suggestion

ℹ️ should get a technical check before making the change

Added ratcheting (behind `CRDValidationRatcheting` feature gate) to tolerate `x-kubernetes-validations` errors when the old resource was equal - `reflect.DeepEqual()` - to the new object.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 6, 2023

k8s-ci-robot assigned apelisse Oct 6, 2023

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels Oct 6, 2023

k8s-ci-robot requested a review from cici37 October 6, 2023 01:53

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 6, 2023

k8s-ci-robot requested a review from sttts October 6, 2023 01:53

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/apiserver labels Oct 6, 2023

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-cel branch 5 times, most recently from 5522aa6 to 543f8d3 Compare October 9, 2023 19:27

This was referenced Oct 10, 2023

add rest of accessors to common.Schema #121096

Merged

CRDValidationRatcheting: Factor object correlation and comparison into reusable component #121118

Merged

apelisse reviewed Oct 13, 2023

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go Outdated Show resolved Hide resolved

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2023

alexzielenski mentioned this pull request Oct 16, 2023

CRD Validation Ratcheting kubernetes/enhancements#4008

Open

12 tasks

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-cel branch from 543f8d3 to d8df5a1 Compare October 16, 2023 22:31

alexzielenski marked this pull request as ready for review October 16, 2023 22:52

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2023

alexzielenski commented Oct 16, 2023

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go Outdated Show resolved Hide resolved

apelisse reviewed Oct 16, 2023

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go Outdated Show resolved Hide resolved

k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Oct 17, 2023

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-cel branch 4 times, most recently from ce9c864 to 519b7ed Compare October 18, 2023 20:07

apelisse reviewed Oct 18, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2023

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2023

alexzielenski mentioned this pull request Oct 20, 2023

KEP-4008: CRDValidationRatcheting: Add Benchmarks #121405

Merged

jpbetz suggested changes Oct 23, 2023

View reviewed changes

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-cel branch from 519b7ed to f075f4c Compare October 23, 2023 17:19

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2023

k8s-ci-robot requested a review from apelisse October 23, 2023 17:19

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-cel branch from f075f4c to 7840f8e Compare October 23, 2023 17:24

ratcheting: ratchet CEL expressions for no-op updates

6d580da

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-cel branch from 7840f8e to 6d580da Compare October 23, 2023 17:28

k8s-ci-robot assigned jpbetz Oct 23, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2023

k8s-ci-robot merged commit 3930f3f into kubernetes:master Oct 24, 2023
14 checks passed

k8s-ci-robot added this to the v1.29 milestone Oct 24, 2023

alexzielenski mentioned this pull request Oct 24, 2023

KEP-4008: CRDValidationRatcheting Bump Feature Gate To Beta #121461

Merged

9 tasks

alexzielenski changed the title ~~CRDValidationRatcheting: Ratchet errors from CEL expressions if old DeepEqual new~~ KEP-4008: CRDValidationRatcheting: Ratchet errors from CEL expressions if old DeepEqual new Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-4008: CRDValidationRatcheting: Ratchet errors from CEL expressions if `old` DeepEqual `new` #121016

KEP-4008: CRDValidationRatcheting: Ratchet errors from CEL expressions if `old` DeepEqual `new` #121016

alexzielenski commented Oct 6, 2023 •

edited

k8s-ci-robot commented Oct 6, 2023

apelisse left a comment

Jefftree commented Oct 17, 2023

apelisse Oct 18, 2023

jpbetz Oct 23, 2023

apelisse Oct 18, 2023

alexzielenski Oct 18, 2023

apelisse Oct 18, 2023

apelisse commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

jpbetz left a comment

jpbetz Oct 23, 2023

jpbetz Oct 23, 2023

alexzielenski Oct 23, 2023

alexzielenski commented Oct 23, 2023

jpbetz commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

sftim commented Oct 25, 2023

KEP-4008: CRDValidationRatcheting: Ratchet errors from CEL expressions if old DeepEqual new #121016

KEP-4008: CRDValidationRatcheting: Ratchet errors from CEL expressions if old DeepEqual new #121016

Conversation

alexzielenski commented Oct 6, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Oct 6, 2023

apelisse left a comment

Choose a reason for hiding this comment

Jefftree commented Oct 17, 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

apelisse commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

jpbetz 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

alexzielenski commented Oct 23, 2023

jpbetz commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

sftim commented Oct 25, 2023

KEP-4008: CRDValidationRatcheting: Ratchet errors from CEL expressions if `old` DeepEqual `new` #121016

KEP-4008: CRDValidationRatcheting: Ratchet errors from CEL expressions if `old` DeepEqual `new` #121016

alexzielenski commented Oct 6, 2023 •

edited