Skip to content

Commit

Permalink
Hot fix for panic on schema conversion.
Browse files Browse the repository at this point in the history
  • Loading branch information
cici37 committed Jul 23, 2024
1 parent 6e15ab5 commit f1cd068
Show file tree
Hide file tree
Showing 5 changed files with 235 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package cel

import (
"errors"
"fmt"
"strings"
"time"
Expand Down Expand Up @@ -125,6 +126,9 @@ func Compile(s *schema.Structural, declType *apiservercel.DeclType, perCallLimit
if len(s.Extensions.XValidations) == 0 {
return nil, nil
}
if declType == nil {
return nil, errors.New("failed to convert to declType for CEL validation rules")
}
celRules := s.Extensions.XValidations

oldSelfEnvSet, optionalOldSelfEnvSet, err := prepareEnvSet(baseEnvSet, declType)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ func TestEstimateMaxLengthJSON(t *testing.T) {
Name string
InputSchema *schema.Structural
ExpectedMaxElements int64
ExpectNilType bool
}
tests := []maxLengthTest{
{
Expand Down Expand Up @@ -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)
}
})
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,24 @@ func validator(s *schema.Structural, isResourceRoot bool, declType *cel.DeclType
compiledRules, err := Compile(s, declType, perCallLimit, environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()), StoredExpressionsEnvLoader())
var itemsValidator, additionalPropertiesValidator *Validator
var propertiesValidators map[string]Validator
var elemType *cel.DeclType
if declType != nil {
elemType = declType.ElemType
} else {
elemType = declType
}
if s.Items != nil {
itemsValidator = validator(s.Items, s.Items.XEmbeddedResource, declType.ElemType, perCallLimit)
itemsValidator = validator(s.Items, s.Items.XEmbeddedResource, elemType, perCallLimit)
}
if len(s.Properties) > 0 {
propertiesValidators = make(map[string]Validator, len(s.Properties))
for k, p := range s.Properties {
prop := p
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 {
Expand All @@ -123,7 +132,7 @@ func validator(s *schema.Structural, isResourceRoot bool, declType *cel.DeclType
}
}
if s.AdditionalProperties != nil && s.AdditionalProperties.Structural != nil {
additionalPropertiesValidator = validator(s.AdditionalProperties.Structural, s.AdditionalProperties.Structural.XEmbeddedResource, declType.ElemType, perCallLimit)
additionalPropertiesValidator = validator(s.AdditionalProperties.Structural, s.AdditionalProperties.Structural.XEmbeddedResource, elemType, perCallLimit)
}
if len(compiledRules) > 0 || err != nil || itemsValidator != nil || additionalPropertiesValidator != nil || len(propertiesValidators) > 0 {
activationFactory := validationActivationWithoutOldSelf
Expand Down
167 changes: 167 additions & 0 deletions test/integration/apiserver/crd_validation_expressions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,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{
Expand Down Expand Up @@ -888,6 +985,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": {
Expand Down

0 comments on commit f1cd068

Please sign in to comment.