Skip to content

Commit 72a3520

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes-sigs#10809 from sbueringer/pr-sep-errors
🌱 Separate schema and CEL errors in CC variable validation
2 parents 41c38f2 + 9d8ada1 commit 72a3520

File tree

1 file changed

+66
-37
lines changed

1 file changed

+66
-37
lines changed

‎internal/topology/variables/clusterclass_variable_validation.go

Lines changed: 66 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ func validateRootSchema(ctx context.Context, oldClusterClassVariables, clusterCl
280280
opts.preexistingExpressions = findPreexistingExpressions(oldAPIExtensionsSchema)
281281
}
282282

283-
allErrs = append(allErrs, validateSchema(ctx, apiExtensionsSchema, fldPath, opts, celContext, nil)...)
283+
allErrs = append(allErrs, validateSchema(ctx, apiExtensionsSchema, fldPath, opts, celContext, nil).AllErrors()...)
284284
if celContext != nil && celContext.TotalCost != nil {
285285
if celContext.TotalCost.Total > StaticEstimatedCRDCostLimit {
286286
for _, expensive := range celContext.TotalCost.MostExpensive {
@@ -309,55 +309,58 @@ var supportedValidationReason = sets.NewString(
309309
string(clusterv1.FieldValueDuplicate),
310310
)
311311

312-
func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps, fldPath *field.Path, opts *validationOptions, celContext *apiextensionsvalidation.CELSchemaContext, uncorrelatablePath *field.Path) field.ErrorList {
313-
var allErrs field.ErrorList
312+
func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps, fldPath *field.Path, opts *validationOptions, celContext *apiextensionsvalidation.CELSchemaContext, uncorrelatablePath *field.Path) *OpenAPISchemaErrorList {
313+
allErrs := &OpenAPISchemaErrorList{SchemaErrors: field.ErrorList{}, CELErrors: field.ErrorList{}}
314314

315315
// Validate that type is one of the validVariableTypes.
316316
switch {
317317
case schema.Type == "":
318-
return field.ErrorList{field.Required(fldPath.Child("type"), "type cannot be empty")}
318+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("type"), "type cannot be empty"))
319+
return allErrs
319320
case !validVariableTypes.Has(schema.Type):
320-
return field.ErrorList{field.NotSupported(fldPath.Child("type"), schema.Type, sets.List(validVariableTypes))}
321+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.NotSupported(fldPath.Child("type"), schema.Type, sets.List(validVariableTypes)))
322+
return allErrs
321323
}
322324

323325
// If the structural schema is valid, ensure a schema validator can be constructed.
324326
validator, _, err := validation.NewSchemaValidator(schema)
325327
if err != nil {
326-
return append(allErrs, field.Invalid(fldPath, "", fmt.Sprintf("failed to build schema validator: %v", err)))
328+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath, "", fmt.Sprintf("failed to build schema validator: %v", err)))
329+
return allErrs
327330
}
328331

329332
if schema.Example != nil {
330333
if errs := validation.ValidateCustomResource(fldPath.Child("example"), *schema.Example, validator); len(errs) > 0 {
331-
allErrs = append(allErrs, errs...)
334+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, errs...)
332335
}
333336
}
334337

335338
for i, enum := range schema.Enum {
336339
if enum != nil {
337340
if errs := validation.ValidateCustomResource(fldPath.Child("enum").Index(i), enum, validator); len(errs) > 0 {
338-
allErrs = append(allErrs, errs...)
341+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, errs...)
339342
}
340343
}
341344
}
342345

343346
if schema.AdditionalProperties != nil {
344347
if len(schema.Properties) > 0 {
345-
allErrs = append(allErrs, field.Forbidden(fldPath, "additionalProperties and properties are mutually exclusive"))
348+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Forbidden(fldPath, "additionalProperties and properties are mutually exclusive"))
346349
}
347-
allErrs = append(allErrs, validateSchema(ctx, schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), opts, celContext.ChildAdditionalPropertiesContext(schema.AdditionalProperties.Schema), uncorrelatablePath)...)
350+
allErrs.AppendErrors(validateSchema(ctx, schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), opts, celContext.ChildAdditionalPropertiesContext(schema.AdditionalProperties.Schema), uncorrelatablePath))
348351
}
349352

350353
for propertyName, propertySchema := range schema.Properties {
351354
p := propertySchema
352-
allErrs = append(allErrs, validateSchema(ctx, &p, fldPath.Child("properties").Key(propertyName), opts, celContext.ChildPropertyContext(&p, propertyName), uncorrelatablePath)...)
355+
allErrs.AppendErrors(validateSchema(ctx, &p, fldPath.Child("properties").Key(propertyName), opts, celContext.ChildPropertyContext(&p, propertyName), uncorrelatablePath))
353356
}
354357

