Implement kro CEL library for maps #628

gfrey · 2025-08-13T09:46:24Z

Only function right now is merge. Potentially there are more. This implementation is inspired by the CEL lists extension.

jakobmoellerdev

Generally nice feature but we need to probably define a boundary of what to include and what not to include. im certain at some point people will ask for kubernetes CEL function as well! (https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/apiserver/pkg/cel/library)

a-hilaly · 2025-08-14T20:16:32Z

@jakobmoellerdev i think that's a great point for our next community meeting conversaiton. My mental model is that we'll need both kubernetes upstream CEL env combined with some KRO specific CEL functions

gfrey · 2025-08-18T07:02:35Z

@a-hilaly this lint error is weird. merge is implementing a function type for the cel.Function, so the "(error) is always nil (unparam)" problem is not something I think I can/want to change.
I'm a noob to GH actions, but the golangci-lint action seems to use v6 whilst v8 would be latest. Maybe that resolves the problem?

a-hilaly

Thanks a lot @gfrey ! left a few comments inline

a-hilaly · 2025-08-18T08:53:47Z

pkg/cel/library/maps.go

+//
+// # Merge
+//
+// Merges two maps. Keys from the first map take precedence over keys in the second map.


Keys from the first map take precedence over keys in the second map. - is this a common behaviour? i have the impression that most languages/libraries do the opposite - where the second map's values overwrite the first one. i.e in python

a = {'x':1} b= {'x':2} print(**a, **b) # prints {'x':2}

Yes, I was thinking the same, when picking this up again a few weeks back. It was motivated by the implementation, which it shouldn't be (cel map implementation doesn't seem to allow to replace values). I can fix that easily.

a-hilaly · 2025-08-18T08:54:20Z

pkg/cel/library/maps.go

+//
+// Merges two maps. Keys from the first map take precedence over keys in the second map.
+//
+//	map(string, int).merge(map(string, int)) -> map(string, int)


does this mean that .merges won't work with map(string, string) types?

No, it takes string keys, but values can be anything. I'll change int to any, unless you have a better idea.

a-hilaly · 2025-08-18T08:56:58Z

pkg/cel/library/maps.go

+// LibraryName implements the cel.SingletonLibrary interface method.
+func (l *mapsLib) LibraryName() string {
+	return "cel.lib.ext.kro.maps"
+}


shall we edit the library in name for random to match this one and similar patterns in cel-go? https://github.com/kro-run/kro/blob/main/pkg/cel/library/random.go#L53-L55 - https://github.com/google/cel-go/blob/master/ext/lists.go

I've just matched the patterns ;). So, I think changing random would make sense from that perspective.

+1 for both of these. Let's change up the random lib name in a separate mini PR. Shouldnt block anything here though since its mostly for conflict resolution

Wondering if we need to rename the folder from library to ext ?
Even a separate repo under kro-run ?

a-hilaly · 2025-08-18T09:17:18Z

pkg/cel/library/maps.go

+	mapType := cel.MapType(cel.TypeParamType("K"), cel.DynType)
+	// mapDynType := cel.MapType(cel.DynType, cel.DynType)


Suggested change

mapType := cel.MapType(cel.TypeParamType("K"), cel.DynType)

// mapDynType := cel.MapType(cel.DynType, cel.DynType)

