OpenAPI-based CEL type library #113312

jiahuif · 2022-10-24T22:49:31Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

rewrite the CEL schema library to use kube-openapi, based on that of staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel, which uses structural.Structural.
two implementations of schema resolver, one using client-go OpenAPI v3 discovery mechanism, the other with compiled-in OpenAPI definitions. Compiled-in resolver can be useful for the API server itself while ongoing development of client side CEL evaluation needs the discovery resolver.
create an adaptor between CRD Structural and OpenAPI schemas. This is to avoid recursive/deep conversion that takes an O(N) time where N is the depth of nesting in a schema.
change existing library for CRDs to use the adaptor.
add integration test for above

Special notes for your reviewer:

To limit the size of this PR, it does not include the part where the admission plugin uses the library.

Does this PR introduce a user-facing change?

NONE

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

- [KEP]: https://github.com/kubernetes/enhancements/issues/3488

cici37

Note: I guess we don't need the compilation and validation files here(might cause confusion). And the code would be tested in #113314. Currently the conversion(between object and ref.Val) failed(possible reason of lacking ref resolver?). Will retest after changes are made. Thank you!

Maybe worth to also add builtin type tests in this PR?

staging/src/k8s.io/apiserver/pkg/cel/builtins/compilation.go

leilajal · 2022-10-27T16:48:09Z

/triage accepted

staging/src/k8s.io/apiserver/pkg/cel/builtins/openapi.go

jpbetz

Also add tests showing how this can be used with a native types?

staging/src/k8s.io/apiserver/pkg/cel/builtins/compilation.go

staging/src/k8s.io/apiserver/pkg/cel/builtins/validation.go

jpbetz · 2022-11-02T00:19:52Z

staging/src/k8s.io/apiserver/pkg/cel/builtins/values.go