355358
if schema.Items != nil {
356359
// We cannot correlate old/new items on atomic list types, which is the only list type supported in ClusterClass variable schema.
357360
if uncorrelatablePath == nil {
358361
uncorrelatablePath = fldPath.Child("items")
359362
}
360-
allErrs = append(allErrs, validateSchema(ctx, schema.Items.Schema, fldPath.Child("items"), opts, celContext.ChildItemsContext(schema.Items.Schema), uncorrelatablePath)...)
363+
allErrs.AppendErrors(validateSchema(ctx, schema.Items.Schema, fldPath.Child("items"), opts, celContext.ChildItemsContext(schema.Items.Schema), uncorrelatablePath))
361364
}
362365

363366
// This validation is duplicated from upstream CRD validation at
@@ -366,40 +369,41 @@ func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps,
366369
// Return if schema is not a structural schema (this should enver happen, but let's handle it anyway).
367370
ss, err := structuralschema.NewStructural(schema)
368371
if err != nil {
369-
return append(allErrs, field.Invalid(fldPath, "", fmt.Sprintf("schema is not a structural schema: %v", err)))
372+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath, "", fmt.Sprintf("schema is not a structural schema: %v", err)))
373+
return allErrs
370374
}
371375

372376
for i, rule := range schema.XValidations {
373377
trimmedRule := strings.TrimSpace(rule.Rule)
374378
trimmedMsg := strings.TrimSpace(rule.Message)
375379
trimmedMsgExpr := strings.TrimSpace(rule.MessageExpression)
376380
if len(trimmedRule) == 0 {
377-
allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), "rule is not specified"))
381+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), "rule is not specified"))
378382
} else if len(rule.Message) > 0 && len(trimmedMsg) == 0 {
379-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must be non-empty if specified"))
383+
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"))
380384
} else if len(rule.Message) > 2048 {
381-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message[0:10]+"...", "message must have a maximum length of 2048 characters"))
385+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message[0:10]+"...", "message must have a maximum length of 2048 characters"))
382386
} else if hasNewlines(trimmedMsg) {
383-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must not contain line breaks"))
387+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must not contain line breaks"))
384388
} else if hasNewlines(trimmedRule) && len(trimmedMsg) == 0 {
385-
allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), "message must be specified if rule contains line breaks"))
389+
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"))
386390
}
387391
if len(rule.MessageExpression) > 0 && len(trimmedMsgExpr) == 0 {
388-
allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), "messageExpression must be non-empty if specified"))
392+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), "messageExpression must be non-empty if specified"))
389393
}
390394
if rule.Reason != nil && !supportedValidationReason.Has(string(*rule.Reason)) {
391-
allErrs = append(allErrs, field.NotSupported(fldPath.Child("x-kubernetes-validations").Index(i).Child("reason"), *rule.Reason, supportedValidationReason.List()))
395+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.NotSupported(fldPath.Child("x-kubernetes-validations").Index(i).Child("reason"), *rule.Reason, supportedValidationReason.List()))
392396
}
393397
trimmedFieldPath := strings.TrimSpace(rule.FieldPath)
394398
if len(rule.FieldPath) > 0 && len(trimmedFieldPath) == 0 {
395-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must be non-empty if specified"))
399+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must be non-empty if specified"))
396400
}
397401
if hasNewlines(rule.FieldPath) {
398-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must not contain line breaks"))
402+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must not contain line breaks"))
399403
}
400404
if len(rule.FieldPath) > 0 {
401405
if _, _, err := cel.ValidFieldPath(rule.FieldPath, ss); err != nil {
402-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, fmt.Sprintf("fieldPath must be a valid path: %v", err)))
406+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, fmt.Sprintf("fieldPath must be a valid path: %v", err)))
403407
}
404408
}
405409
}
@@ -408,49 +412,49 @@ func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps,
408412
// Invalid OpenAPISchemas are not always possible to convert into valid CEL DeclTypes, and can lead to CEL
409413
// validation error messages that are not actionable (will go away once the schema errors are resolved) and that
410414
// are difficult for CEL expression authors to understand.
411-
if len(allErrs) == 0 && celContext != nil {
415+
if len(allErrs.SchemaErrors) == 0 && celContext != nil {
412416
typeInfo, err := celContext.TypeInfo()
413417
if err != nil {
414-
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)))
418+
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)))
415419
} else if typeInfo == nil {
416-
allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), fmt.Errorf("internal error: failed to retrieve type information for x-kubernetes-validations")))
420+
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")))
417421
} else {
418422
// Note: k/k CRD validation also uses celconfig.PerCallLimit when creating the validator.
419423
// The current PerCallLimit gives roughly 0.1 second for each expression validation call.
420424
compResults, err := cel.Compile(typeInfo.Schema, typeInfo.DeclType, celconfig.PerCallLimit, opts.celEnvironmentSet, opts.preexistingExpressions)
421425
if err != nil {
422-
allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), err))
426+
allErrs.CELErrors = append(allErrs.CELErrors, field.InternalError(fldPath.Child("x-kubernetes-validations"), err))
423427
} else {
424428
for i, cr := range compResults {
425429
expressionCost := getExpressionCost(cr, celContext)
426430
if expressionCost > StaticEstimatedCostLimit {
427431
costErrorMsg := getCostErrorMessage("estimated rule cost", expressionCost, StaticEstimatedCostLimit)
428-
allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), costErrorMsg))
432+
allErrs.CELErrors = append(allErrs.CELErrors, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), costErrorMsg))
429433
}
430434
if celContext.TotalCost != nil {
431435
celContext.TotalCost.ObserveExpressionCost(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), expressionCost)
432436
}
433437
if cr.Error != nil {
434438
if cr.Error.Type == apiservercel.ErrorTypeRequired {
435-
allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), cr.Error.Detail))
439+
allErrs.CELErrors = append(allErrs.CELErrors, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), cr.Error.Detail))
436440
} else {
437-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i], cr.Error.Detail))
441+
allErrs.CELErrors = append(allErrs.CELErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i], cr.Error.Detail))
438442
}
439443
}
440444
if cr.MessageExpressionError != nil {
441-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), schema.XValidations[i], cr.MessageExpressionError.Detail))
445+
allErrs.CELErrors = append(allErrs.CELErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), schema.XValidations[i], cr.MessageExpressionError.Detail))
442446
} else if cr.MessageExpression != nil {
443447
if cr.MessageExpressionMaxCost > StaticEstimatedCostLimit {
444448
costErrorMsg := getCostErrorMessage("estimated messageExpression cost", cr.MessageExpressionMaxCost, StaticEstimatedCostLimit)
445-
allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), costErrorMsg))
449+
allErrs.CELErrors = append(allErrs.CELErrors, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), costErrorMsg))
446450
}
447451
if celContext.TotalCost != nil {
448452
celContext.TotalCost.ObserveExpressionCost(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), cr.MessageExpressionMaxCost)
449453
}
450454
}
451455
if cr.UsesOldSelf {
452456
if uncorrelatablePath != nil {
453-
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)))
457+
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)))
454458
}
455459
}
456460
}
@@ -459,7 +463,7 @@ func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps,
459463
}
460464

