From 4940c42c6842d5a04843b568d58f074e796ba4f2 Mon Sep 17 00:00:00 2001 From: Cici Huang <8658046+cici37@users.noreply.github.com> Date: Tue, 23 Jul 2024 15:45:20 -0700 Subject: [PATCH] Allowing direct CEL reserved keyword usage in CRD (#126188) * automatically escape reserved keywords for direct usage * Add reserved keyword support in a ratcheting way, add tests. --------- Co-authored-by: Wenxue Zhao Kubernetes-commit: a48a92c72ec7d4e2a8da396309abff9360faae75 --- go.mod | 10 +- go.sum | 12 +- .../validation/validation_test.go | 305 +++++++++++++++++- pkg/apiserver/schema/cel/model/types_test.go | 22 +- pkg/apiserver/schema/cel/validation_test.go | 42 +++ 5 files changed, 364 insertions(+), 27 deletions(-) diff --git a/go.mod b/go.mod index 28eeccad5..874fd8bb8 100644 --- a/go.mod +++ b/go.mod @@ -20,15 +20,14 @@ require ( go.etcd.io/etcd/client/v3 v3.5.14 go.opentelemetry.io/otel v1.28.0 go.opentelemetry.io/otel/trace v1.28.0 - google.golang.org/genproto/googleapis/api v0.0.0-20240528184218-531527333157 google.golang.org/grpc v1.65.0 google.golang.org/protobuf v1.34.2 gopkg.in/evanphx/json-patch.v4 v4.12.0 gopkg.in/yaml.v2 v2.4.0 - k8s.io/api v0.0.0-20240723194852-871340c2e998 + k8s.io/api v0.0.0-20240724010313-f04ea0bc861d k8s.io/apimachinery v0.0.0-20240720202316-95b78024e3fe - k8s.io/apiserver v0.0.0-20240723210659-c90207143c20 - k8s.io/client-go v0.0.0-20240723200359-dcfcc90795cc + k8s.io/apiserver v0.0.0-20240724012351-92ee9330ce6a + k8s.io/client-go v0.0.0-20240724010704-ac9204c6195b k8s.io/code-generator v0.0.0-20240720023521-ec3cc888df4c k8s.io/component-base v0.0.0-20240722183709-6cc953a9d440 k8s.io/klog/v2 v2.130.1 @@ -117,6 +116,7 @@ require ( golang.org/x/time v0.3.0 // indirect golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d // indirect google.golang.org/genproto v0.0.0-20230822172742-b8732ec3820d // indirect + google.golang.org/genproto/googleapis/api v0.0.0-20240528184218-531527333157 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240701130421-f6361c86f094 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect @@ -125,3 +125,5 @@ require ( k8s.io/kms v0.0.0-20240707024556-6e3528fa4c33 // indirect sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.30.3 // indirect ) + +replace k8s.io/api => k8s.io/api v0.0.0-20240724010313-a789efa287e8 diff --git a/go.sum b/go.sum index 27a5cfa86..9a48a8378 100644 --- a/go.sum +++ b/go.sum @@ -365,14 +365,14 @@ gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= -k8s.io/api v0.0.0-20240723194852-871340c2e998 h1:XvMrEqepRsNkn8Bl60PB5TO4ZEOgr70bYrpAedjvTV8= -k8s.io/api v0.0.0-20240723194852-871340c2e998/go.mod h1:ytlEzqC2wOTwYET71W7+J+k7O2V7vrDuzmNLBSpgT+k= +k8s.io/api v0.0.0-20240724010313-a789efa287e8 h1:TISAHWnfAdn420WpN+fEHG6snbLbfaCAp3kHDoAkxIc= +k8s.io/api v0.0.0-20240724010313-a789efa287e8/go.mod h1:ytlEzqC2wOTwYET71W7+J+k7O2V7vrDuzmNLBSpgT+k= k8s.io/apimachinery v0.0.0-20240720202316-95b78024e3fe h1:V9MwpYUwbKlfLKVrhpVuKWiat/LBIhm1pGB9/xdHm5Q= k8s.io/apimachinery v0.0.0-20240720202316-95b78024e3fe/go.mod h1:rsPdaZJfTfLsNJSQzNHQvYoTmxhoOEofxtOsF3rtsMo= -k8s.io/apiserver v0.0.0-20240723210659-c90207143c20 h1:32/qwZM293YJ71bcGQD2PhnPOLQBqQoDf6WvzQ/kpxI= -k8s.io/apiserver v0.0.0-20240723210659-c90207143c20/go.mod h1:NQDM9jV7nUGYBnMcr1Wm1zo17QIrCW1RWoD1kcuH/80= -k8s.io/client-go v0.0.0-20240723200359-dcfcc90795cc h1:qe0SREEjfE5w3ANvrSURWv00J/ISlqa9Sa3FCBYKRlg= -k8s.io/client-go v0.0.0-20240723200359-dcfcc90795cc/go.mod h1:XfEsPNNFOR0wNkr3BtkPUN668l7Sx1W4ECSUolQ0mA4= +k8s.io/apiserver v0.0.0-20240724012351-92ee9330ce6a h1:hbHaTqnKJ9RTFAgGkhffQR1+O6RgxZnCMGIJe6paWrw= +k8s.io/apiserver v0.0.0-20240724012351-92ee9330ce6a/go.mod h1:evDFvWJ5AkEQ1nRZOHulw/OKntvjsYuxyZtZQNPhz5o= +k8s.io/client-go v0.0.0-20240724010704-ac9204c6195b h1:NTLYx38CAu+VstHvPLosqB6uSQUtSM+3Mqz2D/C5JpE= +k8s.io/client-go v0.0.0-20240724010704-ac9204c6195b/go.mod h1:Y6CzOT21oLI4O66cjiV5oSSUgOL7gG/VCG9n8XI8OxU= k8s.io/code-generator v0.0.0-20240720023521-ec3cc888df4c h1:oiNPH9Y/YrQfxo8eTW/w71aBrSyr9MX/wGBKTwDSZsc= k8s.io/code-generator v0.0.0-20240720023521-ec3cc888df4c/go.mod h1:TVAwbna2B36D+IsWJ5oHqKZKSU8ZBtxeiMTb7uKM6Z0= k8s.io/component-base v0.0.0-20240722183709-6cc953a9d440 h1:14X+5sRQRsul6tLxIKTP0/DotvWlMd9DFCgMqHP1hZY= diff --git a/pkg/apis/apiextensions/validation/validation_test.go b/pkg/apis/apiextensions/validation/validation_test.go index b0fe74a83..93f684ba5 100644 --- a/pkg/apis/apiextensions/validation/validation_test.go +++ b/pkg/apis/apiextensions/validation/validation_test.go @@ -9013,7 +9013,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, { - name: "valid x-kubernetes-validations for escaping", + name: "invalid x-kubernetes-validations for escaping (keywords are invalid field names before v1.30)", input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -9024,12 +9024,75 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { { Rule: "self.__namespace__ > 0", }, + { + Rule: "self.if > 0", + }, + { + Rule: "self.namespace > 0", + }, + { + Rule: "self.as > 0", + }, + { + Rule: "self.break > 0", + }, + { + Rule: "self.const > 0", + }, + { + Rule: "self.continue > 0", + }, + { + Rule: "self.else > 0", + }, + { + Rule: "self.for > 0", + }, + { + Rule: "self.function > 0", + }, + { + Rule: "self.import > 0", + }, + { + Rule: "self.let > 0", + }, + { + Rule: "self.loop > 0", + }, + { + Rule: "self.package > 0", + }, + { + Rule: "self.return > 0", + }, + { + Rule: "self.var > 0", + }, + { + Rule: "self.void > 0", + }, + { + Rule: "self.while > 0", + }, { Rule: "self.self > 0", }, { Rule: "self.int > 0", }, + { + Rule: "self.true > 0", + }, + { + Rule: "self.false > 0", + }, + { + Rule: "self.null > 0", + }, + { + Rule: "self.in > 0", + }, }, Properties: map[string]apiextensions.JSONSchemaProps{ "if": { @@ -9038,25 +9101,112 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { "namespace": { Type: "integer", }, + "as": { + Type: "integer", + }, + "break": { + Type: "integer", + }, + "const": { + Type: "integer", + }, + "continue": { + Type: "integer", + }, + "else": { + Type: "integer", + }, + "for": { + Type: "integer", + }, + "function": { + Type: "integer", + }, + "import": { + Type: "integer", + }, + "let": { + Type: "integer", + }, + "loop": { + Type: "integer", + }, + "package": { + Type: "integer", + }, + "return": { + Type: "integer", + }, + "var": { + Type: "integer", + }, + "void": { + Type: "integer", + }, + "while": { + Type: "integer", + }, "self": { Type: "integer", }, "int": { Type: "integer", }, + "true": { + Type: "integer", + }, + "false": { + Type: "integer", + }, + "null": { + Type: "integer", + }, + "in": { + Type: "integer", + }, }, }, }, opts: validationOptions{ requireStructuralSchema: true, + celEnvironmentSet: environment.MustBaseEnvSet(version.MajorMinor(1, 30), true), + }, + expectedErrors: []validationMatch{ + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[2].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[3].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[4].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[5].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[6].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[7].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[8].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[9].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[10].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[11].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[12].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[13].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[14].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[15].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[16].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[17].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[18].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[21].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[22].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[23].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[24].rule"), }, }, { - name: "invalid x-kubernetes-validations for escaping", + name: "valid x-kubernetes-validations for escaping", input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", XValidations: apiextensions.ValidationRules{ + { + Rule: "self.__if__ > 0", + }, + { + Rule: "self.__namespace__ > 0", + }, { Rule: "self.if > 0", }, @@ -9064,7 +9214,68 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { Rule: "self.namespace > 0", }, { - Rule: "self.unknownProp > 0", + Rule: "self.as > 0", + }, + { + Rule: "self.break > 0", + }, + { + Rule: "self.const > 0", + }, + { + Rule: "self.continue > 0", + }, + { + Rule: "self.else > 0", + }, + { + Rule: "self.for > 0", + }, + { + Rule: "self.function > 0", + }, + { + Rule: "self.import > 0", + }, + { + Rule: "self.let > 0", + }, + { + Rule: "self.loop > 0", + }, + { + Rule: "self.package > 0", + }, + { + Rule: "self.return > 0", + }, + { + Rule: "self.var > 0", + }, + { + Rule: "self.void > 0", + }, + { + Rule: "self.while > 0", + }, + { + Rule: "self.self > 0", + }, + { + Rule: "self.int > 0", + }, + // reserved keywords `true`, `false`, `null` and `in` are not supported + { + Rule: "self.true > 0", + }, + { + Rule: "self.false > 0", + }, + { + Rule: "self.null > 0", + }, + { + Rule: "self.in > 0", }, }, Properties: map[string]apiextensions.JSONSchemaProps{ @@ -9074,13 +9285,97 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { "namespace": { Type: "integer", }, + "as": { + Type: "integer", + }, + "break": { + Type: "integer", + }, + "const": { + Type: "integer", + }, + "continue": { + Type: "integer", + }, + "else": { + Type: "integer", + }, + "for": { + Type: "integer", + }, + "function": { + Type: "integer", + }, + "import": { + Type: "integer", + }, + "let": { + Type: "integer", + }, + "loop": { + Type: "integer", + }, + "package": { + Type: "integer", + }, + "return": { + Type: "integer", + }, + "var": { + Type: "integer", + }, + "void": { + Type: "integer", + }, + "while": { + Type: "integer", + }, + "self": { + Type: "integer", + }, + "int": { + Type: "integer", + }, + "true": { + Type: "integer", + }, + "false": { + Type: "integer", + }, + "null": { + Type: "integer", + }, + "in": { + Type: "integer", + }, + }, + }, + }, + opts: validationOptions{ + requireStructuralSchema: true, + celEnvironmentSet: environment.MustBaseEnvSet(version.MajorMinor(1, 31), true), + }, + expectedErrors: []validationMatch{ + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[21].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[22].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[23].rule"), + invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[24].rule"), + }, + }, + { + name: "invalid x-kubernetes-validations for unknown property", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "self.unknownProp > 0", + }, }, }, }, expectedErrors: []validationMatch{ invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[0].rule"), - invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[1].rule"), - invalid("spec.validation.openAPIV3Schema.x-kubernetes-validations[2].rule"), }, opts: validationOptions{ requireStructuralSchema: true, diff --git a/pkg/apiserver/schema/cel/model/types_test.go b/pkg/apiserver/schema/cel/model/types_test.go index 1c66d4bb0..deaba8208 100644 --- a/pkg/apiserver/schema/cel/model/types_test.go +++ b/pkg/apiserver/schema/cel/model/types_test.go @@ -22,40 +22,38 @@ import ( "github.com/google/cel-go/cel" "github.com/google/cel-go/common/types" - exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1" - apiservercel "k8s.io/apiserver/pkg/cel" ) func TestTypes_RuleTypesFieldMapping(t *testing.T) { stdEnv, _ := cel.NewEnv() rt := apiservercel.NewDeclTypeProvider(SchemaDeclType(testSchema(), true).MaybeAssignTypeName("CustomObject")) - nestedFieldType, found := rt.FindFieldType("CustomObject", "nested") + nestedFieldType, found := rt.FindStructFieldType("CustomObject", "nested") if !found { t.Fatal("got field not found for 'CustomObject.nested', wanted found") } - if nestedFieldType.Type.GetMessageType() != "CustomObject.nested" { + if nestedFieldType.Type.DeclaredTypeName() != "CustomObject.nested" { t.Errorf("got field type %v, wanted mock_template.nested", nestedFieldType.Type) } - subnameFieldType, found := rt.FindFieldType("CustomObject.nested", "subname") + subnameFieldType, found := rt.FindStructFieldType("CustomObject.nested", "subname") if !found { t.Fatal("got field not found for 'CustomObject.nested.subname', wanted found") } - if subnameFieldType.Type.GetPrimitive() != exprpb.Type_STRING { + if subnameFieldType.Type.TypeName() != "string" { t.Errorf("got field type %v, wanted string", subnameFieldType.Type) } - flagsFieldType, found := rt.FindFieldType("CustomObject.nested", "flags") + flagsFieldType, found := rt.FindStructFieldType("CustomObject.nested", "flags") if !found { t.Fatal("got field not found for 'CustomObject.nested.flags', wanted found") } - if flagsFieldType.Type.GetMapType() == nil { + if flagsFieldType.Type.Kind() != types.MapKind { t.Errorf("got field type %v, wanted map", flagsFieldType.Type) } - flagFieldType, found := rt.FindFieldType("CustomObject.nested.flags", "my_flag") + flagFieldType, found := rt.FindStructFieldType("CustomObject.nested.flags", "my_flag") if !found { t.Fatal("got field not found for 'CustomObject.nested.flags.my_flag', wanted found") } - if flagFieldType.Type.GetPrimitive() != exprpb.Type_BOOL { + if flagFieldType.Type.Kind() != types.BoolKind { t.Errorf("got field type %v, wanted bool", flagFieldType.Type) } @@ -90,7 +88,7 @@ func TestTypes_RuleTypesFieldMapping(t *testing.T) { // t.Error("got CEL rule value of nil, wanted non-nil") //} - opts, err := rt.EnvOptions(stdEnv.TypeProvider()) + opts, err := rt.EnvOptions(stdEnv.CELTypeProvider()) if err != nil { t.Fatal(err) } @@ -98,7 +96,7 @@ func TestTypes_RuleTypesFieldMapping(t *testing.T) { if err != nil { t.Fatal(err) } - helloVal := ruleEnv.TypeAdapter().NativeToValue("hello") + helloVal := ruleEnv.CELTypeAdapter().NativeToValue("hello") if helloVal.Equal(types.String("hello")) != types.True { t.Errorf("got %v, wanted types.String('hello')", helloVal) } diff --git a/pkg/apiserver/schema/cel/validation_test.go b/pkg/apiserver/schema/cel/validation_test.go index 87a1faebd..af9f9699e 100644 --- a/pkg/apiserver/schema/cel/validation_test.go +++ b/pkg/apiserver/schema/cel/validation_test.go @@ -899,6 +899,48 @@ func TestValidationExpressions(t *testing.T) { "has(self.embedded.metadata.namespace)": "undefined field 'namespace'", }, }, + {name: "embedded object with usage of reserved keywords", + obj: map[string]interface{}{ + "embedded": map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "name": "foo", + "generateName": "pickItForMe", + "namespace": "reserved_keyword_namespace", + }, + "spec": map[string]interface{}{ + "if": "reserved_keyword_if", + }, + }, + }, + schema: objectTypePtr(map[string]schema.Structural{ + "embedded": { + Generic: schema.Generic{Type: "object"}, + Extensions: schema.Extensions{ + XEmbeddedResource: true, + }, + Properties: map[string]schema.Structural{ + "kind": stringType, + "apiVersion": stringType, + "metadata": objectType(map[string]schema.Structural{ + "name": stringType, + "generateName": stringType, + "namespace": stringType, + }), + "spec": objectType(map[string]schema.Structural{ + "if": stringType, + }), + }, + }, + }), + valid: []string{ + "has(self.embedded.metadata.namespace)", + "self.embedded.metadata.namespace == 'reserved_keyword_namespace'", + "has(self.embedded.spec.if)", + "self.embedded.spec.if == 'reserved_keyword_if'", + }, + }, {name: "embedded object with preserve unknown", obj: map[string]interface{}{ "embedded": map[string]interface{}{