Skip to content

Commit 9d8ada1

Browse files
sbueringersbueringer
committed
Separate schema and CEL errors in CC variable validation
Signed-off-by: Stefan Büringer [email protected]
1 parent ada2764 commit 9d8ada1

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
@@ -246,7 +246,7 @@ func validateRootSchema(ctx context.Context, oldClusterClassVariables, clusterCl
246246
opts.preexistingExpressions = findPreexistingExpressions(oldAPIExtensionsSchema)
247247
}
248248

249-
allErrs = append(allErrs, validateSchema(ctx, apiExtensionsSchema, fldPath, opts, celContext, nil)...)
249+
allErrs = append(allErrs, validateSchema(ctx, apiExtensionsSchema, fldPath, opts, celContext, nil).AllErrors()...)
250250
if celContext != nil && celContext.TotalCost != nil {
251251
if celContext.TotalCost.Total > StaticEstimatedCRDCostLimit {
252252
for _, expensive := range celContext.TotalCost.MostExpensive {
@@ -275,55 +275,58 @@ var supportedValidationReason = sets.NewString(
275275
string(clusterv1.FieldValueDuplicate),
276276
)
277277

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

281281
// Validate that type is one of the validVariableTypes.
282282
switch {
283283
case schema.Type == "":
284-
return field.ErrorList{field.Required(fldPath.Child("type"), "type cannot be empty")}
284+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("type"), "type cannot be empty"))
285+
return allErrs
285286
case !validVariableTypes.Has(schema.Type):
286-
return field.ErrorList{field.NotSupported(fldPath.Child("type"), schema.Type, sets.List(validVariableTypes))}
287+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.NotSupported(fldPath.Child("type"), schema.Type, sets.List(validVariableTypes)))
288+
return allErrs
287289
}
288290

289291
// If the structural schema is valid, ensure a schema validator can be constructed.
290292
validator, _, err := validation.NewSchemaValidator(schema)
291293
if err != nil {
292-
return append(allErrs, field.Invalid(fldPath, "", fmt.Sprintf("failed to build schema validator: %v", err)))
294+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath, "", fmt.Sprintf("failed to build schema validator: %v", err)))
295+
return allErrs
293296
}
294297

295298
if schema.Example != nil {
296299
if errs := validation.ValidateCustomResource(fldPath.Child("example"), *schema.Example, validator); len(errs) > 0 {
297-
allErrs = append(allErrs, errs...)
300+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, errs...)
298301
}
299302
}
300303

301304
for i, enum := range schema.Enum {
302305
if enum != nil {
303306
if errs := validation.ValidateCustomResource(fldPath.Child("enum").Index(i), enum, validator); len(errs) > 0 {
304-
allErrs = append(allErrs, errs...)
307+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, errs...)
305308
}
306309
}
307310
}
308311

309312
if schema.AdditionalProperties != nil {
310313
if len(schema.Properties) > 0 {
311-
allErrs = append(allErrs, field.Forbidden(fldPath, "additionalProperties and properties are mutually exclusive"))
314+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Forbidden(fldPath, "additionalProperties and properties are mutually exclusive"))
312315
}
313-
allErrs = append(allErrs, validateSchema(ctx, schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), opts, celContext.ChildAdditionalPropertiesContext(schema.AdditionalProperties.Schema), uncorrelatablePath)...)
316+
allErrs.AppendErrors(validateSchema(ctx, schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), opts, celContext.ChildAdditionalPropertiesContext(schema.AdditionalProperties.Schema), uncorrelatablePath))
314317
}
315318

316319
for propertyName, propertySchema := range schema.Properties {
317320
p := propertySchema
318-
allErrs = append(allErrs, validateSchema(ctx, &p, fldPath.Child("properties").Key(propertyName), opts, celContext.ChildPropertyContext(&p, propertyName), uncorrelatablePath)...)
321+
allErrs.AppendErrors(validateSchema(ctx, &p, fldPath.Child("properties").Key(propertyName), opts, celContext.ChildPropertyContext(&p, propertyName), uncorrelatablePath))
319322
}
320323

321324
if schema.Items != nil {
322325
// We cannot correlate old/new items on atomic list types, which is the only list type supported in ClusterClass variable schema.
323326
if uncorrelatablePath == nil {
324327
uncorrelatablePath = fldPath.Child("items")
325328
}
326-
allErrs = append(allErrs, validateSchema(ctx, schema.Items.Schema, fldPath.Child("items"), opts, celContext.ChildItemsContext(schema.Items.Schema), uncorrelatablePath)...)
329+
allErrs.AppendErrors(validateSchema(ctx, schema.Items.Schema, fldPath.Child("items"), opts, celContext.ChildItemsContext(schema.Items.Schema), uncorrelatablePath))
327330
}
328331

