Move CEL semver library into common libs, fix cost tests to use registered types

jpbetz · jpbetz · commit e085f3818a3a · 2024-09-10T16:55:57.000-04:00
diff --git a/staging/src/k8s.io/apiserver/go.mod b/staging/src/k8s.io/apiserver/go.mod
@@ -6,6 +6,7 @@ go 1.22.0
 
 require (
 	github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a
+	github.com/blang/semver/v4 v4.0.0
 	github.com/coreos/go-oidc v2.2.1+incompatible
 	github.com/coreos/go-systemd/v22 v22.5.0
 	github.com/emicklei/go-restful/v3 v3.11.0
@@ -63,7 +64,6 @@ require (
 	github.com/NYTimes/gziphandler v1.1.1 // indirect
 	github.com/antlr4-go/antlr/v4 v4.13.0 // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
-	github.com/blang/semver/v4 v4.0.0 // indirect
 	github.com/cenkalti/backoff/v4 v4.3.0 // indirect
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/coreos/go-semver v0.3.1 // indirect
diff --git a/staging/src/k8s.io/apiserver/pkg/cel/format.go b/staging/src/k8s.io/apiserver/pkg/cel/format.go
@@ -41,11 +41,11 @@ type Format struct {
 	MaxRegexSize int
 }
 
-func (d *Format) ConvertToNative(typeDesc reflect.Type) (interface{}, error) {
+func (d Format) ConvertToNative(typeDesc reflect.Type) (interface{}, error) {
 	return nil, fmt.Errorf("type conversion error from 'Format' to '%v'", typeDesc)
 }
 
-func (d *Format) ConvertToType(typeVal ref.Type) ref.Val {
+func (d Format) ConvertToType(typeVal ref.Type) ref.Val {
 	switch typeVal {
 	case FormatType:
 		return d
@@ -56,18 +56,18 @@ func (d *Format) ConvertToType(typeVal ref.Type) ref.Val {
 	}
 }
 
-func (d *Format) Equal(other ref.Val) ref.Val {
-	otherDur, ok := other.(*Format)
+func (d Format) Equal(other ref.Val) ref.Val {
+	otherDur, ok := other.(Format)
 	if !ok {
 		return types.MaybeNoSuchOverloadErr(other)
 	}
 	return types.Bool(d.Name == otherDur.Name)
 }
 
-func (d *Format) Type() ref.Type {
+func (d Format) Type() ref.Type {
 	return FormatType
 }
 
-func (d *Format) Value() interface{} {
+func (d Format) Value() interface{} {
 	return d
 }
diff --git a/staging/src/k8s.io/apiserver/pkg/cel/library/cidr.go b/staging/src/k8s.io/apiserver/pkg/cel/library/cidr.go
@@ -109,7 +109,7 @@ var cidrsLib = &cidrs{}
 type cidrs struct{}
 
 func (*cidrs) LibraryName() string {
-	return "net.cidr"
+	return "kubernetes.net.cidr"
 }
 
 func (*cidrs) declarations() map[string][]cel.FunctionOpt {
diff --git a/staging/src/k8s.io/apiserver/pkg/cel/library/cost.go b/staging/src/k8s.io/apiserver/pkg/cel/library/cost.go
@@ -25,7 +25,6 @@ import (
 	"github.com/google/cel-go/common/types/ref"
 	"github.com/google/cel-go/common/types/traits"
 	"math"
-	"strings"
 
 	"k8s.io/apiserver/pkg/cel"
 )
@@ -201,7 +200,7 @@ func (l *CostEstimator) CallCost(function, overloadId string, args []ref.Val, re
 		}
 	case "validate":
 		if len(args) >= 2 {
-			format, isFormat := args[0].Value().(*cel.Format)
+			format, isFormat := args[0].Value().(cel.Format)
 			if isFormat {
 				strSize := actualSize(args[1])
 
@@ -243,13 +242,14 @@ func (l *CostEstimator) CallCost(function, overloadId string, args []ref.Val, re
 			case *cel.Quantity, cel.Quantity,
 				*cel.IP, cel.IP,
 				*cel.CIDR, cel.CIDR,
-				*cel.Format,       // Formats have a small max size. Format takes pointer receiver.
+				*cel.Format, cel.Format, // Formats have a small max size. Format takes pointer receiver.
 				*cel.URL, cel.URL, // TODO: Computing the actual cost is expensive, and changing this would be a breaking change
+				*cel.Semver, cel.Semver,
 				*authorizerVal, authorizerVal, *pathCheckVal, pathCheckVal, *groupCheckVal, groupCheckVal,
 				*resourceCheckVal, resourceCheckVal, *decisionVal, decisionVal:
 				return &unitCost
 			default:
-				if panicOnUnknown && isKubernetesType(lhs.Type()) {
+				if panicOnUnknown && lhs.Type() != nil && isRegisteredType(lhs.Type().TypeName()) {
 					panic(fmt.Errorf("CallCost: unhandled equality for Kubernetes type %T", lhs))
 				}
 			}
@@ -509,7 +509,7 @@ func (l *CostEstimator) EstimateCallCost(function, overloadId string, target *ch
 				if t.Kind() == types.StructKind {
 					switch t {
 					case cel.QuantityType, AuthorizerType, PathCheckType, // O(1) cost equality checks
-						GroupCheckType, ResourceCheckType, DecisionType:
+						GroupCheckType, ResourceCheckType, DecisionType, cel.SemverType:
 						return &checker.CallEstimate{CostEstimate: checker.CostEstimate{Min: 1, Max: 1}}
 					case cel.FormatType:
 						return &checker.CallEstimate{CostEstimate: checker.CostEstimate{Min: 1, Max: cel.MaxFormatSize}.MultiplyByCostFactor(common.StringTraversalCostFactor)}
@@ -523,7 +523,7 @@ func (l *CostEstimator) EstimateCallCost(function, overloadId string, target *ch
 						return &checker.CallEstimate{CostEstimate: checker.CostEstimate{Min: 1, Max: size.Max}.MultiplyByCostFactor(common.StringTraversalCostFactor)}
 					}
 				}
-				if panicOnUnknown && isKubernetesType(t) {
+				if panicOnUnknown && isRegisteredType(t.TypeName()) {
 					panic(fmt.Errorf("EstimateCallCost: unhandled equality for Kubernetes type %v", t))
 				}
 			}
@@ -632,17 +632,3 @@ func traversalCost(v ref.Val) uint64 {
 		return 1
 	}
 }
-
-// isKubernetesType returns ture if a  type is type defined by Kubernetes,
-// as identified by opaque or struct types with a "kubernetes." prefix.
-func isKubernetesType(t ref.Type) bool {
-	if tt, ok := t.(*types.Type); ok {
-		switch tt.Kind() {
-		case types.OpaqueKind, types.StructKind:
-			return strings.HasPrefix(tt.TypeName(), "kubernetes.")
-		default:
-			return false
-		}
-	}
-	return false
-}
diff --git a/staging/src/k8s.io/apiserver/pkg/cel/library/cost_test.go b/staging/src/k8s.io/apiserver/pkg/cel/library/cost_test.go
@@ -1262,7 +1262,8 @@ func TestTypeEquality(t *testing.T) {
 		"kubernetes.Quantity":                    apiservercel.Quantity{},
 		"net.IP":                                 apiservercel.IP{},
 		"net.CIDR":                               apiservercel.CIDR{},
-		"kubernetes.NamedFormat":                 &apiservercel.Format{},
+		"kubernetes.NamedFormat":                 apiservercel.Format{},
+		"kubernetes.Semver":                      apiservercel.Semver{},
 	}
 
 	originalPanicOnUnknown := panicOnUnknown
diff --git a/staging/src/k8s.io/apiserver/pkg/cel/library/format.go b/staging/src/k8s.io/apiserver/pkg/cel/library/format.go
@@ -133,7 +133,7 @@ func (*format) ProgramOptions() []cel.ProgramOption {
 	return []cel.ProgramOption{}
 }
 
-var ConstantFormats map[string]*apiservercel.Format = map[string]*apiservercel.Format{
+var ConstantFormats = map[string]apiservercel.Format{
 	"dns1123Label": {
 		Name:         "DNS1123Label",
 		ValidateFunc: func(s string) []string { return apimachineryvalidation.NameIsDNSLabel(s, false) },
@@ -261,7 +261,7 @@ var formatLibraryDecls = map[string][]cel.FunctionOpt{
 }
 
 func formatValidate(arg1, arg2 ref.Val) ref.Val {
-	f, ok := arg1.Value().(*apiservercel.Format)
+	f, ok := arg1.Value().(apiservercel.Format)
 	if !ok {
 		return types.MaybeNoSuchOverloadErr(arg1)
 	}
diff --git a/staging/src/k8s.io/apiserver/pkg/cel/library/ip.go b/staging/src/k8s.io/apiserver/pkg/cel/library/ip.go
@@ -132,7 +132,7 @@ var ipLib = &ip{}
 type ip struct{}
 
 func (*ip) LibraryName() string {
-	return "net.ip"
+	return "kubernetes.net.ip"
 }
 
 func (*ip) declarations() map[string][]cel.FunctionOpt {
diff --git a/staging/src/k8s.io/apiserver/pkg/cel/library/libraries.go b/staging/src/k8s.io/apiserver/pkg/cel/library/libraries.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2023 The Kubernetes Authors.
+Copyright 2024 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -16,7 +16,9 @@ limitations under the License.
 
 package library
 
-import "github.com/google/cel-go/cel"
+import (
+	"github.com/google/cel-go/cel"
+)
 
 // Library represents a CEL library used by kubernetes.
 type Library interface {
@@ -42,5 +44,17 @@ func KnownLibraries() []Library {
 		ipLib,
 		cidrsLib,
 		formatLib,
+		semverLib,
 	}
 }
+
+func isRegisteredType(typeName string) bool {
+	for _, lib := range KnownLibraries() {
+		for _, rt := range lib.Types() {
+			if rt.TypeName() == typeName {
+				return true
+			}
+		}
+	}
+	return false
+}
diff --git a/staging/src/k8s.io/apiserver/pkg/cel/library/library_compatibility_test.go b/staging/src/k8s.io/apiserver/pkg/cel/library/library_compatibility_test.go
@@ -49,7 +49,7 @@ func TestLibraryCompatibility(t *testing.T) {
 		// Kubernetes <1.30>:
 		"ip", "family", "isUnspecified", "isLoopback", "isLinkLocalMulticast", "isLinkLocalUnicast", "isGlobalUnicast", "ip.isCanonical", "isIP", "cidr", "containsIP", "containsCIDR", "masked", "prefixLength", "isCIDR", "string",
 		// Kubernetes <1.31>:
-		"fieldSelector", "labelSelector", "validate", "format.named",
+		"fieldSelector", "labelSelector", "validate", "format.named", "isSemver", "major", "minor", "patch", "semver",
 		// Kubernetes <1.??>:
 	)
 
@@ -101,5 +101,4 @@ func TestTypeRegistration(t *testing.T) {
 			t.Errorf("Expected types to be registered with the %s library Type() functions, but they were not: %v", lib.LibraryName(), unregistered)
 		}
 	}
-
 }
diff --git a/staging/src/k8s.io/apiserver/pkg/cel/library/semver_test.go b/staging/src/k8s.io/apiserver/pkg/cel/library/semver_test.go
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package cel_test
+package library_test
 
 import (
 	"regexp"
@@ -27,7 +27,8 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"k8s.io/apimachinery/pkg/util/sets"
-	library "k8s.io/dynamic-resource-allocation/cel"
+	apiservercel "k8s.io/apiserver/pkg/cel"
+	library "k8s.io/apiserver/pkg/cel/library"
 )
 
 func testSemver(t *testing.T, expr string, expectResult ref.Val, expectRuntimeErrPattern string, expectCompileErrs []string) {
@@ -117,7 +118,7 @@ func TestSemver(t *testing.T) {
 		{
 			name:        "parse",
 			expr:        `semver("1.2.3")`,
-			expectValue: library.Semver{Version: semver.MustParse("1.2.3")},
+			expectValue: apiservercel.Semver{Version: semver.MustParse("1.2.3")},
 		},
 		{
 			name:               "parseInvalidVersion",
diff --git a/staging/src/k8s.io/apiserver/pkg/cel/library/semverlib.go b/staging/src/k8s.io/apiserver/pkg/cel/library/semverlib.go
@@ -14,13 +14,15 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package cel
+package library
 
 import (
 	"github.com/blang/semver/v4"
 	"github.com/google/cel-go/cel"
 	"github.com/google/cel-go/common/types"
 	"github.com/google/cel-go/common/types/ref"
+
+	apiservercel "k8s.io/apiserver/pkg/cel"
 )
 
 // Semver provides a CEL function library extension for [semver.Version].
@@ -91,38 +93,45 @@ var semverLib = &semverLibType{}
 type semverLibType struct{}
 
 func (*semverLibType) LibraryName() string {
-	return "k8s.semver"
+	return "kubernetes.Semver"
 }
 
-func (*semverLibType) CompileOptions() []cel.EnvOption {
-	// Defined in this function to avoid an initialization order problem.
-	semverLibraryDecls := map[string][]cel.FunctionOpt{
+func (*semverLibType) Types() []*cel.Type {
+	return []*cel.Type{apiservercel.SemverType}
+}
+
+func (*semverLibType) declarations() map[string][]cel.FunctionOpt {
+	return map[string][]cel.FunctionOpt{
 		"semver": {
-			cel.Overload("string_to_semver", []*cel.Type{cel.StringType}, SemverType, cel.UnaryBinding((stringToSemver))),
+			cel.Overload("string_to_semver", []*cel.Type{cel.StringType}, apiservercel.SemverType, cel.UnaryBinding((stringToSemver))),
 		},
 		"isSemver": {
 			cel.Overload("is_semver_string", []*cel.Type{cel.StringType}, cel.BoolType, cel.UnaryBinding(isSemver)),
 		},
 		"isGreaterThan": {
-			cel.MemberOverload("semver_is_greater_than", []*cel.Type{SemverType, SemverType}, cel.BoolType, cel.BinaryBinding(semverIsGreaterThan)),
+			cel.MemberOverload("semver_is_greater_than", []*cel.Type{apiservercel.SemverType, apiservercel.SemverType}, cel.BoolType, cel.BinaryBinding(semverIsGreaterThan)),
 		},
 		"isLessThan": {
-			cel.MemberOverload("semver_is_less_than", []*cel.Type{SemverType, SemverType}, cel.BoolType, cel.BinaryBinding(semverIsLessThan)),
+			cel.MemberOverload("semver_is_less_than", []*cel.Type{apiservercel.SemverType, apiservercel.SemverType}, cel.BoolType, cel.BinaryBinding(semverIsLessThan)),
 		},
 		"compareTo": {
-			cel.MemberOverload("semver_compare_to", []*cel.Type{SemverType, SemverType}, cel.IntType, cel.BinaryBinding(semverCompareTo)),
+			cel.MemberOverload("semver_compare_to", []*cel.Type{apiservercel.SemverType, apiservercel.SemverType}, cel.IntType, cel.BinaryBinding(semverCompareTo)),
 		},
 		"major": {
-			cel.MemberOverload("semver_major", []*cel.Type{SemverType}, cel.IntType, cel.UnaryBinding(semverMajor)),
+			cel.MemberOverload("semver_major", []*cel.Type{apiservercel.SemverType}, cel.IntType, cel.UnaryBinding(semverMajor)),
 		},
 		"minor": {
-			cel.MemberOverload("semver_minor", []*cel.Type{SemverType}, cel.IntType, cel.UnaryBinding(semverMinor)),
+			cel.MemberOverload("semver_minor", []*cel.Type{apiservercel.SemverType}, cel.IntType, cel.UnaryBinding(semverMinor)),
 		},
 		"patch": {
-			cel.MemberOverload("semver_patch", []*cel.Type{SemverType}, cel.IntType, cel.UnaryBinding(semverPatch)),
+			cel.MemberOverload("semver_patch", []*cel.Type{apiservercel.SemverType}, cel.IntType, cel.UnaryBinding(semverPatch)),
 		},
 	}
+}
 
+func (s *semverLibType) CompileOptions() []cel.EnvOption {
+	// Defined in this function to avoid an initialization order problem.
+	semverLibraryDecls := s.declarations()
 	options := make([]cel.EnvOption, 0, len(semverLibraryDecls))
 	for name, overloads := range semverLibraryDecls {
 		options = append(options, cel.Function(name, overloads...))
@@ -168,7 +177,7 @@ func stringToSemver(arg ref.Val) ref.Val {
 		return types.WrapErr(err)
 	}
 
-	return Semver{Version: v}
+	return apiservercel.Semver{Version: v}
 }
 
 func semverMajor(arg ref.Val) ref.Val {
diff --git a/staging/src/k8s.io/apiserver/pkg/cel/semver.go b/staging/src/k8s.io/apiserver/pkg/cel/semver.go
diff --git a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go
@@ -37,6 +37,7 @@ import (
 	celconfig "k8s.io/apiserver/pkg/apis/cel"
 	apiservercel "k8s.io/apiserver/pkg/cel"
 	"k8s.io/apiserver/pkg/cel/environment"
+	"k8s.io/apiserver/pkg/cel/library"
 )
 
 const (
@@ -151,7 +152,7 @@ func getAttributeValue(attr resourceapi.DeviceAttribute) (any, error) {
 		if err != nil {
 			return nil, fmt.Errorf("parse semantic version: %w", err)
 		}
-		return Semver{Version: v}, nil
+		return apiservercel.Semver{Version: v}, nil
 	default:
 		return nil, errors.New("unsupported attribute value")
 	}
@@ -236,7 +237,7 @@ func mustBuildEnv() *environment.EnvSet {
 			EnvOptions: []cel.EnvOption{
 				cel.Variable(deviceVar, deviceType.CelType()),
 
-				SemverLib(),
+				library.SemverLib(),
 
 				// https://pkg.go.dev/github.com/google/cel-go/ext#Bindings
 				//

Original file line number	Diff line number	Diff line change
`@@ -41,11 +41,11 @@ type Format struct {`
`41`	`41`	`MaxRegexSize int`
`42`	`42`	`}`
`43`	`43`
`44`		`-func (d *Format) ConvertToNative(typeDesc reflect.Type) (interface{}, error) {`
	`44`	`+func (d Format) ConvertToNative(typeDesc reflect.Type) (interface{}, error) {`
`45`	`45`	`return nil, fmt.Errorf("type conversion error from 'Format' to '%v'", typeDesc)`
`46`	`46`	`}`
`47`	`47`
`48`		`-func (d *Format) ConvertToType(typeVal ref.Type) ref.Val {`
	`48`	`+func (d Format) ConvertToType(typeVal ref.Type) ref.Val {`
`49`	`49`	`switch typeVal {`
`50`	`50`	`case FormatType:`
`51`	`51`	`return d`
`@@ -56,18 +56,18 @@ func (d *Format) ConvertToType(typeVal ref.Type) ref.Val {`
`56`	`56`	`}`
`57`	`57`	`}`
`58`	`58`
`59`		`-func (d *Format) Equal(other ref.Val) ref.Val {`
`60`		`- otherDur, ok := other.(*Format)`
	`59`	`+func (d Format) Equal(other ref.Val) ref.Val {`
	`60`	`+ otherDur, ok := other.(Format)`
`61`	`61`	`if !ok {`
`62`	`62`	`return types.MaybeNoSuchOverloadErr(other)`
`63`	`63`	`}`
`64`	`64`	`return types.Bool(d.Name == otherDur.Name)`
`65`	`65`	`}`
`66`	`66`
`67`		`-func (d *Format) Type() ref.Type {`
	`67`	`+func (d Format) Type() ref.Type {`
`68`	`68`	`return FormatType`
`69`	`69`	`}`
`70`	`70`
`71`		`-func (d *Format) Value() interface{} {`
	`71`	`+func (d Format) Value() interface{} {`
`72`	`72`	`return d`
`73`	`73`	`}`
Original file line number	Diff line number	Diff line change
`@@ -109,7 +109,7 @@ var cidrsLib = &cidrs{}`
`109`	`109`	`type cidrs struct{}`
`110`	`110`
`111`	`111`	`func (*cidrs) LibraryName() string {`
`112`		`- return "net.cidr"`
	`112`	`+ return "kubernetes.net.cidr"`
`113`	`113`	`}`
`114`	`114`
`115`	`115`	`func (*cidrs) declarations() map[string][]cel.FunctionOpt {`
Original file line number	Diff line number	Diff line change
`@@ -1262,7 +1262,8 @@ func TestTypeEquality(t *testing.T) {`
`1262`	`1262`	`"kubernetes.Quantity": apiservercel.Quantity{},`
`1263`	`1263`	`"net.IP": apiservercel.IP{},`
`1264`	`1264`	`"net.CIDR": apiservercel.CIDR{},`
`1265`		`- "kubernetes.NamedFormat": &apiservercel.Format{},`
	`1265`	`+ "kubernetes.NamedFormat": apiservercel.Format{},`
	`1266`	`+ "kubernetes.Semver": apiservercel.Semver{},`
`1266`	`1267`	`}`
`1267`	`1268`
`1268`	`1269`	`originalPanicOnUnknown := panicOnUnknown`
Original file line number	Diff line number	Diff line change
`@@ -133,7 +133,7 @@ func (*format) ProgramOptions() []cel.ProgramOption {`
`133`	`133`	`return []cel.ProgramOption{}`
`134`	`134`	`}`
`135`	`135`
`136`		`-var ConstantFormats map[string]apiservercel.Format = map[string]apiservercel.Format{`
	`136`	`+var ConstantFormats = map[string]apiservercel.Format{`
`137`	`137`	`"dns1123Label": {`
`138`	`138`	`Name: "DNS1123Label",`
`139`	`139`	`ValidateFunc: func(s string) []string { return apimachineryvalidation.NameIsDNSLabel(s, false) },`
`@@ -261,7 +261,7 @@ var formatLibraryDecls = map[string][]cel.FunctionOpt{`
`261`	`261`	`}`
`262`	`262`
`263`	`263`	`func formatValidate(arg1, arg2 ref.Val) ref.Val {`
`264`		`- f, ok := arg1.Value().(*apiservercel.Format)`
	`264`	`+ f, ok := arg1.Value().(apiservercel.Format)`
`265`	`265`	`if !ok {`
`266`	`266`	`return types.MaybeNoSuchOverloadErr(arg1)`
`267`	`267`	`}`
Original file line number	Diff line number	Diff line change
`@@ -132,7 +132,7 @@ var ipLib = &ip{}`
`132`	`132`	`type ip struct{}`
`133`	`133`
`134`	`134`	`func (*ip) LibraryName() string {`
`135`		`- return "net.ip"`
	`135`	`+ return "kubernetes.net.ip"`
`136`	`136`	`}`
`137`	`137`
`138`	`138`	`func (*ip) declarations() map[string][]cel.FunctionOpt {`
Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,7 @@ func TestLibraryCompatibility(t *testing.T) {`
`49`	`49`	`// Kubernetes <1.30>:`
`50`	`50`	`"ip", "family", "isUnspecified", "isLoopback", "isLinkLocalMulticast", "isLinkLocalUnicast", "isGlobalUnicast", "ip.isCanonical", "isIP", "cidr", "containsIP", "containsCIDR", "masked", "prefixLength", "isCIDR", "string",`
`51`	`51`	`// Kubernetes <1.31>:`
`52`		`- "fieldSelector", "labelSelector", "validate", "format.named",`
	`52`	`+ "fieldSelector", "labelSelector", "validate", "format.named", "isSemver", "major", "minor", "patch", "semver",`
`53`	`53`	`// Kubernetes <1.??>:`
`54`	`54`	`)`
`55`	`55`
`@@ -101,5 +101,4 @@ func TestTypeRegistration(t *testing.T) {`
`101`	`101`	`t.Errorf("Expected types to be registered with the %s library Type() functions, but they were not: %v", lib.LibraryName(), unregistered)`
`102`	`102`	`}`
`103`	`103`	`}`
`104`		`-`
`105`	`104`	`}`