mapType := cel.MapType(cel.TypeParamType("K"), cel.TypeParamType("V")

I'm wondering if we should restrict this function to only act on maps of similar types? Like map[string]int only merging with map[string]int. Users could leverage macros to transform map types if needed. My highlevel toughts here is that i'd rather restrict this now and relax it later, than try to make a generic function and hit tricky type situations.

It gets worse. It allows memory to grow, which undermines some of the core CEL assumptions, if I understand that correctly.
Despite all of this, my motivation is to get from a list of strings (say a list of availability zones to a list of subnet configurations that I can use for setting up a VPC. This would require this feature, but also the ability to merge maps of arbitrary types (not sure the suggested cel.TypeParamType("V") would allow for that).
Upon reflecting on that for some more, I agree. Let's start simple and extend if necessary. We know it is possible, but not need to go there immediately. I'll try to build it with pinned types.

a-hilaly · 2025-08-18T09:21:22Z

pkg/cel/library/maps.go

+					self, ok := arg1.(traits.Mapper)
+					if !ok {
+						return types.ValOrErr(arg1, "no such overload: %v.merge(%v)", arg1.Type(), arg2.Type())
+					}
+					other, ok := arg2.(traits.Mapper)
+					if !ok {
+						return types.ValOrErr(arg1, "no such overload: %v.merge(%v)", arg1.Type(), arg2.Type())
+					}


i wonder if the !ok checks are redundant here - doesn't CEL already validate argument types?

PS: I see what we're doing in https://github.com/kro-run/kro/blob/main/pkg/cel/library/random.go#L53-L55 and i wonder if we should fix that too :)

I think you're right, at least in the test added, the error is found prior to any execution in the env.Check call.

a-hilaly · 2025-08-18T09:24:15Z

pkg/cel/library/maps.go

+
+// merge merges two maps. Keys from the first map take precedence over keys in
+// the second map.
+func merge(self traits.Mapper, other traits.Mapper) (traits.Mapper, error) {


golangcilint is complaining here because merge never returns a non-nil error (which makes it a redundant parameter)

D'oh, my bad. Removed the unused error return value.

There will never be an error, so no need to always return nil.

The first map's values will be overwritten by values from the second map. This is what merge should do.

This is more restrictive. The other way is supported, but the consequences are not clear. So let's keep it simple.

a-hilaly

Thanks @gfrey , left two tiny comments below :)

a-hilaly · 2025-08-18T18:15:45Z

pkg/cel/library/maps.go

@@ -0,0 +1,103 @@
+package library


nit: missing license header

@a-hilaly we should maybe write a linter for new files to avoid this. WDYT?

I thought we had one! :) go generate should get that done. cc @tomasaschan

a-hilaly · 2025-08-18T18:15:50Z

pkg/cel/library/maps_test.go

@@ -0,0 +1,76 @@
+package library


a-hilaly · 2025-08-18T18:16:44Z

pkg/cel/library/maps.go

+	var result traits.MutableMapper
+
+	if mapVal, ok := other.Value().(map[ref.Val]ref.Val); ok {
+		result = types.NewMutableMap(types.DefaultTypeAdapter, mapVal)
+	} else {
+		result = types.NewMutableMap(types.DefaultTypeAdapter, nil)
+		for i := other.Iterator(); i.HasNext().(types.Bool); {
+			k := i.Next()
+			v := other.Get(k)
+			result.Insert(k, v)
+		}
+	}
+
+	for i := self.Iterator(); i.HasNext().(types.Bool); {
+		k := i.Next()
+		if result.Contains(k).(types.Bool) {
+			continue
+		}
+		v := self.Get(k)
+		result.Insert(k, v)
+	}
+	return result.ToImmutableMap()


i'm thinking, wouldn't something like this just work?

// populate with self map for it := self.Iterator(); it.HasNext() == types.True; { k := it.Next() result.Insert(k, self.Get(k)) } // overwrides with other map for it := other.Iterator(); it.HasNext() == types.True; { k := it.Next() result.Insert(k, other.Get(k)) }

this works in general (flip order so that other is written first, then fill in whatever keys aren't there from self); have picked suggestions from @jakobmoellerdev

a-hilaly · 2025-08-19T07:37:44Z

Cc @jakobmoellerdev

jakobmoellerdev

This looks great: Ive mostly had style remarks with this, logically its sound. Unsure if we want dyn type support in this lib

jakobmoellerdev · 2025-08-19T09:00:31Z

pkg/cel/library/maps.go

@@ -0,0 +1,103 @@
+package library


@a-hilaly we should maybe write a linter for new files to avoid this. WDYT?

jakobmoellerdev · 2025-08-19T09:01:08Z

pkg/cel/library/maps.go

+// LibraryName implements the cel.SingletonLibrary interface method.
+func (l *mapsLib) LibraryName() string {
+	return "cel.lib.ext.kro.maps"
+}


+1 for both of these. Let's change up the random lib name in a separate mini PR. Shouldnt block anything here though since its mostly for conflict resolution

jakobmoellerdev · 2025-08-19T09:04:26Z

pkg/cel/library/maps.go

+			cel.MemberOverload("map_merge",
+				[]*cel.Type{mapType, mapType},
+				mapType,
+				cel.BinaryBinding(func(arg1, arg2 ref.Val) ref.Val {


nit: expose this function separately instead of inlining, and then unit test this function as such instead of having to go through the environment

jakobmoellerdev · 2025-08-19T09:21:48Z

pkg/cel/library/maps.go

+
+// merge merges two maps. Keys from the first map take precedence over keys in
+// the second map.
+func merge(self traits.Mapper, other traits.Mapper) traits.Mapper {


nit: this looks very complex (similar to https://github.com/kro-run/kro/pull/628/files#r2283120971 comment I looked into this a bit). why not go for something like this:

func mergeVals(lhs, rhs ref.Val) ref.Val { left, lok := lhs.(traits.Mapper) right, rok := rhs.(traits.Mapper) if !lok || !rok { return types.ValOrErr(lhs, "no such overload: %v.merge(%v)", lhs.Type(), rhs.Type()) } return merge(left, right) } // merge returns a new map containing entries from both maps. // Keys in 'other' overwrite keys in 'self'. func merge(self, other traits.Mapper) traits.Mapper { result := mapperTraitToMutableMapper(other) for i := self.Iterator(); i.HasNext().(types.Bool); { k := i.Next() if !result.Contains(k).(types.Bool) { result.Insert(k, self.Get(k)) } } return result.ToImmutableMap() } // mapperTraitToMutableMapper copies a traits.Mapper into a MutableMap. func mapperTraitToMutableMapper(m traits.Mapper) traits.MutableMapper { vals := make(map[ref.Val]ref.Val, m.Size().(types.Int)) for it := m.Iterator(); it.HasNext().(types.Bool); { k := it.Next() vals[k] = m.Get(k) } return types.NewMutableMap(types.DefaultTypeAdapter, vals) }

Then you can call

cel.Function("merge", cel.MemberOverload("map_merge", []*cel.Type{mapType, mapType}, mapType, cel.BinaryBinding(mergeVals), ), )

jakobmoellerdev · 2025-08-19T09:30:34Z

pkg/cel/library/maps_test.go

+	"github.com/google/cel-go/cel"
+)
+
+func TestMaps(t *testing.T) {


nit: using testify can make your life a lot easier here probably (we already use it in other tests in this package):

package library import ( "fmt" "testing" "github.com/google/cel-go/cel" "github.com/stretchr/testify/require" ) func TestMaps(t *testing.T) { tests := []struct { expr string err require.ErrorAssertionFunc }{ {expr: `{}.merge({}) == {}`}, {expr: `{}.merge({'a': 1}) == {'a': 1}`}, {expr: `{}.merge({'a': 2.1}) == {'a': 2.1}`}, {expr: `{}.merge({'a': 'foo'}) == {'a': 'foo'}`}, {expr: `{'a': 1}.merge({}) == {'a': 1}`}, {expr: `{'a': 1}.merge({'b': 2}) == {'a': 1, 'b': 2}`}, {expr: `{'a': 1}.merge({'a': 2, 'b': 2}) == {'a': 2, 'b': 2}`}, {expr: `{}.merge([])`, err: func(t require.TestingT, err error, i ...interface{}) { require.ErrorContains(t, err, "ERROR: <input>:1:9: found no matching overload for 'merge' applied to 'map(dyn, dyn).(list(dyn))'") }}, } env := testMapsEnv(t) for i, tc := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { r := require.New(t) ast, iss := env.Compile(tc.expr) if tc.err != nil { tc.err(t, iss.Err()) return } r.NoError(iss.Err(), "compile failed for expr: %s", tc.expr) prg, err := env.Program(ast) r.NoError(err, "program failed for expr: %s", tc.expr) out, _, err := prg.Eval(cel.NoVars()) if tc.err != nil { r.Error(err, "expected error for expr: %s", tc.expr) r.Contains(err.Error(), tc.err) } else { r.NoError(err, "unexpected error for expr: %s", tc.expr) r.IsType(cel.BoolType, out.Type(), "unexpected type for expr: %s", tc.expr) r.True(out.Value().(bool), "unexpected result for expr: %s", tc.expr) } }) } } func testMapsEnv(t *testing.T, opts ...cel.EnvOption) *cel.Env { t.Helper() env, err := cel.NewEnv(append([]cel.EnvOption{Maps()}, opts...)...) require.NoError(t, err, "cel.NewEnv(Maps()) failed") return env }

Haha, I had started converting the tests to using testify (back when I originally wrote this code, I didn't see that testify was available). Thanks, learnt a few tricks from this (r := require.New(t) f.ex. and usage of ErrorAssertionFunc)

jakobmoellerdev · 2025-08-19T09:32:45Z

pkg/cel/library/maps_test.go

+		{expr: `{'a': 1}.merge({'b': 2}) == {'a': 1, 'b': 2}`},
+		{expr: `{'a': 1}.merge({'a': 2, 'b': 2}) == {'a': 2, 'b': 2}`},
+
+		// {expr: `{}.merge([])`, err: "ERROR: <input>:1:9: found no matching overload for 'merge' applied to 'map(dyn, dyn).(list(dyn))'"},


nit: do we want dynamic support in here? If so, either uncomment the test, add todo for the comment if outstanding, or remove the comments

a-hilaly

Thank you @gfrey !

matthchr

Before we merge this it might be good to check with the cel-go maintainers if they'd just accept this upstream as an ext/maps.go extension.

It doesn't really seem kro specific and I'd rather we:

Depend on shared logic rather than roll our own as much as possible.
Share these sorts of "generally useful CEL helpers" with upstream

If making that happen is going to be difficult or take a lot of time we can just merge here and upstream later, but it feels like we should check first.

a-hilaly · 2025-08-19T17:08:02Z

pinged Joe on upstream slack, let's see what cel-go folks think.

barney-s · 2025-08-19T17:41:45Z

Reached out to CEL team internally this week. They seem to be open. They were expressing concerns about merge semantics (overwrite, replace, combine etc) especially when the values are lists. Will keep working with them and get something part of standard extensions. Meanwhile we can merge this here in KRO or K8s (Joe Betz).

My recommendation would be to indicate experimental in the library name or alpha ?
"cel.lib.ext.kro.maps" -> "cel.lib.ext.kro.experimental.maps" ?

barney-s · 2025-08-19T18:08:46Z

pkg/cel/library/maps.go

+//	{'a': 1}.merge({'b': 2}) == {'a': 1, 'b': 2}
+//	{'a': 1}.merge({'a': 2, 'b': 2}) == {'a': 1, 'b': 2}
+func Maps(options ...MapsOption) cel.EnvOption {
+	l := &mapsLib{version: math.MaxUint32}


Is this common to version it to MAX_INT ?

barney-s · 2025-08-19T18:09:36Z

pkg/cel/library/maps.go

+
+// LibraryName implements the cel.SingletonLibrary interface method.
+func (l *mapsLib) LibraryName() string {
+	return "cel.lib.ext.kro.maps"


is there a way to indicate this is still experimental/alpha.

cel.lib.ext.experimental.maps ?
or
cel.lib.ext.kro.experimental.maps ?

barney-s · 2025-08-19T18:10:50Z

pkg/cel/library/maps.go

+// LibraryName implements the cel.SingletonLibrary interface method.
+func (l *mapsLib) LibraryName() string {
+	return "cel.lib.ext.kro.maps"
+}


Wondering if we need to rename the folder from library to ext ?
Even a separate repo under kro-run ?

barney-s · 2025-08-19T18:19:31Z

pkg/cel/library/maps.go

+
+// CompileOptions implements the cel.Library interface method.
+func (l *mapsLib) CompileOptions() []cel.EnvOption {
+	mapType := cel.MapType(cel.TypeParamType("K"), cel.TypeParamType("V"))


More of a Q: "K" and "V" are not used here. Are those more for documentation ?>
If so can we rename to Key, Value ?

k8s-triage-robot · 2025-09-12T20:17:41Z

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

linux-foundation-easycla · 2025-09-12T20:17:45Z

❌ The email address for the commit (3bb33f5, 1aaecef, 44ece4d, e28f93e, efa762e, 89467b4, 904e210) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

a-hilaly · 2025-09-14T07:22:51Z

ping @gfrey - as Barni stated would you like to open a PR against CEL official std libs?

a-hilaly · 2025-09-14T07:23:02Z

also can you please sign the CLA?

Implement kro CEL library for maps

3bb33f5

Only function right now is merge. Potentially there are more. This implementation is inspired by the CEL lists extension.

gfrey mentioned this pull request Aug 13, 2025

[Question] Merging values #548

Open

matthchr requested review from matthchr, a-hilaly and barney-s and removed request for a-hilaly August 13, 2025 20:06

jakobmoellerdev reviewed Aug 14, 2025

View reviewed changes

a-hilaly reviewed Aug 18, 2025

View reviewed changes

Gereon Frey added 3 commits August 18, 2025 15:08

Remove redundant return value

1aaecef

There will never be an error, so no need to always return nil.

Change order of merge operation

efa762e

The first map's values will be overwritten by values from the second map. This is what merge should do.

Make sure merged map values have same type

e28f93e

This is more restrictive. The other way is supported, but the consequences are not clear. So let's keep it simple.

a-hilaly reviewed Aug 18, 2025

View reviewed changes

jakobmoellerdev reviewed Aug 19, 2025

View reviewed changes

Gereon Frey added 3 commits August 19, 2025 16:04

Add license headers

44ece4d

Simplify merge function

89467b4

Simplify merge support function call and tests

904e210

a-hilaly approved these changes Aug 19, 2025

View reviewed changes

matthchr reviewed Aug 19, 2025

View reviewed changes

barney-s reviewed Aug 19, 2025

View reviewed changes

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 12, 2025

		mapType := cel.MapType(cel.TypeParamType("K"), cel.DynType)
		// mapDynType := cel.MapType(cel.DynType, cel.DynType)

Implement kro CEL library for maps #628

Are you sure you want to change the base?

Implement kro CEL library for maps #628

Uh oh!

Conversation

gfrey commented Aug 13, 2025

Uh oh!

jakobmoellerdev left a comment

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Aug 14, 2025

Uh oh!

gfrey commented Aug 18, 2025

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfrey Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Aug 19, 2025

Uh oh!

jakobmoellerdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobmoellerdev Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

gfrey Aug 18, 2025 •

edited

Loading

a-hilaly Aug 18, 2025 •

edited

Loading

jakobmoellerdev Aug 19, 2025 •

edited

Loading

matthchr left a comment •

edited

Loading