From 95b0d44a56b74d72efc629871864202c1510e0ee Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Mon, 25 Jul 2022 14:25:55 -0400 Subject: [PATCH] Skip CEL expression validation if OpenAPIv3 schema is invalid Co-authored-by: Jordan Liggitt --- .../apiextensions/validation/validation.go | 173 +++++++++++------- .../validation/validation_test.go | 166 ++++++++++++++++- 2 files changed, 270 insertions(+), 69 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go index 1f51c641930..855bb249311 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go @@ -722,10 +722,35 @@ func validateCustomResourceDefinitionValidation(ctx context.Context, customResou requireValidPropertyType: opts.requireValidPropertyType, } - celContext := RootCELContext(schema) - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema, true, &opts, celContext)...) + var celContext *CELSchemaContext + var structuralSchemaInitErrs field.ErrorList + if opts.requireStructuralSchema { + if ss, err := structuralschema.NewStructural(schema); err != nil { + // These validation errors overlap with OpenAPISchema validation errors so we keep track of them + // separately and only show them if OpenAPISchema validation does not report any errors. + structuralSchemaInitErrs = append(structuralSchemaInitErrs, field.Invalid(fldPath.Child("openAPIV3Schema"), "", err.Error())) + } else if validationErrors := structuralschema.ValidateStructural(fldPath.Child("openAPIV3Schema"), ss); len(validationErrors) > 0 { + allErrs = append(allErrs, validationErrors...) + } else if validationErrors, err := structuraldefaulting.ValidateDefaults(ctx, fldPath.Child("openAPIV3Schema"), ss, true, opts.requirePrunedDefaults); err != nil { + // this should never happen + allErrs = append(allErrs, field.Invalid(fldPath.Child("openAPIV3Schema"), "", err.Error())) + } else if len(validationErrors) > 0 { + allErrs = append(allErrs, validationErrors...) + } else { + // Only initialize CEL rule validation context if the structural schemas are valid. + // A nil CELSchemaContext indicates that no CEL validation should be attempted. + celContext = RootCELContext(schema) + } + } + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema, true, &opts, celContext).AllErrors()...) - if celContext.TotalCost != nil { + if len(allErrs) == 0 && len(structuralSchemaInitErrs) > 0 { + // Structural schema initialization errors overlap with OpenAPISchema validation errors so we only show them + // if there are no OpenAPISchema validation errors. + allErrs = append(allErrs, structuralSchemaInitErrs...) + } + + if celContext != nil && celContext.TotalCost != nil { if celContext.TotalCost.Total > StaticEstimatedCRDCostLimit { for _, expensive := range celContext.TotalCost.MostExpensive { costErrorMsg := fmt.Sprintf("contributed to estimated rule cost total exceeding cost limit for entire OpenAPIv3 schema") @@ -736,22 +761,6 @@ func validateCustomResourceDefinitionValidation(ctx context.Context, customResou allErrs = append(allErrs, field.Forbidden(fldPath.Child("openAPIV3Schema"), costErrorMsg)) } } - - if opts.requireStructuralSchema { - if ss, err := structuralschema.NewStructural(schema); err != nil { - // if the generic schema validation did its job, we should never get an error here. Hence, we hide it if there are validation errors already. - if len(allErrs) == 0 { - allErrs = append(allErrs, field.Invalid(fldPath.Child("openAPIV3Schema"), "", err.Error())) - } - } else if validationErrors := structuralschema.ValidateStructural(fldPath.Child("openAPIV3Schema"), ss); len(validationErrors) > 0 { - allErrs = append(allErrs, validationErrors...) - } else if validationErrors, err := structuraldefaulting.ValidateDefaults(ctx, fldPath.Child("openAPIV3Schema"), ss, true, opts.requirePrunedDefaults); err != nil { - // this should never happen - allErrs = append(allErrs, field.Invalid(fldPath.Child("openAPIV3Schema"), "", err.Error())) - } else { - allErrs = append(allErrs, validationErrors...) - } - } } // if validation passed otherwise, make sure we can actually construct a schema validator from this custom resource validation. @@ -765,17 +774,41 @@ func validateCustomResourceDefinitionValidation(ctx context.Context, customResou var metaFields = sets.NewString("metadata", "kind", "apiVersion") +// OpenAPISchemaErrorList tracks all validation errors reported ValidateCustomResourceDefinitionOpenAPISchema +// with CEL related errors kept separate from schema related errors. +type OpenAPISchemaErrorList struct { + SchemaErrors field.ErrorList + CELErrors field.ErrorList +} + +// AppendErrors appends all errors in the provided list with the errors of this list. +func (o *OpenAPISchemaErrorList) AppendErrors(list *OpenAPISchemaErrorList) { + if o == nil || list == nil { + return + } + o.SchemaErrors = append(o.SchemaErrors, list.SchemaErrors...) + o.CELErrors = append(o.CELErrors, list.CELErrors...) +} + +// AllErrors returns a list containing both schema and CEL errors. +func (o *OpenAPISchemaErrorList) AllErrors() field.ErrorList { + if o == nil { + return field.ErrorList{} + } + return append(o.SchemaErrors, o.CELErrors...) +} + // ValidateCustomResourceDefinitionOpenAPISchema statically validates -func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSchemaProps, fldPath *field.Path, ssv specStandardValidator, isRoot bool, opts *validationOptions, celContext *CELSchemaContext) field.ErrorList { - allErrs := field.ErrorList{} +func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSchemaProps, fldPath *field.Path, ssv specStandardValidator, isRoot bool, opts *validationOptions, celContext *CELSchemaContext) *OpenAPISchemaErrorList { + allErrs := &OpenAPISchemaErrorList{SchemaErrors: field.ErrorList{}, CELErrors: field.ErrorList{}} if schema == nil { return allErrs } - allErrs = append(allErrs, ssv.validate(schema, fldPath)...) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, ssv.validate(schema, fldPath)...) if schema.UniqueItems == true { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("uniqueItems"), "uniqueItems cannot be set to true since the runtime complexity becomes quadratic")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Forbidden(fldPath.Child("uniqueItems"), "uniqueItems cannot be set to true since the runtime complexity becomes quadratic")) } // additionalProperties and properties are mutual exclusive because otherwise they @@ -790,7 +823,7 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch if schema.AdditionalProperties != nil { if len(schema.Properties) != 0 { if schema.AdditionalProperties.Allows == false || schema.AdditionalProperties.Schema != nil { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("additionalProperties"), "additionalProperties and properties are mutual exclusive")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Forbidden(fldPath.Child("additionalProperties"), "additionalProperties and properties are mutual exclusive")) } } // Note: we forbid additionalProperties at resource root, both embedded and top-level. @@ -800,7 +833,7 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch // we have to forbid defaults inside additionalProperties because pruning without actual value is ambiguous subSsv = ssv.withForbiddenDefaults("inside additionalProperties applying to object metadata") } - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), subSsv, false, opts, celContext.ChildAdditionalPropertiesContext(schema.AdditionalProperties.Schema))...) + allErrs.AppendErrors(ValidateCustomResourceDefinitionOpenAPISchema(schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), subSsv, false, opts, celContext.ChildAdditionalPropertiesContext(schema.AdditionalProperties.Schema))) } if len(schema.Properties) != 0 { @@ -819,37 +852,37 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch } } propertySchema := jsonSchema - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&propertySchema, fldPath.Child("properties").Key(property), subSsv, false, opts, celContext.ChildPropertyContext(&propertySchema, property))...) + allErrs.AppendErrors(ValidateCustomResourceDefinitionOpenAPISchema(&propertySchema, fldPath.Child("properties").Key(property), subSsv, false, opts, celContext.ChildPropertyContext(&propertySchema, property))) } } - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.Not, fldPath.Child("not"), ssv, false, opts, nil)...) + allErrs.AppendErrors(ValidateCustomResourceDefinitionOpenAPISchema(schema.Not, fldPath.Child("not"), ssv, false, opts, nil)) if len(schema.AllOf) != 0 { for i, jsonSchema := range schema.AllOf { allOfSchema := jsonSchema - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&allOfSchema, fldPath.Child("allOf").Index(i), ssv, false, opts, nil)...) + allErrs.AppendErrors(ValidateCustomResourceDefinitionOpenAPISchema(&allOfSchema, fldPath.Child("allOf").Index(i), ssv, false, opts, nil)) } } if len(schema.OneOf) != 0 { for i, jsonSchema := range schema.OneOf { oneOfSchema := jsonSchema - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&oneOfSchema, fldPath.Child("oneOf").Index(i), ssv, false, opts, nil)...) + allErrs.AppendErrors(ValidateCustomResourceDefinitionOpenAPISchema(&oneOfSchema, fldPath.Child("oneOf").Index(i), ssv, false, opts, nil)) } } if len(schema.AnyOf) != 0 { for i, jsonSchema := range schema.AnyOf { anyOfSchema := jsonSchema - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&anyOfSchema, fldPath.Child("anyOf").Index(i), ssv, false, opts, nil)...) + allErrs.AppendErrors(ValidateCustomResourceDefinitionOpenAPISchema(&anyOfSchema, fldPath.Child("anyOf").Index(i), ssv, false, opts, nil)) } } if len(schema.Definitions) != 0 { for definition, jsonSchema := range schema.Definitions { definitionSchema := jsonSchema - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&definitionSchema, fldPath.Child("definitions").Key(definition), ssv, false, opts, nil)...) + allErrs.AppendErrors(ValidateCustomResourceDefinitionOpenAPISchema(&definitionSchema, fldPath.Child("definitions").Key(definition), ssv, false, opts, nil)) } } @@ -863,84 +896,84 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch subSsv = subSsv.withForbidOldSelfValidations(fldPath) } - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.Items.Schema, fldPath.Child("items"), subSsv, false, opts, celContext.ChildItemsContext(schema.Items.Schema))...) + allErrs.AppendErrors(ValidateCustomResourceDefinitionOpenAPISchema(schema.Items.Schema, fldPath.Child("items"), subSsv, false, opts, celContext.ChildItemsContext(schema.Items.Schema))) if len(schema.Items.JSONSchemas) != 0 { for i, jsonSchema := range schema.Items.JSONSchemas { itemsSchema := jsonSchema - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&itemsSchema, fldPath.Child("items").Index(i), subSsv, false, opts, celContext.ChildItemsContext(&itemsSchema))...) + allErrs.AppendErrors(ValidateCustomResourceDefinitionOpenAPISchema(&itemsSchema, fldPath.Child("items").Index(i), subSsv, false, opts, celContext.ChildItemsContext(&itemsSchema))) } } } if schema.Dependencies != nil { for dependency, jsonSchemaPropsOrStringArray := range schema.Dependencies { - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(jsonSchemaPropsOrStringArray.Schema, fldPath.Child("dependencies").Key(dependency), ssv, false, opts, nil)...) + allErrs.AppendErrors(ValidateCustomResourceDefinitionOpenAPISchema(jsonSchemaPropsOrStringArray.Schema, fldPath.Child("dependencies").Key(dependency), ssv, false, opts, nil)) } } if schema.XPreserveUnknownFields != nil && *schema.XPreserveUnknownFields == false { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-preserve-unknown-fields"), *schema.XPreserveUnknownFields, "must be true or undefined")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-preserve-unknown-fields"), *schema.XPreserveUnknownFields, "must be true or undefined")) } if schema.XMapType != nil && schema.Type != "object" { if len(schema.Type) == 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("type"), "must be object if x-kubernetes-map-type is specified")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("type"), "must be object if x-kubernetes-map-type is specified")) } else { - allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), schema.Type, "must be object if x-kubernetes-map-type is specified")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("type"), schema.Type, "must be object if x-kubernetes-map-type is specified")) } } if schema.XMapType != nil && *schema.XMapType != "atomic" && *schema.XMapType != "granular" { - allErrs = append(allErrs, field.NotSupported(fldPath.Child("x-kubernetes-map-type"), *schema.XMapType, []string{"atomic", "granular"})) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.NotSupported(fldPath.Child("x-kubernetes-map-type"), *schema.XMapType, []string{"atomic", "granular"})) } if schema.XListType != nil && schema.Type != "array" { if len(schema.Type) == 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("type"), "must be array if x-kubernetes-list-type is specified")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("type"), "must be array if x-kubernetes-list-type is specified")) } else { - allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), schema.Type, "must be array if x-kubernetes-list-type is specified")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("type"), schema.Type, "must be array if x-kubernetes-list-type is specified")) } } else if opts.requireAtomicSetType && schema.XListType != nil && *schema.XListType == "set" && schema.Items != nil && schema.Items.Schema != nil { // by structural schema items are present is := schema.Items.Schema switch is.Type { case "array": if is.XListType != nil && *is.XListType != "atomic" { // atomic is the implicit default behaviour if unset, hence != atomic is wrong - allErrs = append(allErrs, field.Invalid(fldPath.Child("items").Child("x-kubernetes-list-type"), is.XListType, "must be atomic as item of a list with x-kubernetes-list-type=set")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("items").Child("x-kubernetes-list-type"), is.XListType, "must be atomic as item of a list with x-kubernetes-list-type=set")) } case "object": if is.XMapType == nil || *is.XMapType != "atomic" { // granular is the implicit default behaviour if unset, hence nil and != atomic are wrong - allErrs = append(allErrs, field.Invalid(fldPath.Child("items").Child("x-kubernetes-map-type"), is.XListType, "must be atomic as item of a list with x-kubernetes-list-type=set")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("items").Child("x-kubernetes-map-type"), is.XListType, "must be atomic as item of a list with x-kubernetes-list-type=set")) } } } if schema.XListType != nil && *schema.XListType != "atomic" && *schema.XListType != "set" && *schema.XListType != "map" { - allErrs = append(allErrs, field.NotSupported(fldPath.Child("x-kubernetes-list-type"), *schema.XListType, []string{"atomic", "set", "map"})) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.NotSupported(fldPath.Child("x-kubernetes-list-type"), *schema.XListType, []string{"atomic", "set", "map"})) } if len(schema.XListMapKeys) > 0 { if schema.XListType == nil { - allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-list-type"), "must be map if x-kubernetes-list-map-keys is non-empty")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("x-kubernetes-list-type"), "must be map if x-kubernetes-list-map-keys is non-empty")) } else if *schema.XListType != "map" { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-list-type"), *schema.XListType, "must be map if x-kubernetes-list-map-keys is non-empty")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-list-type"), *schema.XListType, "must be map if x-kubernetes-list-map-keys is non-empty")) } } if schema.XListType != nil && *schema.XListType == "map" { if len(schema.XListMapKeys) == 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-list-map-keys"), "must not be empty if x-kubernetes-list-type is map")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("x-kubernetes-list-map-keys"), "must not be empty if x-kubernetes-list-type is map")) } if schema.Items == nil { - allErrs = append(allErrs, field.Required(fldPath.Child("items"), "must have a schema if x-kubernetes-list-type is map")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("items"), "must have a schema if x-kubernetes-list-type is map")) } if schema.Items != nil && schema.Items.Schema == nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("items"), schema.Items, "must only have a single schema if x-kubernetes-list-type is map")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("items"), schema.Items, "must only have a single schema if x-kubernetes-list-type is map")) } if schema.Items != nil && schema.Items.Schema != nil && schema.Items.Schema.Type != "object" { - allErrs = append(allErrs, field.Invalid(fldPath.Child("items").Child("type"), schema.Items.Schema.Type, "must be object if parent array's x-kubernetes-list-type is map")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("items").Child("type"), schema.Items.Schema.Type, "must be object if parent array's x-kubernetes-list-type is map")) } if schema.Items != nil && schema.Items.Schema != nil && schema.Items.Schema.Type == "object" { @@ -948,64 +981,72 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch for _, k := range schema.XListMapKeys { if s, ok := schema.Items.Schema.Properties[k]; ok { if s.Type == "array" || s.Type == "object" { - allErrs = append(allErrs, field.Invalid(fldPath.Child("items").Child("properties").Key(k).Child("type"), schema.Items.Schema.Type, "must be a scalar type if parent array's x-kubernetes-list-type is map")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("items").Child("properties").Key(k).Child("type"), schema.Items.Schema.Type, "must be a scalar type if parent array's x-kubernetes-list-type is map")) } } else { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-list-map-keys"), schema.XListMapKeys, "entries must all be names of item properties")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-list-map-keys"), schema.XListMapKeys, "entries must all be names of item properties")) } if _, ok := keys[k]; ok { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-list-map-keys"), schema.XListMapKeys, "must not contain duplicate entries")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-list-map-keys"), schema.XListMapKeys, "must not contain duplicate entries")) } keys[k] = struct{}{} } } } + if opts.requireMapListKeysMapSetValidation { + allErrs.SchemaErrors = append(allErrs.SchemaErrors, validateMapListKeysMapSet(schema, fldPath)...) + } + if len(schema.XValidations) > 0 { for i, rule := range schema.XValidations { trimmedRule := strings.TrimSpace(rule.Rule) trimmedMsg := strings.TrimSpace(rule.Message) if len(trimmedRule) == 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), "rule is not specified")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), "rule is not specified")) } else if len(rule.Message) > 0 && len(trimmedMsg) == 0 { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must be non-empty if specified")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must be non-empty if specified")) } else if hasNewlines(trimmedMsg) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must not contain line breaks")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must not contain line breaks")) } else if hasNewlines(trimmedRule) && len(trimmedMsg) == 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), "message must be specified if rule contains line breaks")) + allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), "message must be specified if rule contains line breaks")) } } - if celContext != nil { + // If any schema related validation errors have been found at this level or deeper, skip CEL expression validation. + // Invalid OpenAPISchemas are not always possible to convert into valid CEL DeclTypes, and can lead to CEL + // validation error messages that are not actionable (will go away once the schema errors are resolved) and that + // are difficult for CEL expression authors to understand. + if len(allErrs.SchemaErrors) == 0 && celContext != nil { typeInfo, err := celContext.TypeInfo() if err != nil { - allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), fmt.Errorf("internal error: failed to construct type information for x-kubernetes-validations rules: %s", err))) + allErrs.CELErrors = append(allErrs.CELErrors, field.InternalError(fldPath.Child("x-kubernetes-validations"), fmt.Errorf("internal error: failed to construct type information for x-kubernetes-validations rules: %s", err))) } else if typeInfo == nil { - allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), fmt.Errorf("internal error: failed to retrieve type information for x-kubernetes-validations"))) + allErrs.CELErrors = append(allErrs.CELErrors, field.InternalError(fldPath.Child("x-kubernetes-validations"), fmt.Errorf("internal error: failed to retrieve type information for x-kubernetes-validations"))) } else { compResults, err := cel.Compile(typeInfo.Schema, typeInfo.DeclType, cel.PerCallLimit) if err != nil { - allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), err)) + allErrs.CELErrors = append(allErrs.CELErrors, field.InternalError(fldPath.Child("x-kubernetes-validations"), err)) } else { for i, cr := range compResults { expressionCost := getExpressionCost(cr, celContext) if expressionCost > StaticEstimatedCostLimit { costErrorMsg := getCostErrorMessage("estimated rule cost", expressionCost, StaticEstimatedCostLimit) - allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), costErrorMsg)) + allErrs.CELErrors = append(allErrs.CELErrors, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), costErrorMsg)) } if celContext.TotalCost != nil { celContext.TotalCost.ObserveExpressionCost(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), expressionCost) } if cr.Error != nil { if cr.Error.Type == cel.ErrorTypeRequired { - allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), cr.Error.Detail)) + allErrs.CELErrors = append(allErrs.CELErrors, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), cr.Error.Detail)) } else { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i], cr.Error.Detail)) + allErrs.CELErrors = append(allErrs.CELErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i], cr.Error.Detail)) } } if cr.TransitionRule { if uncorrelatablePath := ssv.forbidOldSelfValidations(); uncorrelatablePath != nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i].Rule, fmt.Sprintf("oldSelf cannot be used on the uncorrelatable portion of the schema within %v", uncorrelatablePath))) + allErrs.CELErrors = append(allErrs.CELErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i].Rule, fmt.Sprintf("oldSelf cannot be used on the uncorrelatable portion of the schema within %v", uncorrelatablePath))) } } } @@ -1014,10 +1055,6 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch } } - if opts.requireMapListKeysMapSetValidation { - allErrs = append(allErrs, validateMapListKeysMapSet(schema, fldPath)...) - } - return allErrs } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go index 05f3d6a9b9f..853110c2d6d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go @@ -7598,6 +7598,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "forbid transition rule on element of list of type atomic", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7624,6 +7625,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "forbid transition rule on element of list defaulting to type atomic", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7649,6 +7651,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow transition rule on list of type atomic", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7695,6 +7698,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "forbid transition rule on element of list of type set", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7722,6 +7726,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow transition rule on list of type set", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7746,6 +7751,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow transition rule on element of list of type map", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7773,6 +7779,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow transition rule on list of type map", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7801,6 +7808,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow transition rule on element of map of type granular", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7824,6 +7832,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "forbid transition rule on element of map of unrecognized type", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7851,6 +7860,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow transition rule on element of map defaulting to type granular", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7873,6 +7883,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow transition rule on map of type granular", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7890,6 +7901,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow transition rule on map defaulting to type granular", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7906,6 +7918,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow transition rule on element of map of type atomic", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7928,6 +7941,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow transition rule on map of type atomic", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7945,6 +7959,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "forbid double-nested rule with no limit set", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -7982,6 +7997,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "forbid double-nested rule with one limit set", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8019,6 +8035,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow double-nested rule with three limits set", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8052,6 +8069,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "allow double-nested rule with one limit set on outermost array", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8084,6 +8102,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "check for cardinality of 1 under root object", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8101,6 +8120,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "forbid validation rules where cost total exceeds total limit", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8151,8 +8171,142 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { forbidden("spec.validation.openAPIV3Schema"), }, }, + { + name: "skip CEL expression validation when OpenAPIv3 schema is an invalid structural schema", + opts: validationOptions{requireStructuralSchema: true}, + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + // illegal to have both Properties and AdditionalProperties + Properties: map[string]apiextensions.JSONSchemaProps{ + "field": { + Type: "integer", + }, + }, + AdditionalProperties: &apiextensions.JSONSchemaPropsOrBool{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "string", + }, + }, + XValidations: apiextensions.ValidationRules{ + {Rule: "self.invalidFieldName > 50"}, // invalid CEL rule + }, + }, + }, + expectedErrors: []validationMatch{ + forbidden("spec.validation.openAPIV3Schema.additionalProperties"), // illegal to have both properties and additional properties + forbidden("spec.validation.openAPIV3Schema.additionalProperties"), // structural schema rule: illegal to have additional properties at root + // Error for invalid CEL rule is NOT expected here because CEL rules are not checked when the schema is invalid + }, + }, + { + name: "skip CEL expression validation when OpenAPIv3 schema is an invalid structural schema at level below", + opts: validationOptions{requireStructuralSchema: true}, + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "field": { + Type: "object", + // illegal to have both Properties and AdditionalProperties + Properties: map[string]apiextensions.JSONSchemaProps{ + "field": { + Type: "integer", + }, + }, + AdditionalProperties: &apiextensions.JSONSchemaPropsOrBool{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "string", + }, + }, + }, + }, + XValidations: apiextensions.ValidationRules{ + {Rule: "self.invalidFieldName > 50"}, + }, + }, + }, + expectedErrors: []validationMatch{ + forbidden("spec.validation.openAPIV3Schema.properties[field].additionalProperties"), + }, + }, + { + // So long at the schema information accessible to the CEL expression is valid, the expression should be validated. + name: "do not skip when OpenAPIv3 schema is an invalid structural schema in a separate part of the schema tree", + opts: validationOptions{requireStructuralSchema: true}, + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "a": { + Type: "object", + // illegal to have both Properties and AdditionalProperties + Properties: map[string]apiextensions.JSONSchemaProps{ + "field": { + Type: "integer", + }, + }, + AdditionalProperties: &apiextensions.JSONSchemaPropsOrBool{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "string", + }, + }, + }, + "b": { + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "field": { + Type: "integer", + }, + }, + XValidations: apiextensions.ValidationRules{ + {Rule: "self.invalidFieldName > 50"}, + }, + }, + }, + }, + }, + expectedErrors: []validationMatch{ + forbidden("spec.validation.openAPIV3Schema.properties[a].additionalProperties"), + invalid("spec.validation.openAPIV3Schema.properties[b].x-kubernetes-validations[0].rule"), + }, + }, + { + // So long at the schema information accessible to the CEL expression is valid, the expression should be validated. + name: "do not skip CEL expression validation when OpenAPIv3 schema is an invalid structural schema at level above", + opts: validationOptions{requireStructuralSchema: true}, + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "a": { + Type: "object", + // illegal to have both Properties and AdditionalProperties + Properties: map[string]apiextensions.JSONSchemaProps{ + "b": { + Type: "integer", + XValidations: apiextensions.ValidationRules{ + {Rule: "self == 'abc'"}, + }, + }, + }, + AdditionalProperties: &apiextensions.JSONSchemaPropsOrBool{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "string", + }, + }, + }, + }, + }, + }, + expectedErrors: []validationMatch{ + forbidden("spec.validation.openAPIV3Schema.properties[a].additionalProperties"), + invalid("spec.validation.openAPIV3Schema.properties[a].properties[b].x-kubernetes-validations[0].rule"), + }, + }, { name: "x-kubernetes-validations rule validated for escaped property name", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8172,6 +8326,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "x-kubernetes-validations rule validated under array items", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8196,6 +8351,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "x-kubernetes-validations rule validated under array items, parent has rule", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8222,6 +8378,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "x-kubernetes-validations rule validated under additionalProperties", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8246,6 +8403,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "x-kubernetes-validations rule validated under additionalProperties, parent has rule", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8273,6 +8431,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "x-kubernetes-validations rule validated under unescaped property name", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8292,6 +8451,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "x-kubernetes-validations rule validated under unescaped property name, parent has rule", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8314,6 +8474,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "x-kubernetes-validations rule validated under escaped property name", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8333,6 +8494,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "x-kubernetes-validations rule validated under escaped property name, parent has rule", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8355,6 +8517,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "x-kubernetes-validations rule validated under unescapable property name", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8374,6 +8537,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, { name: "x-kubernetes-validations rule validated under unescapable property name, parent has rule", + opts: validationOptions{requireStructuralSchema: true}, input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", @@ -8814,7 +8978,7 @@ func TestCelContext(t *testing.T) { disallowDefaultsReason: opts.disallowDefaultsReason, requireValidPropertyType: opts.requireValidPropertyType, } - errors := ValidateCustomResourceDefinitionOpenAPISchema(tt.schema, field.NewPath("openAPIV3Schema"), openAPIV3Schema, true, &opts, celContext) + errors := ValidateCustomResourceDefinitionOpenAPISchema(tt.schema, field.NewPath("openAPIV3Schema"), openAPIV3Schema, true, &opts, celContext).AllErrors() if len(errors) != 0 { t.Errorf("Expected no validate errors but got %v", errors) }