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

alexzielenski · 2023-10-10T17:58:19Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

CachedDeepEqual is needed for #121016 to use with the CEL validator. It will also be neeed for native types in declarative validation. This PR factors out the object correlation and comparison tree into a re-usable component. At validation time the instance can be shared by all validation passes.

Tried to keep all commits semantically a no-op

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

/assign @apelisse
/sig api-machinery

Does this PR introduce a user-facing change?

NONE

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

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

inlineValidator unnecessary, since we already have access to the `new` object

so it is reusable by CEL validators

going to be used from CEL validator too

for reuseability with CEL structural schema impl

jiahuif · 2023-10-10T20:04:24Z

/triage accepted

apelisse

Thanks! I don't see a lot of tests throughout, maybe they didn't need to be updated, if so that's great!

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

before it required running validation first, now it builds the tree as needed

no changes except package naming

apelisse · 2023-10-13T02:43:26Z

/lgtm
/approve
/retest

k8s-ci-robot · 2023-10-13T02:43:33Z

LGTM label has been added.

Git tree hash: 243cf219e4927f96da3939517d6e4e1f64f3383b

alexzielenski · 2023-10-13T03:13:54Z

/assign jpbetz

jpbetz

Nothing major, just some godoc suggestions and switch handling suggestions.

staging/src/k8s.io/apiserver/pkg/cel/common/equality.go

jpbetz · 2023-10-13T20:29:40Z

staging/src/k8s.io/apiserver/pkg/cel/common/equality.go

+		// Atomic lists are not correlatable by item
+		// Ratcheting is not available on a per-index basis
+		return nil
+	default:


label this as case "granular" since that is what it's for and leave a default with panic as the fall-through if somehow this function ever receives a list type we don't expect?

@apelisse pointed out the default list type was atomic, so this default case treating it as granular may be a bug. (I don't believe we have granular lists)

This was also pointed out by @apelisse when the code originally went in last release, but we decided to punt whether we should treat list items as granular for the purposes of DeepEqual comparison in ratcheting.

On one hand it'd be nice to allow users to do a point modification on a single element with ratcheting. On the other hand, there would be no CEL escape hatch for schemas that want to opt out of that behavior, and it'd be weird to allow point modification without allowing adding/removing.

Given the inconsistency and the low perceived benefit. I think we should drop support for ratcheting uncorrelatable array items in beta. (i.e. only map-type is ratchetable. Same as CEL). WDYT @apelisse @jpbetz ?

only map-type is ratchetable. Same as CEL

That's the way to go. You can't correlate atomic, and it's either atomic or map-type/set. Mapping by index doesn't really make sense to me.

I'm okay with deferring ratcheting on listType=atomic. I see the main benefits being cases where we're working on values nested under a listType=map.

Good catch on the default=atomic BTW. We should strive to always structure switch statements to reflect that.

added case for "" to be equivalent with atomic, disallowed default case ratcheting, and updated tests

staging/src/k8s.io/apiserver/pkg/cel/common/equality.go

discussion: kubernetes#121118 (comment)

alexzielenski · 2023-10-13T23:03:18Z

Updated with latest feedback. @apelisse PTAL

apelisse

/lgtm

k8s-ci-robot · 2023-10-16T15:36:39Z

LGTM label has been added.

Git tree hash: 36e85dbd304de1c8900d6d9818e7a162cc7d500a

jpbetz · 2023-10-16T16:51:22Z

/approve

k8s-ci-robot · 2023-10-16T16:51:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, apelisse, 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]
~~staging/src/k8s.io/apiserver/pkg/cel/OWNERS~~ [jpbetz]

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

discussion: kubernetes/kubernetes#121118 (comment) Kubernetes-commit: fb1fc8b4a72758688d1251278579b2b0ac666fc7

discussion: kubernetes#121118 (comment)

Alexander Zielenski added 6 commits October 10, 2023 09:48

refactor: remove unnecessary lambda (noop)

4fb5f1a

refactor: directly implement ValueValidator

30cf9ed

inlineValidator unnecessary, since we already have access to the `new` object

refactor: factor out object correlation

471e3ab

so it is reusable by CEL validators

refactor: add methods for Key and Index on CorrelatedObject

e73f3c8

going to be used from CEL validator too

refactor: export correlated fields

83a1dbf

refactor: use common.Schema in CorrelatedObject

e76aad1

for reuseability with CEL structural schema impl

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 10, 2023

k8s-ci-robot assigned apelisse Oct 10, 2023

k8s-ci-robot requested review from jiahuif and roycaihw October 10, 2023 17:59

k8s-ci-robot added the area/apiserver label Oct 10, 2023

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 10, 2023

apelisse reviewed Oct 10, 2023

