Skip to content

Commit 5420b2f

Browse files
cici37cici37
authored
Hot fix for panic on schema conversion. (kubernetes#126167)
1 parent 04cc0a1 commit 5420b2f

File tree

5 files changed

+235
-3
lines changed

5 files changed

+235
-3
lines changed

‎staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"github.com/google/cel-go/cel"
2525
"github.com/google/cel-go/checker"
2626
"github.com/google/cel-go/common/types"
27+
"github.com/pkg/errors"
2728

2829
apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
2930
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
@@ -125,6 +126,9 @@ func Compile(s *schema.Structural, declType *apiservercel.DeclType, perCallLimit
125126
if len(s.XValidations) == 0 {
126127
return nil, nil
127128
}
129+
if declType == nil {
130+
return nil, errors.New("Failed to convert to declType for CEL validation rules")
131+
}
128132
celRules := s.XValidations
129133

130134
oldSelfEnvSet, optionalOldSelfEnvSet, err := prepareEnvSet(baseEnvSet, declType)

‎staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model/adaptor.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,9 @@ func (s *Structural) Pattern() string {
6262
}
6363

6464
func (s *Structural) Items() common.Schema {
65+
if s.Structural.Items == nil {
66+
return nil
67+
}
6568
return &Structural{Structural: s.Structural.Items}
6669
}
6770

‎staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model/schemas_test.go‎

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,7 @@ func TestEstimateMaxLengthJSON(t *testing.T) {
274274
Name string
275275
InputSchema *schema.Structural
276276
ExpectedMaxElements int64
277+
ExpectNilType bool
277278
}
278279
tests := []maxLengthTest{
279280
{
@@ -499,13 +500,61 @@ func TestEstimateMaxLengthJSON(t *testing.T) {
499500
// so we expect the max length to be exactly equal to the user-supplied one
500501
ExpectedMaxElements: 20,
501502
},
503+
{
504+
Name: "Property under array",
505+
InputSchema: &schema.Structural{
506+
Generic: schema.Generic{
507+
Type: "array",
508+
},
509+
Properties: map[string]schema.Structural{
510+
"field": {
511+
Generic: schema.Generic{
512+
Type: "string",
513+
Default: schema.JSON{Object: "default"},
514+
},
515+
},
516+
},
517+
},
518+
// Got nil for delType
519+
ExpectedMaxElements: 0,
520+
ExpectNilType: true,
521+
},
522+
{
523+
Name: "Items under object",
524+
InputSchema: &schema.Structural{
525+
Generic: schema.Generic{
526+
Type: "object",
527+
},
528+
Items: &schema.Structural{
529+
Generic: schema.Generic{
530+
Type: "array",
531+
},
532+
Properties: map[string]schema.Structural{
533+
"field": {
534+
Generic: schema.Generic{
535+
Type: "string",
536+
Default: schema.JSON{Object: "default"},
537+
},
538+
},
539+
},
540+
ValueValidation: &schema.ValueValidation{
541+
Required: []string{"field"},
542+
},
543+
},
544+
},
545+
// Skip items under object for schema conversion.
546+
ExpectedMaxElements: 0,
547+
},
502548
}
503549
for _, testCase := range tests {
504550
t.Run(testCase.Name, func(t *testing.T) {
505551
decl := SchemaDeclType(testCase.InputSchema, false)
506-
if decl.MaxElements != testCase.ExpectedMaxElements {
552+
if decl != nil && decl.MaxElements != testCase.ExpectedMaxElements {
507553
t.Errorf("wrong maxElements (got %d, expected %d)", decl.MaxElements, testCase.ExpectedMaxElements)
508554
}
555+
if testCase.ExpectNilType && decl != nil {
556+
t.Errorf("expected nil type, got %v", decl)
557+
}
509558
})
510559
}
511560
}

‎staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,9 +100,15 @@ func validator(validationSchema, nodeSchema *schema.Structural, isResourceRoot b
100100
var itemsValidator, additionalPropertiesValidator *Validator
101101
var propertiesValidators map[string]Validator
102102
var allOfValidators []*Validator
103+
var elemType *cel.DeclType
104+
if declType != nil {
105+
elemType = declType.ElemType
106+
} else {
107+
elemType = declType
108+
}
103109

104110
if validationSchema.Items != nil && nodeSchema.Items != nil {
105-
itemsValidator = validator(validationSchema.Items, nodeSchema.Items, nodeSchema.Items.XEmbeddedResource, declType.ElemType, perCallLimit)
111+
itemsValidator = validator(validationSchema.Items, nodeSchema.Items, nodeSchema.Items.XEmbeddedResource, elemType, perCallLimit)
106112
}
107113

108114
if len(validationSchema.Properties) > 0 {
@@ -117,6 +123,9 @@ func validator(validationSchema, nodeSchema *schema.Structural, isResourceRoot b
117123

118124
var fieldType *cel.DeclType
119125
if escapedPropName, ok := cel.Escape(k); ok {
126+
if declType == nil {
127+
continue
128+
}
120129
if f, ok := declType.Fields[escapedPropName]; ok {
121130
fieldType = f.Type
122131
} else {
@@ -138,7 +147,7 @@ func validator(validationSchema, nodeSchema *schema.Structural, isResourceRoot b
138147
}
139148
if validationSchema.AdditionalProperties != nil && validationSchema.AdditionalProperties.Structural != nil &&
140149
nodeSchema.AdditionalProperties != nil && nodeSchema.AdditionalProperties.Structural != nil {
141-
additionalPropertiesValidator = validator(validationSchema.AdditionalProperties.Structural, nodeSchema.AdditionalProperties.Structural, nodeSchema.AdditionalProperties.Structural.XEmbeddedResource, declType.ElemType, perCallLimit)
150+
additionalPropertiesValidator = validator(validationSchema.AdditionalProperties.Structural, nodeSchema.AdditionalProperties.Structural, nodeSchema.AdditionalProperties.Structural.XEmbeddedResource, elemType, perCallLimit)
142151
}
143152

144153
if validationSchema.ValueValidation != nil && len(validationSchema.ValueValidation.AllOf) > 0 {

‎test/integration/apiserver/crd_validation_expressions_test.go‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,103 @@ func TestCustomResourceValidatorsWithBlockingErrors(t *testing.T) {
650650
})
651651
}
652652

653+
// TestCustomResourceValidatorsWithSchemaConversion tests CRD replacement with schema conversion issue should not panic.
654+
func TestCustomResourceValidatorsWithSchemaConversion(t *testing.T) {
655+
server, err := apiservertesting.StartTestServer(t, apiservertesting.NewDefaultTestServerOptions(), nil, framework.SharedEtcd())
656+
if err != nil {
657+
t.Fatal(err)
658+
}
659+
defer server.TearDownFn()
660+
config := server.ClientConfig
661+
662+
apiExtensionClient, err := clientset.NewForConfig(config)
663+
if err != nil {
664+
t.Fatal(err)
665+
}
666+
dynamicClient, err := dynamic.NewForConfig(config)
667+
if err != nil {
668+
t.Fatal(err)
669+
}
670+
671+
// Create CRD with normal items+array schema
672+
structuralWithValidators := crdWithSchema(t, "Structural", structuralSchemaWithItemsUnderArray)
673+
crd, err := fixtures.CreateNewV1CustomResourceDefinition(structuralWithValidators, apiExtensionClient, dynamicClient)
674+
if err != nil {
675+
t.Fatal(err)
676+
}
677+
gvr := schema.GroupVersionResource{
678+
Group: crd.Spec.Group,
679+
Version: crd.Spec.Versions[0].Name,
680+
Resource: crd.Spec.Names.Plural,
681+
}
682+
crClient := dynamicClient.Resource(gvr)
683+
684+
// Create a valid CR instance
685+
name1 := names.SimpleNameGenerator.GenerateName("cr-1")
686+
_, err = crClient.Create(context.TODO(), &unstructured.Unstructured{Object: map[string]interface{}{
687+
"apiVersion": gvr.Group + "/" + gvr.Version,
688+
"kind": crd.Spec.Names.Kind,
689+
"metadata": map[string]interface{}{
690+
"name": name1,
691+
},
692+
"spec": map[string]interface{}{
693+
"backend": []interface{}{
694+
map[string]interface{}{
695+
"replicas": 8,
696+
},
697+
},
698+
},
699+
}}, metav1.CreateOptions{})
700+
if err != nil {
701+
t.Errorf("Failed to create custom resource: %v", err)
702+
}
703+
crd, err = apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crd.Name, metav1.GetOptions{})
704+
if err != nil {
705+
t.Fatal(err)
706+
}
707+
structuralSchemaWithItemsUnderObject := crdWithSchema(t, "Structural", structuralSchemaWithItemsUnderObject)
708+
structuralSchemaWithItemsUnderObject.SetResourceVersion(crd.GetResourceVersion())
709+
// Update CRD with invalid schema items under object
710+
crd, err = apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), structuralSchemaWithItemsUnderObject, metav1.UpdateOptions{})
711+
if err != nil {
712+
t.Fatal(err)
713+
}
714+
// Make an unrelated update to the previous persisted CR instance to make sure CRD handler doesn't panic
715+
oldCR, err := crClient.Get(context.TODO(), name1, metav1.GetOptions{})
716+
if err != nil {
717+
t.Fatal(err)
718+
}
719+
oldCR.Object["metadata"].(map[string]interface{})["labels"] = map[string]interface{}{"key": "value"}
720+
_, err = crClient.Update(context.TODO(), oldCR, metav1.UpdateOptions{})
721+
if err == nil || !strings.Contains(err.Error(), "rule compiler initialization error: Failed to convert to declType for CEL validation rules") {
722+
t.Fatalf("expect error to contain \rule compiler initialization error: Failed to convert to declType for CEL validation rules\" but get: %v", err)
723+
}
724+
// Create another CR instance with an array and be rejected
725+
name2 := names.SimpleNameGenerator.GenerateName("cr-2")
726+
_, err = crClient.Create(context.TODO(), &unstructured.Unstructured{Object: map[string]interface{}{
727+
"apiVersion": gvr.Group + "/" + gvr.Version,
728+
"kind": crd.Spec.Names.Kind,
729+
"metadata": map[string]interface{}{
730+
"name": name2,
731+
},
732+
"spec": map[string]interface{}{
733+
"backend": []interface{}{
734+
map[string]interface{}{
735+
"replicas": 7,
736+
},
737+
},
738+
},
739+
}}, metav1.CreateOptions{})
740+
if err == nil || !strings.Contains(err.Error(), "Invalid value: \"array\": spec.backend in body must be of type object: \"array\"") {
741+
t.Fatalf("expect error to contain \"Invalid value: \"array\": spec.backend in body must be of type object: \"array\"\" but get: %v", err)
742+
}
743+
// Delete the CRD
744+
err = fixtures.DeleteV1CustomResourceDefinition(structuralWithValidators, apiExtensionClient)
745+
if err != nil {
746+
t.Fatal(err)
747+
}
748+
}
749+
653750
func nonStructuralCrdWithValidations() *apiextensionsv1beta1.CustomResourceDefinition {
654751
return &apiextensionsv1beta1.CustomResourceDefinition{
655752
ObjectMeta: metav1.ObjectMeta{
@@ -880,6 +977,76 @@ var structuralSchemaWithBlockingErr = []byte(`
880977
}
881978
}`)
882979

980+
var structuralSchemaWithItemsUnderArray = []byte(`
981+
{
982+
"openAPIV3Schema": {
983+
"description": "CRD with CEL validators",
984+
"type": "object",
985+
"properties": {
986+
"spec": {
987+
"type": "object",
988+
"properties": {
989+
"backend": {
990+
"type": "array",
991+
"maxItems": 100,
992+
"items": {
993+
"type": "object",
994+
"properties": {
995+
"replicas": {
996+
"type": "integer"
997+
}
998+
},
999+
"required": [
1000+
"replicas"
1001+
],
1002+
"x-kubernetes-validations": [
1003+
{
1004+
"rule": "0 <= self.replicas && self.replicas <= 10"
1005+
}
1006+
]
1007+
}
1008+
}
1009+
}
1010+
}
1011+
}
1012+
}
1013+
}`)
1014+
1015+
var structuralSchemaWithItemsUnderObject = []byte(`
1016+
{
1017+
"openAPIV3Schema": {
1018+
"description": "CRD with CEL validators",
1019+
"type": "object",
1020+
"properties": {
1021+
"spec": {
1022+
"type": "object",
1023+
"properties": {
1024+
"backend": {
1025+
"type": "object",
1026+
"maxItems": 100,
1027+
"items": {
1028+
"type": "object",
1029+
"properties": {
1030+
"replicas": {
1031+
"type": "integer"
1032+
}
1033+
},
1034+
"required": [
1035+
"replicas"
1036+
],
1037+
"x-kubernetes-validations": [
1038+
{
1039+
"rule": "0 <= self.replicas && self.replicas <= 10"
1040+
}
1041+
]
1042+
}
1043+
}
1044+
}
1045+
}
1046+
}
1047+
}
1048+
}`)
1049+
8831050
var structuralSchemaWithValidMetadataValidators = []byte(`
8841051
{
8851052
"openAPIV3Schema": {

0 commit comments

Comments
 (0)