329332
// This validation is duplicated from upstream CRD validation at
@@ -332,40 +335,41 @@ func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps,
332335
// Return if schema is not a structural schema (this should enver happen, but let's handle it anyway).
333336
ss, err := structuralschema.NewStructural(schema)
334337
if err != nil {
335-
return append(allErrs, field.Invalid(fldPath, "", fmt.Sprintf("schema is not a structural schema: %v", err)))
338+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath, "", fmt.Sprintf("schema is not a structural schema: %v", err)))
339+
return allErrs
336340
}
337341

338342
for i, rule := range schema.XValidations {
339343
trimmedRule := strings.TrimSpace(rule.Rule)
340344
trimmedMsg := strings.TrimSpace(rule.Message)
341345
trimmedMsgExpr := strings.TrimSpace(rule.MessageExpression)
342346
if len(trimmedRule) == 0 {
343-
allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), "rule is not specified"))
347+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), "rule is not specified"))
344348
} else if len(rule.Message) > 0 && len(trimmedMsg) == 0 {
345-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must be non-empty if specified"))
349+
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"))
346350
} else if len(rule.Message) > 2048 {
347-
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"))
351+
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"))
348352
} else if hasNewlines(trimmedMsg) {
349-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must not contain line breaks"))
353+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must not contain line breaks"))
350354
} else if hasNewlines(trimmedRule) && len(trimmedMsg) == 0 {
351-
allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), "message must be specified if rule contains line breaks"))
355+
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"))
352356
}
353357
if len(rule.MessageExpression) > 0 && len(trimmedMsgExpr) == 0 {
354-
allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), "messageExpression must be non-empty if specified"))
358+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), "messageExpression must be non-empty if specified"))
355359
}
356360
if rule.Reason != nil && !supportedValidationReason.Has(string(*rule.Reason)) {
357-
allErrs = append(allErrs, field.NotSupported(fldPath.Child("x-kubernetes-validations").Index(i).Child("reason"), *rule.Reason, supportedValidationReason.List()))
361+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.NotSupported(fldPath.Child("x-kubernetes-validations").Index(i).Child("reason"), *rule.Reason, supportedValidationReason.List()))
358362
}
359363
trimmedFieldPath := strings.TrimSpace(rule.FieldPath)
360364
if len(rule.FieldPath) > 0 && len(trimmedFieldPath) == 0 {
361-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must be non-empty if specified"))
365+
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"))
362366
}
363367
if hasNewlines(rule.FieldPath) {
364-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must not contain line breaks"))
368+
allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must not contain line breaks"))
365369
}
366370
if len(rule.FieldPath) > 0 {
367371
if _, _, err := cel.ValidFieldPath(rule.FieldPath, ss); err != nil {
368-
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)))
372+
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)))
369373
}
370374
}
371375
}
@@ -374,49 +378,49 @@ func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps,
374378
// Invalid OpenAPISchemas are not always possible to convert into valid CEL DeclTypes, and can lead to CEL
375379
// validation error messages that are not actionable (will go away once the schema errors are resolved) and that
376380
// are difficult for CEL expression authors to understand.
377-
if len(allErrs) == 0 && celContext != nil {
381+
if len(allErrs.SchemaErrors) == 0 && celContext != nil {
378382
typeInfo, err := celContext.TypeInfo()
379383
if err != nil {
380-
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)))
384+
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)))
381385
} else if typeInfo == nil {
382-
allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), fmt.Errorf("internal error: failed to retrieve type information for x-kubernetes-validations")))
386+
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")))
383387
} else {
384388
// Note: k/k CRD validation also uses celconfig.PerCallLimit when creating the validator.
385389
// The current PerCallLimit gives roughly 0.1 second for each expression validation call.
386390
compResults, err := cel.Compile(typeInfo.Schema, typeInfo.DeclType, celconfig.PerCallLimit, opts.celEnvironmentSet, opts.preexistingExpressions)
387391
if err != nil {
388-
allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), err))
392+
allErrs.CELErrors = append(allErrs.CELErrors, field.InternalError(fldPath.Child("x-kubernetes-validations"), err))
389393
} else {
390394
for i, cr := range compResults {
391395
expressionCost := getExpressionCost(cr, celContext)
392396
if expressionCost > StaticEstimatedCostLimit {
393397
costErrorMsg := getCostErrorMessage("estimated rule cost", expressionCost, StaticEstimatedCostLimit)
394-
allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), costErrorMsg))
398+
allErrs.CELErrors = append(allErrs.CELErrors, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), costErrorMsg))
395399
}
396400
if celContext.TotalCost != nil {
397401
celContext.TotalCost.ObserveExpressionCost(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), expressionCost)
398402
}
399403
if cr.Error != nil {
400404
if cr.Error.Type == apiservercel.ErrorTypeRequired {
401-
allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), cr.Error.Detail))
405+
allErrs.CELErrors = append(allErrs.CELErrors, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), cr.Error.Detail))
402406
} else {
403-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i], cr.Error.Detail))
407+
allErrs.CELErrors = append(allErrs.CELErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i], cr.Error.Detail))
404408
}
405409
}
406410
if cr.MessageExpressionError != nil {
407-
allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), schema.XValidations[i], cr.MessageExpressionError.Detail))
411+
allErrs.CELErrors = append(allErrs.CELErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), schema.XValidations[i], cr.MessageExpressionError.Detail))
408412
} else if cr.MessageExpression != nil {
409413
if cr.MessageExpressionMaxCost > StaticEstimatedCostLimit {
410414
costErrorMsg := getCostErrorMessage("estimated messageExpression cost", cr.MessageExpressionMaxCost, StaticEstimatedCostLimit)
411-
allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), costErrorMsg))
415+
allErrs.CELErrors = append(allErrs.CELErrors, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), costErrorMsg))
412416
}
413417
if celContext.TotalCost != nil {
414418
celContext.TotalCost.ObserveExpressionCost(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), cr.MessageExpressionMaxCost)
415419
}
416420
}
417421
if cr.UsesOldSelf {
418422
if uncorrelatablePath != nil {
419-
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)))
423+
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)))
420424
}
421425
}
422426
}
@@ -425,7 +429,7 @@ func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps,
425429
}
426430