View reviewed changes

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-factor-correlation branch from b7b93de to 92158e0 Compare October 11, 2023 19:19

Alexander Zielenski added 3 commits October 11, 2023 14:30

refactor: make CachedDeepEqual independent of validation

b321e8b

before it required running validation first, now it builds the tree as needed

refactor: move correlatedObject to its own file

27cb869

no changes except package naming

cleanup: add header and fix spelling

c08a932

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-factor-correlation branch from be95cb9 to e129fb6 Compare October 11, 2023 21:31

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 Oct 11, 2023

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-factor-correlation branch from e129fb6 to 1103e23 Compare October 11, 2023 22:26

test: add correlatedobject test cases

ba93472

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

k8s-ci-robot assigned jpbetz Oct 13, 2023

jpbetz suggested changes Oct 13, 2023

View reviewed changes

Alexander Zielenski added 5 commits October 13, 2023 13:50

cleanup: add godoc

0495616

cleanup: consistent interface{} and any

e1fa1df

cleanup: clarify correlatedOldValueForChildAtNewIndex comment

abb6859

cleanup: consistently support nil receiver and document

60c90fc

cleanup: use swtich in CachedDeepEqual and add more comments

0ed67c9

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

k8s-ci-robot requested review from apelisse and jpbetz October 13, 2023 21:06

Alexander Zielenski added 2 commits October 13, 2023 14:11

comments: clear up correlateOldValueForChildAtNewIndex godoc

d991ed5

ratcheting: disable correlation by index

fb1fc8b

discussion: kubernetes#121118 (comment)

apelisse reviewed Oct 16, 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 16, 2023

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

alexzielenski mentioned this pull request Oct 16, 2023

CRD Validation Ratcheting kubernetes/enhancements#4008

Open

16 tasks

k8s-ci-robot merged commit 3a3dc87 into kubernetes:master Oct 16, 2023

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

k8s-publishing-bot pushed a commit to kubernetes/apiserver that referenced this pull request Oct 17, 2023

ratcheting: disable correlation by index

8a3fe0e

discussion: kubernetes/kubernetes#121118 (comment) Kubernetes-commit: fb1fc8b4a72758688d1251278579b2b0ac666fc7

k8s-publishing-bot pushed a commit to kubernetes/apiextensions-apiserver that referenced this pull request Oct 17, 2023

ratcheting: disable correlation by index

c82832d

discussion: kubernetes/kubernetes#121118 (comment) Kubernetes-commit: fb1fc8b4a72758688d1251278579b2b0ac666fc7

Sharpz7 pushed a commit to Sharpz7/kubernetes that referenced this pull request Oct 27, 2023

ratcheting: disable correlation by index

66b92fe

discussion: kubernetes#121118 (comment)

richabanker pushed a commit to richabanker/kubernetes that referenced this pull request Jan 9, 2024

ratcheting: disable correlation by index

c685dba

discussion: kubernetes#121118 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alexzielenski commented Oct 10, 2023 •

edited

Loading

jiahuif commented Oct 10, 2023

apelisse left a comment

apelisse commented Oct 13, 2023

k8s-ci-robot commented Oct 13, 2023

alexzielenski commented Oct 13, 2023

jpbetz left a comment

jpbetz Oct 13, 2023

alexzielenski Oct 13, 2023 •

edited

Loading

apelisse Oct 13, 2023

jpbetz Oct 13, 2023

alexzielenski Oct 13, 2023 •

edited

Loading

alexzielenski commented Oct 13, 2023

apelisse left a comment

k8s-ci-robot commented Oct 16, 2023

jpbetz commented Oct 16, 2023

k8s-ci-robot commented Oct 16, 2023

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

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

Conversation

alexzielenski commented Oct 10, 2023 • edited Loading

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

jiahuif commented Oct 10, 2023

apelisse left a comment

Choose a reason for hiding this comment

apelisse commented Oct 13, 2023

k8s-ci-robot commented Oct 13, 2023

alexzielenski commented Oct 13, 2023

jpbetz left a comment

Choose a reason for hiding this comment

jpbetz Oct 13, 2023

Choose a reason for hiding this comment

alexzielenski Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

apelisse Oct 13, 2023

Choose a reason for hiding this comment

jpbetz Oct 13, 2023

Choose a reason for hiding this comment

alexzielenski Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

alexzielenski commented Oct 13, 2023

apelisse left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 16, 2023

jpbetz commented Oct 16, 2023

k8s-ci-robot commented Oct 16, 2023

alexzielenski commented Oct 10, 2023 •

edited

Loading

alexzielenski Oct 13, 2023 •

edited

Loading

alexzielenski Oct 13, 2023 •

edited

Loading