From 3fb14cf4e7a0230d57f579b86262d9df6997e5e3 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Tue, 22 Aug 2023 13:05:09 -0400 Subject: [PATCH] Bump cel string lib to v2, add tests --- .../apiserver/schema/cel/validation_test.go | 8 ++++--- .../apiserver/pkg/cel/environment/base.go | 17 ++++++++++++--- .../src/k8s.io/apiserver/pkg/cel/lazy/lazy.go | 2 +- .../k8s.io/apiserver/pkg/cel/library/cost.go | 2 +- .../apiserver/pkg/cel/library/cost_test.go | 21 ++++++++++--------- .../pkg/cel/library/quantity_test.go | 6 +++--- 6 files changed, 35 insertions(+), 21 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go index 6a63a64ce16..899ef4e97e0 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go @@ -259,13 +259,15 @@ func TestValidationExpressions(t *testing.T) { "self.val1.substring(4, 10).trim() == 'takes'", "self.val1.upperAscii() == 'ROOK TAKES 👑'", "self.val1.lowerAscii() == 'rook takes 👑'", + + "'%d %s %f %s %s'.format([1, 'abc', 1.0, duration('1m'), timestamp('2000-01-01T00:00:00.000Z')]) == '1 abc 1.000000 60s 2000-01-01T00:00:00Z'", + "'%e'.format([3.14]) == '3.140000 × 10⁰⁰'", + "'%o %o %o'.format([7, 8, 9]) == '7 10 11'", + "'%b %b %b'.format([7, 8, 9]) == '111 1000 1001'", }, errors: map[string]string{ // Invalid regex with a string constant regex pattern is compile time error "self.val1.matches(')')": "compile error: program instantiation failed: error parsing regexp: unexpected ): `)`", - // strings version 0 does not have format or join - // TODO: Replace this error test with valid tests when the string version is bumped. - "'%s %i'.format('a', 1) == 'a 1'": "undeclared reference to 'format'", }, }, {name: "escaped strings", diff --git a/staging/src/k8s.io/apiserver/pkg/cel/environment/base.go b/staging/src/k8s.io/apiserver/pkg/cel/environment/base.go index ed0d3404116..bf3144ab448 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/environment/base.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/environment/base.go @@ -41,7 +41,7 @@ import ( // desirable because it means that CEL expressions are portable across a wider range // of Kubernetes versions. func DefaultCompatibilityVersion() *version.Version { - return version.MajorMinor(1, 27) + return version.MajorMinor(1, 28) } var baseOpts = []VersionedOptions{ @@ -57,7 +57,6 @@ var baseOpts = []VersionedOptions{ cel.EagerlyValidateDeclarations(true), cel.DefaultUTCTimeZone(true), - ext.Strings(ext.StringsVersion(0)), library.URLs(), library.Regex(), library.Lists(), @@ -67,6 +66,13 @@ var baseOpts = []VersionedOptions{ cel.CostLimit(celconfig.PerCallLimit), }, }, + { + IntroducedVersion: version.MajorMinor(1, 0), + RemovedVersion: version.MajorMinor(1, 29), + EnvOptions: []cel.EnvOption{ + ext.Strings(ext.StringsVersion(0)), + }, + }, { IntroducedVersion: version.MajorMinor(1, 27), EnvOptions: []cel.EnvOption{ @@ -81,7 +87,12 @@ var baseOpts = []VersionedOptions{ library.Quantity(), }, }, - // TODO: switch to ext.Strings version 2 once format() is fixed to work with HomogeneousAggregateLiterals. + { + IntroducedVersion: version.MajorMinor(1, 29), + EnvOptions: []cel.EnvOption{ + ext.Strings(ext.StringsVersion(2)), + }, + }, } // MustBaseEnvSet returns the common CEL base environments for Kubernetes for Version, or panics diff --git a/staging/src/k8s.io/apiserver/pkg/cel/lazy/lazy.go b/staging/src/k8s.io/apiserver/pkg/cel/lazy/lazy.go index 1742deb0a2f..16183050d9b 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/lazy/lazy.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/lazy/lazy.go @@ -35,7 +35,7 @@ var _ traits.Mapper = (*MapValue)(nil) // MapValue is a map that lazily evaluate its value when a field is first accessed. // The map value is not designed to be thread-safe. type MapValue struct { - typeValue *types.TypeValue + typeValue *types.Type // values are previously evaluated values obtained from callbacks values map[string]ref.Val diff --git a/staging/src/k8s.io/apiserver/pkg/cel/library/cost.go b/staging/src/k8s.io/apiserver/pkg/cel/library/cost.go index c3cacf155af..d18c138ec8f 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/library/cost.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/library/cost.go @@ -102,7 +102,7 @@ func (l *CostEstimator) EstimateCallCost(function, overloadId string, target *ch // of estimating the additional comparison cost. if elNode := l.listElementNode(*target); elNode != nil { k := elNode.Type().Kind() - if k == types.StructKind || k == types.BytesKind { + if k == types.StringKind || k == types.BytesKind { sz := l.sizeEstimate(elNode) elCost = elCost.Add(sz.MultiplyByCostFactor(common.StringTraversalCostFactor)) } 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 index 006e2719d14..fbe7511fcf1 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/library/cost_test.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/library/cost_test.go @@ -24,7 +24,7 @@ import ( "github.com/google/cel-go/cel" "github.com/google/cel-go/checker" "github.com/google/cel-go/ext" - expr "google.golang.org/genproto/googleapis/api/expr/v1alpha1" + exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1" "k8s.io/apiserver/pkg/authorization/authorizer" ) @@ -411,7 +411,7 @@ func TestAuthzLibrary(t *testing.T) { func testCost(t *testing.T, expr string, expectEsimatedCost checker.CostEstimate, expectRuntimeCost uint64) { est := &CostEstimator{SizeEstimator: &testCostEstimator{}} env, err := cel.NewEnv( - ext.Strings(), + ext.Strings(ext.StringsVersion(2)), URLs(), Regex(), Lists(), @@ -554,14 +554,15 @@ type testCostEstimator struct { } func (t *testCostEstimator) EstimateSize(element checker.AstNode) *checker.SizeEstimate { - switch t := element.Type().TypeKind.(type) { - case *expr.Type_Primitive: - switch t.Primitive { - case expr.Type_STRING: - return &checker.SizeEstimate{Min: 0, Max: 12} - case expr.Type_BYTES: - return &checker.SizeEstimate{Min: 0, Max: 12} - } + expr, err := cel.TypeToExprType(element.Type()) + if err != nil { + return nil + } + switch expr.GetPrimitive() { + case exprpb.Type_STRING: + return &checker.SizeEstimate{Min: 0, Max: 12} + case exprpb.Type_BYTES: + return &checker.SizeEstimate{Min: 0, Max: 12} } return nil } diff --git a/staging/src/k8s.io/apiserver/pkg/cel/library/quantity_test.go b/staging/src/k8s.io/apiserver/pkg/cel/library/quantity_test.go index 3eda894468b..d42a6ecb879 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/library/quantity_test.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/library/quantity_test.go @@ -21,11 +21,11 @@ import ( "testing" "github.com/google/cel-go/cel" - "github.com/google/cel-go/common" "github.com/google/cel-go/common/types" "github.com/google/cel-go/common/types/ref" "github.com/google/cel-go/ext" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/util/sets" apiservercel "k8s.io/apiserver/pkg/cel" @@ -66,10 +66,10 @@ func testQuantity(t *testing.T, expr string, expectResult ref.Val, expectRuntime if !didMatch { missingCompileErrs = append(missingCompileErrs, expectedCompileErr) } else if len(matchedCompileErrs) != len(issues.Errors()) { - unmatchedErrs := []common.Error{} + unmatchedErrs := []cel.Error{} for i, issue := range issues.Errors() { if !matchedCompileErrs.Has(i) { - unmatchedErrs = append(unmatchedErrs, issue) + unmatchedErrs = append(unmatchedErrs, *issue) } } require.Empty(t, unmatchedErrs, "unexpected compilation errors")