+// UnstructuredToVal converts a Kubernetes unstructured data element to a CEL Val.
+// The root schema of custom resource schema is expected contain type meta and object meta schemas.
+// If Embedded resources do not contain type meta and object meta schemas, they will be added automatically.
+func UnstructuredToVal(unstructured interface{}, schema *spec.Schema) ref.Val {


Given that we primarily intend to support native types with this, should this instead be something like ObjectToVal(obj runtime.Object) ref.Val and we can have the implementation look up the schema and convert the runtime.Object to unstructured using DefaultUnstructuredConverter? This would allow us to change the implementation in the future (e.g. use a reflection based approach instead of conversion to unstructured) without changing the function signature.

Current implementation let the caller to specify the schema because eventually the schema needs to be cached. It's better for the caller to maintain the cache to avoid having to provide another set of schema cache invalidation.

wenjiaswe · 2023-02-09T01:55:56Z

/test pull-kubernetes-unit

jpbetz

Functionality looks good. The copy-if-changed looks right. The wrapper is nice. Just a few minor comments.

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

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

jiahuif · 2023-02-13T19:01:51Z

/cc @alexzielenski

jpbetz · 2023-02-13T21:55:50Z

/lgtm

k8s-ci-robot · 2023-02-13T21:55:56Z

LGTM label has been added.

Git tree hash: 6d4c70f11001bbb6888a57a3c0ce989f7a28681e

jiahuif · 2023-02-14T22:41:07Z

All comments addressed, could you take another look when available? @liggitt

liggitt · 2023-02-17T22:30:12Z

All comments addressed, could you take another look when available? @liggitt

ack, should be able to look later today

liggitt

/approve

Thanks for all the iterations on this. There are still a couple spots that would be good to be sure we have benchmarks exercising (we might already? I'm not sure), but I'm very happy to see the full deep copies and recursive conversions go away

liggitt · 2023-02-18T04:58:20Z

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

+	for n, prop := range s.Structural.Properties {
+		s := prop
+		res[n] = &Structural{Structural: &s}
+	}


as a follow-up, it would be good to make sure we have benchmarks of validation-time calls that verify even this shallow-copy isn't problematic in terms of allocations

liggitt · 2023-02-18T05:00:32Z

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

-		unkeyedItems: items,
-	}
+func makeMapList(sts *schema.Structural, items []interface{}) (rv common.MapList) {
+	return common.MakeMapList(&model.Structural{Structural: sts}, items)


same comment here about a follow-up benchmark that calls validate with a CRD and CR that ends up exercising this call so we have visibility to allocations

k8s-ci-robot · 2023-02-18T05:02:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jiahuif, liggitt

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:

~~OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]
~~test/integration/apiserver/OWNERS~~ [liggitt]
~~vendor/OWNERS~~ [liggitt]

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

k8s-triage-robot · 2023-02-18T07:30:42Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot requested review from apelisse and smarterclayton October 24, 2022 22:50

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 24, 2022

jiahuif force-pushed the feature/cel/builtins branch 4 times, most recently from b6c7697 to 68225fb Compare October 25, 2022 00:15

jiahuif changed the title ~~[WIP] rewrite the CEL schema library~~ rewrite the CEL schema library Oct 25, 2022

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 25, 2022

jiahuif force-pushed the feature/cel/builtins branch from 59e4eaf to 10ccc01 Compare October 25, 2022 22:45

cici37 reviewed Oct 26, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/cel/builtins/compilation.go Outdated Show resolved Hide resolved

jiahuif force-pushed the feature/cel/builtins branch 2 times, most recently from acdcd86 to 32f791f Compare October 27, 2022 00:07

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 27, 2022

cici37 reviewed Oct 28, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/cel/builtins/openapi.go Outdated Show resolved Hide resolved

This was referenced Oct 28, 2022

Implement CEL for Admission Control #113314

Merged

CEL in Admission Control Tracking #113440

Closed

jpbetz reviewed Nov 2, 2022

View reviewed changes

jiahuif force-pushed the feature/cel/builtins branch 8 times, most recently from 5accc99 to f619a34 Compare February 8, 2023 23:50

jiahuif mentioned this pull request Feb 9, 2023

Type System for ValidatingAdmissionPolicy #115668

Merged

jiahuif force-pushed the feature/cel/builtins branch from f619a34 to 9f98812 Compare February 10, 2023 22:18

jpbetz reviewed Feb 11, 2023

View reviewed changes

jiahuif force-pushed the feature/cel/builtins branch 2 times, most recently from 4bb675d to b29f618 Compare February 13, 2023 18:38

k8s-ci-robot requested a review from alexzielenski February 13, 2023 19:01

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

liggitt reviewed Feb 18, 2023

View reviewed changes

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

k8s-ci-robot merged commit 70b2e4a into kubernetes:master Feb 18, 2023

k8s-ci-robot added this to the v1.27 milestone Feb 18, 2023

DangerOnTheRanger mentioned this pull request Feb 24, 2023

refactor validatingadmissionpolicy cel validator and compiler to be reusable #115816

Merged

cici37 mentioned this pull request Mar 16, 2023

CEL for Admission Control kubernetes/enhancements#3488

Open

16 tasks

jiahuif deleted the feature/cel/builtins branch October 19, 2023 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenAPI-based CEL type library #113312

OpenAPI-based CEL type library #113312

jiahuif commented Oct 24, 2022 •

edited

Loading

cici37 left a comment •

edited

Loading

leilajal commented Oct 27, 2022

jpbetz left a comment

jpbetz Nov 2, 2022

jiahuif Dec 15, 2022

wenjiaswe commented Feb 9, 2023

jpbetz left a comment

jiahuif commented Feb 13, 2023

jpbetz commented Feb 13, 2023

k8s-ci-robot commented Feb 13, 2023

jiahuif commented Feb 14, 2023

liggitt commented Feb 17, 2023

liggitt left a comment

liggitt Feb 18, 2023

liggitt Feb 18, 2023

k8s-ci-robot commented Feb 18, 2023

k8s-triage-robot commented Feb 18, 2023

OpenAPI-based CEL type library #113312

OpenAPI-based CEL type library #113312

Conversation

jiahuif commented Oct 24, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

cici37 left a comment • edited Loading

Choose a reason for hiding this comment

leilajal commented Oct 27, 2022

jpbetz left a comment

Choose a reason for hiding this comment

jpbetz Nov 2, 2022

Choose a reason for hiding this comment

jiahuif Dec 15, 2022

Choose a reason for hiding this comment

wenjiaswe commented Feb 9, 2023

jpbetz left a comment

Choose a reason for hiding this comment

jiahuif commented Feb 13, 2023

jpbetz commented Feb 13, 2023

k8s-ci-robot commented Feb 13, 2023

jiahuif commented Feb 14, 2023

liggitt commented Feb 17, 2023

liggitt left a comment

Choose a reason for hiding this comment

liggitt Feb 18, 2023

Choose a reason for hiding this comment

liggitt Feb 18, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 18, 2023

k8s-triage-robot commented Feb 18, 2023

jiahuif commented Oct 24, 2022 •

edited

Loading

cici37 left a comment •

edited

Loading