From 68901de8981a0908d7cfdba0d9c19aeb50385f35 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Fri, 26 May 2023 20:04:35 -0400 Subject: [PATCH] Enable optionals and add tests --- .../src/k8s.io/apiextensions-apiserver/go.sum | 1 + .../schema/cel/celcoststability_test.go | 35 +++++++ .../apiserver/schema/cel/validation_test.go | 91 ++++++++++++++++++- staging/src/k8s.io/apiserver/go.sum | 1 + .../apiserver/pkg/cel/environment/base.go | 3 +- .../cel/validatingadmissionpolicy_test.go | 2 +- 6 files changed, 128 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/go.sum b/staging/src/k8s.io/apiextensions-apiserver/go.sum index 23b2f716e9c..4cecd10b157 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/go.sum +++ b/staging/src/k8s.io/apiextensions-apiserver/go.sum @@ -13,6 +13,7 @@ cloud.google.com/go v0.56.0/go.mod h1:jr7tqZxxKOVYizybht9+26Z/gUq7tiRzu+ACVAMbKV cloud.google.com/go v0.57.0/go.mod h1:oXiQ6Rzq3RAkkY7N6t3TcE6jE+CIBBbA36lwQ1JyzZs= cloud.google.com/go v0.62.0/go.mod h1:jmCYTdRCQuc1PHIIJ/maLInMho30T/Y0M4hTdTShOYc= cloud.google.com/go v0.65.0/go.mod h1:O5N8zS7uWy9vkA9vayVHs65eM1ubvY4h553ofrNHObY= +cloud.google.com/go v0.110.0 h1:Zc8gqp3+a9/Eyph2KDmcGaPtbKRIoqq4YTlL4NMD0Ys= cloud.google.com/go/bigquery v1.0.1/go.mod h1:i/xbL2UlR5RvWAURpBYZTtm/cXjCha9lbfbpx4poX+o= cloud.google.com/go/bigquery v1.3.0/go.mod h1:PjpwJnslEMmckchkHFfq+HTD2DmtT67aNFKH1/VBDHE= cloud.google.com/go/bigquery v1.4.0/go.mod h1:S8dzgnTigyfTmLBfrtrhyYhwRxG72rYxvftPBK2Dvzc= diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/celcoststability_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/celcoststability_test.go index 656f8898a66..ab614bcd390 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/celcoststability_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/celcoststability_test.go @@ -1081,6 +1081,41 @@ func TestCelCostStability(t *testing.T) { "self.listOfListMap[0].exists(e, e.k3 == '3' && e.v3 == 'i')": 14, }, }, + {name: "optionals", + obj: map[string]interface{}{ + "obj": map[string]interface{}{ + "field": "a", + }, + "m": map[string]interface{}{ + "k": "v", + }, + "l": []interface{}{ + "a", + }, + }, + schema: objectTypePtr(map[string]schema.Structural{ + "obj": objectType(map[string]schema.Structural{ + "field": stringType, + "absentField": stringType, + }), + "m": mapType(&stringType), + "l": listType(&stringType), + }), + expectCost: map[string]int64{ + "optional.of('a') != optional.of('b')": 3, + "optional.of('a') != optional.none()": 3, + "optional.of('a').hasValue()": 2, + "optional.of('a').or(optional.of('a')).hasValue()": 2, // or() is short-circuited + "optional.none().or(optional.of('a')).hasValue()": 3, + "optional.of('a').optMap(v, v == 'value').hasValue()": 8, + "self.obj.?field == optional.of('a')": 5, + "self.obj.?absentField == optional.none()": 4, + "self.obj.?field.orValue('v') == 'a'": 4, + "self.m[?'k'] == optional.of('v')": 5, + "self.l[?0] == optional.of('a')": 5, + "optional.ofNonZeroValue(1).hasValue()": 2, + }, + }, } for _, tt := range cases { 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 c399584796f..6d01782881a 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 @@ -263,6 +263,9 @@ func TestValidationExpressions(t *testing.T) { 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", @@ -1842,6 +1845,88 @@ func TestValidationExpressions(t *testing.T) { "authorizer.path('/healthz').check('get').allowed()": "undeclared reference to 'authorizer'", }, }, + {name: "optionals", // https://github.com/google/cel-spec/wiki/proposal-246 + obj: map[string]interface{}{ + "presentObj": map[string]interface{}{ + "presentStr": "value", + }, + "m": map[string]interface{}{"k": "v"}, + "l": []interface{}{"a"}, + }, + schema: objectTypePtr(map[string]schema.Structural{ + "presentObj": objectType(map[string]schema.Structural{ + "presentStr": stringType, + }), + "absentObj": objectType(map[string]schema.Structural{ + "absentStr": stringType, + }), + "m": mapType(&stringType), + "l": listType(&stringType), + }), + valid: []string{ + "self.?presentObj.?presentStr == optional.of('value')", + "self.presentObj.?presentStr == optional.of('value')", + "self.presentObj.?presentStr.or(optional.of('nope')) == optional.of('value')", + "self.presentObj.?presentStr.orValue('') == 'value'", + "self.presentObj.?presentStr.hasValue() == true", + "self.presentObj.?presentStr.optMap(v, v == 'value').hasValue()", + "self.?absentObj.?absentStr == optional.none()", + "self.?absentObj.?absentStr.or(optional.of('nope')) == optional.of('nope')", + "self.?absentObj.?absentStr.orValue('nope') == 'nope'", + "self.?absentObj.?absentStr.hasValue() == false", + "self.?absentObj.?absentStr.optMap(v, v == 'value').hasValue() == false", + + "self.m[?'k'] == optional.of('v')", + "self.m[?'k'].or(optional.of('nope')) == optional.of('v')", + "self.m[?'k'].orValue('') == 'v'", + "self.m[?'k'].hasValue() == true", + "self.m[?'k'].optMap(v, v == 'v').hasValue()", + "self.m[?'x'] == optional.none()", + "self.m[?'x'].or(optional.of('nope')) == optional.of('nope')", + "self.m[?'x'].orValue('nope') == 'nope'", + "self.m[?'x'].hasValue() == false", + "self.m[?'x'].hasValue() == false", + + "self.l[?0] == optional.of('a')", + "self.l[?1] == optional.none()", + "self.l[?0].orValue('') == 'a'", + "self.l[?0].hasValue() == true", + "self.l[?0].optMap(v, v == 'a').hasValue()", + "self.l[?1] == optional.none()", + "self.l[?1].or(optional.of('nope')) == optional.of('nope')", + "self.l[?1].orValue('nope') == 'nope'", + "self.l[?1].hasValue() == false", + "self.l[?1].hasValue() == false", + + "optional.ofNonZeroValue(1).hasValue()", + "optional.ofNonZeroValue(uint(1)).hasValue()", + "optional.ofNonZeroValue(1.1).hasValue()", + "optional.ofNonZeroValue('a').hasValue()", + "optional.ofNonZeroValue(true).hasValue()", + "optional.ofNonZeroValue(['a']).hasValue()", + "optional.ofNonZeroValue({'k': 'v'}).hasValue()", + "optional.ofNonZeroValue(timestamp('2011-08-18T00:00:00.000+01:00')).hasValue()", + "optional.ofNonZeroValue(duration('19h3m37s10ms')).hasValue()", + "optional.ofNonZeroValue(null) == optional.none()", + "optional.ofNonZeroValue(0) == optional.none()", + "optional.ofNonZeroValue(uint(0)) == optional.none()", + "optional.ofNonZeroValue(0.0) == optional.none()", + "optional.ofNonZeroValue('') == optional.none()", + "optional.ofNonZeroValue(false) == optional.none()", + "optional.ofNonZeroValue([]) == optional.none()", + "optional.ofNonZeroValue({}) == optional.none()", + "optional.ofNonZeroValue(timestamp('0001-01-01T00:00:00.000+00:00')) == optional.none()", + "optional.ofNonZeroValue(duration('0s')) == optional.none()", + + "{?'k': optional.none(), 'k2': 'v2'} == {'k2': 'v2'}", + "{?'k': optional.of('v'), 'k2': 'v2'} == {'k': 'v', 'k2': 'v2'}", + "['a', ?optional.none(), 'c'] == ['a', 'c']", + "['a', ?optional.of('v'), 'c'] == ['a', 'v', 'c']", + }, + errors: map[string]string{ + "self.absentObj.?absentStr == optional.none()": "no such key: absentObj", // missing ?. operator on first deref is an error + }, + }, } for i := range tests { @@ -2262,7 +2347,7 @@ func TestCELValidationContextCancellation(t *testing.T) { // This is the most recursive operations we expect to be able to include in an expression. // This number could get larger with more improvements in the grammar or ANTLR stack, but should *never* decrease or previously valid expressions could be treated as invalid. -const maxValidDepth = 243 +const maxValidDepth = 250 // TestCELMaxRecursionDepth tests CEL setting for maxRecursionDepth. func TestCELMaxRecursionDepth(t *testing.T) { @@ -2397,7 +2482,7 @@ func TestMessageExpression(t *testing.T) { message: "message not messageExpression", messageExpression: `"str1 " + ["a", "b", "c", "d"][4]`, costBudget: 50, - expectedLogErr: "messageExpression evaluation failed due to: index '4' out of range in list size '4'", + expectedLogErr: "messageExpression evaluation failed due to: index out of bounds: 4", expectedValidationErr: "message not messageExpression", expectedRemainingBudget: 47, }, @@ -2405,7 +2490,7 @@ func TestMessageExpression(t *testing.T) { name: "runtime cost preserved if messageExpression fails during evaluation (no message set)", messageExpression: `"str1 " + ["a", "b", "c", "d"][4]`, costBudget: 50, - expectedLogErr: "messageExpression evaluation failed due to: index '4' out of range in list size '4'", + expectedLogErr: "messageExpression evaluation failed due to: index out of bounds: 4", expectedValidationErr: "failed rule", expectedRemainingBudget: 47, }, diff --git a/staging/src/k8s.io/apiserver/go.sum b/staging/src/k8s.io/apiserver/go.sum index 92002284198..6754b7e9633 100644 --- a/staging/src/k8s.io/apiserver/go.sum +++ b/staging/src/k8s.io/apiserver/go.sum @@ -13,6 +13,7 @@ cloud.google.com/go v0.56.0/go.mod h1:jr7tqZxxKOVYizybht9+26Z/gUq7tiRzu+ACVAMbKV cloud.google.com/go v0.57.0/go.mod h1:oXiQ6Rzq3RAkkY7N6t3TcE6jE+CIBBbA36lwQ1JyzZs= cloud.google.com/go v0.62.0/go.mod h1:jmCYTdRCQuc1PHIIJ/maLInMho30T/Y0M4hTdTShOYc= cloud.google.com/go v0.65.0/go.mod h1:O5N8zS7uWy9vkA9vayVHs65eM1ubvY4h553ofrNHObY= +cloud.google.com/go v0.110.0 h1:Zc8gqp3+a9/Eyph2KDmcGaPtbKRIoqq4YTlL4NMD0Ys= cloud.google.com/go/bigquery v1.0.1/go.mod h1:i/xbL2UlR5RvWAURpBYZTtm/cXjCha9lbfbpx4poX+o= cloud.google.com/go/bigquery v1.3.0/go.mod h1:PjpwJnslEMmckchkHFfq+HTD2DmtT67aNFKH1/VBDHE= cloud.google.com/go/bigquery v1.4.0/go.mod h1:S8dzgnTigyfTmLBfrtrhyYhwRxG72rYxvftPBK2Dvzc= 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 a190639dd1d..641e5857be0 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/environment/base.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/environment/base.go @@ -57,7 +57,7 @@ var baseOpts = []VersionedOptions{ cel.EagerlyValidateDeclarations(true), cel.DefaultUTCTimeZone(true), - ext.Strings(), + ext.Strings(ext.StringsVersion(0)), library.URLs(), library.Regex(), library.Lists(), @@ -80,6 +80,7 @@ var baseOpts = []VersionedOptions{ cel.OptionalTypes(), }, }, + // TODO: switch to ext.Strings version 2 once format() is fixed to work with HomogeneousAggregateLiterals. } // MustBaseEnvSet returns the common CEL base environments for Kubernetes for Version, or panics diff --git a/test/integration/apiserver/cel/validatingadmissionpolicy_test.go b/test/integration/apiserver/cel/validatingadmissionpolicy_test.go index f3e884b01ed..60cbde9bcd7 100644 --- a/test/integration/apiserver/cel/validatingadmissionpolicy_test.go +++ b/test/integration/apiserver/cel/validatingadmissionpolicy_test.go @@ -210,7 +210,7 @@ func Test_ValidateNamespace_NoParams(t *testing.T) { Name: "test-k8s", }, }, - err: "namespaces \"test-k8s\" is forbidden: ValidatingAdmissionPolicy 'validate-namespace-suffix' with binding 'validate-namespace-suffix-binding' denied request: expression 'has(params.metadata) && has(params.metadata.name) && object.metadata.name.endsWith(params.metadata.name)' resulted in error: invalid type for field selection.", + err: "namespaces \"test-k8s\" is forbidden: ValidatingAdmissionPolicy 'validate-namespace-suffix' with binding 'validate-namespace-suffix-binding' denied request: failed expression: has(params.metadata) && has(params.metadata.name) && object.metadata.name.endsWith(params.metadata.name)", failureReason: metav1.StatusReasonInvalid, }, {