diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go index aba8f067417..ff931c5f54c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go @@ -24,6 +24,7 @@ import ( "github.com/google/cel-go/cel" "github.com/google/cel-go/checker" "github.com/google/cel-go/common/types" + "github.com/pkg/errors" apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" @@ -125,6 +126,9 @@ func Compile(s *schema.Structural, declType *apiservercel.DeclType, perCallLimit if len(s.XValidations) == 0 { return nil, nil } + if declType == nil { + return nil, errors.New("Failed to convert to declType for CEL validation rules") + } celRules := s.XValidations oldSelfEnvSet, optionalOldSelfEnvSet, err := prepareEnvSet(baseEnvSet, declType) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model/adaptor.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model/adaptor.go index 2228b0b8468..a66ab42937c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model/adaptor.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model/adaptor.go @@ -62,6 +62,9 @@ func (s *Structural) Pattern() string { } func (s *Structural) Items() common.Schema { + if s.Structural.Items == nil { + return nil + } return &Structural{Structural: s.Structural.Items} } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model/schemas_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model/schemas_test.go index 4665d21651c..99c09a99af2 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model/schemas_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model/schemas_test.go @@ -274,6 +274,7 @@ func TestEstimateMaxLengthJSON(t *testing.T) { Name string InputSchema *schema.Structural ExpectedMaxElements int64 + ExpectNilType bool } tests := []maxLengthTest{ { @@ -499,13 +500,61 @@ func TestEstimateMaxLengthJSON(t *testing.T) { // so we expect the max length to be exactly equal to the user-supplied one ExpectedMaxElements: 20, }, + { + Name: "Property under array", + InputSchema: &schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Properties: map[string]schema.Structural{ + "field": { + Generic: schema.Generic{ + Type: "string", + Default: schema.JSON{Object: "default"}, + }, + }, + }, + }, + // Got nil for delType + ExpectedMaxElements: 0, + ExpectNilType: true, + }, + { + Name: "Items under object", + InputSchema: &schema.Structural{ + Generic: schema.Generic{ + Type: "object", + }, + Items: &schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Properties: map[string]schema.Structural{ + "field": { + Generic: schema.Generic{ + Type: "string", + Default: schema.JSON{Object: "default"}, + }, + }, + }, + ValueValidation: &schema.ValueValidation{ + Required: []string{"field"}, + }, + }, + }, + // Skip items under object for schema conversion. + ExpectedMaxElements: 0, + }, } for _, testCase := range tests { t.Run(testCase.Name, func(t *testing.T) { decl := SchemaDeclType(testCase.InputSchema, false) - if decl.MaxElements != testCase.ExpectedMaxElements { + if decl != nil && decl.MaxElements != testCase.ExpectedMaxElements { t.Errorf("wrong maxElements (got %d, expected %d)", decl.MaxElements, testCase.ExpectedMaxElements) } + if testCase.ExpectNilType && decl != nil { + t.Errorf("expected nil type, got %v", decl) + } }) } } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go index b265271555f..575fd5e2e9a 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go @@ -100,9 +100,15 @@ func validator(validationSchema, nodeSchema *schema.Structural, isResourceRoot b var itemsValidator, additionalPropertiesValidator *Validator var propertiesValidators map[string]Validator var allOfValidators []*Validator + var elemType *cel.DeclType + if declType != nil { + elemType = declType.ElemType + } else { + elemType = declType + } if validationSchema.Items != nil && nodeSchema.Items != nil { - itemsValidator = validator(validationSchema.Items, nodeSchema.Items, nodeSchema.Items.XEmbeddedResource, declType.ElemType, perCallLimit) + itemsValidator = validator(validationSchema.Items, nodeSchema.Items, nodeSchema.Items.XEmbeddedResource, elemType, perCallLimit) } if len(validationSchema.Properties) > 0 { @@ -117,6 +123,9 @@ func validator(validationSchema, nodeSchema *schema.Structural, isResourceRoot b var fieldType *cel.DeclType if escapedPropName, ok := cel.Escape(k); ok { + if declType == nil { + continue + } if f, ok := declType.Fields[escapedPropName]; ok { fieldType = f.Type } else { @@ -138,7 +147,7 @@ func validator(validationSchema, nodeSchema *schema.Structural, isResourceRoot b } if validationSchema.AdditionalProperties != nil && validationSchema.AdditionalProperties.Structural != nil && nodeSchema.AdditionalProperties != nil && nodeSchema.AdditionalProperties.Structural != nil { - additionalPropertiesValidator = validator(validationSchema.AdditionalProperties.Structural, nodeSchema.AdditionalProperties.Structural, nodeSchema.AdditionalProperties.Structural.XEmbeddedResource, declType.ElemType, perCallLimit) + additionalPropertiesValidator = validator(validationSchema.AdditionalProperties.Structural, nodeSchema.AdditionalProperties.Structural, nodeSchema.AdditionalProperties.Structural.XEmbeddedResource, elemType, perCallLimit) } if validationSchema.ValueValidation != nil && len(validationSchema.ValueValidation.AllOf) > 0 { diff --git a/test/integration/apiserver/crd_validation_expressions_test.go b/test/integration/apiserver/crd_validation_expressions_test.go index efc6488699e..0038c8117e5 100644 --- a/test/integration/apiserver/crd_validation_expressions_test.go +++ b/test/integration/apiserver/crd_validation_expressions_test.go @@ -650,6 +650,103 @@ func TestCustomResourceValidatorsWithBlockingErrors(t *testing.T) { }) } +// TestCustomResourceValidatorsWithSchemaConversion tests CRD replacement with schema conversion issue should not panic. +func TestCustomResourceValidatorsWithSchemaConversion(t *testing.T) { + server, err := apiservertesting.StartTestServer(t, apiservertesting.NewDefaultTestServerOptions(), nil, framework.SharedEtcd()) + if err != nil { + t.Fatal(err) + } + defer server.TearDownFn() + config := server.ClientConfig + + apiExtensionClient, err := clientset.NewForConfig(config) + if err != nil { + t.Fatal(err) + } + dynamicClient, err := dynamic.NewForConfig(config) + if err != nil { + t.Fatal(err) + } + + // Create CRD with normal items+array schema + structuralWithValidators := crdWithSchema(t, "Structural", structuralSchemaWithItemsUnderArray) + crd, err := fixtures.CreateNewV1CustomResourceDefinition(structuralWithValidators, apiExtensionClient, dynamicClient) + if err != nil { + t.Fatal(err) + } + gvr := schema.GroupVersionResource{ + Group: crd.Spec.Group, + Version: crd.Spec.Versions[0].Name, + Resource: crd.Spec.Names.Plural, + } + crClient := dynamicClient.Resource(gvr) + + // Create a valid CR instance + name1 := names.SimpleNameGenerator.GenerateName("cr-1") + _, err = crClient.Create(context.TODO(), &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": gvr.Group + "/" + gvr.Version, + "kind": crd.Spec.Names.Kind, + "metadata": map[string]interface{}{ + "name": name1, + }, + "spec": map[string]interface{}{ + "backend": []interface{}{ + map[string]interface{}{ + "replicas": 8, + }, + }, + }, + }}, metav1.CreateOptions{}) + if err != nil { + t.Errorf("Failed to create custom resource: %v", err) + } + crd, err = apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crd.Name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + structuralSchemaWithItemsUnderObject := crdWithSchema(t, "Structural", structuralSchemaWithItemsUnderObject) + structuralSchemaWithItemsUnderObject.SetResourceVersion(crd.GetResourceVersion()) + // Update CRD with invalid schema items under object + crd, err = apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), structuralSchemaWithItemsUnderObject, metav1.UpdateOptions{}) + if err != nil { + t.Fatal(err) + } + // Make an unrelated update to the previous persisted CR instance to make sure CRD handler doesn't panic + oldCR, err := crClient.Get(context.TODO(), name1, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + oldCR.Object["metadata"].(map[string]interface{})["labels"] = map[string]interface{}{"key": "value"} + _, err = crClient.Update(context.TODO(), oldCR, metav1.UpdateOptions{}) + if err == nil || !strings.Contains(err.Error(), "rule compiler initialization error: Failed to convert to declType for CEL validation rules") { + t.Fatalf("expect error to contain \rule compiler initialization error: Failed to convert to declType for CEL validation rules\" but get: %v", err) + } + // Create another CR instance with an array and be rejected + name2 := names.SimpleNameGenerator.GenerateName("cr-2") + _, err = crClient.Create(context.TODO(), &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": gvr.Group + "/" + gvr.Version, + "kind": crd.Spec.Names.Kind, + "metadata": map[string]interface{}{ + "name": name2, + }, + "spec": map[string]interface{}{ + "backend": []interface{}{ + map[string]interface{}{ + "replicas": 7, + }, + }, + }, + }}, metav1.CreateOptions{}) + if err == nil || !strings.Contains(err.Error(), "Invalid value: \"array\": spec.backend in body must be of type object: \"array\"") { + t.Fatalf("expect error to contain \"Invalid value: \"array\": spec.backend in body must be of type object: \"array\"\" but get: %v", err) + } + // Delete the CRD + err = fixtures.DeleteV1CustomResourceDefinition(structuralWithValidators, apiExtensionClient) + if err != nil { + t.Fatal(err) + } +} + func nonStructuralCrdWithValidations() *apiextensionsv1beta1.CustomResourceDefinition { return &apiextensionsv1beta1.CustomResourceDefinition{ ObjectMeta: metav1.ObjectMeta{ @@ -880,6 +977,76 @@ var structuralSchemaWithBlockingErr = []byte(` } }`) +var structuralSchemaWithItemsUnderArray = []byte(` +{ + "openAPIV3Schema": { + "description": "CRD with CEL validators", + "type": "object", + "properties": { + "spec": { + "type": "object", + "properties": { + "backend": { + "type": "array", + "maxItems": 100, + "items": { + "type": "object", + "properties": { + "replicas": { + "type": "integer" + } + }, + "required": [ + "replicas" + ], + "x-kubernetes-validations": [ + { + "rule": "0 <= self.replicas && self.replicas <= 10" + } + ] + } + } + } + } + } + } +}`) + +var structuralSchemaWithItemsUnderObject = []byte(` +{ + "openAPIV3Schema": { + "description": "CRD with CEL validators", + "type": "object", + "properties": { + "spec": { + "type": "object", + "properties": { + "backend": { + "type": "object", + "maxItems": 100, + "items": { + "type": "object", + "properties": { + "replicas": { + "type": "integer" + } + }, + "required": [ + "replicas" + ], + "x-kubernetes-validations": [ + { + "rule": "0 <= self.replicas && self.replicas <= 10" + } + ] + } + } + } + } + } + } +}`) + var structuralSchemaWithValidMetadataValidators = []byte(` { "openAPIV3Schema": {