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.

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

Add extended functions for data manipulation in CEL expressions #22

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aerfio
Copy link
Contributor

@aerfio aerfio commented May 27, 2024

Description of your changes

This PR adds a few of extended functions for data manipulation in CEL expressions. Most of those functions are already in use in k8s codebase here: https://github.com/kubernetes/kubernetes/blob/v1.29.3/staging/src/k8s.io/apiserver/pkg/cel/environment/base.go#L49. I've also added few additional ones from the github.com/google/cel-go/cel and "github.com/google/cel-go/ext.

Fixes #

I have:

fn.go Outdated
Comment on lines 37 to 50
ext.Encoders(),
ext.Lists(),
ext.Math(),
cel.ExtendedValidations(),
cel.EagerlyValidateDeclarations(true),
cel.DefaultUTCTimeZone(true),
cel.CrossTypeNumericComparisons(true),
cel.OptionalTypes(),
ext.Strings(ext.StringsVersion(2)),
ext.Sets(),
library.URLs(),
library.Lists(),
library.Regex(),
library.Quantity(),
Copy link
Member

Choose a reason for hiding this comment

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

How did you land on this set of options? Is it pretty much what the Kubernetes API server uses? I'm wondering if there's some good way to document them without necessarily going so far as a comment for each option.

Also, do you know if these are all backward compatible? Could any of them break an existing function usage?

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 it's still a draft PR and in a miserable shape, I'm still experimenting 😅. But ok, my current thought process is to add all extended functions from k8s codebase, because then writing CEL expressions would match the dev experience of writing CEL rules for CRDs. So I'd add all/most of functions from k8s.io/apiserver/pkg/cel/library and some from github.com/google/cel-go/cel and github.com/google/cel-go/ext, at least those that you can find in this slice: https://github.com/kubernetes/kubernetes/blob/v1.29.3/staging/src/k8s.io/apiserver/pkg/cel/environment/base.go#L49

I'm not sure whether adding more validation rules would be a breaking change, probably yeah but this project is still at v0.1.1 so I guess it's good time for that 🤷🏻

Copy link
Member

Choose a reason for hiding this comment

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

Haha no pressure, I was just curious. 🙂

By the way we definitely need more maintainers on this function. I'd be happy to add you if it's something you're passionate about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not experienced in CEL expressions at all and neither in cel-go library :P I just used it in my private project once, in a custom crossplane provider-k8s where I used it to allow users to derive readiness of underlying object by passing a CEL expression, see https://github.com/aerfio/provider-k8s/blob/4610c123ea33f8c3faa45036318681eb817aa492/examples/sample/deploy.yaml#L33. In my project the CEL environment creation is almost the same as here, it's a bit worse because I dont reuse cel.Env. But that was only a PoC project, made for fun with 0 users :P

So back to the point: I think I dont have enough experience in CEL related stuff to get maintainer status, but feel free to ping me in the future 👍🏻

Signed-off-by: Mateusz Puczyński <[email protected]>
@aerfio
Copy link
Contributor Author

aerfio commented May 30, 2024

There were some compilation errors because of new dependency on k8s.io/[email protected] that itself depends on github.com/google/[email protected], and downgrading cel-go to v0.17.7 downgrades the function-sdk-go package to v0.2.0-rc.0:
image.
So I removed all uses of k8s.io/apiserver/pkg/cel/library, fixing this situation would probably require changes in https://github.com/crossplane/function-sdk-go/blob/11b59ad46986d3736e928c95ad5dc712aa15c724/go.mod#L70, if I understand the situation correctly and that's where the [email protected] comes from.

Edit: yeah, I played with function-sdk-go for a couple of minutes and it seems like the github.com/bufbuild/buf is the culprit there. Offtopic: the habit of putting various tools into the same go.mod as the main deps bit me few times with crossplane deps, wouldnt it be better to do something like https://github.com/goph/licensei#installation?

Comment on lines -69 to +71
k8s.io/api v0.29.1 // indirect
k8s.io/apiextensions-apiserver v0.29.1 // indirect
k8s.io/client-go v0.29.1 // indirect
k8s.io/api v0.29.3 // indirect
k8s.io/apiextensions-apiserver v0.29.3 // indirect
k8s.io/client-go v0.29.3 // indirect
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumped those to match direct dependecy on k8s.io/[email protected]

@negz
Copy link
Member

negz commented May 30, 2024

Edit: yeah, I played with function-sdk-go for a couple of minutes and it seems like the github.com/bufbuild/buf is the culprit there. Offtopic: the habit of putting various tools into the same go.mod as the main deps bit me few times with crossplane deps, wouldnt it be better to do something like https://github.com/goph/licensei#installation?

Yeah buf specifically has been quite a pain in this regard recently. We switched to using go install in c/c per crossplane/crossplane#5588 to workaround this, but that has its own issues.

I like having tools in go.mod because it means a plain go generate will work for everyone, even if not wrapped by some build tool (e.g. Make). It looks like there's a new build directive that can help with this - https://go.dev/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module.

Edit: I was reading //go:build tool as //go:build:tool thinking it was a special new directive but it's actually just a build tag, which is what we already have (//go:build generate).

@aerfio
Copy link
Contributor Author

aerfio commented May 31, 2024

but that has its own issues.

Like the inability to force renovate to update those go generate lines? I usually use Makefile like this, renovate is able to update Makefile without a problem (I know it because I've copied interesting parts of renovate config from crossplane-runtime 😂)

Either way, this PR is now undrafted

@aerfio aerfio marked this pull request as ready for review May 31, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants