Merge pull request #2375 from JoelSpeed/fix-max-elements-x-int-or-string-release-4.20

OCPBUGS-59756: Fix IntOrString cost estimation when schema has a MaxLength constraint

Author: openshift-merge-bot[bot]

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

@@ -1872,6 +1872,18 @@ func TestCostEstimation(t *testing.T) {
 			setMaxElements:   1000,
 			expectedSetCost:  401,
 		},
+		{
+			name: "IntOrString type with quantity rule",
+			schemaGenerator: func(max *int64) *schema.Structural {
+				intOrString := intOrStringType()
+				intOrString = withRule(intOrString, "isQuantity(self)")
+				intOrString = withMaxLength(intOrString, max)
+				return &intOrString
+			},
+			expectedCalcCost: 314574,
+			setMaxElements:   20,
+			expectedSetCost:  9,
+		},
 	}
 	for _, testCase := range cases {
 		t.Run(testCase.name, func(t *testing.T) {

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

@@ -34,8 +34,8 @@ func TestSchemaDeclType(t *testing.T) {
 	if cust.TypeName() != "object" {
 		t.Errorf("incorrect type name, got %v, wanted object", cust.TypeName())
 	}
-	if len(cust.Fields) != 4 {
-		t.Errorf("incorrect number of fields, got %d, wanted 4", len(cust.Fields))
+	if len(cust.Fields) != 5 {
+		t.Errorf("incorrect number of fields, got %d, wanted 5", len(cust.Fields))
 	}
 	for _, f := range cust.Fields {
 		prop, found := ts.Properties[f.Name]
@@ -70,6 +70,13 @@ func TestSchemaDeclType(t *testing.T) {
 				}
 			}
 		}
+		if prop.ValueValidation != nil && prop.ValueValidation.MaxLength != nil {
+			if f.Type.MaxElements != 4*(*prop.ValueValidation.MaxLength) {
+				// When converting maxLength to maxElements, it's based on the number of bytes.]
+				// Worst case is that one rune is 4 bytes, so maxElements should be 4x maxLength.
+				t.Errorf("field maxElements does not match property 4x maxLength. field: %s, maxElements: %d, maxLength: %d", f.Name, f.Type.MaxElements, *prop.ValueValidation.MaxLength)
+			}
+		}
 	}
 	if ts.ValueValidation != nil {
 		for _, name := range ts.ValueValidation.Required {
@@ -137,6 +144,7 @@ func testSchema() *schema.Structural {
 	//   properties:
 	//     name:
 	//       type: string
+	//       maxLength: 256
 	//     nested:
 	//       type: object
 	//       properties:
@@ -166,6 +174,12 @@ func testSchema() *schema.Structural {
 	//       format: int64
 	//       default: 1
 	//       enum: [1,2,3]
+	//     intOrString:
+	//       x-kubernetes-int-or-string: true
+	//       anyOf:
+	//       - type: "integer"
+	//		 - type: "string"
+	//       maxLength: 20
 	ts := &schema.Structural{
 		Generic: schema.Generic{
 			Type: "object",
@@ -175,6 +189,9 @@ func testSchema() *schema.Structural {
 				Generic: schema.Generic{
 					Type: "string",
 				},
+				ValueValidation: &schema.ValueValidation{
+					MaxLength: maxPtr(256),
+				},
 			},
 			"value": {
 				Generic: schema.Generic{
@@ -245,6 +262,26 @@ func testSchema() *schema.Structural {
 					},
 				},
 			},
+			"intOrString": {
+				Extensions: schema.Extensions{
+					XIntOrString: true,
+				},
+				ValueValidation: &schema.ValueValidation{
+					MaxLength: maxPtr(20),
+					AnyOf: []schema.NestedValueValidation{
+						{
+							ForbiddenGenerics: schema.Generic{
+								Type: "integer",
+							},
+						},
+						{
+							ForbiddenGenerics: schema.Generic{
+								Type: "string",
+							},
+						},
+					},
+				},
+			},
 		},
 	}
 	return ts

staging/src/k8s.io/apiserver/pkg/cel/common/schemas.go

@@ -55,8 +55,15 @@ func SchemaDeclType(s Schema, isResourceRoot bool) *apiservercel.DeclType {
 		//  `type(intOrStringField) == int ? intOrStringField < 5 : double(intOrStringField.replace('%', '')) < 0.5
 		//
 		dyn := apiservercel.NewSimpleTypeWithMinSize("dyn", cel.DynType, nil, 1) // smallest value for a serialized x-kubernetes-int-or-string is 0
-		// handle x-kubernetes-int-or-string by returning the max length/min serialized size of the largest possible string
-		dyn.MaxElements = maxRequestSizeBytes - 2
+
+		// If the schema has a maxlength constraint, bound the max elements based on the max length.
+		// Otherwise, fallback to the max request size.
+		if s.MaxLength() != nil {
+			dyn.MaxElements = estimateMaxElementsFromMaxLength(s)
+		} else {
+			dyn.MaxElements = estimateMaxStringLengthPerRequest(s)
+		}
+
 		return dyn
 	}
 
@@ -159,11 +166,7 @@ func SchemaDeclType(s Schema, isResourceRoot bool) *apiservercel.DeclType {
 
 		strWithMaxLength := apiservercel.NewSimpleTypeWithMinSize("string", cel.StringType, types.String(""), apiservercel.MinStringSize)
 		if s.MaxLength() != nil {
-			// multiply the user-provided max length by 4 in the case of an otherwise-untyped string
-			// we do this because the OpenAPIv3 spec indicates that maxLength is specified in runes/code points,
-			// but we need to reason about length for things like request size, so we use bytes in this code (and an individual
-			// unicode code point can be up to 4 bytes long)
-			strWithMaxLength.MaxElements = zeroIfNegative(*s.MaxLength()) * 4
+			strWithMaxLength.MaxElements = estimateMaxElementsFromMaxLength(s)
 		} else {
 			if len(s.Enum()) > 0 {
 				strWithMaxLength.MaxElements = estimateMaxStringEnumLength(s)
@@ -228,6 +231,7 @@ func WithTypeAndObjectMeta(s *spec.Schema) *spec.Schema {
 // must only be called on schemas of type "string" or x-kubernetes-int-or-string: true
 func estimateMaxStringLengthPerRequest(s Schema) int64 {
 	if s.IsXIntOrString() {
+		// handle x-kubernetes-int-or-string by returning the max length/min serialized size of the largest possible string
 		return maxRequestSizeBytes - 2
 	}
 	switch s.Format() {
@@ -272,3 +276,13 @@ func estimateMaxAdditionalPropertiesFromMinSize(minSize int64) int64 {
 	// subtract 2 to account for { and }
 	return (maxRequestSizeBytes - 2) / keyValuePairSize
 }
+
+// estimateMaxElementsFromMaxLength estimates the maximum number of elements for a string schema
+// that is bound with a maxLength constraint.
+func estimateMaxElementsFromMaxLength(s Schema) int64 {
+	// multiply the user-provided max length by 4 in the case of an otherwise-untyped string
+	// we do this because the OpenAPIv3 spec indicates that maxLength is specified in runes/code points,
+	// but we need to reason about length for things like request size, so we use bytes in this code (and an individual
+	// unicode code point can be up to 4 bytes long)
+	return zeroIfNegative(*s.MaxLength()) * 4
+}