427431
// Return if we found some errors in the CEL validations.
428-
if len(allErrs) > 0 {
432+
if len(allErrs.AllErrors()) > 0 {
429433
return allErrs
430434
}
431435

@@ -434,16 +438,17 @@ func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps,
434438
// CEL validation for Default values was already done via: structuraldefaulting.ValidateDefaults
435439
celValidator := cel.NewValidator(ss, false, celconfig.PerCallLimit)
436440
if celValidator == nil {
437-
return append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations"), "", "failed to create CEL validator"))
441+
allErrs.CELErrors = append(allErrs.CELErrors, field.Invalid(fldPath.Child("x-kubernetes-validations"), "", "failed to create CEL validator"))
442+
return allErrs
438443
}
439444
if schema.Example != nil {
440445
errs, _ := celValidator.Validate(ctx, fldPath.Child("example"), ss, *schema.Example, nil, celconfig.RuntimeCELCostBudget)
441-
allErrs = append(allErrs, errs...)
446+
allErrs.CELErrors = append(allErrs.CELErrors, errs...)
442447
}
443448
for i, enum := range schema.Enum {
444449
if enum != nil {
445450
errs, _ := celValidator.Validate(ctx, fldPath.Child("enum").Index(i), ss, enum, nil, celconfig.RuntimeCELCostBudget)
446-
allErrs = append(allErrs, errs...)
451+
allErrs.CELErrors = append(allErrs.CELErrors, errs...)
447452
}
448453
}
449454
}
@@ -454,6 +459,30 @@ func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps,
454459
// The following funcs are all duplicated from upstream CRD validation at
455460
// https://github.com/kubernetes/apiextensions-apiserver/blob/v0.30.0/pkg/apis/apiextensions/validation/validation.go#L1317.
456461

462+
// OpenAPISchemaErrorList tracks validation errors reported
463+
// with CEL related errors kept separate from schema related errors.
464+
type OpenAPISchemaErrorList struct {
465+
SchemaErrors field.ErrorList
466+
CELErrors field.ErrorList
467+
}
468+
469+
// AppendErrors appends all errors in the provided list with the errors of this list.
470+
func (o *OpenAPISchemaErrorList) AppendErrors(list *OpenAPISchemaErrorList) {
471+
if o == nil || list == nil {
472+
return
473+
}
474+
o.SchemaErrors = append(o.SchemaErrors, list.SchemaErrors...)
475+
o.CELErrors = append(o.CELErrors, list.CELErrors...)
476+
}
477+
478+
// AllErrors returns a list containing both schema and CEL errors.
479+
func (o *OpenAPISchemaErrorList) AllErrors() field.ErrorList {
480+
if o == nil {
481+
return field.ErrorList{}
482+
}
483+
return append(o.SchemaErrors, o.CELErrors...)
484+
}
485+
457486
// validationOptions groups several validation options, to avoid passing multiple bool parameters to methods.
458487
type validationOptions struct {
459488
preexistingExpressions preexistingExpressions

0 commit comments

Comments
 (0)