Skip to content
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

KEP-2876: CEL expression language: Clarify escaping, root field access, int-or-string/embedded/unknown types #3039

Merged
merged 15 commits into from
Dec 15, 2021
Merged

KEP-2876: CEL expression language: Clarify escaping, root field access, int-or-string/embedded/unknown types #3039

merged 15 commits into from
Dec 15, 2021

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Nov 6, 2021 •
edited
Loading

We've dug into some important details of CEL during implementation. This PR aims to update the KEP to keep it accurate and in-sync, and enable discussion of the decisions before we merge the implementation.

The main points are:

  • escaping details
  • Validator field access to metadata, and more generally how validation rules defined on the root of the object behave
  • handling of int-or-string, embedded and unknown data

@cici37 @sttts @alculquicondor @liggitt @deads2k

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 6, 2021
@k8s-ci-robot
Copy link
Contributor

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

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

@k8s-ci-robot k8s-ci-robot added 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. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 6, 2021
@jpbetz jpbetz force-pushed the cel-escaping-types branch from 05d818f to c553802 Compare November 6, 2021 19:40
@jpbetz jpbetz force-pushed the cel-escaping-types branch from c553802 to 6af4a04 Compare November 6, 2021 22:04
liggitt
liggitt reviewed Nov 8, 2021
View reviewed changes
keps/sig-api-machinery/2876-crd-validation-expression-language/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/2876-crd-validation-expression-language/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/2876-crd-validation-expression-language/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/2876-crd-validation-expression-language/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/2876-crd-validation-expression-language/README.md Show resolved Hide resolved
keps/sig-api-machinery/2876-crd-validation-expression-language/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/2876-crd-validation-expression-language/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/2876-crd-validation-expression-language/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/2876-crd-validation-expression-language/README.md Outdated Show resolved Hide resolved
jpbetz and others added 2 commits November 8, 2021 12:15
@jpbetz @liggitt
Update keps/sig-api-machinery/2876-crd-validation-expression-language…
c3bc36f 
…/README.md

Co-authored-by: Jordan Liggitt <jordan@liggitt.net>
@jpbetz @liggitt
Update keps/sig-api-machinery/2876-crd-validation-expression-language…
36184df 
…/README.md

Co-authored-by: Jordan Liggitt <jordan@liggitt.net>
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 8, 2021
@jpbetz jpbetz force-pushed the cel-escaping-types branch from d530e0a to 566b0ea Compare November 8, 2021 23:52
@alculquicondor
Copy link
Member

lgtm, but leaving the official one to approvers

@jpbetz jpbetz force-pushed the cel-escaping-types branch from 4097c7e to 8d65820 Compare November 9, 2021 23:10
@liggitt liggitt mentioned this pull request Nov 10, 2021
Merged
deads2k
deads2k reviewed Nov 10, 2021
View reviewed changes
Copy link
Contributor

@deads2k deads2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Did we decide on oldSelf and did that get written down somewhere?
  2. Do we have an example of selecting a list item based on its key field?
  3. Are we able to validate specifically named labels or annotations? While discouraged by kube API conventions, this validation pattern is used in real-world applications.

keps/sig-api-machinery/2876-crd-validation-expression-language/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/2876-crd-validation-expression-language/README.md Outdated Show resolved Hide resolved
@jpbetz jpbetz force-pushed the cel-escaping-types branch from 8d65820 to 71308bb Compare November 11, 2021 04:32
jpbetz and others added 2 commits November 12, 2021 11:06
@jpbetz @liggitt
Clean up examples
645a7ec 
Co-authored-by: Jordan Liggitt <jordan@liggitt.net>
@jpbetz
Add more examples, remove obsolete info, add note about oldSelf in gr…
e73e7e3 
…aduation criteria
@liggitt
Copy link
Member

liggitt commented Nov 12, 2021

lgtm

@jpbetz jpbetz mentioned this pull request Nov 16, 2021
Merged
@liggitt
Copy link
Member

liggitt commented Nov 17, 2021

/lgtm
as-is

/hold if you want to address @TristonianJones's comments here instead of in a follow-up

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2021
@jpbetz
Copy link
Contributor Author

jpbetz commented Nov 19, 2021

I've applied the last round of feedback now since there was a formatting issue I really wanted to fix before merging.

@jpbetz
Copy link
Contributor Author

jpbetz commented Dec 15, 2021

Friendly nudge

@liggitt
Copy link
Member

liggitt commented Dec 15, 2021

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 15, 2021
@k8s-ci-robot k8s-ci-robot merged commit 4bd496d into kubernetes:master Dec 15, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Dec 15, 2021
wojtek-t
wojtek-t reviewed Jan 4, 2022
View reviewed changes
keps/sig-api-machinery/2876-crd-validation-expression-language/kep.yaml
@@ -6,7 +6,7 @@ authors:
- "@DangerOnTheRanger"
- "@leilajal"
owning-sig: sig-api-machinery
status: implementable
status: implemented
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're generally switching to implemented only after GA-ing the feature - this one is just Alpha, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, true, missed that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay, I didn't realize that was the rule. I'll revert this.

@wojtek-t wojtek-t mentioned this pull request Jan 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wojtek-t wojtek-t wojtek-t left review comments

@cici37 cici37 cici37 left review comments

@deads2k deads2k deads2k left review comments

@alculquicondor alculquicondor alculquicondor left review comments

@liggitt liggitt liggitt left review comments

@sttts sttts sttts left review comments

@TristonianJones TristonianJones TristonianJones left review comments

@cbandy cbandy cbandy left review comments

@apelisse apelisse Awaiting requested review from apelisse

@fedebongio fedebongio Awaiting requested review from fedebongio

Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.
10 participants
@jpbetz @k8s-ci-robot @alculquicondor @liggitt @cbandy @TristonianJones @sttts @deads2k @cici37 @wojtek-t