461465
// Return if we found some errors in the CEL validations.
462-
if len(allErrs) > 0 {
466+
if len(allErrs.AllErrors()) > 0 {
463467
return allErrs
464468
}
465469

@@ -468,16 +472,17 @@ func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps,
468472
// CEL validation for Default values was already done via: structuraldefaulting.ValidateDefaults
469473
celValidator := cel.NewValidator(ss, false, celconfig.PerCallLimit)
470474
if celValidator == nil {
471-
return append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations"), "", "failed to create CEL validator"))
475+
allErrs.CELErrors = append(allErrs.CELErrors, field.Invalid(fldPath.Child("x-kubernetes-validations"), "", "failed to create CEL validator"))
476+
return allErrs
472477
}
473478
if schema.Example != nil {
474479
errs, _ := celValidator.Validate(ctx, fldPath.Child("example"), ss, *schema.Example, nil, celconfig.RuntimeCELCostBudget)
475-
allErrs = append(allErrs, errs...)
480+
allErrs.CELErrors = append(allErrs.CELErrors, errs...)
476481
}
477482
for i, enum := range schema.Enum {
478483
if enum != nil {
479484
errs, _ := celValidator.Validate(ctx, fldPath.Child("enum").Index(i), ss, enum, nil, celconfig.RuntimeCELCostBudget)
480-
allErrs = append(allErrs, errs...)
485+
allErrs.CELErrors = append(allErrs.CELErrors, errs...)
481486
}
482487
}
483488
}
@@ -488,6 +493,30 @@ func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps,
488493
// The following funcs are all duplicated from upstream CRD validation at
489494
// https://github.com/kubernetes/apiextensions-apiserver/blob/v0.30.0/pkg/apis/apiextensions/validation/validation.go.
490495

496+
// OpenAPISchemaErrorList tracks validation errors reported
497+
// with CEL related errors kept separate from schema related errors.
498+
type OpenAPISchemaErrorList struct {
499+
SchemaErrors field.ErrorList
500+
CELErrors field.ErrorList
501+
}
502+
503+
// AppendErrors appends all errors in the provided list with the errors of this list.
504+
func (o *OpenAPISchemaErrorList) AppendErrors(list *OpenAPISchemaErrorList) {
505+
if o == nil || list == nil {
506+
return
507+
}
508+
o.SchemaErrors = append(o.SchemaErrors, list.SchemaErrors...)
509+
o.CELErrors = append(o.CELErrors, list.CELErrors...)
510+
}
511+
512+
// AllErrors returns a list containing both schema and CEL errors.
513+
func (o *OpenAPISchemaErrorList) AllErrors() field.ErrorList {
514+
if o == nil {
515+
return field.ErrorList{}
516+
}
517+
return append(o.SchemaErrors, o.CELErrors...)
518+
}
519+
491520
// validationOptions groups several validation options, to avoid passing multiple bool parameters to methods.
492521
type validationOptions struct {
493522
preexistingExpressions preexistingExpressions

0 commit comments

Comments
 (0)