-
Notifications
You must be signed in to change notification settings - Fork 257
Upstreaming network.IP/CIDR to CEL-go from kubernetes #1238
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
base: master
Are you sure you want to change the base?
Conversation
|
/gcbrun |
1 similar comment
|
/gcbrun |
…server implementation
|
/gcbrun |
|
/gcbrun |
|
A note about these changes: To maintain strict AST-level compatibility with k8s, I'm going with a split registration pattern (declaring signatures in CompileOptions and bindings in ProgramOptions), rather than bundling them in CompileOptions via cel.Function. When using the modern cel-go helper cel.Function() to register both the global overload (ip(string)) and the member overload (cidr.ip()) simultaneously, the internal dispatcher validation incorrectly flags the self-referencing overload ID "ip" as a collision (overload already exists). To resolve this collision with the modern helper, we would be forced to rename the overload ID to something distinct like "ip_string". While functionally equivalent, this breaks strict parity with the Kubernetes AST reference data. If requested, we can go this route (and maintain parity minus the "ip" --> "ip_string" difference in overload IDs). |
|
/gcbrun |
Actually, there really isn't any difference here between how the bindings are configured. K8s stages it a little differently, but it's materially identical between the two approaches.
It looks like the function It's worth looking at the other overloads as well. |
|
Thanks Tristan, here's one more pass rechecking overloads and member overloads. |
TristonianJones
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've raised google/cel-spec#507 to cel-spec, so shortly after both artifacts are checked in and a new cel-spec release cut, let's enable the conformance tests
| // cidr('192.168.1.5/24').ip() == ip('192.168.1.5') | ||
| // cidr('192.168.1.5/24').masked() == cidr('192.168.1.0/24') | ||
| // cidr('192.168.1.0/24').prefixLength() == 24 | ||
| func Network(version uint32) cel.EnvOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing version directly, prefer a functional NetworkOption that indicates the NetworkVersion(int), similar to how the BindingsOption is configured:
Lines 69 to 78 in e9f15ea
| // BindingsOption declares a functional operator for configuring the Bindings library behavior. | |
| type BindingsOption func(*celBindings) *celBindings | |
| // BindingsVersion sets the version of the bindings library to an explicit version. | |
| func BindingsVersion(version uint32) BindingsOption { | |
| return func(lib *celBindings) *celBindings { | |
| lib.version = version | |
| return lib | |
| } | |
| } |
Lines 54 to 56 in e9f15ea
| func Bindings(options ...BindingsOption) cel.EnvOption { | |
| b := &celBindings{version: math.MaxUint32} | |
| for _, o := range options { |
| type networkLib struct { | ||
| version uint32 | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comments to all exported (UpperCase) functions
| ), | ||
|
|
||
| // 2. Register Functions | ||
| cel.Function(isIPFunc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit, but could you order these functions by name alphabetically? The K8s files do this and it makes the review diff a bit easier to reason about. Perhaps group by CIDR first, then IP?
| netip.Prefix | ||
| } | ||
|
|
||
| func (c CIDR) ConvertToNative(typeDesc reflect.Type) (any, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add unit tests for conversion functions.
| func (c CIDR) Equal(other ref.Val) ref.Val { | ||
| o, ok := other.(CIDR) | ||
| if !ok { | ||
| return types.ValOrErr(other, "no such overload") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CEL types no longer error when the type is not the same. The current semantic is to treat the runtime equality check as 'false'
| func (i IP) Equal(other ref.Val) ref.Val { | ||
| o, ok := other.(IP) | ||
| if !ok { | ||
| return types.ValOrErr(other, "no such overload") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CEL types should return false if the types aren't the same. This is a shift in behavior from how CEL originally started.
| cel.Function(prefixLengthFunc, | ||
| cel.MemberOverload("cidr_prefix_length", []*cel.Type{CIDRType}, cel.IntType, | ||
| cel.UnaryBinding(netCIDRPrefixLength)), | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional, but consider introducing static validators for ip and cidr conversions from string literals as this will save people a lot of trouble with runtime errors which could have been caught at compile time.
Lines 261 to 279 in e9f15ea
| // Validate searches the AST for uses of a given function name with a constant argument and performs a check | |
| // on whether the argument is a valid literal value. | |
| func (v formatValidator) Validate(e *Env, _ ValidatorConfig, a *ast.AST, iss *Issues) { | |
| root := ast.NavigateAST(a) | |
| funcCalls := ast.MatchDescendants(root, ast.FunctionMatcher(v.funcName)) | |
| for _, call := range funcCalls { | |
| callArgs := call.AsCall().Args() | |
| if len(callArgs) <= v.argNum { | |
| continue | |
| } | |
| litArg := callArgs[v.argNum] | |
| if litArg.Kind() != ast.LiteralKind { | |
| continue | |
| } | |
| if err := v.check(e, call, litArg); err != nil { | |
| iss.ReportErrorAtID(litArg.ID(), "invalid %s argument", v.funcName) | |
| } | |
| } | |
| } |
| } | ||
|
|
||
| const ( | ||
| // Function names matching Kubernetes implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Function names matching Kubernetes implementation | |
| // Function names matching the original Kubernetes implementation of this networking library |
?
| @@ -0,0 +1,526 @@ | |||
| // Copyright 2025 Google LLC | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cost calculations for this library exist in https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/cel/library/cost.go in the Kuberhetes code. Is it possible to also upstream the cost calculations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'll tackle this after the initial implementation is in
Upstream CIDR and IP-related functions from kubernetes into cel-go
This is part of a broader effort to bring network functions from the kubernetes
project into CEL specifications upstream. This is related directly to
issues/1237.
These are currently locked inside k8s.io/apiserver, but they are generally
useful for any policy engine dealing with network logic (firewalls, access lists, etc.).