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

runtime: downgrade github.com/google/cel-go and cel.dev/expr #865

Closed
wants to merge 1 commit into from

Conversation

aerfio
Copy link

@aerfio aerfio commented Feb 3, 2025

Downgrade these deps to versions used in k8s.io/apiserver:

Reason: this change prevents compilation error when the project's dependencies (or the project itself) imports both k8s.io/apiserver/pkg/cel/environment and github.com/fluxcd/pkg/runtime/*.

Reproduction:

package main

import (
	"fmt"

	"github.com/fluxcd/pkg/runtime/client"
	"k8s.io/apiserver/pkg/cel/environment"
)

func main() {
	_ = environment.StoredExpressions
	_ = client.GetConfigOrDie
	fmt.Println("exit")
}

go.mod:

module github.com/aerfio/repoduction

go 1.23.5

require (
	github.com/fluxcd/pkg/runtime v0.53.0
	k8s.io/apiserver v0.32.1
)


require (
	cel.dev/expr v0.19.1 // indirect
	github.com/google/cel-go v0.23.1 // indirect
        // other indirect deps
)

compiling this simple app results in:

# k8s.io/apiserver/pkg/cel/environment
/Users/aerfio/go/pkg/mod/k8s.io/[email protected]/pkg/cel/environment/base.go:176:19: cannot use ext.TwoVarComprehensions (value of type func(options ...ext.TwoVarComprehensionsOption) "github.com/google/cel-go/cel".EnvOption) as func() "github.com/google/cel-go/cel".EnvOption value in argument to UnversionedLib

Trying to force correct deps also fails:

❯ go get -u github.com/google/[email protected] cel.dev/[email protected] github.com/fluxcd/pkg/[email protected]
go: github.com/fluxcd/pkg/[email protected] requires github.com/google/[email protected], not github.com/google/[email protected]

Signed-off-by: Mateusz Puczyński <[email protected]>
@aerfio aerfio force-pushed the aerfio/downgrade-deps branch from 187feea to 014283a Compare February 3, 2025 16:45
@matheuscscp
Copy link
Member

@aerfio Flux doesn't seem to be using k8s.io/apiserver/pkg/cel/environment anywhere:

https://github.com/search?q=org%3Afluxcd%20k8s.io%2Fapiserver%2Fpkg%2Fcel%2Fenvironment&type=code

This seems to be a bug/regression in upstream CEL for users in your specific situation. Did you raise an issue with upstream CEL about this?

@aerfio
Copy link
Author

aerfio commented Feb 3, 2025

@matheuscscp No, it's not about CEL libs. The fluxcd/pkg/runtime depends on cel libs (cel-go and cel.dev/expr) and on several k8s.io libs @ v0.32.1. If I have the project that uses fluxcd/pkg/runtime and I'd like to to also use the k8s.io/apiserver/pkg/cel/environment (or have any other dependency/import that uses that package), then I get a compilation error. This PR downgrades the CEL libs in fluxcd/pkg/runtime to the same version used in the k8s.io/[email protected], which is the same version used for other k8s deps.

IIUC this issue is primarily caused by the fact that cel-go has a breaking change in it's v0.23.0 minor version, but they're still v0 so they're allowed to do this. k8s.io/apiserver still uses older version of this lib, github.com/fluxcd/runtime uses newest version, so they clash. But you do not use any feature from newest version of these CEL libs.

It's a choice of compatibility with k8s.io/apiserver or keeping newest cel-go and cel.dev/expr libs. controller-runtime recently introduced a tool that keeps its deps synced with k/k: kubernetes-sigs/controller-runtime#2774

@matheuscscp
Copy link
Member

matheuscscp commented Feb 3, 2025

But you do not use any feature from newest version of these CEL libs.

Actually we do, we bumped to v0.23.0 specifically to support the functionality landed in CEL upstream by @bigkevmcd in this PR:

google/cel-go#1067

Kevin was initially introducing this functionality as a custom CEL library in this PR:

fluxcd/notification-controller#948

And using it like this:

https://github.com/fluxcd/notification-controller/pull/948/files#diff-59ed780af7ec7af60ec655b3c2252349c39224916bfb104ee8fc2773fec0773dR96-R113

Now bumping to v0.23.0 we can support this use case without the custom CEL library.

What's your use case for including both fluxcd/pkg/runtime and k8s.io/apiserver/pkg/cel/environment?

IIUC this issue is primarily caused by the fact that cel-go has a breaking change in it's v0.23.0 minor version, but they're still v0 so they're allowed to do this.

fluxcd/pkg/runtime is v0 too

@aerfio
Copy link
Author

aerfio commented Feb 4, 2025

What's your use case for including both fluxcd/pkg/runtime and k8s.io/apiserver/pkg/cel/environment?

I'm using fluxcd/pkg/* libs and k8s.io/apiserver through the whole codebase for various things

fluxcd/pkg/runtime is v0 too

Yup, I get that, and all the implications that stem from it.

Ok, given that you already use newest cel-go somewhere else I don't think this PR can be merged, I'll try to find another way or wait for k8s.io/apiserver to catch up. Thank you for your attention @matheuscscp

@aerfio aerfio closed this Feb 4, 2025
@aerfio aerfio deleted the aerfio/downgrade-deps branch February 4, 2025 08:17
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